[Flow EVM] balance type improvement #5271
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5271 +/- ##
==========================================
- Coverage 55.60% 53.44% -2.17%
==========================================
Files 1002 224 -778
Lines 96627 22049 -74578
==========================================
- Hits 53725 11783 -41942
+ Misses 38845 9286 -29559
+ Partials 4057 980 -3077
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
fvm/evm/types/balance.go
Outdated
| func (b Balance) ToAttoFlow() *big.Int { | ||
| return new(big.Int).Mul(new(big.Int).SetUint64(uint64(b)), SmallestAcceptableBalanceValueInAttoFlow) | ||
| // HasUFix64RoundingError returns true if casting to UFix64 has rounding error | ||
| func (b *Balance) HasUFix64RoundingError() bool { |
There was a problem hiding this comment.
Is there a way to avoid having this method as an explicit check, instead encapsulating it so there's no state that this could be possible? What I'm trying to say is, that a developer might forget to run this check and thus it's bug-prone. If I understand correctly the only place a balance could be created that would lead to rounding error is by using NewBalanceFromAttoFlow so why wouldn't the check be part of that factory?
There was a problem hiding this comment.
typical developer doesn't have to deal with structure, they use the Cadence one, this is on the backbone.
initially we had the approach you suggested but, when you query an account balance and account state has been created then you have some balance that can be rounded off when converting to UFix but user might ask for AttoFlow value and we can't ignore the real balance of the account.
fvm/evm/types/balance.go
Outdated
| if b.Value.Cmp(otherInAtto) == -1 { | ||
| return ErrWithdrawBalanceRounding | ||
| } | ||
| b.Value = new(big.Int).Sub(b.Value, otherInAtto) |
There was a problem hiding this comment.
this result could be a value that produces a rounding error when converted to Flow no?
fvm/evm/types/balance.go
Outdated
| // no need to check for overflow, as go does it | ||
| return Balance(uint64(b) + uint64(other)) | ||
| b.Value = new(big.Int).Add(b.Value, other.ToAttoFlow()) |
There was a problem hiding this comment.
this could also lead to a rounding error after converting to flow ufix no?
| ErrBalanceConversion = errors.New("balance converion error") | ||
| // ErrWithdrawBalanceRounding is returned when withdraw call has a balance that could | ||
| // yeild to rounding error, i.e. the balance contains fractions smaller than 10^8 Flow (smallest unit allowed to transfer). | ||
| ErrWithdrawBalanceRounding = errors.New("withdraw failed! balance is susceptible to the rounding error") |
There was a problem hiding this comment.
should we add what is the biggest precision we support in the error description so the user knows how to react
There was a problem hiding this comment.
hmm, not sure if I follow, we have 10^8 in the go doc?
|
Let me clarify what the purpose of this PR is:
|
|
one option that I initially started what when you do the UFix casting we also return a boolean giving heads up about the potential rounding error. |
…low-go into ramtin/evm-improve-balance-type
This makes sense. |
This PR updates the balance type definition to use a
*big.Intunder the hood instead of the Ufix64 which has less precision.The balance object is used for multiple purposes, such as returning the balance for an account or setting an amount to be bridged.
Given that we will let accounts balances show the AttoFlow amounts, we need to make sure the underlying data structure can have the precision needed. We still need to protect users when calling withdraw with a balance that could result in rounding off errors.
To do so we are exposing new utility functions on the Balance method and we are adding this protection to the withdrawal request.
We are protecting users from FLOW loss during operations though maybe for the Cadence side, we protect the value from access and let users call a method called
castToUFix64or something similar to know about the rounding error that could happen.