[FVM] Add token movements inspection#8424
Open
janezpodhostnik wants to merge 6 commits intomasterfrom
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review infoConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds an inspection framework and a TokenChanges inspector to the FVM, wires inspectors into the VM Context and ProcedureOutput, exposes a token-tracking ExecutionConfig flag and DefaultFVMOptions parameter, logs inspection results, updates call sites and tests, and updates a few module versions. Changes
Sequence DiagramsequenceDiagram
participant Client
participant Manager as FVM_Manager
participant VM as VirtualMachine
participant Inspector as TokenChangesInspector
participant Logger as ResultLogger
Client->>Manager: submit procedure/transaction (tokenTracking enabled)
Manager->>Manager: build Context with Inspectors (WithInspectors)
Manager->>VM: Run procedure with Context
VM->>VM: execute procedure, emit events, capture snapshots
VM->>Inspector: Inspect(preState, postSnapshot, events)
Inspector->>Inspector: read ledgers, traverse Cadence values, compute diffs
Inspector->>VM: return []inspection.Result
VM->>Manager: return ProcedureOutput + InspectionResults
Manager->>Logger: logInspectionResults(InspectionResults)
Logger->>Client: emit aggregated inspection log events
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 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 |
Contributor
Dependency ReviewThe following issues were found:
License Issuesgo.mod
insecure/go.mod
integration/go.mod
OpenSSF Scorecard
Scanned Files
|
janezpodhostnik
force-pushed
the
janez/transaction-result-inspection
branch
from
63b3786 to
1b53bdc
Compare
janezpodhostnik
force-pushed
the
janez/transaction-result-inspection
branch
from
1b53bdc to
5917d36
Compare
6 tasks
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
janezpodhostnik
force-pushed
the
janez/transaction-result-inspection
branch
from
9f63f22 to
946dd3f
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (11)
fvm/inspection/token_changes.go (4)
60-84: Inspect: panic recovery catches all panics including programming errors.The
deferon Lines 72-80 catches any panic (e.g., nil pointer dereferences from bugs) and converts it to an error. While this is defensible for a diagnostic inspector that must not crash the node, it can mask real bugs during development. Consider logging the panic stack trace before wrapping it as an error, so issues are visible in logs.Proposed improvement
defer func() { if r := recover(); r != nil { - err = fmt.Errorf("panic: %v", r) + err = fmt.Errorf("panic in token inspection: %v\n%s", r, debug.Stack()) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/inspection/token_changes.go` around lines 60 - 84, The defer in TokenChanges.Inspect currently converts any panic into an error, which can hide programming bugs; modify the panic-recovery block in Inspect to capture the panic value and also log the stack trace (e.g., via runtime/debug.Stack() or your logger) before wrapping it as an error so developers can see the full panic context during development; keep the existing behavior of returning a non-panicking error from getTokenDiff/getSearchedTokensRef, but ensure the log call includes the panic value and stack trace for easier debugging.
414-423:allTouchedRegisters()allocates a new map on every call.This method is called multiple times: once in
getTokenDiff(Line 99), once inexecutionSnapshotLedgersNew.ForEach(Line 395), and once inexecutionSnapshotLedgersNew.Count(Line 410). Each call allocates and populates a new map. Consider caching the result or computing it once.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/inspection/token_changes.go` around lines 414 - 423, allTouchedRegisters currently builds a new map on every call which wastes allocations; fix by computing it once and reusing it: add a cached field (e.g., touchedRegisters map[flow.RegisterID]struct{} or touchedRegistersOnce flag + map) to the executionSnapshotLedgers struct and populate that map lazily either in executionSnapshotLedgersNew (constructor) or on the first call to allTouchedRegisters(); then have allTouchedRegisters return the cached map on subsequent calls so callers like getTokenDiff, executionSnapshotLedgersNew.ForEach, and executionSnapshotLedgers.Count reuse the single allocation.
647-654: Unresolved TODOs on storage domain coverage.Lines 651-653 contain TODO comments questioning whether
StorageDomainPathPrivateandStorageDomainInboxare needed, plus a broader "do we need any other?" question. Private paths were deprecated in Cadence 1.0. Consider resolving these before merging, or at minimum filing issues to track them.Would you like me to open an issue to track these domain coverage questions?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/inspection/token_changes.go` around lines 647 - 654, The tokenDiffSearchDomains slice includes ambiguous entries (StorageDomainPathPrivate and StorageDomainInbox) and a lingering "do we need any other?" TODO; resolve by either (A) removing StorageDomainPathPrivate (deprecated in Cadence 1.0) and removing or justifying StorageDomainInbox after auditing current storage usage in tokenDiffSearchDomains, or (B) if you cannot fully resolve now, replace the inline TODO with a clear actionable issue reference: create an issue that lists tokenDiffSearchDomains, asks to validate each StorageDomain (including StorageDomainPathPrivate and StorageDomainInbox) against current Cadence/storage semantics, and add the created issue ID in the comment so the PR no longer contains unresolved TODOs; then run unit/integration tests and a storage-domain usage grep to confirm no regressions.
105-105:common.Address([]byte(k.Owner))will panic if Owner is not exactly 8 bytes.In Go 1.20+, converting a slice to a fixed-size array panics if the slice is shorter than the array. While register owners in the Flow protocol should always be 8 bytes (or empty, which is already filtered on Line 102), any unexpected owner length will cause a runtime panic. The
recoverinInspectwill catch this, but it will abort the entire inspection rather than just skipping the problematic register.Consider using
flow.BytesToAddress(which handles shorter slices safely) and then converting tocommon.Address:addresses[common.Address(flow.BytesToAddress([]byte(k.Owner)))] = struct{}{}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/inspection/token_changes.go` at line 105, The current conversion common.Address([]byte(k.Owner)) can panic if k.Owner is not exactly the expected length; in the Inspect flow use flow.BytesToAddress to safely handle shorter/variable-length owner slices and then cast to common.Address when storing into the addresses map (replace the direct conversion with a safe conversion using flow.BytesToAddress for k.Owner), so modify the code around where addresses is populated (the addresses map and k.Owner usage) to call flow.BytesToAddress([]byte(k.Owner)) then convert to common.Address before assignment; this avoids panics that would trigger Inspect's recover and skips only the bad register.fvm/inspection/inspector.go (2)
13-23: Clarify thestorageparameter comment and document the read-key constraint.The current comment "only the executionSnapshot.Reads, will be read" is ambiguous — it appears under
storagebut referencesexecutionSnapshot. The intent seems to be that implementors should only accessstorageat register IDs that appear inexecutionSnapshot.Reads(to avoid arbitrary ledger reads). Make the constraint explicit:💬 Suggested comment clarification
// Inspect -// - storage is the execution state before the procedure was executed. -// only the executionSnapshot.Reads, will be read +// - storage is the execution state before the procedure was executed. +// Implementations should only read from storage at register IDs that appear +// in executionSnapshot.Reads; arbitrary reads outside that set are unexpected. // - executionSnapshot is the reads and writes of the procedure // - events are all of the events the procedure is emitting🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/inspection/inspector.go` around lines 13 - 23, Clarify the Inspect method's comment to explicitly state that implementors must only read from the provided storage (snapshot.StorageSnapshot) at register IDs listed in executionSnapshot.Reads to avoid arbitrary ledger access; update the storage parameter documentation to remove the ambiguous reference to executionSnapshot and instead say something like "storage is the execution state before the procedure was executed; callers MUST restrict reads to register IDs present in executionSnapshot.Reads." Also mention that executionSnapshot is the procedure's reads/writes and events are emitted events so implementors understand the constraint when implementing Inspect.
25-27: Document the implicit contract offunc(e *zerolog.Event)inAsLogEvent.The
result_collector.goaggregation appends fields from multiple results into a singlezerolog.Eventbefore callingMsg. The returned function must therefore only add fields (e.g. viae.Str(...)) and must not calle.Msg(),e.Send(), ore.Discard(). This contract is not documented, making it easy to violate inadvertently in futureResultimplementations.💬 Suggested doc addition
// Result is the result of a procedure inspector type Result interface { - AsLogEvent() (zerolog.Level, func(e *zerolog.Event)) + // AsLogEvent returns the log level at which this result should be reported, + // and a function that adds result-specific fields to a zerolog event. + // The returned function MUST only add fields to the event (e.g. e.Str, e.Int) + // and MUST NOT call Msg, Send, or Discard, as the caller aggregates multiple + // results into a single log entry. A nil function indicates no fields to add. + AsLogEvent() (zerolog.Level, func(e *zerolog.Event)) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/inspection/inspector.go` around lines 25 - 27, Update the Result.AsLogEvent documentation to state the implicit contract: the returned func(e *zerolog.Event) is only allowed to add fields to the provided zerolog.Event (e.g., e.Str, e.Int, e.Interface, etc.) and must NOT call e.Msg(), e.Send(), e.Discard(), or otherwise finalize the event because result_collector.go aggregates multiple Results into one zerolog.Event before calling Msg; refer to the Result interface and AsLogEvent so future implementers know to only append fields and leave sending to the collector.engine/execution/computation/manager.go (1)
109-112:tokenTrackinghardcoded tofalseinNew()— inconsistent withExtensiveTracingand fragile.Token tracking is currently configured by pre-setting inspectors on the
vmCtxbefore passing toNew()(seeexecution_builder.go). InsideNew(), the secondDefaultFVMOptions(..., false)call doesn't invokeWithInspectors, so the parent context'sInspectorsfield is preserved throughNewContextFromParent. This works but:
- Inconsistency:
ExtensiveTracinglives inComputationConfigand flows intoNew(), buttokenTrackingdoes not — callers must configure it externally viavmCtx.- Fragility: Any future call to
WithInspectorsinsideDefaultFVMOptions(e.g., for a second inspector type) would silently overwrite externally configured inspectors sinceWithInspectorsreplaces rather than appends.Consider adding
TokenTrackingEnabled booltoComputationConfigand passing it through:♻️ Suggested approach
type ComputationConfig struct { query.QueryConfig ExtensiveTracing bool DerivedDataCacheSize uint MaxConcurrency int + TokenTrackingEnabled bool NewCustomVirtualMachine func() fvm.VM }-options := DefaultFVMOptions(chainID, params.ExtensiveTracing, vmCtx.ScheduledTransactionsEnabled, false) +options := DefaultFVMOptions(chainID, params.ExtensiveTracing, vmCtx.ScheduledTransactionsEnabled, params.TokenTrackingEnabled)Callers that today pass
tokenTrackingEnabledvia the externalDefaultFVMOptionscall would then setComputationConfig.TokenTrackingEnabledinstead, and the externalDefaultFVMOptionscall inexecution_builder.gocan be dropped to avoid creatingReusableCadenceRuntimePooltwice.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/execution/computation/manager.go` around lines 109 - 112, The code hardcodes tokenTracking=false when constructing the FVM context, causing inconsistency and fragility because token tracking is set externally via vmCtx inspectors; update the ComputationConfig to include TokenTrackingEnabled bool, pass that value into DefaultFVMOptions in manager.go (instead of the hardcoded false) so New() receives token tracking intent directly, and remove the external ad-hoc DefaultFVMOptions call in execution_builder.go (which set inspectors) so callers set ComputationConfig.TokenTrackingEnabled and you avoid creating ReusableCadenceRuntimePool twice; ensure you continue to use NewContextFromParent and avoid calling WithInspectors in a way that would overwrite externally provided inspectors.fvm/context.go (1)
418-424:WithInspectorsis missing a doc comment.Every other
With*option function in this file has a doc comment. Consider adding one for consistency.💬 Suggested doc comment
+// WithInspectors sets the list of inspectors that will be run after each +// transaction procedure to produce inspection results (e.g. token movement tracking). +// Each call replaces the previously configured inspectors. func WithInspectors(inspectors []inspection.Inspector) Option {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/context.go` around lines 418 - 424, Add a doc comment for the WithInspectors option function to match the style of other With* functions; update the comment immediately above func WithInspectors(inspectors []inspection.Inspector) Option to briefly describe that it returns an Option which sets the Context.Inspectors slice on the provided Context and mention the parameter purpose (inspectors) and effect (assigns to ctx.Inspectors).fvm/fvm_test.go (2)
4436-4441: Hardcoded Testnet FlowToken address across multiple test cases.The string
"A.7e60df042a9c0868.FlowToken.Vault"is repeated in several test cases. Extract it into a constant or derive it fromsystemcontracts.SystemContractsForChain(flow.Testnet.Chain().ChainID())to avoid silent breakage if the test chain changes.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/fvm_test.go` around lines 4436 - 4441, The test currently hardcodes the FlowToken address string "A.7e60df042a9c0868.FlowToken.Vault" in the map key and in the ID field inside fvm_test.go; replace this literal with a single constant or compute it from systemcontracts.SystemContractsForChain(flow.Testnet.Chain().ChainID()) (e.g., derive the FlowToken contract address and format it as "<address>.FlowToken.Vault"), then use that constant/value for both the map key and the ID field and ensure the GetBalance closure remains unchanged except for referencing the new constant where needed.
4587-4652: Tests exercise the inspector directly but not the VM'sinspectProcedureResultsintegration.The test manually calls
differ.Inspect(...)aftervm.Run, but never wires an inspector viafvm.WithInspectors(...). This means the new code path inVirtualMachine.inspectProcedureResults(andProcedureOutput.InspectionResults) remains untested.Consider adding at least one sub-test that passes the inspector through
fvm.WithInspectors(...)and asserts onoutput.InspectionResultsto cover the integration.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/fvm_test.go` around lines 4587 - 4652, The tests call the inspector directly via differ.Inspect(...) after vm.Run, so the VM integration path (VirtualMachine.inspectProcedureResults and ProcedureOutput.InspectionResults) is not exercised; update runAndCheckTransactionTest to include at least one sub-test that registers the inspector via fvm.WithInspectors(...) in the VM bootstrap options (instead of only calling differ.Inspect), run vm.Run(...) and then assert on output.InspectionResults (and/or that output.InspectionResults contains the TokenDiffResult) to cover the end-to-end integration of inspectProcedureResults and ensure ProcedureOutput.InspectionResults is populated.fvm/fvm.go (1)
232-239: Error frominspector.Inspectis logged and swallowed; nil results may propagate to consumers.Two concerns:
Per coding guidelines, errors should be explicitly handled rather than logged and continued. Even though inspection is informational today, silently storing a potentially-nil
Resultalongside valid ones creates a latent footgun for downstream consumers that iterateInspectionResultswithout nil-checking.If any inspector fails, consider either filtering it out of the returned slice or wrapping the error so callers can distinguish a "no result" entry from a valid one.
Suggested approach
inspectionResults := make([]inspection.Result, len(context.Inspectors)) var err error for i, inspector := range context.Inspectors { inspectionResults[i], err = inspector.Inspect(storageSnapshot, executionSnapshot, evts) if err != nil { - logger.Warn().Err(err).Msg("failed to inspect procedure results") + logger.Warn().Err(err).Msg("failed to inspect procedure results") + inspectionResults[i] = nil // make the gap explicit } }At minimum, document that callers must nil-check each element.
As per coding guidelines: "NEVER log and continue on best effort basis, ALWAYS explicitly handle errors."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/fvm.go` around lines 232 - 239, The loop over context.Inspectors currently logs and swallows errors from inspector.Inspect, risking nil entries in inspectionResults; change this to explicitly handle failures by either (a) appending only successful results to inspectionResults (filter out nil/failed entries) or (b) return an error (or a wrapped aggregate error) alongside the results so callers can distinguish failures; locate the loop using inspectionResults, context.Inspectors, inspector.Inspect(storageSnapshot, executionSnapshot, evts) and replace the logger.Warn-only path (logger.Warn().Err(err).Msg(...)) with code that either skips adding the failed result and records the error in a returned error value (or error list) or immediately returns the error, ensuring downstream consumers never receive nil entries without an explicit error.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@engine/execution/computation/computer/result_collector.go`:
- Around line 247-249: Fix the typo in the comment that reads "We log inspection
results here, because if we logged them ih the FVM" by changing "ih" to "in" so
the comment reads "We log inspection results here, because if we logged them in
the FVM"; update the same typo in the following sentence if repeated and leave
surrounding punctuation/formatting intact (file: result_collector.go, comment
text: "We log inspection results here, because if we logged them ih the FVM").
In `@fvm/fvm_test.go`:
- Around line 4421-4430: Dead code: remove the unused makeKey function,
numAccounts variable, and accountKeys slice allocation since account keys are
generated inside runAndCheckTransactionTest via
testutil.GenerateAccountPrivateKey(); locate and delete the makeKey closure, the
numAccounts declaration, and the accountKeys initialization (and the loop
filling it) to eliminate the unused symbols (makeKey, numAccounts, accountKeys)
and rely on the existing testutil.GenerateAccountPrivateKey() calls in
runAndCheckTransactionTest.
In `@fvm/fvm.go`:
- Line 227: Fix the typo in the TODO comment in fvm.go: change "imspector" to
"inspector" in the TODO that reads "imspector should be able to receive
ProcedureOutput directly" so the comment correctly reads "inspector should be
able to receive ProcedureOutput directly"; update the TODO text wherever that
exact phrase appears (e.g., near the TODO in fvm/fvm.go).
In `@fvm/inspection/token_changes.go`:
- Around line 577-598: The diffAccountTokens function converts uint64 counts (a,
b, v) to int64 and can overflow; update diffAccountTokens to detect values >
math.MaxInt64 before casting and clamp the reported delta to math.MaxInt64 (or
-math.MaxInt64 for negatives) or switch to using math/big.Int for exact diffs;
specifically, at the four conversion sites (int64(a - b), -int64(b - a),
int64(a), -int64(v)) compute the unsigned delta as uint64, check if it exceeds
math.MaxInt64 and if so set change[k] to math.MaxInt64 or -math.MaxInt64 as
appropriate (or replace change's type and logic to use big.Int across
AccountChange if you choose arbitrary-precision), ensuring no silent cast
overflow for variables a, b, and v in diffAccountTokens.
- Around line 670-694: In flowTokenMintedEventID, you are calling
payload.(cadence.Event).SearchFieldByName("amount") twice and introduced an
unused variable v; change the code to call SearchFieldByName("amount") once,
assign it to v, check v for nil, then perform the type assertion on v (e.g.,
v.(cadence.UFix64)), check the value against math.MaxInt64, and return
int64(ufix). This removes the redundant call and the unused variable while
preserving the nil/type/bounds checks in flowTokenMintedEventID.
- Around line 206-209: The code depends on the internal Cadence package type
vm.Context solely to satisfy parameter signatures for
ForEachReadOnlyLoadedField, IterateReadOnlyLoaded, and Walk; replace this
fragile dependency by either (a) switching to the public Cadence traversal API
if available in a newer Cadence release, (b) introducing a lightweight local
interface/type that matches only the members you actually use and change calls
to accept that instead of vm.Context, or (c) if no public API exists, pin or
vendor the specific Cadence version and wrap vm.Context construction in a single
adapter function (e.g., createVMContext()) so the dependency is isolated; update
the uses of vm.Context in ForEachReadOnlyLoadedField / IterateReadOnlyLoaded /
Walk call sites to use the new interface/adapter and remove direct import of
bbq/vm.
---
Nitpick comments:
In `@engine/execution/computation/manager.go`:
- Around line 109-112: The code hardcodes tokenTracking=false when constructing
the FVM context, causing inconsistency and fragility because token tracking is
set externally via vmCtx inspectors; update the ComputationConfig to include
TokenTrackingEnabled bool, pass that value into DefaultFVMOptions in manager.go
(instead of the hardcoded false) so New() receives token tracking intent
directly, and remove the external ad-hoc DefaultFVMOptions call in
execution_builder.go (which set inspectors) so callers set
ComputationConfig.TokenTrackingEnabled and you avoid creating
ReusableCadenceRuntimePool twice; ensure you continue to use
NewContextFromParent and avoid calling WithInspectors in a way that would
overwrite externally provided inspectors.
In `@fvm/context.go`:
- Around line 418-424: Add a doc comment for the WithInspectors option function
to match the style of other With* functions; update the comment immediately
above func WithInspectors(inspectors []inspection.Inspector) Option to briefly
describe that it returns an Option which sets the Context.Inspectors slice on
the provided Context and mention the parameter purpose (inspectors) and effect
(assigns to ctx.Inspectors).
In `@fvm/fvm_test.go`:
- Around line 4436-4441: The test currently hardcodes the FlowToken address
string "A.7e60df042a9c0868.FlowToken.Vault" in the map key and in the ID field
inside fvm_test.go; replace this literal with a single constant or compute it
from systemcontracts.SystemContractsForChain(flow.Testnet.Chain().ChainID())
(e.g., derive the FlowToken contract address and format it as
"<address>.FlowToken.Vault"), then use that constant/value for both the map key
and the ID field and ensure the GetBalance closure remains unchanged except for
referencing the new constant where needed.
- Around line 4587-4652: The tests call the inspector directly via
differ.Inspect(...) after vm.Run, so the VM integration path
(VirtualMachine.inspectProcedureResults and ProcedureOutput.InspectionResults)
is not exercised; update runAndCheckTransactionTest to include at least one
sub-test that registers the inspector via fvm.WithInspectors(...) in the VM
bootstrap options (instead of only calling differ.Inspect), run vm.Run(...) and
then assert on output.InspectionResults (and/or that output.InspectionResults
contains the TokenDiffResult) to cover the end-to-end integration of
inspectProcedureResults and ensure ProcedureOutput.InspectionResults is
populated.
In `@fvm/fvm.go`:
- Around line 232-239: The loop over context.Inspectors currently logs and
swallows errors from inspector.Inspect, risking nil entries in
inspectionResults; change this to explicitly handle failures by either (a)
appending only successful results to inspectionResults (filter out nil/failed
entries) or (b) return an error (or a wrapped aggregate error) alongside the
results so callers can distinguish failures; locate the loop using
inspectionResults, context.Inspectors, inspector.Inspect(storageSnapshot,
executionSnapshot, evts) and replace the logger.Warn-only path
(logger.Warn().Err(err).Msg(...)) with code that either skips adding the failed
result and records the error in a returned error value (or error list) or
immediately returns the error, ensuring downstream consumers never receive nil
entries without an explicit error.
In `@fvm/inspection/inspector.go`:
- Around line 13-23: Clarify the Inspect method's comment to explicitly state
that implementors must only read from the provided storage
(snapshot.StorageSnapshot) at register IDs listed in executionSnapshot.Reads to
avoid arbitrary ledger access; update the storage parameter documentation to
remove the ambiguous reference to executionSnapshot and instead say something
like "storage is the execution state before the procedure was executed; callers
MUST restrict reads to register IDs present in executionSnapshot.Reads." Also
mention that executionSnapshot is the procedure's reads/writes and events are
emitted events so implementors understand the constraint when implementing
Inspect.
- Around line 25-27: Update the Result.AsLogEvent documentation to state the
implicit contract: the returned func(e *zerolog.Event) is only allowed to add
fields to the provided zerolog.Event (e.g., e.Str, e.Int, e.Interface, etc.) and
must NOT call e.Msg(), e.Send(), e.Discard(), or otherwise finalize the event
because result_collector.go aggregates multiple Results into one zerolog.Event
before calling Msg; refer to the Result interface and AsLogEvent so future
implementers know to only append fields and leave sending to the collector.
In `@fvm/inspection/token_changes.go`:
- Around line 60-84: The defer in TokenChanges.Inspect currently converts any
panic into an error, which can hide programming bugs; modify the panic-recovery
block in Inspect to capture the panic value and also log the stack trace (e.g.,
via runtime/debug.Stack() or your logger) before wrapping it as an error so
developers can see the full panic context during development; keep the existing
behavior of returning a non-panicking error from
getTokenDiff/getSearchedTokensRef, but ensure the log call includes the panic
value and stack trace for easier debugging.
- Around line 414-423: allTouchedRegisters currently builds a new map on every
call which wastes allocations; fix by computing it once and reusing it: add a
cached field (e.g., touchedRegisters map[flow.RegisterID]struct{} or
touchedRegistersOnce flag + map) to the executionSnapshotLedgers struct and
populate that map lazily either in executionSnapshotLedgersNew (constructor) or
on the first call to allTouchedRegisters(); then have allTouchedRegisters return
the cached map on subsequent calls so callers like getTokenDiff,
executionSnapshotLedgersNew.ForEach, and executionSnapshotLedgers.Count reuse
the single allocation.
- Around line 647-654: The tokenDiffSearchDomains slice includes ambiguous
entries (StorageDomainPathPrivate and StorageDomainInbox) and a lingering "do we
need any other?" TODO; resolve by either (A) removing StorageDomainPathPrivate
(deprecated in Cadence 1.0) and removing or justifying StorageDomainInbox after
auditing current storage usage in tokenDiffSearchDomains, or (B) if you cannot
fully resolve now, replace the inline TODO with a clear actionable issue
reference: create an issue that lists tokenDiffSearchDomains, asks to validate
each StorageDomain (including StorageDomainPathPrivate and StorageDomainInbox)
against current Cadence/storage semantics, and add the created issue ID in the
comment so the PR no longer contains unresolved TODOs; then run unit/integration
tests and a storage-domain usage grep to confirm no regressions.
- Line 105: The current conversion common.Address([]byte(k.Owner)) can panic if
k.Owner is not exactly the expected length; in the Inspect flow use
flow.BytesToAddress to safely handle shorter/variable-length owner slices and
then cast to common.Address when storing into the addresses map (replace the
direct conversion with a safe conversion using flow.BytesToAddress for k.Owner),
so modify the code around where addresses is populated (the addresses map and
k.Owner usage) to call flow.BytesToAddress([]byte(k.Owner)) then convert to
common.Address before assignment; this avoids panics that would trigger
Inspect's recover and skips only the bad register.
Comment on lines
+247
to
+249
| // We log inspection results here, because if we logged them ih the FVM | ||
| // they would get logged on every transaction retry. | ||
| // Same for the metrics. |
Contributor
There was a problem hiding this comment.
Typo in comment: "ih" → "in".
📝 Fix
-// We log inspection results here, because if we logged them ih the FVM
+// We log inspection results here, because if we logged them in the FVM📝 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.
Suggested change
| // We log inspection results here, because if we logged them ih the FVM | |
| // they would get logged on every transaction retry. | |
| // Same for the metrics. | |
| // We log inspection results here, because if we logged them in the FVM | |
| // they would get logged on every transaction retry. | |
| // Same for the metrics. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/execution/computation/computer/result_collector.go` around lines 247 -
249, Fix the typo in the comment that reads "We log inspection results here,
because if we logged them ih the FVM" by changing "ih" to "in" so the comment
reads "We log inspection results here, because if we logged them in the FVM";
update the same typo in the following sentence if repeated and leave surrounding
punctuation/formatting intact (file: result_collector.go, comment text: "We log
inspection results here, because if we logged them ih the FVM").
Comment on lines
+4421
to
+4430
| makeKey := func() flow.AccountPrivateKey { | ||
| privateKey, err := testutil.GenerateAccountPrivateKey() | ||
| require.NoError(t, err) | ||
| return privateKey | ||
| } | ||
| numAccounts := 5 | ||
| accountKeys := make([]flow.AccountPrivateKey, numAccounts) | ||
| for i := 0; i < numAccounts; i++ { | ||
| accountKeys[i] = makeKey() | ||
| } |
Contributor
There was a problem hiding this comment.
Dead code: accountKeys is allocated but never referenced.
makeKey, numAccounts, and accountKeys are unused — the actual account keys are generated inside runAndCheckTransactionTest via testutil.GenerateAccountPrivateKey().
Suggested removal
- // account keys that can be used in the tests
- makeKey := func() flow.AccountPrivateKey {
- privateKey, err := testutil.GenerateAccountPrivateKey()
- require.NoError(t, err)
- return privateKey
- }
- numAccounts := 5
- accountKeys := make([]flow.AccountPrivateKey, numAccounts)
- for i := 0; i < numAccounts; i++ {
- accountKeys[i] = makeKey()
- }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fvm/fvm_test.go` around lines 4421 - 4430, Dead code: remove the unused
makeKey function, numAccounts variable, and accountKeys slice allocation since
account keys are generated inside runAndCheckTransactionTest via
testutil.GenerateAccountPrivateKey(); locate and delete the makeKey closure, the
numAccounts declaration, and the accountKeys initialization (and the loop
filling it) to eliminate the unused symbols (makeKey, numAccounts, accountKeys)
and rely on the existing testutil.GenerateAccountPrivateKey() calls in
runAndCheckTransactionTest.
| return nil | ||
| } | ||
|
|
||
| // TODO: imspector should be able to receive ProcedureOutput directly |
Contributor
There was a problem hiding this comment.
Typo in TODO comment: "imspector" → "inspector".
- // TODO: imspector should be able to receive ProcedureOutput directly
+ // TODO: inspector should be able to receive ProcedureOutput directly📝 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.
Suggested change
| // TODO: imspector should be able to receive ProcedureOutput directly | |
| // TODO: inspector should be able to receive ProcedureOutput directly |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fvm/fvm.go` at line 227, Fix the typo in the TODO comment in fvm.go: change
"imspector" to "inspector" in the TODO that reads "imspector should be able to
receive ProcedureOutput directly" so the comment correctly reads "inspector
should be able to receive ProcedureOutput directly"; update the TODO text
wherever that exact phrase appears (e.g., near the TODO in fvm/fvm.go).
Comment on lines
+670
to
+694
| flowTokenMintedEventID: func(evt flow.Event) (int64, error) { | ||
| // this decoding will only happen for the specified event (in the case of FlowToken.TokensMinted it | ||
| // is extremely rare). | ||
| payload, err := ccf.Decode(nil, evt.Payload) | ||
| if err != nil { | ||
| return 0, err | ||
| } | ||
| v := payload.(cadence.Event).SearchFieldByName("amount") | ||
| if v == nil { | ||
| return 0, fmt.Errorf("no amount field found for token minted") | ||
| } | ||
|
|
||
| ufix, ok := payload.(cadence.Event).SearchFieldByName("amount").(cadence.UFix64) | ||
| if !ok { | ||
| return 0, fmt.Errorf("amount field is not a cadence.UFix64") | ||
| } | ||
|
|
||
| if ufix > math.MaxInt64 { | ||
| // this is very unlikely | ||
| // but in case it happens, it will get logged | ||
| return 0, fmt.Errorf("amount field is too large") | ||
| } | ||
|
|
||
| return int64(ufix), nil | ||
| }, |
Contributor
There was a problem hiding this comment.
Redundant SearchFieldByName("amount") call and unused variable.
Line 677 calls SearchFieldByName("amount") and stores the result in v, which is only used for a nil check. Line 682 calls SearchFieldByName("amount") again. This is redundant — reuse v from Line 677.
Proposed fix
- v := payload.(cadence.Event).SearchFieldByName("amount")
- if v == nil {
+ amountField := payload.(cadence.Event).SearchFieldByName("amount")
+ if amountField == nil {
return 0, fmt.Errorf("no amount field found for token minted")
}
- ufix, ok := payload.(cadence.Event).SearchFieldByName("amount").(cadence.UFix64)
+ ufix, ok := amountField.(cadence.UFix64)
if !ok {
return 0, fmt.Errorf("amount field is not a cadence.UFix64")
}📝 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.
Suggested change
| flowTokenMintedEventID: func(evt flow.Event) (int64, error) { | |
| // this decoding will only happen for the specified event (in the case of FlowToken.TokensMinted it | |
| // is extremely rare). | |
| payload, err := ccf.Decode(nil, evt.Payload) | |
| if err != nil { | |
| return 0, err | |
| } | |
| v := payload.(cadence.Event).SearchFieldByName("amount") | |
| if v == nil { | |
| return 0, fmt.Errorf("no amount field found for token minted") | |
| } | |
| ufix, ok := payload.(cadence.Event).SearchFieldByName("amount").(cadence.UFix64) | |
| if !ok { | |
| return 0, fmt.Errorf("amount field is not a cadence.UFix64") | |
| } | |
| if ufix > math.MaxInt64 { | |
| // this is very unlikely | |
| // but in case it happens, it will get logged | |
| return 0, fmt.Errorf("amount field is too large") | |
| } | |
| return int64(ufix), nil | |
| }, | |
| flowTokenMintedEventID: func(evt flow.Event) (int64, error) { | |
| // this decoding will only happen for the specified event (in the case of FlowToken.TokensMinted it | |
| // is extremely rare). | |
| payload, err := ccf.Decode(nil, evt.Payload) | |
| if err != nil { | |
| return 0, err | |
| } | |
| amountField := payload.(cadence.Event).SearchFieldByName("amount") | |
| if amountField == nil { | |
| return 0, fmt.Errorf("no amount field found for token minted") | |
| } | |
| ufix, ok := amountField.(cadence.UFix64) | |
| if !ok { | |
| return 0, fmt.Errorf("amount field is not a cadence.UFix64") | |
| } | |
| if ufix > math.MaxInt64 { | |
| // this is very unlikely | |
| // but in case it happens, it will get logged | |
| return 0, fmt.Errorf("amount field is too large") | |
| } | |
| return int64(ufix), nil | |
| }, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fvm/inspection/token_changes.go` around lines 670 - 694, In
flowTokenMintedEventID, you are calling
payload.(cadence.Event).SearchFieldByName("amount") twice and introduced an
unused variable v; change the code to call SearchFieldByName("amount") once,
assign it to v, check v for nil, then perform the type assertion on v (e.g.,
v.(cadence.UFix64)), check the value against math.MaxInt64, and return
int64(ufix). This removes the redundant call and the unused variable while
preserving the nil/type/bounds checks in flowTokenMintedEventID.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
fvm/inspection/token_changes.go (1)
152-154: HardcodednWorkers=1forBatchPreloadis called twice perInspectinvocationThe TODO acknowledges this. With a single worker, slab preloading is serialized. Because
getTokensis called twice (before/after),loadAtreeSlabsInStorageexecutes twice per inspection. For accounts with large storage, this adds latency proportional to slab count × 2. The TODO in the comment suggests a runtime-configurable value, which would also allow tuning based on the execution node's CPU topology.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/inspection/token_changes.go` around lines 152 - 154, The call to loadAtreeSlabsInStorage(runtimeStorage, storage, 1) hardcodes nWorkers=1 causing serialized slab preloads and doubles cost because getTokens calls it twice; change it to use a configurable worker count instead of 1: add a parameter or config value for BatchPreload workers (or compute a default from runtime.NumCPU()/GOMAXPROCS) and pass that into loadAtreeSlabsInStorage from Inspect (and any callers), so slab preloading can run concurrent workers tuned to the node CPU topology; update the Inspect call sites (where getTokens is invoked) to use the new configurable value and document the TODO removal.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fvm/inspection/token_changes.go`:
- Around line 279-283: The loop over tokens silently overwrites entries in the
sourcesSinks map when two token.SinksSources share the same evt key (last writer
wins), causing lost handlers; update the logic in the block iterating tokens and
token.SinksSources to detect collisions or accumulate handlers: either (a)
change the map value type from tokenSourceSink to []tokenSourceSink and append
each tokenSourceSink (referencing tokenSourceSink and token.ID) for a given evt,
or (b) if collisions are invalid, detect an existing entry for evt and return an
error (propagate it from the function that builds sourcesSinks), ensuring you
adjust any downstream code that reads sourcesSinks to handle a slice of handlers
or the new error case.
- Around line 674-676: GetBalance currently does an unchecked type assertion on
value.GetField(nil, "balance") which can panic and abort the entire inspection;
update the GetBalance func (involving interpreter.CompositeValue, GetField and
interpreter.UFix64Value) to first check that field := value.GetField(nil,
"balance") is not nil, then do a comma-ok type assertion to
interpreter.UFix64Value (e.g. v, ok := field.(interpreter.UFix64Value)); if
checks fail return a safe default (e.g. 0) instead of panicking so malformed or
missing fields only affect that account and not the Inspect() recover path.
---
Duplicate comments:
In `@fvm/inspection/token_changes.go`:
- Around line 685-690: The code redundantly calls
payload.(cadence.Event).SearchFieldByName("amount") twice; reuse the previously
retrieved variable v (from the nil-check) for the type assertion instead of
calling SearchFieldByName again. In the token minted handling block (around
where v := payload.(cadence.Event).SearchFieldByName("amount") is set), replace
the second call with a type assertion on v (e.g., ufix, ok :=
v.(cadence.UFix64)), handle the ok boolean and existing error path, and remove
the duplicate SearchFieldByName invocation.
- Around line 207-209: The code currently constructs a bbq/vm.Context
(vm.Context and vm.Config) which relies on an unstable internal Cadence package;
replace this by removing the vm.Context creation and using the Cadence public
interpreter APIs instead (e.g., ForEachReadOnlyLoadedField,
IterateReadOnlyLoaded, or Walk) to traverse and inspect contract/state as
needed, or construct an equivalent context object from supported public
packages; update the logic that consumed vm.Context to call these public
traversal helpers (or a stable public context factory) so the bbq/vm dependency
is no longer required.
---
Nitpick comments:
In `@fvm/inspection/token_changes.go`:
- Around line 152-154: The call to loadAtreeSlabsInStorage(runtimeStorage,
storage, 1) hardcodes nWorkers=1 causing serialized slab preloads and doubles
cost because getTokens calls it twice; change it to use a configurable worker
count instead of 1: add a parameter or config value for BatchPreload workers (or
compute a default from runtime.NumCPU()/GOMAXPROCS) and pass that into
loadAtreeSlabsInStorage from Inspect (and any callers), so slab preloading can
run concurrent workers tuned to the node CPU topology; update the Inspect call
sites (where getTokens is invoked) to use the new configurable value and
document the TODO removal.
Comment on lines
+279
to
+283
| for _, token := range tokens { | ||
| for evt, ss := range token.SinksSources { | ||
| sourcesSinks[evt] = tokenSourceSink{tokenID: token.ID, f: ss} | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Silent overwrite when two tokens register handlers for the same event ID
for _, token := range tokens {
for evt, ss := range token.SinksSources {
sourcesSinks[evt] = tokenSourceSink{tokenID: token.ID, f: ss} // last writer wins
}
}If a future SearchToken entry shares an event type string with an existing one (e.g., two fungible-token contracts emitting an identically-typed event), the earlier handler is silently dropped. The affected token's mint/burn events go unaccounted, producing a false-positive UnaccountedTokens entry.
Consider detecting and returning an error on collision, or accumulating results per event ID into a slice:
🛡️ Proposed fix
- sourcesSinks := make(map[string]tokenSourceSink)
- results := make(map[string]int64)
- for _, token := range tokens {
- for evt, ss := range token.SinksSources {
- sourcesSinks[evt] = tokenSourceSink{tokenID: token.ID, f: ss}
- }
- }
+ type tokenSourceSink struct {
+ tokenID string
+ f func(flow.Event) (int64, error)
+ }
+ sourcesSinks := make(map[string][]tokenSourceSink)
+ results := make(map[string]int64)
+ for _, token := range tokens {
+ for evt, ss := range token.SinksSources {
+ sourcesSinks[evt] = append(sourcesSinks[evt], tokenSourceSink{tokenID: token.ID, f: ss})
+ }
+ }
for _, evt := range events {
id := string(evt.Type)
- if ss, ok := sourcesSinks[id]; ok {
- v, err := ss.f(evt)
- if err != nil {
- return nil, fmt.Errorf("failed to parse source/sink event %s: %w", id, err)
+ if handlers, ok := sourcesSinks[id]; ok {
+ for _, ss := range handlers {
+ v, err := ss.f(evt)
+ if err != nil {
+ return nil, fmt.Errorf("failed to parse source/sink event %s: %w", id, err)
+ }
+ results[ss.tokenID] += v
}
- results[ss.tokenID] += v
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fvm/inspection/token_changes.go` around lines 279 - 283, The loop over tokens
silently overwrites entries in the sourcesSinks map when two token.SinksSources
share the same evt key (last writer wins), causing lost handlers; update the
logic in the block iterating tokens and token.SinksSources to detect collisions
or accumulate handlers: either (a) change the map value type from
tokenSourceSink to []tokenSourceSink and append each tokenSourceSink
(referencing tokenSourceSink and token.ID) for a given evt, or (b) if collisions
are invalid, detect an existing entry for evt and return an error (propagate it
from the function that builds sourcesSinks), ensuring you adjust any downstream
code that reads sourcesSinks to handle a slice of handlers or the new error
case.
Comment on lines
+674
to
+676
| GetBalance: func(value *interpreter.CompositeValue) uint64 { | ||
| return uint64(value.GetField(nil, "balance").(interpreter.UFix64Value).UFix64Value) | ||
| }, |
Contributor
There was a problem hiding this comment.
Unguarded type assertion in GetBalance can abort the entire inspection
GetBalance: func(value *interpreter.CompositeValue) uint64 {
return uint64(value.GetField(nil, "balance").(interpreter.UFix64Value).UFix64Value)
},If GetField(nil, "balance") returns nil (field absent or encoding error) or a non-UFix64Value, the non-comma-ok assertion panics. The panic propagates up to Inspect's recover(), which aborts the whole inspection for that procedure — all accounts lose their diff result, not just the one with the malformed field. As per the coding guidelines, inputs should be treated as potentially byzantine.
🛡️ Proposed fix
GetBalance: func(value *interpreter.CompositeValue) uint64 {
- return uint64(value.GetField(nil, "balance").(interpreter.UFix64Value).UFix64Value)
+ field := value.GetField(nil, "balance")
+ if field == nil {
+ return 0
+ }
+ ufix, ok := field.(interpreter.UFix64Value)
+ if !ok {
+ return 0
+ }
+ return uint64(ufix)
},🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fvm/inspection/token_changes.go` around lines 674 - 676, GetBalance currently
does an unchecked type assertion on value.GetField(nil, "balance") which can
panic and abort the entire inspection; update the GetBalance func (involving
interpreter.CompositeValue, GetField and interpreter.UFix64Value) to first check
that field := value.GetField(nil, "balance") is not nil, then do a comma-ok type
assertion to interpreter.UFix64Value (e.g. v, ok :=
field.(interpreter.UFix64Value)); if checks fail return a safe default (e.g. 0)
instead of panicking so malformed or missing fields only affect that account and
not the Inspect() recover path.
zhangchiqing
force-pushed
the
janez/transaction-result-inspection
branch
from
f30759b to
6a67aeb
Compare
go.mod
Outdated
| github.com/multiformats/go-multihash v0.2.3 | ||
| github.com/onflow/atree v0.12.1 | ||
| github.com/onflow/cadence v1.9.9 | ||
| github.com/onflow/cadence v1.9.9-0.20260219134638-a806b020c8d8 |
Member
There was a problem hiding this comment.
I thought this is a mistake, and tried to use v1.9.9 and then I saw CI is failing and realized this PR actually depend on a pre-released cadence function ReadOnlyLoadedValueIterator ( used in token_changes.go).
fvm/inspection/token_changes.go
Outdated
| } | ||
| var key atree.Value | ||
| for { | ||
| key, _, err = iter.Next() |
Member
There was a problem hiding this comment.
We are throwing away the value, but after this function collected all the keys, we are the following to read the value again:
callinginterpreterValue, err := td.getValues(storageMap, k)Wouldn't it be better if we iterate both key and value here?
This commit updates TokenChanges to use the new storageMap.ReadOnlyLoadedValueIterator() API.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
fvm/inspection/token_changes.go (3)
617-619:⚠️ Potential issue | 🟠 MajorGuard
balanceextraction to prevent panic on malformed values.Unchecked assertion on
GetFieldcan panic and terminate the full inspection path.🔧 Proposed fix
GetBalance: func(value *interpreter.CompositeValue) uint64 { - return uint64(value.GetField(nil, "balance").(interpreter.UFix64Value).UFix64Value) + field := value.GetField(nil, "balance") + ufix, ok := field.(interpreter.UFix64Value) + if !ok { + return 0 + } + return uint64(ufix.UFix64Value) },Based on learnings: "Treat all inputs as potentially byzantine and classify errors in a context-dependent manner; no code path is safe unless explicitly proven and documented".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/inspection/token_changes.go` around lines 617 - 619, The GetBalance extractor currently does an unchecked type assertion and can panic; update the GetBalance function to safely retrieve the "balance" field from the interpreter.CompositeValue by checking the result of value.GetField(nil, "balance") for nil, performing a type-assertion with the comma-ok form against interpreter.UFix64Value, and only converting to uint64 when the assert succeeds; on nil/mismatched type return a safe default (e.g., 0) or handle the error path appropriate for the inspection flow so it never panics.
624-634:⚠️ Potential issue | 🟠 MajorHarden mint-event decoding: avoid unchecked assertions and duplicate field lookup.
payload.(cadence.Event)can panic if decoding yields an unexpected value, and"amount"is searched twice. Decode once, type-check once, field-check once.🔧 Proposed fix
payload, err := ccf.Decode(nil, evt.Payload) if err != nil { return 0, err } - v := payload.(cadence.Event).SearchFieldByName("amount") - if v == nil { + event, ok := payload.(cadence.Event) + if !ok { + return 0, fmt.Errorf("decoded payload is not a cadence.Event") + } + amountField := event.SearchFieldByName("amount") + if amountField == nil { return 0, fmt.Errorf("no amount field found for token minted") } - ufix, ok := payload.(cadence.Event).SearchFieldByName("amount").(cadence.UFix64) + ufix, ok := amountField.(cadence.UFix64) if !ok { return 0, fmt.Errorf("amount field is not a cadence.UFix64") }Based on learnings: "Treat all inputs as potentially byzantine and classify errors in a context-dependent manner; no code path is safe unless explicitly proven and documented".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/inspection/token_changes.go` around lines 624 - 634, Avoid panics and duplicate lookups by decoding once into a local value, asserting its type safely, and retrieving the "amount" field only once: call ccf.Decode(nil, evt.Payload), check err, assign payload to a variable (e.g., dec := payload), use a type-safe check like ev, ok := dec.(cadence.Event) and return an error if not ok, then call ev.SearchFieldByName("amount") once and verify it's non-nil before doing a type assertion to cadence.UFix64 (e.g., ufix, ok := field.(cadence.UFix64)); return clear errors for each failure path. Ensure you update uses of payload, payload.(cadence.Event), and the duplicate SearchFieldByName to these safe locals.
242-247:⚠️ Potential issue | 🟡 MinorDetect duplicate event IDs instead of silently overwriting handlers.
sourcesSinks[evt] = ...is last-writer-wins. If two tracked tokens share an event type, one handler is dropped and accounting becomes incomplete.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fvm/inspection/token_changes.go` around lines 242 - 247, The current loop over tokens silently overwrites handlers with sourcesSinks[evt] = ..., causing dropped handlers when multiple tokens share the same event id; update the implementation to detect and handle duplicates by either (A) changing sourcesSinks from map[string]tokenSourceSink to map[string][]tokenSourceSink and append tokenSourceSink entries for each evt found in token.SinksSources, or (B) check for an existing key in sourcesSinks inside the loop and return/log an explicit error indicating the conflicting event id and the involved token IDs (use symbols sourcesSinks, token.SinksSources, tokenSourceSink, and tokens to locate the code), then update downstream code that consumes sourcesSinks to handle the new slice form or the error path accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fvm/inspection/token_changes.go`:
- Around line 36-52: NewTokenChangesInspector currently assigns the incoming
searchedTokens map directly and SetSearchedTokens only shallow-copies the outer
map, leaving each SearchToken.SinksSources map shared and racy; add a helper
cloneSearchTokens that deep-clones the outer map and, for each SearchToken,
makes a copy of its SinksSources map (and any other mutable fields), then use
cloneSearchTokens both in NewTokenChangesInspector (when constructing
TokenChanges) and in SetSearchedTokens (before acquiring searchedTokensMu and
assigning td.searchedTokens) so Inspect and other concurrent readers never
observe shared inner maps.
In `@insecure/go.mod`:
- Line 219: The go.mod currently pins a pseudo-version of
github.com/onflow/cadence that includes an API-breaking change:
ReadOnlyLoadedValueIterator() no longer returns an error and will panic on
failure; update the repository to either (a) explicitly document this dependency
by adding a comment in go.mod or an adjacent README explaining why the
pseudo-version v1.9.9-0.20260226153458-8a199b890dfa is required and that callers
must handle panic semantics from ReadOnlyLoadedValueIterator(), or (b) if the
change was accidental, revert the dependency to the stable v1.9.9 release to
keep the old error-returning behavior and update any code using
ReadOnlyLoadedValueIterator() accordingly; ensure you reference
ReadOnlyLoadedValueIterator() in the comment so reviewers know the rationale.
- Line 226: The go.mod currently pins github.com/onflow/flow-go-sdk to v1.9.14
which is built against Cadence v1.9.8; update the module requirement for
github.com/onflow/flow-go-sdk to v1.9.15 or later (preferably v1.9.16) to match
Cadence v1.9.9 and include the transaction ID encoding fixes, then run go get/go
mod tidy to refresh go.sum and ensure the project builds with the new SDK
version; look for the flow-go-sdk entry in go.mod to make this change.
---
Duplicate comments:
In `@fvm/inspection/token_changes.go`:
- Around line 617-619: The GetBalance extractor currently does an unchecked type
assertion and can panic; update the GetBalance function to safely retrieve the
"balance" field from the interpreter.CompositeValue by checking the result of
value.GetField(nil, "balance") for nil, performing a type-assertion with the
comma-ok form against interpreter.UFix64Value, and only converting to uint64
when the assert succeeds; on nil/mismatched type return a safe default (e.g., 0)
or handle the error path appropriate for the inspection flow so it never panics.
- Around line 624-634: Avoid panics and duplicate lookups by decoding once into
a local value, asserting its type safely, and retrieving the "amount" field only
once: call ccf.Decode(nil, evt.Payload), check err, assign payload to a variable
(e.g., dec := payload), use a type-safe check like ev, ok := dec.(cadence.Event)
and return an error if not ok, then call ev.SearchFieldByName("amount") once and
verify it's non-nil before doing a type assertion to cadence.UFix64 (e.g., ufix,
ok := field.(cadence.UFix64)); return clear errors for each failure path. Ensure
you update uses of payload, payload.(cadence.Event), and the duplicate
SearchFieldByName to these safe locals.
- Around line 242-247: The current loop over tokens silently overwrites handlers
with sourcesSinks[evt] = ..., causing dropped handlers when multiple tokens
share the same event id; update the implementation to detect and handle
duplicates by either (A) changing sourcesSinks from map[string]tokenSourceSink
to map[string][]tokenSourceSink and append tokenSourceSink entries for each evt
found in token.SinksSources, or (B) check for an existing key in sourcesSinks
inside the loop and return/log an explicit error indicating the conflicting
event id and the involved token IDs (use symbols sourcesSinks,
token.SinksSources, tokenSourceSink, and tokens to locate the code), then update
downstream code that consumes sourcesSinks to handle the new slice form or the
error path accordingly.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
go.sumis excluded by!**/*.suminsecure/go.sumis excluded by!**/*.sumintegration/go.sumis excluded by!**/*.sum
📒 Files selected for processing (4)
fvm/inspection/token_changes.gogo.modinsecure/go.modintegration/go.mod
🚧 Files skipped from review as they are similar to previous changes (1)
- go.mod
Comment on lines
+36
to
+52
| func NewTokenChangesInspector(searchedTokens TokenChangesSearchTokens) *TokenChanges { | ||
| return &TokenChanges{searchedTokens: searchedTokens} | ||
| } | ||
|
|
||
| // SetSearchedTokens are safe to replace whenever. | ||
| // The change will not affect the inspections already in progress. | ||
| // TODO: this can be tied into the admin commands | ||
| func (td *TokenChanges) SetSearchedTokens(searchedTokens TokenChangesSearchTokens) { | ||
| // copy the map in case the user tries to modify the map | ||
| st := make(map[string]SearchToken, len(searchedTokens)) | ||
| for k, v := range searchedTokens { | ||
| st[k] = v | ||
| } | ||
| td.searchedTokensMu.Lock() | ||
| defer td.searchedTokensMu.Unlock() | ||
| td.searchedTokens = st | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
fd token_changes.goRepository: onflow/flow-go
Length of output: 89
🏁 Script executed:
wc -l fvm/inspection/token_changes.goRepository: onflow/flow-go
Length of output: 93
🏁 Script executed:
cat -n fvm/inspection/token_changes.goRepository: onflow/flow-go
Length of output: 22504
🏁 Script executed:
rg "SinksSources\[" fvm/inspection/Repository: onflow/flow-go
Length of output: 40
🏁 Script executed:
rg "DefaultTokenDiffSearchTokens" --type goRepository: onflow/flow-go
Length of output: 506
🏁 Script executed:
rg "NewTokenChangesInspector" --type go -A 2 -B 2Repository: onflow/flow-go
Length of output: 1286
🏁 Script executed:
rg "SetSearchedTokens" --type go -A 2 -B 2Repository: onflow/flow-go
Length of output: 684
🏁 Script executed:
rg "\.SinksSources\[" --type goRepository: onflow/flow-go
Length of output: 40
🏁 Script executed:
rg "SinksSources\s*=" --type go -A 1 -B 1Repository: onflow/flow-go
Length of output: 40
🏁 Script executed:
git log --oneline --all -- fvm/inspection/token_changes.go | head -20Repository: onflow/flow-go
Length of output: 121
🏁 Script executed:
grep -n "SinksSources.*:" fvm/inspection/token_changes.go | head -10Repository: onflow/flow-go
Length of output: 122
🏁 Script executed:
rg "range.*\.SinksSources" --type go -B 2 -A 3Repository: onflow/flow-go
Length of output: 432
🏁 Script executed:
cat -n fvm/inspection/token_changes.go | sed -n '235,250p'Repository: onflow/flow-go
Length of output: 721
🏁 Script executed:
cat -n fvm/inspection/token_changes.go | sed -n '54,84p'Repository: onflow/flow-go
Length of output: 1213
🏁 Script executed:
cat -n fvm/inspection/token_changes.go | sed -n '432,437p'Repository: onflow/flow-go
Length of output: 305
🏁 Script executed:
cat -n fvm/inspection/token_changes.go | sed -n '607,607p'Repository: onflow/flow-go
Length of output: 117
🏁 Script executed:
python3 << 'EOF'
# Verify Go map semantics: copying a struct with a map field
# In Go, when you copy a struct that contains a map field,
# the map field is copied by reference (the pointer), not the map contents
code = """
type SearchToken struct {
ID string
SinksSources map[string]func() int64
}
// Original map
original := make(map[string]func() int64)
original["evt"] = func() int64 { return 1 }
// Copy via struct
token1 := SearchToken{ID: "t1", SinksSources: original}
token2 := token1 // struct copy
// Both point to same underlying map
token2.SinksSources["evt2"] = func() int64 { return 2 }
// This modifies the map that token1 also points to
// External mutation after shallow copy
token1.SinksSources["evt3"] = func() int64 { return 3 }
// This also affects token2
// Concurrent access to same map = UNSAFE
"""
print("Go struct copy semantics with map fields:")
print("- When copying a struct containing a map field")
print("- The map field pointer is copied (shallow copy)")
print("- Both structs point to the SAME underlying map")
print("- Concurrent reads/writes to that map → PANIC")
print("\nReview claim: VALID")
EOFRepository: onflow/flow-go
Length of output: 310
🏁 Script executed:
# Check if DefaultTokenDiffSearchTokens maps are reused or created fresh
rg -A 30 "func DefaultTokenDiffSearchTokens" fvm/inspection/token_changes.go | head -40Repository: onflow/flow-go
Length of output: 1359
🏁 Script executed:
# Check if there are any actual usages where SetSearchedTokens is called
rg "SetSearchedTokens" --type go -cRepository: onflow/flow-go
Length of output: 91
🏁 Script executed:
# Verify the lock-release-before-use pattern in detail
cat -n fvm/inspection/token_changes.go | sed -n '67,84p'Repository: onflow/flow-go
Length of output: 626
🏁 Script executed:
# Check all usages of SetSearchedTokens
rg "SetSearchedTokens" --type go -B 3 -A 3Repository: onflow/flow-go
Length of output: 838
🏁 Script executed:
# Examine the exact type definition and how maps are handled
cat -n fvm/inspection/token_changes.go | sed -n '23,29p'Repository: onflow/flow-go
Length of output: 390
🏁 Script executed:
# Check actual usage in tests and production
rg "NewTokenChangesInspector.*DefaultTokenDiffSearchTokens" --type go -B 2 -A 2Repository: onflow/flow-go
Length of output: 455
🏁 Script executed:
# Verify DefaultTokenDiffSearchTokens is called fresh each time or cached
rg "tokenDefinitions.*DefaultTokenDiffSearchTokens" --type go -B 3 -A 3Repository: onflow/flow-go
Length of output: 932
Deep-clone searchedTokens in constructor and setter to prevent shared-map races.
NewTokenChangesInspector stores the map directly without copying, and SetSearchedTokens only shallow-copies the outer map; the nested SearchToken.SinksSources maps remain shared references. If callers mutate these maps after passing them in, concurrent Inspect operations will access the same map without synchronization, causing panics.
The proposed cloneSearchTokens helper providing deep copy of both the outer map and each SearchToken's SinksSources is the correct fix.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@fvm/inspection/token_changes.go` around lines 36 - 52,
NewTokenChangesInspector currently assigns the incoming searchedTokens map
directly and SetSearchedTokens only shallow-copies the outer map, leaving each
SearchToken.SinksSources map shared and racy; add a helper cloneSearchTokens
that deep-clones the outer map and, for each SearchToken, makes a copy of its
SinksSources map (and any other mutable fields), then use cloneSearchTokens both
in NewTokenChangesInspector (when constructing TokenChanges) and in
SetSearchedTokens (before acquiring searchedTokensMu and assigning
td.searchedTokens) so Inspect and other concurrent readers never observe shared
inner maps.
insecure/go.mod
Outdated
| github.com/olekukonko/tablewriter v0.0.5 // indirect | ||
| github.com/onflow/atree v0.12.1 // indirect | ||
| github.com/onflow/cadence v1.9.9 // indirect | ||
| github.com/onflow/cadence v1.9.9-0.20260226153458-8a199b890dfa // indirect |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
onflow/cadence github v1.9.9 release tag
💡 Result:
The onflow/cadence v1.9.9 GitHub release/tag is:
- Tag:
v1.9.9 - Date: Feb 13, 2026
- Commit:
f858551[1] - Release page / tag ref:
https://github.com/onflow/cadence/releases/tag/v1.9.9
https://github.com/onflow/cadence/tree/v1.9.9
[1][2]
Sources: [1] [2]
🌐 Web query:
github.com/onflow/cadence commit 8a199b890dfa
💡 Result:
Commit 8a199b890dfa in onflow/cadence (author: fxamacker) is titled “Update ReadOnlyLoadedValueIterator() to return DomainStorageMapIterator”. It changes 1 file: interpreter/domain_storagemap.go (+34 / -3). [1]
Key change:
(*DomainStorageMap).ReadOnlyLoadedValueIterator()no longer returns(*atree.MapLoadedValueIterator, error); it now returns aDomainStorageMapIteratorand panics on error (wrapping the error as an external error). [1]- Adds a small wrapper type
mapLoadedValueIteratorthat embeds*atree.MapLoadedValueIterator, declares it implementsatree.MapIterator, and providesCanMutate() bool(returnsfalse), plusNextKey()/NextValue()helpers built onNext(). [1]
Sources: [1]
🌐 Web query:
onflow/cadence latest stable release 2026
💡 Result:
The latest stable release of onflow/cadence (as of February 26, 2026) is v1.9.9, published February 13, 2026. [1]
Document why the pseudo-version with API-breaking changes from cadence is required.
At Line 219, the pseudo-version v1.9.9-0.20260226153458-8a199b890dfa pins to a commit (Feb 26) after the v1.9.9 stable release (Feb 13, 2026). This commit changes the API of ReadOnlyLoadedValueIterator(): the method no longer returns an error but instead panics on failure. If this PR's code depends on this unreleased API change, add a comment explaining the dependency and ensure the code handles the new panic-based error semantics. If this is unintentional dependency drift, align with the stable v1.9.9 release.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@insecure/go.mod` at line 219, The go.mod currently pins a pseudo-version of
github.com/onflow/cadence that includes an API-breaking change:
ReadOnlyLoadedValueIterator() no longer returns an error and will panic on
failure; update the repository to either (a) explicitly document this dependency
by adding a comment in go.mod or an adjacent README explaining why the
pseudo-version v1.9.9-0.20260226153458-8a199b890dfa is required and that callers
must handle panic semantics from ReadOnlyLoadedValueIterator(), or (b) if the
change was accidental, revert the dependency to the stable v1.9.9 release to
keep the old error-returning behavior and update any code using
ReadOnlyLoadedValueIterator() accordingly; ensure you reference
ReadOnlyLoadedValueIterator() in the comment so reviewers know the rationale.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The PR adds a token movement inspection framework that:
Summary by CodeRabbit
New Features
Improvements
Tests
Chores