ref-fvm icon indicating copy to clipboard operation
ref-fvm copied to clipboard

Change TokenAmount type to a new-type

Open anorth opened this issue 3 years ago • 2 comments

The TokenAmount type is currently just an alias for BigInt. This means values can usually be transparently interpreted, compared etc between TokenAmount and other types, despite TokenAmount having a logical interpretation as a decimal. This exposes significant potential for error, e.g. as I discovered in in https://github.com/filecoin-project/builtin-actors/pull/514

If we make it a struct type and then implement pass-through methods for the common operations, and explicit conversion to strings, big decimals etc, then downstream users (which includes all actors) will have to work hard to mess it up. As a bonus, we can implement serialisation so we don't have to wrap in BigIntDe everywhere it's used in a HAMT, etc.

I'm willing to attempt this if FVM people concur.

(If you don't, in built-in actors I'll probably do it anyway and hide the FVM's version of the type in our runtime layer).

anorth avatar Aug 11 '22 08:08 anorth

Proposal SGTM -- as user actors begin depending on these types, I suspect there'll be more changes we need to make to remove footguns and make them friendlier. I agree with the opportunity here. Folks tend to reason about TokenAmounts as denominated in their full unit (FIL), so there's expectation dissonance here because BigInt is denominated in attoFIL. Thus besides the safety concerns, there is an opportunity to improve DX.

raulk avatar Aug 11 '22 12:08 raulk

I also agree. Note: We've also discussed modeling it as a single u128 to avoid overhead (although we'd need to carefully handle overflow).

Stebalien avatar Aug 11 '22 14:08 Stebalien

Re modelling as u128, I've also run into this e.g https://github.com/filecoin-project/FIPs/discussions/407#discussioncomment-3176582.

Even if we did that in RAM, I'm guessing we would still serialize as varint, since that will usually be smaller, and avoids us having to migrate anything. If we add this stricter abstraction, we'd have the opportunity to do this without large downstream churn (we'll have front-loaded the churn into the newtype). I suspect we won't get round to that prior to M2.2, but it could still happen later as an optimisation.

anorth avatar Aug 17 '22 01:08 anorth

Even if we did that in RAM, I'm guessing we would still serialize as varint, since that will usually be smaller, and avoids us having to migrate anything. If we add this stricter abstraction, we'd have the opportunity to do this without large downstream churn (we'll have front-loaded the churn into the newtype). I suspect we won't get round to that prior to M2.2, but it could still happen later as an optimisation.

Yeah, that's what I assume we'd do. Although CBOR ints are stored as varints anyways. We could probably even submit a spec proposal to allow for 128/256 bit integers (it's alluded to in the current spec).

However, there's nothing particularly wrong with encoding as bigints (for now).

Stebalien avatar Aug 17 '22 06:08 Stebalien