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

(Feature) DAI chain support #153

Merged
merged 12 commits into from
Oct 17, 2018
Merged

(Feature) DAI chain support #153

merged 12 commits into from
Oct 17, 2018

Conversation

vbaranov
Copy link
Collaborator

Relates to #152
DAI chain support is added

screen shot 2018-10-11 at 20 57 31

screen shot 2018-10-11 at 20 57 54

screen shot 2018-10-11 at 20 58 19

@vbaranov
Copy link
Collaborator Author

@dennis00010011b please actualize e2e tests to support DAI chain.

@dennis00010011b
Copy link

@vbaranov
Invalid custom RPC can be added

screen shot 2018-10-11 at 12 31 09 pm

@vbaranov
Copy link
Collaborator Author

@dennis00010011b Is it reproducible in develop branch? If yes, please open a separate issue.

@vbaranov
Copy link
Collaborator Author

@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)

@dennis00010011b dennis00010011b mentioned this pull request Oct 12, 2018
@@ -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'

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.

Copy link
Collaborator Author

@vbaranov vbaranov Oct 12, 2018

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.

@SiddigZeidan
Copy link

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'

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'

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@SiddigZeidan changed

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

screen shot 2018-10-12 at 20 34 54

@dennis00010011b
Copy link

@vbaranov
There is still 'Dai chain' in dropdown menu and in Settings ->Current Network
screen shot 2018-10-12 at 10 38 37 am

@vbaranov
Copy link
Collaborator Author

@dennis00010011b you should update npm packages with npm i. Because display name of the network and ticker are from eth-net-props package.

@vbaranov
Copy link
Collaborator Author

@dennis00010011b please, resolve merging conflicts related to e2e tests

@vbaranov
Copy link
Collaborator Author

@dennis00010011b please take a look to broken e2e tests and also to DeepScan suggestions ☝️

@vbaranov
Copy link
Collaborator Author

@dennis00010011b all tests were passed successfully after merging with develop branch.

@vbaranov
Copy link
Collaborator Author

Dai Chain is now called xDai Chain. The ticket will be xDAI.

@SiddigZeidan it has been implemented. Is it final naming? Could we proceed to merge this PR to develop branch?

@SiddigZeidan
Copy link

Dai Chain is now called xDai Chain. The ticket will be xDAI.

@SiddigZeidan it has been implemented. Is it final naming? Could we proceed to merge this PR to develop branch?

Yes sir, please proceed and merge

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants