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

Adapts checksum functionality to RSKIP60 and EIP55 #2727

Merged
merged 8 commits into from
Apr 26, 2019
Merged

Conversation

alepc253
Copy link

@alepc253 alepc253 commented Apr 24, 2019

Description

This PR adapts checksum functionality to RSKIP-60 compatible with EIP-55.
The implementation considers sending optional chainId parameter to checksum methods (with chainId = null as default, Ethereum checksum is applied).

Benefit

  • Allows checksum implementation in any network.
  • Can distinguish between testnets and mainnets.

Type of change

  • Bug fix
  • New feature
  • Breaking change
  • Enhancement

Checklist:

  • I have selected the correct base branch.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no warnings.
  • I have updated or added types for all modules I've changed
  • Any dependent changes have been merged and published in downstream modules.
  • I ran npm run test in the root folder with success and extended the tests if necessary.
  • I ran npm run build in the root folder and tested it in the browser and with node.
  • I ran npm run dtslint in the root folder and tested that all my types are correct
  • I have tested my code on an ethereum test network.

@coveralls
Copy link

coveralls commented Apr 24, 2019

Coverage Status

Coverage increased (+0.3%) to 95.969% when pulling 84d28d6 on alepc253:rskip-60 into 98e3a10 on ethereum:1.0.

@nivida nivida added Enhancement Includes improvements or optimizations In Progress Currently being worked on labels Apr 24, 2019
Copy link
Contributor

@nivida nivida left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this PR!

Could you update the related documentation and the types for both methods?

docs/_build/html/_sources/web3-utils.rst.txt Outdated Show resolved Hide resolved
docs/_build/html/_sources/web3-utils.rst.txt Show resolved Hide resolved
docs/_build/html/_sources/web3-utils.rst.txt Outdated Show resolved Hide resolved
@alepc253
Copy link
Author

@nivida thanks! I'm working on your reviews.
One important aspect to consider (and still didn't boarded) is the method inputAddressFormatter from web3-core-helpers/src/Formatters.js. chainId should be added to that formatter to be consistent to this change. I guess it would be very helpful to have chainId as a global parameter but I don't know if that is possible at this level... what do you think?

@nivida
Copy link
Contributor

nivida commented Apr 25, 2019

@alepc253 I would like to write that down in a new issue and to implement it later as an enhancement. The question will be if Web3 should automatically load the chainId from the connected node if no chainId got defined from the developer.

@nivida nivida removed the In Progress Currently being worked on label Apr 26, 2019
@nivida nivida merged commit aa5286e into web3:1.0 Apr 26, 2019
@nivida nivida mentioned this pull request Apr 29, 2019
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Enhancement Includes improvements or optimizations
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants