[Access] Add experimental account transfers rest API#8419
[Access] Add experimental account transfers rest API#8419peterargue wants to merge 41 commits intomasterfrom
Conversation
Co-authored-by: Leo Zhang <zhangchiqing@gmail.com>
Co-authored-by: Leo Zhang <zhangchiqing@gmail.com>
Co-authored-by: Leo Zhang <zhangchiqing@gmail.com>
📝 WalkthroughWalkthroughThis PR adds extended indexing for account transactions and token transfers (fungible and non-fungible) to Flow's access layer. It introduces new Pebble-backed storage indexes, backend APIs with pagination and filtering, experimental REST endpoints for querying account transaction history and transfers, and integration with the access node indexer pipeline to populate these indexes from execution data. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
engine/access/rest/server.go (1)
43-77:⚠️ Potential issue | 🔴 CriticalUpdate
cmd/util/cmd/run-script/cmd.go:173to pass theextendedBackendparameter torest.NewServer.The call at line 173 is missing the
extendedBackendparameter. It currently passes 10 arguments but should pass 11. Addnil(or the appropriate value) as the final parameter before the closing parenthesis.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/access/rest/server.go` around lines 43 - 77, The NewServer function signature includes an extendedBackend parameter (extended.API) but the caller in cmd/util/cmd/run-script/cmd.go currently invokes rest.NewServer with only 10 args; update that call to pass the missing eleventh argument (use nil if no extended backend is available) so the call matches NewServer(ctx, serverAPI, config, logger, chain, restCollector, stateStreamApi, stateStreamConfig, enableNewWebsocketsStreamAPI, wsConfig, extendedBackend).
🟡 Minor comments (14)
integration/localnet/AGENTS.md-3-3 (1)
3-3:⚠️ Potential issue | 🟡 MinorMinor markdown lint issues flagged by static analysis.
The heading at Line 3 jumps from h1 (
#) to h3 (###), skipping h2. Also, code blocks at lines 40, 45, and 68 are missing language identifiers (e.g.,bashorshell).Proposed fix
-### Localnet (Local Network Testing) +## Localnet (Local Network Testing)And for the fenced code blocks, add language identifiers like
```bashinstead of bare```.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration/localnet/AGENTS.md` at line 3, Update the markdown structure and fenced code blocks in AGENTS.md: change the heading "Localnet (Local Network Testing)" from an h3 to an h2 (use "## Localnet (Local Network Testing)") to restore proper heading order, and add language identifiers to the bare fenced code blocks (replace ``` with ```bash or ```shell as appropriate for the snippets referenced around the Localnet section) so static analysis no longer flags missing code-block languages.integration/testnet/client.go-494-494 (1)
494-494:⚠️ Potential issue | 🟡 MinorTypo in error message: "cusnctruct" → "construct".
✏️ Proposed fix
- return sdk.Address{}, nil, fmt.Errorf("failed cusnctruct create account transaction %w", err) + return sdk.Address{}, nil, fmt.Errorf("failed to construct create account transaction: %w", err)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration/testnet/client.go` at line 494, Fix the typo in the error message returned from the account creation failure (the statement returning sdk.Address{}, nil, fmt.Errorf("failed cusnctruct create account transaction %w", err)); change "cusnctruct" to "construct" so the fmt.Errorf reads "failed construct create account transaction %w" (or better: "failed to construct create account transaction %w") to produce a clear, correctly spelled error string.model/access/account_transfer.go-22-22 (1)
22-22:⚠️ Potential issue | 🟡 MinorMinor doc inconsistency:
EventIndicescomment says "Index" (singular) but the field is a slice.The same issue exists on line 39 for
NonFungibleTokenTransfer.✏️ Proposed fix
- EventIndices []uint32 // Index of the event within the transaction + EventIndices []uint32 // Indices of the events within the transaction🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@model/access/account_transfer.go` at line 22, The comments for the slice fields are misleading: update the doc comment for EventIndices (in the struct that declares EventIndices []uint32) to use the plural "Indices" and reflect that it's a slice (e.g., "Indices of the events within the transaction"), and do the same for the EventIndices field inside the NonFungibleTokenTransfer struct so both comments correctly indicate multiple indices rather than a single "Index".engine/access/rest/experimental/request/transfer_cursor.go-43-54 (1)
43-54:⚠️ Potential issue | 🟡 Minor
EncodeTransferCursorwill panic on nil input, same asEncodeAccountTransactionCursorin the sibling file.🛡️ Proposed fix
func EncodeTransferCursor(cursor *accessmodel.TransferCursor) (string, error) { + if cursor == nil { + return "", nil + } c := transferCursor{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/access/rest/experimental/request/transfer_cursor.go` around lines 43 - 54, EncodeTransferCursor dereferences a nil pointer and can panic; add a nil-check at the top of EncodeTransferCursor (same pattern used in EncodeAccountTransactionCursor) and return an empty string and nil error when cursor == nil to avoid accessing transferCursor fields; update the EncodeTransferCursor function to immediately handle nil before constructing transferCursor and keep existing JSON marshal and base64 encode logic otherwise.engine/access/rest/experimental/request/get_account_transactions.go-92-102 (1)
92-102:⚠️ Potential issue | 🟡 Minor
EncodeAccountTransactionCursorwill panic on nil input.If
cursoris nil, line 94 dereferences it (cursor.BlockHeight), causing a nil-pointer panic. The correspondingEncodeTransferCursorintransfer_cursor.gohas the same pattern. Consider adding a nil guard or documenting the precondition.🛡️ Proposed fix
func EncodeAccountTransactionCursor(cursor *accessmodel.AccountTransactionCursor) (string, error) { + if cursor == nil { + return "", nil + } c := accountTransactionCursor{🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/access/rest/experimental/request/get_account_transactions.go` around lines 92 - 102, The EncodeAccountTransactionCursor function dereferences cursor without a nil check which will panic; add a nil guard at the top of EncodeAccountTransactionCursor (and mirror the same guard in EncodeTransferCursor) that returns an appropriate zero value (e.g., empty string and nil error or a clear error) when cursor == nil; implement the chosen behavior consistently across accountTransactionCursor encoding functions and ensure you reference EncodeAccountTransactionCursor and EncodeTransferCursor when making the change.integration/tests/access/cohort3/extended_indexing_test.go-270-284 (1)
270-284:⚠️ Potential issue | 🟡 MinorMissing length assertion before indexing into
allPaginated— potential panic.Line 276 indexes into
allPaginated[i]using the loop counter fromallUnpaginated.Transactions, but there's no prior assertion that both slices have the same length. If the paginated result has fewer entries, this will panic with an index-out-of-range.Proposed fix
func (s *ExtendedIndexingSuite) verifyPagination(address string) { // Paginating through all results should yield the same count as a single unpaginated request allUnpaginated := s.fetchAccountTransactions(address, nil) allPaginated := s.collectAllPages(address, 1, nil, nil) + s.Require().Equal(len(allUnpaginated.Transactions), len(allPaginated), + "paginated and unpaginated result counts should match") + for i, unpagedTx := range allUnpaginated.Transactions { pagedTx := allPaginated[i]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration/tests/access/cohort3/extended_indexing_test.go` around lines 270 - 284, In verifyPagination, assert that the paginated and unpaginated result counts match before indexing: after obtaining allUnpaginated := s.fetchAccountTransactions(address, nil) and allPaginated := s.collectAllPages(address, 1, nil, nil), add a length assertion (e.g. s.Require().Equal(len(allUnpaginated.Transactions), len(allPaginated)) or s.Require().Len(allPaginated, len(allUnpaginated.Transactions))) so the test fails safely instead of panicking when iterating and accessing allPaginated[i]; this keeps the rest of the comparisons in verifyPagination valid.engine/access/rest/experimental/models/model_account_transactions_response.go-11-13 (1)
11-13:⚠️ Potential issue | 🟡 Minor
Transactions []AccountTransactionwithoutomitemptywill serialize as"transactions":nullfor a nil slice.REST clients typically expect
[]for empty collections rather thannull. The field should either:
- Always be initialized to a non-nil slice by callers building this response (and rely on the no-
omitemptytag to always emit"transactions":[]), or- Be initialized in the constructor/builder that produces
AccountTransactionsResponse.If consumers already guarantee the slice is never nil (e.g., the backend returns an empty
[]AccountTransaction{}), this is fine as-is — just worth an explicit callout in the builder. If the field can be nil, the JSON output will be surprising.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/access/rest/experimental/models/model_account_transactions_response.go` around lines 11 - 13, AccountTransactionsResponse currently has Transactions []AccountTransaction which may serialize to "transactions":null; ensure the slice is never nil by initializing it to an empty slice in the response construction path (e.g., in the constructor/builder that produces AccountTransactionsResponse or wherever AccountTransactionsResponse values are created) so that Transactions is always set to []AccountTransaction{} for empty results; alternatively, if you want to allow omission, add `omitempty` to the `transactions` json tag, but prefer initializing the Transactions slice in the code that builds AccountTransactionsResponse to guarantee clients see an empty array rather than null.engine/access/rest/experimental/README.md-3-3 (1)
3-3:⚠️ Potential issue | 🟡 MinorOpenAPI spec link hardcoded to a commit hash — will become stale.
Pinning to
cf44613233910eade91759399f94fadfccd37b72means the README will always point to that specific revision, even after the spec is updated. Consider using a branch reference (e.g.,masterormain) or a tagged release so the link stays current:-The OpenAPI spec for this api can be found at https://github.com/onflow/flow/blob/cf44613233910eade91759399f94fadfccd37b72/openapi/experimental/openapi.yaml +The OpenAPI spec for this api can be found at https://github.com/onflow/flow/blob/main/openapi/experimental/openapi.yaml🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/access/rest/experimental/README.md` at line 3, The README currently hardcodes the OpenAPI URL with a specific commit hash (the string "https://github.com/onflow/flow/blob/cf44613233910eade91759399f94fadfccd37b72/openapi/experimental/openapi.yaml"); replace that commit-hash URL with a stable reference (e.g., use the branch name like "main" or a released tag) so the link remains up-to-date, or point to a short-lived redirect/manifest if your process requires immutable pins—update the single line containing the URL in the README to use the chosen branch/tag reference.engine/access/rest/experimental/models/fungible_token_transfer.go-26-26 (1)
26-26:⚠️ Potential issue | 🟡 MinorPotential nil-pointer panic:
transfer.Amountis*big.Int.Calling
.String()on a nil*big.Intwill panic. While a well-formedFungibleTokenTransferfrom storage should always carry a non-nilAmount, storage or indexer bugs could produce a nil value and crash the request handler.🛡️ Proposed defensive fix
- t.Amount = transfer.Amount.String() + if transfer.Amount != nil { + t.Amount = transfer.Amount.String() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/access/rest/experimental/models/fungible_token_transfer.go` at line 26, The assignment t.Amount = transfer.Amount.String() can panic if transfer.Amount is nil; update the code around the FungibleTokenTransfer conversion (the line assigning t.Amount from transfer.Amount) to defensively check if transfer.Amount == nil and set a safe default string (e.g., "0" or empty string) instead of calling .String(); optionally add a debug/warn log mentioning the nil amount and the enclosing function name to aid tracing.module/state_synchronization/indexer/extended/transfers/ft_group.go-127-140 (1)
127-140:⚠️ Potential issue | 🟡 Minor
ResolvePairsis not idempotent — calling it twice appends duplicate burn entries.Each call appends unmatched withdrawals (burns) to
g.pairedResults. If called more than once, burn entries will be duplicated. Consider adding a guard or documenting single-call semantics.Proposed fix
func (g *ftTxEventGroup) ResolvePairs() []ftPairedResult { // find all unmatched withdrawals for uuid, wIdx := range g.withdrawalByUUID { if _, ok := g.matchedDeposits[uuid]; ok { continue } + // Mark as matched to prevent duplicate burn entries on repeated calls. + g.matchedDeposits[uuid] = struct{}{} // Unmatched withdrawal -- treat as burn. g.pairedResults = append(g.pairedResults, ftPairedResult{ sourceEvents: mergeEvents(g.withdrawals[wIdx], nil), withdrawal: &g.withdrawals[wIdx].decoded, }) } return g.pairedResults }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@module/state_synchronization/indexer/extended/transfers/ft_group.go` around lines 127 - 140, ResolvePairs on ftTxEventGroup appends unmatched withdrawals into g.pairedResults every call, causing duplicate "burn" entries; make it idempotent by adding a guard or reset. Update ResolvePairs to either (a) return early if already resolved (e.g., introduce a bool field like pairsResolved on ftTxEventGroup and check it at the start, setting it true before returning), or (b) clear/rehydrate g.pairedResults at the start of ResolvePairs (e.g., g.pairedResults = g.pairedResults[:0]) before iterating withdrawalByUUID and matchedDeposits to rebuild results; use the existing symbols ResolvePairs, ftTxEventGroup, pairedResults, withdrawalByUUID, matchedDeposits to locate and modify the method.storage/indexes/account_transactions.go-237-238 (1)
237-238:⚠️ Potential issue | 🟡 MinorPotential uint32 overflow if
limitequalsmath.MaxUint32.
fetchLimit := limit + 1wraps to0whenlimit == math.MaxUint32, causing the function to return an empty page. While unlikely in practice (API likely caps limits much lower), consider guarding or documenting this.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/indexes/account_transactions.go` around lines 237 - 238, The calculation fetchLimit := limit + 1 can overflow when limit == math.MaxUint32; update the logic in the pagination code in account_transactions.go (the fetchLimit computation) to guard against overflow by checking if limit == math.MaxUint32 and handling that case (e.g., set fetchLimit to limit or promote to a larger integer type/uint64 before adding) so you never wrap to 0; reference the fetchLimit variable and the surrounding pagination logic to implement the check.storage/indexes/account_ft_transfers.go-243-244 (1)
243-244:⚠️ Potential issue | 🟡 MinorSame uint32 overflow edge case as
lookupAccountTransactions.
fetchLimit := limit + 1wraps to 0 whenlimit == math.MaxUint32, though this is unlikely to occur in practice.storage/indexes/account_ft_transfers.go-438-451 (1)
438-451:⚠️ Potential issue | 🟡 Minor
big.Int.Bytes()loses sign information.
big.Int.Bytes()returns the absolute value, andSetBytes()always produces a non-negative result (Line 284). IfAmountwere ever negative, the sign would be silently dropped during round-trip serialization. This is likely fine for token transfer amounts but worth documenting the non-negative invariant.storage/indexes/account_transactions.go-59-87 (1)
59-87:⚠️ Potential issue | 🟡 MinorMinor typo in doc comment: "constuction" → "construction".
Line 61:
constuctionshould beconstruction.Proposed fix
-// If the index has not been initialized, constuction will fail with [storage.ErrNotBootstrapped]. +// If the index has not been initialized, construction will fail with [storage.ErrNotBootstrapped].🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/indexes/account_transactions.go` around lines 59 - 87, Fix the typo in the doc comment for NewAccountTransactions: change "constuction" to "construction" in the function comment block above the NewAccountTransactions(db storage.DB) signature so the comment reads "If the index has not been initialized, construction will fail with [storage.ErrNotBootstrapped]." Ensure only the comment text is modified and no code behavior is changed.
| extendedIndexerDependable := module.NewProxiedReadyDoneAware() | ||
| builder.IndexerDependencies.Add(extendedIndexerDependable) |
There was a problem hiding this comment.
Critical: extendedIndexerDependable is never initialized when extended indexing is disabled, blocking the execution data indexer.
extendedIndexerDependable is unconditionally added to builder.IndexerDependencies (Line 867), but it is only initialized via Init() inside the if builder.extendedIndexingEnabled block (Line 1131). When executionDataIndexingEnabled=true but extendedIndexingEnabled=false, this dependency is never satisfied, causing the "execution data indexer" DependableComponent (Line 995) to hang indefinitely waiting for it.
Follow the pattern used by other optional dependables (e.g., versionControlDependable at Line 2103, stopControlDependable at Line 2137) by initializing with a noop when the feature is disabled:
🐛 Proposed fix
extendedIndexerDependable := module.NewProxiedReadyDoneAware()
builder.IndexerDependencies.Add(extendedIndexerDependable)
+
+ if !builder.extendedIndexingEnabled {
+ extendedIndexerDependable.Init(&module.NoopReadyDoneAware{})
+ }📝 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.
| extendedIndexerDependable := module.NewProxiedReadyDoneAware() | |
| builder.IndexerDependencies.Add(extendedIndexerDependable) | |
| extendedIndexerDependable := module.NewProxiedReadyDoneAware() | |
| builder.IndexerDependencies.Add(extendedIndexerDependable) | |
| if !builder.extendedIndexingEnabled { | |
| extendedIndexerDependable.Init(&module.NoopReadyDoneAware{}) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/access/node_builder/access_node_builder.go` around lines 866 - 867,
extendedIndexerDependable is added to builder.IndexerDependencies but only
initialized inside the builder.extendedIndexingEnabled Init() path, so when
extendedIndexingEnabled=false the execution data indexer DependableComponent
blocks waiting for it; fix by following the existing pattern used for
versionControlDependable/stopControlDependable: initialize
extendedIndexerDependable to the noop variant when the feature is disabled
(i.e., create a noop ProxiedReadyDoneAware instance instead of leaving it nil)
and then Add it to builder.IndexerDependencies so Init()/Ready/Done semantics
are satisfied even when builder.extendedIndexingEnabled is false.
| }).Module("extended index database", func(node *cmd.NodeConfig) error { | ||
| if !builder.extendedIndexingEnabled { | ||
| return nil | ||
| } | ||
|
|
||
| extendedStorage, err := extended.OpenExtendedIndexDB( | ||
| node.Logger, | ||
| builder.extendedIndexingDBPath, | ||
| builder.State.Params().SealedRoot().Height, | ||
| ) | ||
| if err != nil { | ||
| return fmt.Errorf("could not open extended index database: %w", err) | ||
| } | ||
| builder.ExtendedStorage = extendedStorage | ||
|
|
||
| builder.ShutdownFunc(func() error { | ||
| if err := builder.ExtendedStorage.DB.Close(); err != nil { | ||
| return fmt.Errorf("error closing extended indexer db: %w", err) | ||
| } | ||
| return nil | ||
| }) | ||
|
|
||
| return nil | ||
| }).Module("last full block height consumer progress", func(node *cmd.NodeConfig) error { |
There was a problem hiding this comment.
Guard extended indexing config to avoid nil deref when execution-data indexing is off.
ExtendedStorage and lastFullBlockHeight are only initialized inside the executionDataIndexingEnabled block, but ExtendedBackend creation is gated solely by extendedIndexingEnabled. If an operator enables extended indexing without execution-data indexing, this will panic when ExtendedStorage.* or lastFullBlockHeight are used. Please either enforce extendedIndexingEnabled ⇒ executionDataIndexingEnabled in flag validation or move extended index DB/lastFullBlockHeight initialization outside the execution-data-indexing gate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmd/observer/node_builder/observer_builder.go` around lines 1398 - 1421, The
code opens ExtendedStorage and sets lastFullBlockHeight only when
executionDataIndexingEnabled is true but constructs ExtendedBackend when
extendedIndexingEnabled is true, which can cause nil derefs; update the module
that creates ExtendedStorage/lastFullBlockHeight (using
extended.OpenExtendedIndexDB and setting builder.ExtendedStorage and
builder.lastFullBlockHeight) so it runs whenever extendedIndexingEnabled is true
(move initialization out of the execution-data-indexing gate) or alternatively
add a validation gate that enforces extendedIndexingEnabled ⇒
executionDataIndexingEnabled and return a clear error during flag validation;
also guard the builder.ShutdownFunc and any use-sites of builder.ExtendedStorage
or lastFullBlockHeight behind the same condition so they are never accessed when
nil.
| // NOTE: The filter logic is currently inverted. Despite the comment in the source saying | ||
| // "skip flow fee deposits", the code actually KEEPS only transfers TO the flow fees address | ||
| // (it continues/skips when RecipientAddress != flowFeesAddress). These tests verify the | ||
| // current behavior, not the intended behavior described in the comments. | ||
|
|
||
| t.Run("keeps transfers TO flow fees address with non-zero amount (current inverted behavior)", func(t *testing.T) { | ||
| transfers := []access.FungibleTokenTransfer{ | ||
| { | ||
| RecipientAddress: flowFeesAddress, | ||
| Amount: big.NewInt(100), | ||
| TokenType: "A.0x1.FlowToken", | ||
| }, | ||
| } | ||
|
|
||
| result := a.filterFTTransfers(transfers) | ||
| require.Len(t, result, 1) | ||
| assert.Equal(t, flowFeesAddress, result[0].RecipientAddress) | ||
| }) | ||
|
|
||
| t.Run("filters out transfers NOT to flow fees address (current inverted behavior)", func(t *testing.T) { | ||
| otherAddress := flow.HexToAddress("0x1234567890abcdef") | ||
| transfers := []access.FungibleTokenTransfer{ | ||
| { | ||
| RecipientAddress: otherAddress, | ||
| Amount: big.NewInt(100), | ||
| TokenType: "A.0x1.FlowToken", | ||
| }, | ||
| } | ||
|
|
||
| result := a.filterFTTransfers(transfers) | ||
| assert.Empty(t, result) | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the filterFTTransfers method implementation
ast-grep --pattern $'func ($_) filterFTTransfers($$$) $_ {
$$$
}'Repository: onflow/flow-go
Length of output: 1764
🏁 Script executed:
# Verify the test expectations are actually failing by checking the test more carefully
cat -n module/state_synchronization/indexer/extended/account_transfers_test.go | sed -n '178,209p'Repository: onflow/flow-go
Length of output: 1486
🏁 Script executed:
# Let me verify by looking at the complete context of the production code with more lines
cat -n module/state_synchronization/indexer/extended/account_transfers.go | sed -n '143,165p'Repository: onflow/flow-go
Length of output: 981
🏁 Script executed:
# Let me cross-verify the logic by checking if there are any other implementations or if this test file was recently added
git log --oneline -n 20 module/state_synchronization/indexer/extended/account_transfers_test.goRepository: onflow/flow-go
Length of output: 121
🏁 Script executed:
# Let me trace through the logic step by step by checking what the tests actually expect vs what the code does
cat -n module/state_synchronization/indexer/extended/account_transfers_test.go | sed -n '40,70p'Repository: onflow/flow-go
Length of output: 1296
🏁 Script executed:
# Let me verify by running a logical trace: what does the code actually do?
# If RecipientAddress == flowFeesAddress: continue (skip) -> NOT included in result
# If RecipientAddress != flowFeesAddress: append -> IS included in result
# So: transfers TO flowFees are SKIPPED, transfers NOT to flowFees are KEPT
# Let me check if maybe there's another branch that modifies this
cat -n module/state_synchronization/indexer/extended/account_transfers.go | sed -n '130,160p'Repository: onflow/flow-go
Length of output: 1330
🏁 Script executed:
# Double-check by examining the actual test execution expectations
# Test 1: sends transfer with RecipientAddress = flowFeesAddress, expects len = 1
# Production code: if recipient == flowFees, continue (skip)
# Result: transfer would NOT be in output → test would fail with len = 0
# Let me check if there's a filter in the other direction or if tests are actually passing
git log --all --oneline | grep -i "account_transfer\|filter" | head -20Repository: onflow/flow-go
Length of output: 1501
🏁 Script executed:
# Let me verify the logic is what I think by creating a simple trace
# Production code: if recipient == flowFees THEN continue (skip it)
# This means: flowFees transfers are REMOVED, others are KEPT
#
# Test 1 expects: transfer TO flowFees is KEPT (but code removes it)
# Test 2 expects: transfer NOT to flowFees is REMOVED (but code keeps it)
#
# So tests are inverted. Let me confirm by checking if these tests were added in the same commit
git show 3652635fb5 --stat | head -30Repository: onflow/flow-go
Length of output: 1733
🏁 Script executed:
# Let me verify my analysis is correct by checking the complete context around where filterFTTransfers is called
rg "filterFTTransfers" -A 3 -B 3Repository: onflow/flow-go
Length of output: 4883
🏁 Script executed:
# Let me check all the test cases for filterFTTransfers to see the full picture
cat -n module/state_synchronization/indexer/extended/account_transfers_test.go | sed -n '163,240p'Repository: onflow/flow-go
Length of output: 3105
🏁 Script executed:
# Let me get the rest of the test to see the complete picture
cat -n module/state_synchronization/indexer/extended/account_transfers_test.go | sed -n '240,260p'Repository: onflow/flow-go
Length of output: 776
🏁 Script executed:
# Let me verify the test would actually fail by understanding the complete logic one more time
# Production code at line 147-148: if recipient == flowFees, continue (skip it)
# This means: transfers TO flowFees are FILTERED OUT, others are KEPT
#
# Test at line 193: expects result.Len() == 1 when input has transfer TO flowFees
# This would FAIL because production code filters it out
#
# Let me check if there are any other implementations or recent changes
git log --all --pretty=format:"%h %s" module/state_synchronization/indexer/extended/account_transfers.go | head -5Repository: onflow/flow-go
Length of output: 121
🏁 Script executed:
# Final verification: run the tests to confirm they would fail
cat -n module/state_synchronization/indexer/extended/account_transfers_test.go | sed -n '163,176p'Repository: onflow/flow-go
Length of output: 511
These tests are inverted and would fail against the current production code.
The production code at lines 147–148 of account_transfers.go skips (filters out) transfers where RecipientAddress == flowFeesAddress, then keeps all others. However, the tests expect the opposite: keeping transfers TO the flow fees address and filtering out transfers NOT to it.
Specifically:
- Test at line 193 sends a transfer TO flowFeesAddress and expects
len(result) == 1, but the code skips that transfer, so it would fail withlen(result) == 0 - Test at line 208 sends a transfer NOT to flowFeesAddress and expects the result to be empty, but the code keeps that transfer, so it would fail with
len(result) == 1 - The mixed test at line 259 also expects only transfers TO flowFees to be kept, which contradicts the production logic
Additionally, the test comment itself (lines 178–181) is inaccurate—it claims the code "continues/skips when RecipientAddress != flowFeesAddress", but the code actually skips when RecipientAddress == flowFeesAddress.
Either the production code logic is missing a negation (should skip != instead of ==), or all these tests need to be rewritten to match the current (apparently correct) production logic. The current state is not viable—these tests will not pass.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@module/state_synchronization/indexer/extended/account_transfers_test.go`
around lines 178 - 209, The tests for filterFTTransfers are inverted relative to
production: the code in account_transfers.go currently skips transfers where
RecipientAddress == flowFeesAddress, but the tests expect the opposite. Fix the
tests (and their header comment) to reflect the actual behavior of
filterFTTransfers: 1) update the test cases to expect that transfers TO
flowFeesAddress are filtered out (len==0) and that transfers NOT to
flowFeesAddress are kept (len==1 and RecipientAddress equals the other address);
2) update the descriptive comment that currently claims the logic is inverted to
state the real behavior (skip when RecipientAddress == flowFeesAddress); and 3)
adjust the mixed-case test expectations accordingly so all assertions match the
implementation of filterFTTransfers.
| func ftEventIndex(entry access.FungibleTokenTransfer) uint32 { | ||
| // use the last event index. this is either the deposit event or the last withdrawal event | ||
| // if the vault was destroyed. | ||
| return entry.EventIndices[len(entry.EventIndices)-1] | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, find the file and understand the context around ftEventIndex
fd -t f "account_ft_transfers.go" --exec wc -l {}Repository: onflow/flow-go
Length of output: 244
🏁 Script executed:
# Search for EventIndices definition and usage
rg -n 'EventIndices' --type=go -B2 -A2 | head -120Repository: onflow/flow-go
Length of output: 8899
🏁 Script executed:
# Find the FungibleTokenTransfer struct definition
rg -n 'type FungibleTokenTransfer' --type=go -A10Repository: onflow/flow-go
Length of output: 43567
🏁 Script executed:
# Check the callers of ftEventIndex function
rg -n 'ftEventIndex' --type=go -B3 -A3Repository: onflow/flow-go
Length of output: 2207
🏁 Script executed:
# Look for any validation of EventIndices before ftEventIndex is called
rg -n 'len.*EventIndices' --type=go -B2 -A2Repository: onflow/flow-go
Length of output: 1963
🏁 Script executed:
# Look at the Store method implementation to see if it validates EventIndices
rg -n 'func.*Store.*transfers' storage/indexes/account_ft_transfers.go -A 20 | head -60Repository: onflow/flow-go
Length of output: 842
🏁 Script executed:
# Check for any validation on EventIndices before use
rg -n 'if.*len.*EventIndices' storage/indexes/account_ft_transfers.goRepository: onflow/flow-go
Length of output: 40
🏁 Script executed:
# Search for where FungibleTokenTransfer is created from execution data
rg -n 'EventIndices.*=|EventIndices:' storage/indexes/account_ft_transfers.go -B 2 -A 2Repository: onflow/flow-go
Length of output: 498
🏁 Script executed:
# Check the bootstrapper to see how EventIndices is populated
cat storage/indexes/account_ft_transfers_bootstrapper.go | head -100Repository: onflow/flow-go
Length of output: 3883
🏁 Script executed:
# Search for where FungibleTokenTransfer is created/constructed from execution data
rg -n 'FungibleTokenTransfer{|&access.FungibleTokenTransfer' --type=go | head -40Repository: onflow/flow-go
Length of output: 214
🏁 Script executed:
# Look for where EventIndices is set during transfer creation
rg -n 'EventIndices:' --type=go -B 3 -A 1 | head -80Repository: onflow/flow-go
Length of output: 5869
🏁 Script executed:
# Check the indexFTTransfers function to see if it validates transfers
sed -n '220,360p' storage/indexes/account_ft_transfers.goRepository: onflow/flow-go
Length of output: 5306
🏁 Script executed:
# Check if Store method or any caller validates EventIndices before calling indexFTTransfers
sed -n '185,210p' storage/indexes/account_ft_transfers.goRepository: onflow/flow-go
Length of output: 840
🏁 Script executed:
# Check initializeFTTransfers to see if it validates EventIndices
rg -n 'func initializeFTTransfers' storage/indexes/account_ft_transfers.go -A 30 | head -50Repository: onflow/flow-go
Length of output: 1341
🏁 Script executed:
# Verify there's no runtime guarantee that EventIndices is non-empty by checking where transfers come from
rg -n 'Store.*transfers' storage/indexes/account_ft_transfers_bootstrapper.go -A 10 | head -30Repository: onflow/flow-go
Length of output: 1453
🏁 Script executed:
# Check if there's any validation when transfers slice is received
sed -n '185,210p' storage/indexes/account_ft_transfers_bootstrapper.goRepository: onflow/flow-go
Length of output: 40
Potential panic if EventIndices is empty or nil.
entry.EventIndices[len(entry.EventIndices)-1] will panic if EventIndices is an empty or nil slice. There is no validation of EventIndices at any layer in the call chain—not in Store(), indexFTTransfers(), initializeFTTransfers(), or lookupFTTransfers(). Per the coding guidelines ("Treat all inputs as potentially byzantine and validate accordingly"), EventIndices should be validated before use.
Add a guard in ftEventIndex or validate EventIndices at the Store() entry point:
Proposed fix
func ftEventIndex(entry access.FungibleTokenTransfer) uint32 {
+ if len(entry.EventIndices) == 0 {
+ return 0
+ }
// use the last event index. this is either the deposit event or the last withdrawal event
// if the vault was destroyed.
return entry.EventIndices[len(entry.EventIndices)-1]
}Alternatively, validate in Store() and return an error for transfers with empty EventIndices.
Note: The same vulnerability exists in account_nft_transfers.go at line 415.
📝 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.
| func ftEventIndex(entry access.FungibleTokenTransfer) uint32 { | |
| // use the last event index. this is either the deposit event or the last withdrawal event | |
| // if the vault was destroyed. | |
| return entry.EventIndices[len(entry.EventIndices)-1] | |
| } | |
| func ftEventIndex(entry access.FungibleTokenTransfer) uint32 { | |
| if len(entry.EventIndices) == 0 { | |
| return 0 | |
| } | |
| // use the last event index. this is either the deposit event or the last withdrawal event | |
| // if the vault was destroyed. | |
| return entry.EventIndices[len(entry.EventIndices)-1] | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@storage/indexes/account_ft_transfers.go` around lines 431 - 435, ftEventIndex
currently dereferences entry.EventIndices without checking length, which can
panic when EventIndices is nil/empty; update the validation to reject transfers
with empty EventIndices at the Store() entry path (where indexFTTransfers(),
initializeFTTransfers(), and lookupFTTransfers() begin) by returning an error
for any access.FungibleTokenTransfer with len(EventIndices) == 0, and also add a
defensive guard inside ftEventIndex that checks len(entry.EventIndices) > 0 and
returns a safe zero value (or a documented sentinel) instead of indexing into
the slice; apply the same validation/defensive-guard pattern to the analogous
function in account_nft_transfers.go.
| package mock | ||
|
|
||
| import ( | ||
| "github.com/jordanschalm/lockctx" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check go.mod and go.sum for lockctx
echo "=== Checking go.mod/go.sum ==="
grep -n "jordanschalm/lockctx" go.mod go.sum 2>/dev/null || echo "lockctx not found in go.mod/go.sum"
# Check usage across the codebase
echo -e "\n=== Checking usage in codebase ==="
rg -n "jordanschalm/lockctx" --type goRepository: onflow/flow-go
Length of output: 17769
🏁 Script executed:
# Check the specific file mentioned in the review
echo "=== Reading the file with lockctx import ==="
cat -n storage/mock/account_transactions_bootstrapper.go | head -30Repository: onflow/flow-go
Length of output: 1297
🏁 Script executed:
# Check if lockctx is used in production code or only in test/mock code
echo "=== Checking context of lockctx usage ==="
rg -B 2 -A 5 "lockctx\." --type go | head -50Repository: onflow/flow-go
Length of output: 3450
Address the v0.0.0 pseudo-version risk for the lockctx dependency.
github.com/jordanschalm/lockctx is pinned in go.mod but uses a v0.0.0 pseudo-version (v0.0.0-20250412215529-226f85c10956), indicating it tracks a specific commit rather than a stable release. This dependency is deeply integrated into production code across 150+ files (storage, state, execution, consensus, and main entry points), not isolated to test/mock code. For long-term stability, consider: (1) whether a v1.0.0+ stable release is available, (2) if the v0.0.0 strategy is intentional and documented, or (3) whether a vendored copy would reduce risk.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@storage/mock/account_transactions_bootstrapper.go` at line 8, The project
imports the module github.com/jordanschalm/lockctx (seen in the import list)
which is pinned to a v0.0.0 pseudo-version; inspect go.mod and replace the
pseudo-version with a stable tag if a v1.0.0+ (or other maintained release)
exists, or explicitly document and justify the pseudo-version choice in the repo
README/CONTRIBUTING and add a comment near the import referencing that decision,
or vendor the dependency (update module vendor directory and add a note in
go.mod/GOVENDOR.json) so production code
(storage/state/execution/consensus/main) does not rely on a floating commit.
Ensure you update go.sum accordingly and run go mod tidy and run tests to
validate behavior after changing github.com/jordanschalm/lockctx.
1 participant
Summary by CodeRabbit
Release Notes
New Features
Documentation