Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CSUB-905: Bring back FE commits from main into dev #123

Draft
wants to merge 15 commits into
base: dev
Choose a base branch
from

Conversation

atodorov
Copy link

This PR fixes the current divergence between dev and main caused by recent pull requests merged direclty into the main branch instead of being opened against dev. Namely:

The state of the two branches before this PR is merged is as follows:

  • Last common commit is on Sep 1, 2023: Change network selection to refresh the page
  • On dev we have 3 commits from contributed from the community on Sep 14 and Sep 18:
    image
  • On main we have only the commits from the above listed PRs in November (note that some a merge commits):
    image

WARNING: I propose merging these into dev and then doing a git reset --hard e4e5962754390043bc890c51e250a42d286d9159 on main and force-pushing main in order to bring this branch back into a clean state, which will later be able to receive PRs from dev.

var format, fractional, parsed, precision, radix, repeating, _ref, _ref1;
format = (_ref = this.options.format) != null ? _ref : DIGIT_FORMAT;
format || (format = "d");
parsed = FORMAT_PARSER.exec(format);

Check failure

Code scanning / CodeQL

Polynomial regular expression used on uncontrolled data

This [regular expression](1) that depends on [library input](2) may run slow on strings starting with 'a' and with many repetitions of 'dd'.
Copy link
Author

@atodorov atodorov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some questions for items which are currently not clear to me.

@@ -11,6 +11,8 @@ import { I18nextProvider } from 'react-i18next';
export const App: React.FC = () => {
let network = localStorage.getItem('network');

console.log('Version 1.1.0');
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nbass3 what is this line supposed to do? Is this something only for debugging purposes ?

In any case, of we want to use versions, it's a better idea to probably use the one from package.json or AppVersion in consts.ts.

"@polkadotcloud/react-odometer": "0.1.17",
"@polkadotcloud/utils": "0.2.22",
"@polkadotcloud/react-odometer": "file:./packages/@polkadotcloud/react-odometer",
"@polkadotcloud/utils": "file:./packages/@polkadotcloud/utils",
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rosee-xo can you please explain why are we vendoring in a dependency library and which exactly version of it is that ?

If we need to pin to a specific version of @polkadotcloud/react-odometer and @polkadotcloud/utils why not just pin the version dependency number in package.json ?

@@ -58,7 +58,8 @@ export const NetworkList: Networks = {
dark: '#9fccaa',
},
},
subscanEndpoint: 'https://subscan-mainnet.creditcoin.network/',
subscanEndpoint: 'https://subscan-mainnet.creditcoin.network',
subscanUrl: 'https://creditcoin.subscan.io',
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Sanggon-Kim naming of this is mildly confusing but if I understand it correctly subscanUrl is the value for using Subscan as a service, while subscanEndpoint is the value for self-hosted Subscan fork. Is this correct ?

Q2: if we are switching over to a hosted Susbcan service shouldn't references to the old subscanEndpoint variable be removed then ? I don't see this to be the case.

subscanEraStat: '/api/plugin/staking/eraStat',
subscanRewardSlash: '/api/v2/scan/account/reward_slash',
subscanPoolRewards: '/api/scan/nomination_pool/rewards',
subscanEraStat: '/api/scan/staking/era_stat',
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nbass3 I understand these changes are needed because of the switch to using Subscan as a hosted service. Indeed we've adjusted these API endpoints ourselves in the beginning and now you are just reverting them back.

Question: are we using only Subscan as a service or using both Subscan as a service and self-hosted Subscan fork at the same time together ? (See my comments for src/config/networks.ts for some more context). If both are used at the same time that's going to fail because API paths are different on the self-hosted Subscan fork.

error @polkadot/[email protected]: The engine "node" is incompatible with this module. Expected version ">=18". Got "16.20.2"
error Found incompatible module.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants