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

Fix incorrectly showing checksums on non-ETH blockchains #5973

Closed
wants to merge 18 commits into from
Closed

Fix incorrectly showing checksums on non-ETH blockchains #5973

wants to merge 18 commits into from

Conversation

jonathansmirnoff
Copy link
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
Contributor Author

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

@whymarrh
Copy link
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
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
Contributor Author

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

@whymarrh
Copy link
Contributor

whymarrh commented Jan 3, 2019

It should be working fine on the latest develop

@jonathansmirnoff
Copy link
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
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