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

Fix parse units #597

Merged
merged 2 commits into from
Nov 24, 2021
Merged

Fix parse units #597

merged 2 commits into from
Nov 24, 2021

Conversation

x3ccd4828
Copy link
Contributor

@x3ccd4828 x3ccd4828 commented Nov 19, 2021

Motivation

parse_units and format_units were not behaving as expected when the decimal number was greater than 15 such as ether amounts

Solution

modified parse_units and format_units to use rug::Float of 128bit such that 18 decimal point floats don't get truncated. format_units now returns a rug::Float so that might break code that is using this function however it's an easy fix for such projects.

PR Checklist

  • Added Tests
  • Added Documentation
  • Updated the changelog

@x3ccd4828 x3ccd4828 force-pushed the fix-parse-units branch 2 times, most recently from 9faa56a to a701787 Compare November 19, 2021 21:24
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm, since this is a breaking change, could you make a note in the CHANGELOG and.
a basic example in the function's rustdoc could also be useful, basically copy-paste from test and maybe show how to convert back to U256?

@x3ccd4828
Copy link
Contributor Author

@mattsse I will update the changelog and rust doc when I get back to my computer.

@x3ccd4828
Copy link
Contributor Author

@mattsse it seems the WASM build is broken because of the rug crate https://github.com/gakonst/ethers-rs/runs/4266231384?check_suite_focus=true

is this a problem?

Copy link
Owner

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

I think the requested change should fix the wasm build

ethers-core/Cargo.toml Outdated Show resolved Hide resolved
@x3ccd4828
Copy link
Contributor Author

@gakonst @mattsse it seems the default-features = false doesn't help the wasm compile

@mattsse
Copy link
Collaborator

mattsse commented Nov 20, 2021

uh oh, this appears to be an issue with gmp-mpfr-sys that's not wasm compatible? https://gitlab.com/tspiteri/gmp-mpfr-sys/-/blob/master/build.rs
it requires the force-cross feature apparently, but I'm not sure whether this would work for the wasm32 target, can you try this and adding a wasm test with wasm-bindgen https://rustwasm.github.io/wasm-bindgen/wasm-bindgen-test/usage.html?

@x3ccd4828
Copy link
Contributor Author

yeah I will look into it

@x3ccd4828
Copy link
Contributor Author

it seems the gmp-mpfr-sys lib doesn't support wasm https://gitlab.com/tspiteri/gmp-mpfr-sys/-/issues/18

should I look for a different lib for float?

@gakonst
Copy link
Owner

gakonst commented Nov 21, 2021

Yes I'd prefer if we switched to a different float lib! Also, the function should still return U256, because it's expected that somebody uses it with other eth calls, and we want to keep the UX good (i.e. one should not need to manually cast to u256 after using these functions)

@x3ccd4828
Copy link
Contributor Author

@gakonst I will look into a different implementation that supports wasm however the functions should return a float or a string, not a U256 since that is incorrect functionality. That would mean that all values less than 0 would be returned as 0.

so 0.56749 ETH would be returned as 0 which in my opinion is broken.

the ethers implementation returns a string which is a float number: https://docs.ethers.io/v5/api/utils/display-logic/#utils-formatUnits

@x3ccd4828
Copy link
Contributor Author

@gakonst @mattsse I rebased the branch based on the latest master and I only modified format_units to return a String which preserves the decimal places. This should be a good solution until a better float128 lib is available to support wasm.

@x3ccd4828
Copy link
Contributor Author

I rebased to fix the conflict

Copy link
Owner

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

Looking good.

@gakonst gakonst merged commit 3a768b9 into gakonst:master Nov 24, 2021
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.

None yet

3 participants