Check minting account during token inspection#8590
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
📝 WalkthroughWalkthroughTransaction bodies now produce deduplicated signer lists, and inspector entry points pass those signers through inspection. Token diff inspection uses signer-aware source/sink definitions, records mint allow-list violations, updates default token mappings, and adds tests for the new result fields. ChangesSigner-aware inspection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fvm/inspection/token_changes.go (1)
833-863: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRestore the default
FlowToken.TokensBurnedmapping.
DefaultTokenDiffSearchTokens()now registersTokensMintedplus the EVM bridge events, but notFlowToken.TokensBurned. A normal burn will therefore missfindSourcesSinks()and show up as a negative unaccounted supply delta under the default configuration.Proposed fix
func DefaultTokenDiffSearchTokens(chain flow.Chain) TokenChangesSearchTokens { sc := systemcontracts.SystemContractsForChain(chain.ChainID()) flowTokenID := fmt.Sprintf("A.%s.FlowToken.Vault", sc.FlowToken.Address.Hex()) flowTokenMintedEventID := fmt.Sprintf("A.%s.FlowToken.TokensMinted", sc.FlowToken.Address.Hex()) + flowTokenBurnedEventID := fmt.Sprintf("A.%s.FlowToken.TokensBurned", sc.FlowToken.Address.Hex()) @@ searchTokens[flowTokenID].SinksSources[flowTokenMintedEventID] = SourceSink{ Amount: decodeFlowEventAmount(flowAmountSource), MinterAllowlist: map[flow.Address]struct{}{ chain.ServiceAddress(): {}, }, } + searchTokens[flowTokenID].SinksSources[flowTokenBurnedEventID] = SourceSink{ + Amount: decodeFlowEventAmount(flowAmountSink), + }This is based on the PR objective that FlowToken mint and burn events remain accounted for while adding mint allow-list enforcement.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fvm/inspection/token_changes.go` around lines 833 - 863, The default token search setup in DefaultTokenDiffSearchTokens is missing the FlowToken burn event mapping, so normal burns are not being accounted for. Update the SearchToken configuration for flowTokenID in token_changes.go to also register FlowToken.TokensBurned alongside TokensMinted and the EVM bridge events, using the same burn amount source/sink handling as before. Keep the mint allow-list enforcement on TokensMinted intact while restoring burn detection so findSourcesSinks can classify standard burn transactions correctly.
🧹 Nitpick comments (1)
fvm/fvm_test.go (1)
4547-4623: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a burn-path case for the default token definitions.
The new assertions only exercise mint flows. A transaction that emits
FlowToken.TokensBurnedand asserts zero unaccounted tokens here would lock in the default burn accounting path and catch regressions inDefaultTokenDiffSearchTokens().This is based on the PR objective that burn accounting remains part of the inspector behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fvm/fvm_test.go` around lines 4547 - 4623, Add a burn-path test case alongside the existing mint cases in the TokenDiff inspector table, using the default token definitions from DefaultTokenDiffSearchTokens and a transaction that emits FlowToken.TokensBurned. Reuse the same txBody pattern with templates.GenerateMintFlowScript-style setup replaced by the burn flow, and in resultChecker assert zero unaccounted tokens and the expected token changes/violations so the default burn accounting path is covered by this test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@fvm/inspection/inspector.go`:
- Around line 18-20: Update the signer contract docs in Inspector to match
TransactionBody.Signers() and the inspector inputs: the current comment
under-documents the returned slice by omitting the proposer. Revise the doc
comment near the inspector logic to state that signers are the proposer, payer,
and authorizers, deduplicated, and keep it aligned with
model/flow/transaction.go and TransactionBody.Signers().
---
Outside diff comments:
In `@fvm/inspection/token_changes.go`:
- Around line 833-863: The default token search setup in
DefaultTokenDiffSearchTokens is missing the FlowToken burn event mapping, so
normal burns are not being accounted for. Update the SearchToken configuration
for flowTokenID in token_changes.go to also register FlowToken.TokensBurned
alongside TokensMinted and the EVM bridge events, using the same burn amount
source/sink handling as before. Keep the mint allow-list enforcement on
TokensMinted intact while restoring burn detection so findSourcesSinks can
classify standard burn transactions correctly.
---
Nitpick comments:
In `@fvm/fvm_test.go`:
- Around line 4547-4623: Add a burn-path test case alongside the existing mint
cases in the TokenDiff inspector table, using the default token definitions from
DefaultTokenDiffSearchTokens and a transaction that emits
FlowToken.TokensBurned. Reuse the same txBody pattern with
templates.GenerateMintFlowScript-style setup replaced by the burn flow, and in
resultChecker assert zero unaccounted tokens and the expected token
changes/violations so the default burn accounting path is covered by this test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: df3a1512-9e77-4bbf-9e22-d6e3183d021b
📒 Files selected for processing (8)
cmd/util/cmd/inspect-token-movements/storage.gofvm/fvm.gofvm/fvm_test.gofvm/inspection/inspector.gofvm/inspection/token_changes.gomodel/flow/transaction.gomodel/flow/transaction_body_builder.gomodel/flow/transaction_test.go
| // - signers are the accounts that signed the procedure's transaction (its | ||
| // authorizers and payer), deduplicated. It is empty for procedures that are | ||
| // not transactions (e.g. scripts). |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Include the proposer in the signer contract docs.
TransactionBody.Signers() returns proposer, payer, then authorizers, so this comment currently under-documents the slice that inspectors actually receive. Cross-file, this mismatches model/flow/transaction.go.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@fvm/inspection/inspector.go` around lines 18 - 20, Update the signer contract
docs in Inspector to match TransactionBody.Signers() and the inspector inputs:
the current comment under-documents the returned slice by omitting the proposer.
Revise the doc comment near the inspector logic to state that signers are the
proposer, payer, and authorizers, deduplicated, and keep it aligned with
model/flow/transaction.go and TransactionBody.Signers().
|
|
||
| // If an allow-list is configured for this event, the transaction must be | ||
| // signed by at least one allow-listed account; otherwise record a violation. | ||
| if ts.ss.MinterAllowlist != nil && !anySignerAllowed(signers, ts.ss.MinterAllowlist) { |
There was a problem hiding this comment.
Question 1:
Is the MinterAllowlist deterministic (not configurable by node, because node might use different config)?
Question 2:
Is the MinterAllowlist currently only Service Account? If we track other non-Flow token, does the allow list for those tokens be different from service account?
3 participants
The FVM token-changes inspector accounts for FlowToken mints/burns via events, but did not check who triggered a mint. This PR adds an optional per-event minter allow-list: a mint observed in a transaction not signed by any allow-listed account is recorded as a violation and logged at error level.
By default, FlowToken minting is restricted to the service account (e.g. the system transaction paying epoch staking rewards); any other minter is flagged.
Summary by CodeRabbit
New Features
Bug Fixes