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

ethers v4 and v5 overload behavior fixes & tests #269

Merged
merged 13 commits into from
Aug 2, 2020

Conversation

bradennapier
Copy link
Contributor

@bradennapier bradennapier commented Aug 2, 2020

Lets try this PR

This incorporates the already merged PR's as well as #256 with the conflict fixes. Should work with the tests.

I think the reason the tests failed were something I fixed in this PR but its been awhile so I forget. Either way, all the tests seem to be passing :-)

I am quite certain this is the right behavior as I dug into the ethers code on both v4 and v5 after noticing the comments mentioned the differences in that issue I linked. LMK if anything seems amiss.

@bradennapier bradennapier changed the base branch from master to fork/ethers-v5-target August 2, 2020 17:36
@bradennapier
Copy link
Contributor Author

bradennapier commented Aug 2, 2020

@krzkaczor this passes all the tests for me. I just merged fork-v5-target branch into the combined-prs branch I have been using then ran the tests on both v4 and v5.

Also merged in master to make sure all was still well. Looks good.

Should be good to merge with #252 then to master but lmk if any issues on your end.

The behavior change is a bit annoying :-P but what can ya do... ethers-io/ethers.js#407 (comment)

@bradennapier bradennapier changed the title Combined prs V4 and V5 Overload Behavior Fixes & Tests Aug 2, 2020
@bradennapier bradennapier changed the title V4 and V5 Overload Behavior Fixes & Tests ethers v4 and v5 overload behavior fixes & tests Aug 2, 2020
@krzkaczor krzkaczor merged commit 5623f0c into dethcrypto:fork/ethers-v5-target Aug 2, 2020
@krzkaczor
Copy link
Member

This looks great. Thanks for you work @bradennapier! I will publish new packages probably tmrw.

@bradennapier bradennapier deleted the combined-prs branch August 2, 2020 19:40
@bradennapier
Copy link
Contributor Author

Sounds good! Thanks!

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.

2 participants