Skip to content

Fix incorrectly showing checksums on non-ETH blockchains#5973

Closed
jonathansmirnoff wants to merge 18 commits intoMetaMask:developfrom
jonathansmirnoff:added-support-rsk
Closed

Fix incorrectly showing checksums on non-ETH blockchains#5973
jonathansmirnoff wants to merge 18 commits intoMetaMask:developfrom
jonathansmirnoff:added-support-rsk

Conversation

@jonathansmirnoff
Copy link
Copy Markdown
Contributor

This PR fix the issue: #5838

@jonathansmirnoff jonathansmirnoff changed the title Incorrectly showing checksums on non-ETH blockchains Fix incorrectly showing checksums on non-ETH blockchains Dec 27, 2018
@jonathansmirnoff
Copy link
Copy Markdown
Contributor Author

@whymarrh did you have the chance to review this PR?

@whymarrh
Copy link
Copy Markdown
Contributor

whymarrh commented Jan 3, 2019

Hey @jonathansmirnoff, thanks for this PR! Firstly, can we rebase this on the latest develop to try for a green build. Also, I'm not sure that adding custom RSK-specific logic is a good solution here... maybe there's a way to do this without being specific?

@jonathansmirnoff
Copy link
Copy Markdown
Contributor Author

@whymarrh thanks for your review. I will do the rebase.
Also I will try to some changes in the logic to check the network to avoid RSK specific logic.

@jonathansmirnoff
Copy link
Copy Markdown
Contributor Author

@whymarrh is it possible that ci/circleci: test-e2e-beta-drizzle test is not working?

@whymarrh
Copy link
Copy Markdown
Contributor

whymarrh commented Jan 3, 2019

It should be working fine on the latest develop

@jonathansmirnoff
Copy link
Copy Markdown
Contributor Author

@whymarrh I implemented a few changes, I think that now it is more clear the code. I put the logic of the network check in the function checksumAddress:

function checksumAddress (address, network) {
  const checksummed = address ? ethUtil.toChecksumAddress(address) : ''
  return checksummed && network && !isEthNetwork(network) ? checksummed.toLowerCase() : checksummed
}

So now I pass the networkid to the checksumAddress function. For example:
const checksummedAddress = checksumAddress(selectedAddress, network)

Let me know what do you think about this.
Also yesterday I fixed the issue with the test. But now I get the last of develop and I have a few test that aren't passing. Do you know if there any with them?

Thanks!

@jonathansmirnoff
Copy link
Copy Markdown
Contributor Author

I will do another PR to be more clear with the history.

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.

3 participants