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

Use arbitrary precision when parsing and pretty-printing FIL amounts #2833

Closed
lemmih opened this issue May 8, 2023 · 4 comments · Fixed by #2857
Closed

Use arbitrary precision when parsing and pretty-printing FIL amounts #2833

lemmih opened this issue May 8, 2023 · 4 comments · Fixed by #2857
Assignees
Labels
Ready Issue is ready for work and anyone can freely assign it to themselves

Comments

@lemmih
Copy link
Contributor

lemmih commented May 8, 2023

Issue summary

Forest uses format_balance_string for pretty-printing FIL amounts and FILAmount::from_str to parse FIL amounts. Both functions are limited to 96bit floating point values. In practice, this is sufficient for both mainnet and calibnet as the maximum number of mintable FIL fits easily in 96bit. However, the Filecoin spec only limits FIL to 1024bit values. To match the spec as closely as possible, we should use arbitrary precision values both when pretty-printing and parsing FIL amounts.

Other information and links

See https://docs.rs/fraction/latest/fraction/. Especially https://docs.rs/fraction/latest/fraction/prelude/type.DynaDecimal.html.

Also consider https://docs.rs/num-rational/latest/num_rational/

@lemmih lemmih added the Ready Issue is ready for work and anyone can freely assign it to themselves label May 8, 2023
@aatifsyed aatifsyed self-assigned this May 10, 2023
@aatifsyed
Copy link
Contributor

aatifsyed commented May 10, 2023

Nervous about using fraction::DynaDecimal because:

Precision is being used for representation purposes only. Calculations do not use precision, but comparison does.

I can see future code forgetting this limitation and doing arith with it, losing some attos down the back of the sofa or something.

This might be okay because the FVM does all the arithmetic we need at the moment.

But we allocate for a BigInt anyway, as that's what FVM uses: https://github.com/filecoin-project/ref-fvm/blob/fvm@v3.3.1/shared/src/econ/mod.rs#L22, so makes sense to go for that no?

@aatifsyed
Copy link
Contributor

Doesn't look like the spec has a position on bits in a filecoin amount as of https://github.com/filecoin-project/specs/tree/c13268c05a69851e60ce31cbef783d214fe61c40 (at least not searching for 1024)

Lotus has this line: https://github.com/filecoin-project/lotus/blob/d1d4b35adf934618c9b28196d3d8ba90f3cffcf6/chain/types/bigint.go#L12

const BigIntMaxSerializedLen = 128 // is this big enough? or too big?

However, that constant is dead, never referenced in the codebase

@lemmih
Copy link
Contributor Author

lemmih commented May 10, 2023

The relevant code was moved to the FVM: https://github.com/filecoin-project/ref-fvm/blob/f4f3f340ba29b3800cd8272e34023606def23855/shared/src/bigint/mod.rs#L13

I guess they forgot to remove the unused definitions from Lotus.

@aatifsyed
Copy link
Contributor

Okay here's what I've learned so far.

Either way it looks like we don't have a gate on actual token amounts, and FVM's types are pretty leaky...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready Issue is ready for work and anyone can freely assign it to themselves
Projects
Status: No status
Development

Successfully merging a pull request may close this issue.

2 participants