Skip to content
This repository has been archived by the owner on Jun 17, 2021. It is now read-only.

Implement EIP1191 #204

Merged
merged 3 commits into from
Jun 28, 2019
Merged

Implement EIP1191 #204

merged 3 commits into from
Jun 28, 2019

Conversation

alcuadrado
Copy link
Member

This PR implements EIP-1191 address checksum algorithm.

Fixes #201

@coveralls
Copy link

coveralls commented Jun 24, 2019

Coverage Status

Coverage increased (+0.002%) to 99.336% when pulling beecf2d on eip1191 into a7bfe26 on master.

@alcuadrado
Copy link
Member Author

I removed node 6 from the travis config, as a test was failing because of that.

I'm pretty sure we don't support node 6 anymore as its EOL has already happened. Let me know if i'm wrong and I'll revert that change.

Copy link
Contributor

@s1na s1na left a comment

Choose a reason for hiding this comment

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

Looks good thanks, just a few minor points

src/account.ts Outdated
address = ethjsUtil.stripHexPrefix(address).toLowerCase()
const hash = keccak(address).toString('hex')

const prefix = chainId !== undefined ? chainId.toString() + '0x' : ''
Copy link
Contributor

Choose a reason for hiding this comment

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

The documentation makes it clear, but if someone seeing chainId as the second parameter provides an Ethereum chainId (e.g. 1) then the checksum will fail.

One alternative is to provide a second function toEIP1991ChecksumAddress (maybe with a better name 😄).

I don't have a strong opinon however. @holgerd77 what do you think?

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point.

I made it this way to keep it similar to other implementations, like web3.js' one

Another alternative is to rename the second param to something like eip1191ChainId

Copy link
Member

Choose a reason for hiding this comment

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

I would have a tendency to the renamed parameter. @alcuadrado can you also add "(prefixed with chainId)" and "(no prefix)" or some similar text after the respective EIP mentions in the JSDocs? Think this will also make things more clear, I often find these plain EIP mentions in our docs not very helpful if one is not totally in the EIP specs.

Copy link
Member

Choose a reason for hiding this comment

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

One can also add some "Be are that checksums will be different" or so sentence.

Copy link
Member

Choose a reason for hiding this comment

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

Are -> aware

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed the param name and updated the doc.

src/account.ts Outdated Show resolved Hide resolved
.travis.yml Outdated Show resolved Hide resolved
it("Should return false for invalid cases", function () {
// If we set the chain id, an EIP55 encoded address should be invalid
for (let i = 0; i < eip55ChecksumAddresses.length; i++) {
assert.equal(ethUtils.isValidChecksumAddress(eip55ChecksumAddresses[i], 1), false)
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yeah this is the test case I was thinking about :)

s1na
s1na previously approved these changes Jun 27, 2019
Copy link
Contributor

@s1na s1na left a comment

Choose a reason for hiding this comment

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

lgtm

@alcuadrado alcuadrado closed this Jun 28, 2019
@alcuadrado alcuadrado reopened this Jun 28, 2019
@alcuadrado
Copy link
Member Author

Travis got stuck, so I pushed the same code again. Can you reapprove @s1na? Thanks!

@alcuadrado alcuadrado merged commit 3b10850 into master Jun 28, 2019
@alcuadrado alcuadrado deleted the eip1191 branch June 28, 2019 20:45
@kumavis
Copy link
Member

kumavis commented Sep 4, 2019

@holgerd77 @alcuadrado @s1na can we get a new version published? thanks/

@holgerd77
Copy link
Member

@kumavis Yes - sorry, wasn't aware that actually didn't yet release all the latest changes for such a long time. Will prepare something within the next days, hopefully this, eventually next week.

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

Successfully merging this pull request may close these issues.

EIP-1191: new address checksum algorithm
5 participants