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

BigNumber.toString breaking change not in V5 migration guide #1164

Closed
matYang opened this issue Nov 20, 2020 · 5 comments
Closed

BigNumber.toString breaking change not in V5 migration guide #1164

matYang opened this issue Nov 20, 2020 · 5 comments
Labels
enhancement New feature or improvement. fixed/complete This Bug is fixed or Enhancement is complete and published.

Comments

@matYang
Copy link

matYang commented Nov 20, 2020

This references Why doesn't BigNumber.toString allow an arbitrary base? #889

I'm getting the same error after upgrading to Ethers V5 from V4 because I had places that call .toString(10) on results from Ethers. Read through the #889 thread, I guess it makes sense, but be aware it could confuse people, as toString(10) is the go-to function in the actual BigNumber library to print a really big number without exponential notation.

Anyways, it is a breaking change, but it is not mentioned here https://docs.ethers.io/v5/migration/ethers-v4/#migration-v4--bignumber

@ricmoo
Copy link
Member

ricmoo commented Nov 20, 2020

The base was always ignored, and it is somewhat dangerous to include it (since another developer may believe it follows the BN.js convention), but you are correct it did not throw.

I will add a note to the migration docs. :)

@ricmoo ricmoo added documentation Documentation related issue. on-deck This Enhancement or Bug is currently being worked on. enhancement New feature or improvement. and removed documentation Documentation related issue. labels Nov 20, 2020
@ricmoo
Copy link
Member

ricmoo commented Nov 20, 2020

I actually think, since I already do the check in toString, I will allow the value 10 passed in; however it will emit a warning the first time it is used...

Thoughts?

@ricmoo
Copy link
Member

ricmoo commented Nov 20, 2020

(also, for people who stumble across this in the future, previous relevant issues are: #248, #889)

ricmoo added a commit that referenced this issue Nov 23, 2020
ricmoo added a commit that referenced this issue Nov 23, 2020
@ricmoo
Copy link
Member

ricmoo commented Nov 24, 2020

This has been addressed in 5.0.22. It will now log a warning the first time value.toString(10) is called and throw a more useful error on any other value passed in.

Try it out and let me know how it works for you.

Thanks! :)

@ricmoo ricmoo added fixed/complete This Bug is fixed or Enhancement is complete and published. and removed on-deck This Enhancement or Bug is currently being worked on. labels Nov 24, 2020
@ricmoo
Copy link
Member

ricmoo commented Dec 14, 2020

Closing this now. If you have any further issues, please re-open. :)

@ricmoo ricmoo closed this as completed Dec 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or improvement. fixed/complete This Bug is fixed or Enhancement is complete and published.
Projects
None yet
Development

No branches or pull requests

2 participants