ref-fvm
ref-fvm copied to clipboard
Change TokenAmount type to a new-type
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).
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.
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).
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.
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).