Truncate amount on COA.withdraw call to maximum Flow token vault precision#6877
Truncate amount on COA.withdraw call to maximum Flow token vault precision#6877
COA.withdraw call to maximum Flow token vault precision#6877Conversation
|
cc @sisyphusSmiling Can you take a look at this approach, and the added test cases? |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
m-Peter
force-pushed
the
mpeter/withdraw-balance-with-rounding
branch
2 times, most recently
from
9d80c61 to
1f5fbae
Compare
|
Thanks for the changes @m-Peter! The test cases and behavior look good to me, just left one question. |
m-Peter
force-pushed
the
mpeter/withdraw-balance-with-rounding
branch
from
1f5fbae to
f0e23ff
Compare
m-Peter
requested review from
a team,
janezpodhostnik and
zhangchiqing
as code owners
m-Peter
force-pushed
the
mpeter/withdraw-balance-with-rounding
branch
from
f0e23ff to
79ac35e
Compare
fvm/evm/emulator/emulator.go
Outdated
| value, overflow := uint256.FromBig(input) | ||
| if overflow { | ||
| return true, nil | ||
| return false, nil |
There was a problem hiding this comment.
was this a bug before?
There was a problem hiding this comment.
This seems to be a bug indeed. From the function, before the change:
func convertAndCheckValue(input *big.Int) (isValid bool, converted *uint256.Int) {
// check for negative input
if input.Sign() < 0 {
return false, nil
}
// convert value into uint256
value, overflow := uint256.FromBig(input)
if overflow {
return true, nil
}
return true, value
}We would return true, nil in case of an overflow. This means that the input was found to be valid, however, the converted value is nil, which could result in a possible nil pointer dereference error.
fvm/evm/emulator/emulator.go
Outdated
|
|
||
| // check balance is not prone to rounding error | ||
| if types.BalanceConversionToUFix64ProneToRoundingError(call.Value) { | ||
| if !types.AttoFlowBalanceValidForFlowVault(call.Value) { |
There was a problem hiding this comment.
Is this change needed, since we already changed the behavior in newInternalEVMTypeWithdrawFunction so that we are guaranteed that the value will be rounded to UFix precision?
Is there another path we want to cover with this change?
There was a problem hiding this comment.
The newInternalEVMTypeWithdrawFunction affects only the InternalEVM.withdraw function(https://github.com/onflow/flow-go/blob/master/fvm/evm/stdlib/contract.cdc#L454C26-L457), which is currently being used by CadenceOwnedAccount.withdraw. However, the BlockView.DirectCall might possibly get its input from somewhere else, e.g. the flow-go side or whatever. And since the same check was performed here, regarding the rounding error, I am inclined to leave the same check in as a redundancy, just in case.
| if err != nil { | ||
| panic(err) | ||
| } | ||
| if roundedOff { |
There was a problem hiding this comment.
I know this can now not happen, but I would leave the check in as a redundancy.
m-Peter
force-pushed
the
mpeter/withdraw-balance-with-rounding
branch
from
79ac35e to
fdb9cbd
Compare
fvm/evm/impl/impl.go
Outdated
| ufix, roundedOff, err := types.ConvertBalanceToUFix64(vault.Balance()) | ||
| // We have already truncated the remainder above, so we do not | ||
| // care about rounding loss here. | ||
| ufix, _, err := types.ConvertBalanceToUFix64(vault.Balance()) |
There was a problem hiding this comment.
Maybe I missed something, I'm wondering why do we need to do this rounding for the caller here? Since it's the caller who is sending the amount value, the caller is capable to do the conversion themselves and pass in a correct attoFlow amount to the COA.withdraw method that will have no rounding issue. Why don't we provide them the necessary function for the caller to do the correct conversion, and be strict here?
There was a problem hiding this comment.
That adds a bit of overhead to the developer. A Flow Vault can have a maximum of UFix64 precision. While in Solidity, the Ether precision is 1e18. So an amount of 1230000780000000000, can be truncated under the hood, to 1.23000078, making the developer's life easier, instead of adding an additional step for proper conversion etc, which can cause confusion. See more in #6856, for the original use-case.
m-Peter
force-pushed
the
mpeter/withdraw-balance-with-rounding
branch
from
30a58c8 to
928491d
Compare
m-Peter
requested review from
janezpodhostnik,
sisyphusSmiling and
zhangchiqing
zhangchiqing
left a comment
zhangchiqing
left a comment
There was a problem hiding this comment.
Looks good. Just one more question.
fvm/evm/stdlib/contract.cdc
Outdated
|
|
||
| /// Withdraws the balance from the cadence owned account's balance | ||
| /// Note that amounts smaller than 10nF (10e-8) can't be withdrawn | ||
| /// Note that amounts smaller than 1e10 attoFlow can't be withdrawn |
There was a problem hiding this comment.
| /// Note that amounts smaller than 1e10 attoFlow can't be withdrawn | |
| /// Note that amounts smaller than 1e10 attoFlow can't be withdrawn | |
| /// In other words, the smallest withdrawable balance is 1e10 attoFlow. |
| let cadenceOwnedAccount <- EVM.createCadenceOwnedAccount() | ||
| cadenceOwnedAccount.deposit(from: <-vault) | ||
|
|
||
| let bal = EVM.Balance(attoflow: 9999999999) |
There was a problem hiding this comment.
👍
| let bal = EVM.Balance(attoflow: 9999999999) | |
| // since 1e10 attoFlow is the minimum withdraw amount, | |
| // verify any amount below 1e10 can not be withdrew. | |
| let bal = EVM.Balance(attoflow: 9999999999) |
fvm/evm/stdlib/contract.cdc
Outdated
| /// If the given balance conversion to UFix64 results in | ||
| /// rounding error, this function would fail. | ||
| /// rounding loss, the withdrawal amount will be truncated to the | ||
| /// maximum precision for UFix64. |
There was a problem hiding this comment.
We need to mention the ErrWithdrawBalanceRounding case, otherwise, it would look like the transaction will succeed without withdraw any amount.
fvm/evm/impl/impl.go
Outdated
| if !types.AttoFlowBalanceValidForFlowVault(amountValue.BigInt) { | ||
| panic(types.ErrWithdrawBalanceRounding) | ||
| } |
There was a problem hiding this comment.
Why handling this case and returning error here? If I'm withdrawing 1e10 - 1 attoFlow, what about we still let the transaction succeed, but don't withdraw any token?
There was a problem hiding this comment.
There are certain issues if we allow this. The following DirectCall will be successfully executed:
DirectCall{
Type: 255
SubType: 2
From: 0x000000000000000000000002fa6e6E9775532ee0
To: 0x0000000000000000000000010000000000000000
Data: []
Value: 0
GasLimit: 23300
Nonce: 1
}Having both Data & Value as empty ([] & 0) is counter-intuitive in EVM world. Because we allowed the formation of an EVM transaction that basically did nothing. It didn't make any contract call (Data is []) and it didn't transfer any funds (Value is 0). This most likely will confuse users and might cause issues with 3rd party tools.
What's more, by allowing the transaction to succeed, we will emit the following events:
A.f8d6e0586b0a20c7.EVM.TransactionExecuted(
hash: [186, 16, 205, 45, 27, 186, 227, 21, 23, 2, 228, 248, 112, 77, 217, 82, 233, 185, 19, 163, 246, 144, 21, 88, 36, 94, 37, 94, 212, 27, 79, 83],
index: 2,
type: 255,
payload: [255, 243, 129, 255, 2, 148, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 2, 250, 110, 110, 151, 117, 83, 46, 224, 148, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 1, 0, 0, 0, 0, 0, 0, 0, 0, 128, 128, 130, 91, 4, 1],
errorCode: 0,
errorMessage: "",
gasConsumed: 21000,
contractAddress: "",
logs: [],
blockHeight: 1,
returnedData: [],
precompiledCalls: [],
stateUpdateChecksum: [44, 51, 111, 163]
)
A.f8d6e0586b0a20c7.EVM.FLOWTokensWithdrawn(
address: "000000000000000000000002fa6e6e9775532ee0",
amount: 0.00000000,
withdrawnUUID: 56075093016609,
balanceAfterInAttoFlow: 2340000000000000000
)
There was a problem hiding this comment.
Does coa.withdraw(0) work now?
There was a problem hiding this comment.
That didn't work even before: https://github.com/onflow/flow-go/blob/master/fvm/evm/stdlib/contract.cdc#L451-L453 . I just tried to describe what would happen, if we allowed the scenario that Leo mentioned.
There was a problem hiding this comment.
OK, sounds good. So just to summarize:
- coa.withdraw(0) will fail, it's the same behavior as before
- coa.withdraw(1e10 - 1) will fail, because 1e10 is the minimum withdrawable amount, it's also the same behavior as before
- coa.withdraw(1e10) will succeed because there is no leftover, and it's the same behavior as before
- coa.withdaw(1e10 + 1) will succeed, but only 1e10 will be withdrew. It's different behavior than before, which would fail.
So the only case we are changing the behavior is the case 4 here.
There was a problem hiding this comment.
That is correct. So basically for 4. only multiples of 1e10 will be withdrawn. For example, if a user withdraws coa.withdraw(78000050000), the actual withdrawn amount after the truncation, is 0.00000007 Flow.
m-Peter
force-pushed
the
mpeter/withdraw-balance-with-rounding
branch
from
aed426c to
f6ceedf
Compare
m-Peter
force-pushed
the
mpeter/withdraw-balance-with-rounding
branch
from
fb35603 to
4669999
Compare
m-Peter
force-pushed
the
mpeter/withdraw-balance-with-rounding
branch
from
4669999 to
1261fc3
Compare
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
m-Peter
force-pushed
the
mpeter/withdraw-balance-with-rounding
branch
from
1261fc3 to
8411458
Compare
m-Peter
changed the title
COA.withdraw call to run when there is a possible rounding errorCOA.withdraw call to maximum Flow token vault precision
📝 WalkthroughWalkthroughAdds minimum-withdrawable validation (1e10 attoFlow) and related rounding checks for EVM withdrawals, refactors an emulator helper return order, updates implementations to use the new validation, and adjusts tests and bootstrap constants to match resulting state commit changes. (50 words) Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI Agents
In @fvm/evm/evm_test.go:
- Line 1873: Update the incorrect numeric literal in the inline comment that
reads "2.34000000 - 0.2300078 = 2.10994922" to use the correct withdrawn amount
"0.23005078" so the comment becomes "2.34000000 - 0.23005078 = 2.10994922";
locate this comment in evm_test.go (near the affected test block) and replace
the typo-only—no code changes required.
In @fvm/evm/types/errors.go:
- Around line 100-103: The doc comment above the ErrWithdrawBalanceRounding
constant is misleading (it says “10^8 Flow”); update that comment to match the
error message by stating fractions smaller than 1e‑8 FLOW (equivalently the
smallest transferable unit is 1e10 attoFlow) or similar wording referencing
ErrWithdrawBalanceRounding so the comment and the string in
ErrWithdrawBalanceRounding are consistent.
🧹 Nitpick comments (4)
fvm/evm/stdlib/contract.cdc (1)
309-309: Minor: Consider consistency in capitalization.The function name is
inAttoFLOW()(all caps for FLOW), but the comment uses mixed case. Consider usinginAttoFLOW()consistently in comments for clarity.fvm/evm/impl/impl.go (1)
660-664: Consider clarifying the redundancy comment.The comment states this is a redundant check, but it's not immediately clear why it should never trigger given the explicit truncation above. Consider expanding the comment to explain that this serves as a defensive assertion to catch any unexpected behavior in the conversion utilities.
Suggested clarification
- // We have already truncated the remainder above, but we still leave - // the rounding check in as a redundancy. + // Defensive check: We have already truncated the remainder above via + // division and UFix64 conversion, so this should never trigger. However, + // we leave it as a redundancy to catch any unexpected behavior in the + // balance conversion utilities.fvm/evm/evm_test.go (2)
1664-1734: Consider using event type matching instead of hard-coded indices.The test logic and arithmetic are correct, but accessing
output.Events[7]directly is fragile. If the number or order of events changes in the transaction, this test will break.Recommended refactor to find events by type
Consider adding a helper function to find events by type:
func findEventByType(events []flow.Event, eventType flow.EventType) (flow.Event, bool) { for _, event := range events { if event.Type == eventType { return event, true } } return flow.Event{}, false }Then use it like:
-withdrawEvent := output.Events[7] +withdrawEvent, found := findEventByType(output.Events, + flow.EventType("A."+sc.EVMContract.Address.Hex()+".EVM.FLOWTokensWithdrawn")) +require.True(t, found, "withdraw event not found")This pattern applies to similar cases at lines 1793, 1865, 2002 as well.
1945-2015: Core truncation behavior correctly tested.This test validates the PR's primary objective: withdrawal amounts with precision beyond UFix64 (8 decimals) are truncated without error. The arithmetic is correct, and the test confirms that 9,912,345,678 attoFlow (the remainder beyond 8 decimal places) is silently discarded.
Optional enhancement: Consider adding an assertion that the truncated remainder is not stored in any account balance or emitted in events, to ensure no leakage.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
engine/execution/state/bootstrap/bootstrap_test.gofvm/evm/emulator/emulator.gofvm/evm/emulator/emulator_test.gofvm/evm/evm_test.gofvm/evm/handler/handler_test.gofvm/evm/impl/impl.gofvm/evm/stdlib/contract.cdcfvm/evm/stdlib/contract_test.gofvm/evm/types/balance.gofvm/evm/types/errors.goutils/unittest/execution_state.go
🧰 Additional context used
📓 Path-based instructions (7)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/coding_conventions.mdc)
Follow Go coding conventions as documented in @docs/agents/CodingConventions.md
Follow Go coding standards and conventions as documented in @docs/agents/GoDocs.md
**/*.go: Follow the existing module structure in/module/,/engine/,/model/and use dependency injection patterns for component composition
Implement proper interfaces before concrete types
Follow Go naming conventions and the project's coding style defined in /docs/CodingConventions.md
Use mock generators: runmake generate-mocksafter interface changes
All inputs must be considered potentially byzantine; error classification is context-dependent and no code path is safe unless explicitly proven and documented
Use comprehensive error wrapping for debugging; avoidfmt.Errorf, useirrecoverablepackage for exceptions
NEVER log and continue on best effort basis; ALWAYS explicitly handle errors
Uses golangci-lint with custom configurations (.golangci.yml) and custom linters for Flow-specific conventions (struct write checking)
Files:
engine/execution/state/bootstrap/bootstrap_test.gofvm/evm/types/balance.gofvm/evm/stdlib/contract_test.gofvm/evm/handler/handler_test.gofvm/evm/impl/impl.gofvm/evm/types/errors.gofvm/evm/emulator/emulator_test.goutils/unittest/execution_state.gofvm/evm/emulator/emulator.gofvm/evm/evm_test.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Unit tests should be co-located with the code they test
Follow the existing pattern of*_test.gofiles for test naming
Use fixtures for realistic test data as defined in/utils/unittest/
Files:
engine/execution/state/bootstrap/bootstrap_test.gofvm/evm/stdlib/contract_test.gofvm/evm/handler/handler_test.gofvm/evm/emulator/emulator_test.gofvm/evm/evm_test.go
{module,engine,cmd}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
All major processing components must implement the
Componentinterface from/module/component/component.goto ensure consistent lifecycle management and graceful shutdown patterns
Files:
engine/execution/state/bootstrap/bootstrap_test.go
{crypto,fvm,ledger,access,engine}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Files:
engine/execution/state/bootstrap/bootstrap_test.gofvm/evm/types/balance.gofvm/evm/stdlib/contract_test.gofvm/evm/handler/handler_test.gofvm/evm/impl/impl.gofvm/evm/types/errors.gofvm/evm/emulator/emulator_test.gofvm/evm/emulator/emulator.gofvm/evm/evm_test.go
{network,engine,consensus}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Network messages must be authenticated and validated
Files:
engine/execution/state/bootstrap/bootstrap_test.go
{crypto,fvm,ledger,storage}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Security checks for cryptographic misuse must be enforced using gosec
Files:
fvm/evm/types/balance.gofvm/evm/stdlib/contract_test.gofvm/evm/handler/handler_test.gofvm/evm/impl/impl.gofvm/evm/types/errors.gofvm/evm/emulator/emulator_test.gofvm/evm/emulator/emulator.gofvm/evm/evm_test.go
{storage,ledger,execution,fvm}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
State consistency is paramount; use proper synchronization primitives
Files:
fvm/evm/types/balance.gofvm/evm/stdlib/contract_test.gofvm/evm/handler/handler_test.gofvm/evm/impl/impl.gofvm/evm/types/errors.gofvm/evm/emulator/emulator_test.gofvm/evm/emulator/emulator.gofvm/evm/evm_test.go
🧠 Learnings (2)
📚 Learning: 2025-12-23T00:28:41.005Z
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {crypto,fvm,ledger,access,engine}/**/*.go : Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Applied to files:
fvm/evm/stdlib/contract.cdcfvm/evm/impl/impl.go
📚 Learning: 2025-12-23T00:28:41.005Z
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {storage,ledger,execution,fvm}/**/*.go : State consistency is paramount; use proper synchronization primitives
Applied to files:
utils/unittest/execution_state.go
🧬 Code graph analysis (6)
fvm/evm/handler/handler_test.go (1)
fvm/evm/types/balance.go (1)
OneFlow(133-135)
fvm/evm/impl/impl.go (3)
fvm/evm/types/errors.go (2)
ErrInvalidBalance(91-91)ErrWithdrawBalanceRounding(102-102)fvm/evm/types/balance.go (3)
AttoFlowBalanceIsValidForFlowVault(105-107)UFixToAttoConversionMultiplier(16-16)NewBalanceFromUFix64(43-47)utils/rand/rand.go (1)
Uint64(26-35)
fvm/evm/emulator/emulator_test.go (3)
fvm/evm/types/call.go (2)
NewWithdrawCall(213-229)DirectCall(48-65)fvm/evm/types/balance.go (1)
OneFlow(133-135)fvm/evm/types/errors.go (1)
ErrWithdrawBalanceRounding(102-102)
utils/unittest/execution_state.go (1)
model/flow/chain.go (1)
Sandboxnet(26-26)
fvm/evm/emulator/emulator.go (3)
fvm/evm/types/result.go (1)
NewInvalidResult(57-64)fvm/evm/types/errors.go (2)
ErrInvalidBalance(91-91)ErrWithdrawBalanceRounding(102-102)fvm/evm/types/balance.go (1)
AttoFlowBalanceIsValidForFlowVault(105-107)
fvm/evm/evm_test.go (1)
fvm/evm/types/errors.go (2)
ErrWithdrawBalanceRounding(102-102)ErrInvalidBalance(91-91)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (37)
- GitHub Check: Lint (./)
- GitHub Check: Lint (./insecure/)
- GitHub Check: Lint (./integration/)
- GitHub Check: Integration Tests Others (integration)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/scoring)
- GitHub Check: Unit Tests Insecure (insecure)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/rpc_inspector)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (module/dkg)
- GitHub Check: Unit Tests (module)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (engine/execution/ingestion)
- GitHub Check: Unit Tests (network/p2p/node)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (engine/access)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (storage)
- GitHub Check: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (cmd)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (fvm)
- GitHub Check: Unit Tests (engine)
- GitHub Check: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Unit Tests (consensus)
- GitHub Check: Unit Tests (state)
- GitHub Check: Unit Tests (admin)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Unit Tests (ledger)
- GitHub Check: Docker Build
- GitHub Check: analyze-code (go)
🔇 Additional comments (15)
fvm/evm/stdlib/contract_test.go (1)
5193-5213: COA.withdraw rounding test correctly targets truncation semanticsUsing
EVM.Balance(attoflow: 1230000000900000000)and assertingbalance == 1.23000000correctly exercises the new “truncate to UFix64 precision” behavior (1e10 attoFlow per 1e-8 FLOW unit). The values are consistent with the intended rounding rules.engine/execution/state/bootstrap/bootstrap_test.go (1)
59-64: Updated expected state commitments look consistent with regenerated genesisThe new hard-coded state commitment hex values for the zero-supply and empty-transaction bootstrap tests are well-formed and match the pattern of regenerated commitments after contract changes. Just ensure these stay in sync with the corresponding genesis commitments in
utils/unittest/execution_state.gowhenever the EVM/service contracts change.Also applies to: 109-112
fvm/evm/handler/handler_test.go (1)
1176-1186: Using OneFlow in tracing test better exercises real deposit/withdraw pathsSwitching the open-tracing test to deposit and withdraw
OneFlowinstead of an empty balance makes the spans cover actual value transfers and remains compatible with the new minimum‑unit constraints.utils/unittest/execution_state.go (1)
26-27: Genesis commitment constants updated; keep in lockstep with bootstrap testsThe new
GenesisStateCommitmentHexand the updated Testnet/default entries ingenesisCommitHexByChainIDare well-formed and align with the expected need to refresh commitments after contract changes. Just ensure these values remain consistent with the expectations inengine/execution/state/bootstrap/bootstrap_test.goand any other tests that pin genesis commitments.Also applies to: 85-96
fvm/evm/types/balance.go (1)
97-107: LGTM! Clear validation function with accurate documentation.The function correctly enforces the minimum storable balance (1e10 attoFlow) for Flow Vaults, and the documentation clearly explains the precision constraints (1e-8 Flow = 1e10 attoFlow).
fvm/evm/stdlib/contract.cdc (1)
516-523: LGTM! Clear documentation of withdrawal behavior.The documentation accurately describes:
- Minimum withdrawable amount (1e10 attoFlow)
- Panic behavior for amounts below minimum
- Truncation behavior for precision loss
fvm/evm/impl/impl.go (1)
636-648: LGTM! Comprehensive validation and truncation logic.The implementation correctly:
- Checks for uint256 overflow to ensure EVM compatibility
- Validates minimum withdrawable balance (1e10 attoFlow)
- Truncates to UFix64 precision by dividing by the conversion multiplier
The explicit truncation approach (divide then convert to UFix64) ensures deterministic rounding behavior.
fvm/evm/emulator/emulator_test.go (1)
152-225: LGTM! Comprehensive test coverage for withdrawal scenarios.The expanded test suite effectively covers:
- ✅ Happy path: successful withdrawal of standard amounts
- ✅ Minimum threshold: rejection of 1e9, acceptance of 1e10
- ✅ Overflow detection: amounts beyond UInt256 range
- ✅ Large value handling: within-range but insufficient funds
The test assertions correctly validate gas consumption, transaction hashes, nonce progression, and error messages for each scenario.
fvm/evm/emulator/emulator.go (4)
736-747: LGTM! Bug fix for overflow handling.The refactored
checkAndConvertValuefunction correctly fixes the previous bug where overflow would return(true, nil), potentially causing nil pointer dereferences. Now it properly returns(nil, false)on overflow or negative values.The return order change (converted first, then validity flag) is more conventional and aligns with Go idioms.
Based on learnings, past comments confirmed this was indeed a bug in the previous implementation.
384-391: LGTM! Correctly handles invalid values.The
mintTofunction properly uses the refactoredcheckAndConvertValuesignature and returns an invalid result withErrInvalidBalancewhen validation fails.
429-444: LGTM! Consistent validation logic with impl.go.The
withdrawFromfunction correctly:
- Validates and converts the value using
checkAndConvertValue- Enforces the minimum withdrawable amount using
AttoFlowBalanceIsValidForFlowVault- Returns appropriate
InvalidResultwith specific error messagesThis aligns with the validation logic in
fvm/evm/impl/impl.gofor consistency across withdrawal paths.
485-492: LGTM! Correctly refactored with clearer variable naming.The
deployAtfunction properly handles the newcheckAndConvertValuesignature, and the renamed variablecastedValue(vs previousvalue) improves clarity by distinguishing the converted uint256 value from the originalcall.Value.fvm/evm/evm_test.go (3)
1597-1662: LGTM! Rounding error test correctly validates minimum withdrawal threshold.The test properly verifies that withdrawal amounts below 1e10 attoFlow (the minimum transferable unit) fail with
ErrWithdrawBalanceRounding. The boundary value of 9999999999 attoFlow is correct.
1736-1806: Test arithmetic is correct.The withdrawal amount and balance calculations are accurate. The test properly validates a successful withdrawal scenario.
1880-1943: Excellent overflow protection test.This test correctly validates that withdrawal amounts exceeding uint256 bounds are rejected with
ErrInvalidBalance, preventing potential overflow vulnerabilities.
| evPayload, err := events.DecodeFLOWTokensWithdrawnEventPayload(ev) | ||
| require.NoError(t, err) | ||
|
|
||
| // 2.34000000 - 0.2300078 = 2.10994922 |
There was a problem hiding this comment.
Fix typo in comment.
The comment has an incorrect value. The withdrawn amount is 0.23005078, not 0.2300078.
Proposed fix
- // 2.34000000 - 0.2300078 = 2.10994922
+ // 2.34000000 - 0.23005078 = 2.10994922📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // 2.34000000 - 0.2300078 = 2.10994922 | |
| // 2.34000000 - 0.23005078 = 2.10994922 |
🤖 Prompt for AI Agents
In @fvm/evm/evm_test.go at line 1873, Update the incorrect numeric literal in
the inline comment that reads "2.34000000 - 0.2300078 = 2.10994922" to use the
correct withdrawn amount "0.23005078" so the comment becomes "2.34000000 -
0.23005078 = 2.10994922"; locate this comment in evm_test.go (near the affected
test block) and replace the typo-only—no code changes required.
| // ErrWithdrawBalanceRounding is returned when withdraw call has a balance that could | ||
| // result in rounding error, i.e. the balance contains fractions smaller than 10^8 Flow (smallest unit allowed to transfer). | ||
| ErrWithdrawBalanceRounding = errors.New("withdraw failed! the balance is susceptible to the rounding error") | ||
| ErrWithdrawBalanceRounding = errors.New("withdraw failed! smallest unit allowed to transfer is 1e10 attoFlow") | ||
|
|
There was a problem hiding this comment.
Error message aligns with new minimum unit; doc comment is now misleading
The updated message for ErrWithdrawBalanceRounding correctly documents the minimum transferable unit as 1e10 attoFlow (i.e., 1e−8 FLOW). However, the comment above still says “fractions smaller than 10^8 Flow”, which reads as 100M FLOW and is misleading.
Consider correcting the comment to something like “fractions smaller than 1e‑8 FLOW (smallest unit allowed to transfer)” to keep documentation consistent with the code.
🤖 Prompt for AI Agents
In @fvm/evm/types/errors.go around lines 100 - 103, The doc comment above the
ErrWithdrawBalanceRounding constant is misleading (it says “10^8 Flow”); update
that comment to match the error message by stating fractions smaller than 1e‑8
FLOW (equivalently the smallest transferable unit is 1e10 attoFlow) or similar
wording referencing ErrWithdrawBalanceRounding so the comment and the string in
ErrWithdrawBalanceRounding are consistent.
m-Peter
force-pushed
the
mpeter/withdraw-balance-with-rounding
branch
from
bb0bbfb to
c30bf72
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
fvm/evm/impl/impl.go (1)
660-664: Consider removing redundant rounding check.The comment correctly notes this check is redundant since truncation already happened at lines 647-648. The early validation ensures
roundedOffwill always be false here.Optional: Remove redundant check
- ufix, roundedOff, err := types.ConvertBalanceToUFix64(vault.Balance()) + ufix, _, err := types.ConvertBalanceToUFix64(vault.Balance()) if err != nil { panic(err) } - // We have already truncated the remainder above, but we still leave - // the rounding check in as a redundancy. - if roundedOff { - panic(types.ErrWithdrawBalanceRounding) - }Note: Keeping the redundant check provides defense-in-depth, so this is optional.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
engine/execution/state/bootstrap/bootstrap_test.gofvm/evm/emulator/emulator.gofvm/evm/emulator/emulator_test.gofvm/evm/evm_test.gofvm/evm/handler/handler_test.gofvm/evm/impl/impl.gofvm/evm/stdlib/contract.cdcfvm/evm/stdlib/contract_test.gofvm/evm/types/balance.gofvm/evm/types/errors.goutils/unittest/execution_state.go
✅ Files skipped from review due to trivial changes (2)
- fvm/evm/stdlib/contract.cdc
- fvm/evm/emulator/emulator.go
🚧 Files skipped from review as they are similar to previous changes (4)
- fvm/evm/stdlib/contract_test.go
- fvm/evm/handler/handler_test.go
- fvm/evm/emulator/emulator_test.go
- fvm/evm/types/balance.go
🧰 Additional context used
📓 Path-based instructions (7)
**/*.go
📄 CodeRabbit inference engine (.cursor/rules/coding_conventions.mdc)
Follow Go coding conventions as documented in @docs/agents/CodingConventions.md
Follow Go coding standards and conventions as documented in @docs/agents/GoDocs.md
**/*.go: Follow the existing module structure in/module/,/engine/,/model/and use dependency injection patterns for component composition
Implement proper interfaces before concrete types
Follow Go naming conventions and the project's coding style defined in /docs/CodingConventions.md
Use mock generators: runmake generate-mocksafter interface changes
All inputs must be considered potentially byzantine; error classification is context-dependent and no code path is safe unless explicitly proven and documented
Use comprehensive error wrapping for debugging; avoidfmt.Errorf, useirrecoverablepackage for exceptions
NEVER log and continue on best effort basis; ALWAYS explicitly handle errors
Uses golangci-lint with custom configurations (.golangci.yml) and custom linters for Flow-specific conventions (struct write checking)
Files:
engine/execution/state/bootstrap/bootstrap_test.goutils/unittest/execution_state.gofvm/evm/impl/impl.gofvm/evm/evm_test.gofvm/evm/types/errors.go
**/*_test.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*_test.go: Unit tests should be co-located with the code they test
Follow the existing pattern of*_test.gofiles for test naming
Use fixtures for realistic test data as defined in/utils/unittest/
Files:
engine/execution/state/bootstrap/bootstrap_test.gofvm/evm/evm_test.go
{module,engine,cmd}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
All major processing components must implement the
Componentinterface from/module/component/component.goto ensure consistent lifecycle management and graceful shutdown patterns
Files:
engine/execution/state/bootstrap/bootstrap_test.go
{crypto,fvm,ledger,access,engine}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Files:
engine/execution/state/bootstrap/bootstrap_test.gofvm/evm/impl/impl.gofvm/evm/evm_test.gofvm/evm/types/errors.go
{network,engine,consensus}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Network messages must be authenticated and validated
Files:
engine/execution/state/bootstrap/bootstrap_test.go
{crypto,fvm,ledger,storage}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
Security checks for cryptographic misuse must be enforced using gosec
Files:
fvm/evm/impl/impl.gofvm/evm/evm_test.gofvm/evm/types/errors.go
{storage,ledger,execution,fvm}/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
State consistency is paramount; use proper synchronization primitives
Files:
fvm/evm/impl/impl.gofvm/evm/evm_test.gofvm/evm/types/errors.go
🧠 Learnings (2)
📚 Learning: 2025-12-23T00:28:41.005Z
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {storage,ledger,execution,fvm}/**/*.go : State consistency is paramount; use proper synchronization primitives
Applied to files:
utils/unittest/execution_state.go
📚 Learning: 2025-12-23T00:28:41.005Z
Learnt from: CR
Repo: onflow/flow-go PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-23T00:28:41.005Z
Learning: Applies to {crypto,fvm,ledger,access,engine}/**/*.go : Cryptographic operations require careful handling; refer to crypto library documentation for proper implementation
Applied to files:
fvm/evm/impl/impl.go
🧬 Code graph analysis (3)
utils/unittest/execution_state.go (1)
model/flow/chain.go (1)
Sandboxnet(26-26)
fvm/evm/impl/impl.go (2)
fvm/evm/types/errors.go (2)
ErrInvalidBalance(91-91)ErrWithdrawBalanceRounding(102-102)fvm/evm/types/balance.go (3)
AttoFlowBalanceIsValidForFlowVault(105-107)UFixToAttoConversionMultiplier(16-16)NewBalanceFromUFix64(43-47)
fvm/evm/evm_test.go (2)
utils/unittest/fixtures/util.go (1)
NoError(6-10)fvm/evm/types/errors.go (2)
ErrWithdrawBalanceRounding(102-102)ErrInvalidBalance(91-91)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (37)
- GitHub Check: Lint (./insecure/)
- GitHub Check: Lint (./integration/)
- GitHub Check: Lint (./)
- GitHub Check: Unit Tests (engine/execution)
- GitHub Check: Unit Tests (engine/execution/ingestion)
- GitHub Check: Unit Tests (network/p2p)
- GitHub Check: Unit Tests (module)
- GitHub Check: Unit Tests (network/test/cohort2)
- GitHub Check: Unit Tests (utils)
- GitHub Check: Unit Tests (module/dkg)
- GitHub Check: Unit Tests (others)
- GitHub Check: Unit Tests (engine)
- GitHub Check: Unit Tests (engine/verification)
- GitHub Check: Unit Tests (engine/common)
- GitHub Check: Unit Tests (network/p2p/node)
- GitHub Check: Unit Tests (network)
- GitHub Check: Unit Tests (network/test/cohort1)
- GitHub Check: Unit Tests (engine/execution/computation)
- GitHub Check: Unit Tests (network/p2p/connection)
- GitHub Check: Unit Tests (network/p2p/scoring)
- GitHub Check: Unit Tests (network/alsp)
- GitHub Check: Unit Tests (engine/consensus)
- GitHub Check: Unit Tests (cmd)
- GitHub Check: Unit Tests (ledger)
- GitHub Check: Unit Tests (admin)
- GitHub Check: Unit Tests (state)
- GitHub Check: Unit Tests (fvm)
- GitHub Check: Unit Tests (storage)
- GitHub Check: Unit Tests (engine/collection)
- GitHub Check: Unit Tests (engine/access)
- GitHub Check: Unit Tests (consensus)
- GitHub Check: Integration Tests Others (integration)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/scoring)
- GitHub Check: Unit Tests Insecure (insecure)
- GitHub Check: Unit Tests Insecure (insecure/integration/functional/test/gossipsub/rpc_inspector)
- GitHub Check: analyze-code (go)
- GitHub Check: Docker Build
🔇 Additional comments (12)
fvm/evm/types/errors.go (1)
102-102: LGTM! Clear error message for minimum transferable amount.The error message now explicitly states the minimum transferable unit (1e10 attoFlow), which corresponds to UFix64's smallest representable value (10^-8 FLOW). This provides clear guidance to users when withdrawal amounts fall below the precision threshold.
engine/execution/state/bootstrap/bootstrap_test.go (1)
61-61: LGTM! Test expectations updated for new withdrawal behavior.The expected state commitment values in these bootstrap tests have been updated to reflect the changes in withdrawal validation logic. These values are consistent with the updates in
utils/unittest/execution_state.go.Also applies to: 110-110
fvm/evm/impl/impl.go (3)
636-639: Overflow check prevents invalid balance panics.The uint256 overflow check ensures that withdrawal amounts exceeding the maximum uint256 value are rejected early with
ErrInvalidBalance. This is a necessary safety check for the subsequent conversion operations.
642-644: Minimum withdrawable amount validation is correctly implemented.The validation using
AttoFlowBalanceIsValidForFlowVaultensures that only amounts >= 1e10 attoFlow (the minimum UFix64 precision) can be withdrawn. This prevents rounding errors and aligns with the PR objective.
646-648: Verify truncation behavior preserves correct balance semantics.The conversion logic:
- Divides
amountValue.BigIntbyUFixToAttoConversionMultiplier(10^10)- Constructs a new balance by multiplying back by the same factor via
NewBalanceFromUFix64This effectively truncates any remainder below 1e10 attoFlow. However, this means the actual withdrawn amount may be less than requested.
Verify that this truncation behavior is documented and understood by callers. Consider:
- Should callers be notified of the truncated amount?
- Are there any event emissions that should reflect the actual withdrawn amount vs. requested?
#!/bin/bash # Description: Check if withdrawal event payloads correctly reflect truncated amounts # Search for withdrawal event emission and payload construction ast-grep --pattern $'account.Withdraw($$$)' # Check for related event definitions rg -n "Withdraw.*Event" --type=gofvm/evm/evm_test.go (6)
1597-1662: Comprehensive test for below-minimum withdrawal.The test correctly verifies that withdrawing an amount below the minimum (9999999999 < 1e10 attoFlow) results in
ErrWithdrawBalanceRounding. The test properly executes the transaction and asserts the error.
1664-1734: Verify minimum withdrawal amount and resulting balance.The test validates withdrawal of exactly the minimum amount (1e10 attoFlow = 0.00000001 FLOW) and checks the resulting balance via event payload.
Expected calculation:
- Initial: 2.34000000 FLOW = 2,340,000,000,000,000,000 attoFlow
- Withdraw: 0.00000001 FLOW = 10,000,000,000 attoFlow
- Remaining: 2.33999999 FLOW = 2,339,999,990,000,000,000 attoFlow
The assertion at line 1730 correctly expects
2_339_999_990_000_000_000attoFlow after withdrawal.
1736-1806: Verify balance calculation for standard withdrawal.The test withdraws 1.23000078 FLOW from an initial balance of 2.34000000 FLOW.
Expected calculation:
- Initial: 2,340,000,000,000,000,000 attoFlow
- Withdraw: 1,230,000,780,000,000,000 attoFlow
- Remaining: 1,109,999,220,000,000,000 attoFlow
The assertion at line 1802 correctly expects the remaining balance.
1808-1878: Verify balance calculation for fractional withdrawal.The test withdraws 0.23005078 FLOW (a fraction-only amount).
Expected calculation:
- Initial: 2,340,000,000,000,000,000 attoFlow
- Withdraw: 230,050,780,900,000,000 attoFlow (note: has extra precision)
- Remaining should be: 2,340,000,000,000,000,000 - 230,050,780,000,000,000 = 2,109,949,220,000,000,000
Wait, line 1836 shows withdrawal of
230050780900000000but the comment at line 1873 says remaining is2,109,949,220,000,000,000. Let me recalculate:
- 2,340,000,000,000,000,000 - 230,050,780,900,000,000 = 2,109,949,219,100,000,000
But the comment says: "2.34000000 - 0.2300078 = 2.10994922"
The value at line 1836 is: 230050780900000000 (0.23005078090... FLOW)
After truncation to UFix64: 0.23005078 FLOW = 230,050,780,000,000,000 attoFlowSo the remaining should be: 2,340,000,000 - 230,050,780 = 2,109,949,220 in UFix64 units
= 2,109,949,220,000,000,000 attoFlowThe assertion is correct. The input has extra precision that gets truncated.
1880-1943: Correct validation for uint256 overflow.The test uses a value greater than max uint256 (2^256) to verify that the overflow check at line 636-639 in
impl.gocorrectly rejects it withErrInvalidBalance.The value at line 1908:
115792089237316195423570985008687907853269984665640564039457584007913129639936
= 2^256 (max uint256 + 1)This correctly tests the overflow protection.
1945-2015: Verify remainder truncation behavior.The test withdraws an amount with extra precision that should be truncated:
- Input: 1,230,000,789,912,345,678 attoFlow (line 1973)
- After truncation: 1,230,000,780,000,000,000 attoFlow (1.23000078 FLOW)
- Remaining: 2.34 - 1.23000078 = 1.10999922 FLOW = 1,109,999,220,000,000,000 attoFlow
The test correctly validates that the remainder (9,912,345,678 attoFlow) is truncated and the withdrawal succeeds with the truncated amount. The assertion at line 2012 confirms the expected remaining balance.
utils/unittest/execution_state.go (1)
26-26: These genesis state commitment values are validated by theTestBootstrapLedgertest, which will fail if they don't match the actual bootstrap output.The constants at lines 26, 90, and 95 are pre-calculated state commitments used as test fixtures.
TestBootstrapLedger(inengine/execution/state/bootstrap/bootstrap_test.go) automatically verifies that the Mainnet value matches the actual bootstrap ledger state. If the bootstrap logic changes and these values no longer match, the test will catch the mismatch and needs to be updated accordingly. No manual verification beyond running the existing test suite is required.
7 participants
Closes #6856
In this case, we perform the withdrawal, by simply truncating the remainder that cannot fit in a Flow token vault.
When the attoFlow balance is too small to fit in a Flow vault, we panic with a dedicated error message.
Summary by CodeRabbit
Bug Fixes
Tests
Documentation
Chores
✏️ Tip: You can customize this high-level summary in your review settings.