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

Do not throw on parseUnits if input truncatable #1019

Closed
wants to merge 1 commit into from

Conversation

coinwalletdev
Copy link

Currently, if you do the following:

require('ethers').utils.parseUnits('123.4560000', 4)

ethers.js will raise an exception while it should simply remove the trailing 0 from the string before converting it. This PR does this.

I understand the need to raise an exception in more ambiguous cases (e.g. parseUnits('123.45678', 4) - should you floor, round, ceil, ... ?), but if there is trailing 0, this is a trivial case.

@ricmoo
Copy link
Member

ricmoo commented Aug 27, 2020

I think this should still be an error, I think.

A value with significant digits (0 is a completely valid significant digit) is being asked to truncate those significant digits, and in general I would think this indicates an error somewhere upstream. I agree though, it has no meaningful effect on the value.

Can you give an example where this was an issue?

I'll ponder about it over night. :)

@coinwalletdev
Copy link
Author

coinwalletdev commented Aug 27, 2020

I'm not sure if it's up to the library to decide whether or not 0s have a meaning - IMHO as an author you should just choose sane default options for your users.

Deciding this for your users force them to reimplement by themselves roughly the parseFixed function which makes the whole point of parseUnits more or less useless.

Ideally you'd feed another option to parseUnits allowing the user to choose if they want a strict mode (throw an exception), floor the value, ceil the value, etc..

This is just my 2 cents and nevertheless it seems you know what you're doing with this amazing library :)

@ricmoo ricmoo added discussion Questions, feedback and general information. on-deck This Enhancement or Bug is currently being worked on. labels Apr 18, 2021
@ricmoo
Copy link
Member

ricmoo commented May 20, 2021

I was still a little on the fence, but after thinking about it, agree it makes sense. I don't error if you you include leading-zeros that would expand beyond a support numeric value in magnitude, so should probably not treat precision differently.

I've made this adjustment in 5.2.0.

Please try it out and let me know if you have any problems. Thanks! :)

@ricmoo ricmoo closed this May 20, 2021
@ricmoo ricmoo added enhancement New feature or improvement. fixed/complete This Bug is fixed or Enhancement is complete and published. and removed discussion Questions, feedback and general information. on-deck This Enhancement or Bug is currently being worked on. labels May 20, 2021
pull bot pushed a commit to shapeshift/ethers.js that referenced this pull request Jun 4, 2021
pull bot pushed a commit to shapeshift/ethers.js that referenced this pull request Jun 4, 2021
@ChALkeR
Copy link

ChALkeR commented Aug 31, 2021

See #1974

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

Successfully merging this pull request may close these issues.

3 participants