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

Util: More explicit EIP-1191 checksum usage discouraged note / Client: remove Rinkeby from CLI test matrix #1463

Merged
merged 2 commits into from Sep 10, 2021

Conversation

holgerd77
Copy link
Member

@holgerd77 holgerd77 commented Sep 9, 2021

This PR adds a more explicit note on toChecksumAddress() that the usage of EIP-1191 is discouraged due to the lack of backwards compatibility to the existing checksum format.

On a side line it also tests the reactivation of the E2E hardhat tests which I have reactivated in the Actions tab and where a fixing PR has been submitted on the Hardhat side here #1391 by @ryanio which should have solved the test-breaking issue.

@codecov
Copy link

codecov bot commented Sep 9, 2021

Codecov Report

Merging #1463 (20f221e) into master (ee1a44e) will increase coverage by 0.17%.
The diff coverage is n/a.

Impacted file tree graph

Flag Coverage Δ
block 86.75% <ø> (ø)
blockchain 83.49% <ø> (-0.07%) ⬇️
client 83.50% <ø> (+0.59%) ⬆️
common 94.39% <ø> (+0.19%) ⬆️
devp2p 82.59% <ø> (+0.13%) ⬆️
ethash 86.56% <ø> (ø)
tx 87.47% <ø> (+0.12%) ⬆️
vm 78.96% <ø> (-0.43%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

@holgerd77
Copy link
Member Author

Ok, Hardhat E2E tests are not yet working, I will open a dedicated issue for that.

PR can be nevertheless reviewed and merged independently from this.

ryanio
ryanio previously approved these changes Sep 9, 2021
Copy link
Contributor

@ryanio ryanio left a comment

Choose a reason for hiding this comment

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

lgtm!

…IP-1191 compatible checksums with toChecksumAddress() being discouraged
@holgerd77 holgerd77 force-pushed the util-eip-1191-checksum-method-discouraged-note branch from e34a220 to 20f221e Compare September 10, 2021 08:25
@holgerd77 holgerd77 changed the title Util: More explicit EIP-1191 checksum usage discouraged note Util: More explicit EIP-1191 checksum usage discouraged note / Client: remove Rinkeby from CLI test matrix Sep 10, 2021
@holgerd77
Copy link
Member Author

Already reviewed by @ryanio, will admin merge as mentioned in #1462 since only CI and docs affected by the PR.

@holgerd77 holgerd77 merged commit 69f5ec6 into master Sep 10, 2021
@holgerd77 holgerd77 deleted the util-eip-1191-checksum-method-discouraged-note branch September 10, 2021 09:26
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.

None yet

2 participants