-
Notifications
You must be signed in to change notification settings - Fork 43
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
(Feature) DAI chain support #153
Conversation
@dennis00010011b please actualize e2e tests to support DAI chain. |
@vbaranov |
@dennis00010011b Is it reproducible in |
@dennis00010011b And we only validate accessibility of the provided URL endpoint. We don't check, that it is a valid RPC endpoint actually (see the original feature request #76) |
old-ui/app/components/network.js
Outdated
@@ -60,6 +60,9 @@ Network.prototype.render = function () { | |||
} else if (providerName === 'poa' || parseInt(networkNumber) === 99) { | |||
displayName = 'POA Network' | |||
hoverText = ethNetProps.props.getNetworkDisplayName(networkNumber) | |||
} else if (providerName === 'dai' || parseInt(networkNumber) === 100) { | |||
displayName = 'Dai Chain' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this use the DAI_DISPLAY_NAME
defined in network/enums.js
? The rest of the file seems to be doing the same (using strings instead of those constants) but maybe it should be changed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fvictorio Yes, good idea. All dropdown display values are defined in network/enums.js
now.
Hey guys, We are making slight alterations to the name. Dai Chain is now called xDai Chain. The ticket will be xDAI. Ill make comments along the files here that I can spot will need a change |
|
||
const ROPSTEN_DISPLAY_NAME = 'Ropsten' | ||
const RINKEBY_DISPLAY_NAME = 'Rinkeby' | ||
const KOVAN_DISPLAY_NAME = 'Kovan' | ||
const POA_SOKOL_DISPLAY_NAME = 'Sokol' | ||
const POA_DISPLAY_NAME = 'POA Network' | ||
const DAI_DISPLAY_NAME = 'Dai Chain' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const DAI_DISPLAY_NAME = 'xDai Chain'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@SiddigZeidan changed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vbaranov |
e2e-dai-chain
@dennis00010011b you should update npm packages with |
@dennis00010011b please, resolve merging conflicts related to e2e tests |
e2e dai fix merging conflict
@dennis00010011b please take a look to broken e2e tests and also to DeepScan suggestions ☝️ |
@dennis00010011b all tests were passed successfully after merging with |
@SiddigZeidan it has been implemented. Is it final naming? Could we proceed to merge this PR to |
Yes sir, please proceed and merge |
Relates to #152
DAI chain support is added