[Access] Add index for account transactions#8414
Merged
peterargue merged 34 commits intomasterfrom Feb 25, 2026
Merged
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:
📝 WalkthroughWalkthroughAdds an extended account-transactions API and backend with cursor-based pagination, storage/model changes, REST experimental endpoints and routing, node/RPC wiring, generated mocks, indexer/bootstrap refactors, and integration/test updates across indexer, storage, and REST layers. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/REST
participant Handler as Experimental Handler
participant Backend as Extended Backend
participant Storage as AccountTransactions Storage
Client->>Handler: GET /accounts/{addr}/transactions?limit&cursor&roles
Handler->>Handler: parse inputs, build filter
Handler->>Backend: GetAccountTransactions(ctx, addr, limit, cursor, filter, expand, encoding)
Backend->>Storage: TransactionsByAddress(addr, limit, cursor, filter)
Storage->>Storage: apply cursor, fetch limit+1, apply filter
Storage-->>Backend: AccountTransactionsPage{Transactions, NextCursor}
Backend->>Backend: enrich transactions (timestamp, body, result)
Backend-->>Handler: AccountTransactionsPage
Handler->>Client: JSON AccountTransactionsResponse{Transactions, NextCursor}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
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 Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/access/node_builder/access_node_builder.go (1)
2297-2336:⚠️ Potential issue | 🟠 MajorGuard against
extended-indexing-enabledwithout execution data indexing.
ExtendedStorageis only initialized inside theexecutionDataIndexingEnabledblock, but the extended backend is created wheneverextendedIndexingEnabledis true. If a user enables extended indexing without execution data indexing, this will dereference an uninitialized storage handle. Add a guard (and ideally mirror it in the observer builder).✅ Proposed fix (local guard)
if builder.extendedIndexingEnabled { + if !builder.executionDataIndexingEnabled { + return nil, fmt.Errorf("extended-indexing-enabled requires execution-data-indexing-enabled") + } builder.ExtendedBackend, err = extendedbackend.New( node.Logger, extendedbackend.DefaultConfig(),🤖 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 2297 - 2336, When creating the extended backend (call to extendedbackend.New) guard the path so that extendedIndexingEnabled is only honored when execution data indexing/ExtendedStorage is initialized: change the condition that currently checks builder.extendedIndexingEnabled to also require builder.ExtendedStorage (or the flag executionDataIndexingEnabled) be non-nil/true before calling extendedbackend.New, and return a clear error if extendedIndexingEnabled is set without execution data indexing; apply the same guard/mirroring in the observer builder initialization to avoid dereferencing an uninitialized ExtendedStorage.
🧹 Nitpick comments (9)
integration/localnet/AGENTS.md (1)
69-69: Consider updating to Docker Compose V2 CLI syntax (docker compose)
docker-compose(V1) reached end-of-life in July 2023. Most current Docker Desktop and CI environments ship only the V2 plugin, invoked asdocker compose(no hyphen). Keeping V1 syntax may silently break on developer machines that don't have it installed.📝 Proposed fix
-docker-compose -f docker-compose.nodes.yml build access_1 && docker-compose -f docker-compose.nodes.yml up -d access_1 +docker compose -f docker-compose.nodes.yml build access_1 && docker compose -f docker-compose.nodes.yml up -d access_1-- Specific node: `docker-compose -f docker-compose.nodes.yml logs -f access_1` +- Specific node: `docker compose -f docker-compose.nodes.yml logs -f access_1`Also applies to: 75-75
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration/localnet/AGENTS.md` at line 69, Replace deprecated V1 CLI invocations that use the hyphenated command string "docker-compose -f docker-compose.nodes.yml ..." with the V2 plugin syntax "docker compose -f docker-compose.nodes.yml ..." in the AGENTS.md entries (the two occurrences referenced around line 69 and line 75); update both commands so they use "docker compose" and verify the flags/order remain identical to preserve behavior.engine/access/rest/experimental/README.md (1)
3-3: Pinned commit hash will go stale as the spec evolves.The OpenAPI spec URL is locked to commit
cf44613233910eade91759399f94fadfccd37b72. As the spec inonflow/flowevolves, this link will fall behind. Consider using a branch-relative URL (e.g., pointing tomain) or updating it during releases.📄 Suggested fix
-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 links the OpenAPI spec to a specific commit hash which will go stale; update the URL string referencing https://github.com/onflow/flow/blob/cf44613233910eade91759399f94fadfccd37b72/openapi/experimental/openapi.yaml to use a branch-relative path (e.g., replace the commit hash with main or a release tag) or add a short sentence explaining the pinned-commit is intentional and must be updated during releases so the spec remains current.access/backends/extended/api.go (1)
23-31: Consider co-locatingAccountTransactionFilterwith the interface it parameterises.
AccountTransactionFilteris referenced in the publicAPIinterface but defined inbackend_account_transactions.go(an implementation file). This means the interface has a compile-time dependency on a type defined in its own implementation — a reversed dependency direction. Moving it toapi.go(or a dedicatedtypes.go) keeps the interface self-contained and makes the package boundary cleaner for future callers or alternative implementations.Additionally, the method comment does not document what an empty/nil
Rolesslice inAccountTransactionFiltermeans (no filtering vs. empty result). Documenting the zero-value semantics prevents misuse.♻️ Suggested reorganisation
Move
AccountTransactionFilter(and theAccountTransactionCursorre-export if needed) intoapi.go:package extended import ( "context" "github.com/onflow/flow/protobuf/go/flow/entities" accessmodel "github.com/onflow/flow-go/model/access" "github.com/onflow/flow-go/model/flow" ) +// AccountTransactionFilter filters account transactions by role. +// An empty or nil Roles slice means no filtering (all roles are returned). +type AccountTransactionFilter struct { + Roles []accessmodel.TransactionRole +} + // API defines the extended access API for querying account transaction history. type API interface { ... }Then remove the duplicate definition from
backend_account_transactions.go.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@access/backends/extended/api.go` around lines 23 - 31, The API interface references AccountTransactionFilter in GetAccountTransactions but that type is defined in an implementation file, creating a reversed dependency; move the AccountTransactionFilter definition (and any re-exported AccountTransactionCursor) into the same public API file where the interface is declared so the interface is self-contained, remove the duplicate/old definition from the implementation file, and update imports/usages accordingly; also add a method comment on GetAccountTransactions (or a doc comment on AccountTransactionFilter) explicitly documenting the zero-value/empty/nil semantics for the Roles slice (whether nil/empty means “no filtering” vs “match nothing”) so callers know expected behavior.integration/testnet/experimental_client.go (1)
21-26: Constructor never returns an error.
NewExperimentalAPIClienthas anerrorreturn but all paths returnnil. Consider simplifying the signature to just return*ExperimentalAPIClientunless you anticipate future validation (e.g., URL parsing).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration/testnet/experimental_client.go` around lines 21 - 26, The NewExperimentalAPIClient function currently declares an error return but never returns one; change its signature to return only *ExperimentalAPIClient (remove the error) and update all call sites to match, or alternatively add input validation (e.g., use net/url.Parse on baseURL inside NewExperimentalAPIClient and return a non-nil error if parsing fails) — locate the function NewExperimentalAPIClient and either remove the error from its signature and return values or add URL validation before creating swagger.NewConfiguration() so the error return is meaningfully used.integration/testnet/client.go (1)
503-503: Missing:separator before%win error wrapping.The same pattern repeats on lines 503, 508, and 512. Go convention is
"message: %w"so the wrapped error is visually separated.Proposed fix for all three
- return sdk.Address{}, nil, fmt.Errorf("failed to sign and send create account transaction %w", err) + return sdk.Address{}, nil, fmt.Errorf("failed to sign and send create account transaction: %w", err)- return sdk.Address{}, nil, fmt.Errorf("failed to wait for create account transaction to seal %w", err) + return sdk.Address{}, nil, fmt.Errorf("failed to wait for create account transaction to seal: %w", err)- return sdk.Address{}, nil, fmt.Errorf("failed to create new account %w", result.Error) + return sdk.Address{}, nil, fmt.Errorf("failed to create new account: %w", result.Error)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration/testnet/client.go` at line 503, Update the fmt.Errorf calls that wrap errors to include a colon before the %w so the wrapped error is clearly separated; specifically replace messages like "failed to sign and send create account transaction %w" with "failed to sign and send create account transaction: %w" (and do the same for the other two similar error messages in the same function/method where create account signing/sending occurs) so all fmt.Errorf usages use the "message: %w" pattern.engine/access/rest/router/routes_experimental.go (1)
9-15: Nit: doc comment says "Route" but the type is unexportedexperimentalRoute.📝 Suggested fix
-// Route defines an experimental API route. +// experimentalRoute defines an experimental API route. type experimentalRoute struct {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/access/rest/router/routes_experimental.go` around lines 9 - 15, The doc comment currently says "Route defines an experimental API route." but the type is unexported named experimentalRoute; update the comment to match the identifier (e.g., change the comment to "experimentalRoute defines an experimental API route.") or alternatively export the type by renaming experimentalRoute to Route and adjust usages accordingly; ensure the comment and the type name (experimentalRoute or Route) are consistent and reflect whether the type is exported.engine/access/rest/router/metrics.go (1)
56-78: Latent metrics-labeling ambiguity inMethodURLToRoute.The two sequential
TrimPrefixcalls strip/v1and/experimental/v1independently. If a future standard route is added whose pattern (after/v1is stripped) is identical to an experimental route pattern (e.g., a future/v1/accounts/{addr}/transactionsstandard endpoint), the firstTrimPrefixwould strip/v1and the resulting path would match the experimental route's regex, mislabeling the metric.This has no impact today (no conflicting patterns exist), but the approach doesn't distinguish route namespaces. A defensive alternative:
♻️ Suggested improvement
func MethodURLToRoute(method, url string) (string, error) { - path := strings.TrimPrefix(url, "/v1") - path = strings.TrimPrefix(path, "/experimental/v1") + var path string + switch { + case strings.HasPrefix(url, "/experimental/v1"): + path = strings.TrimPrefix(url, "/experimental/v1") + default: + path = strings.TrimPrefix(url, "/v1") + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/access/rest/router/metrics.go` around lines 56 - 78, MethodURLToRoute currently calls TrimPrefix("/v1") then TrimPrefix("/experimental/v1"), which can misclassify routes when patterns overlap; change the trimming logic to detect and remove the longest/more specific known prefix first (e.g., check and strip "/experimental/v1" before "/v1") or explicitly record which namespace prefix was matched and strip only that so the remaining path fed to matchers preserves namespace semantics; update the path normalization in MethodURLToRoute and ensure matchers are evaluated against the correctly namespaced path (use the existing matchers slice and m.method/m.re/m.name to locate where the change is applied).storage/indexes/account_transactions_bootstrapper_test.go (1)
146-153: Tests look thorough for the new pagination API.The assertions cover
TransactionID,BlockHeight,TransactionIndex, andRoles. The AI summary mentionsBlockTimestampas a field in the page structure — consider adding an assertion for it here to ensure it's populated correctly during bootstrap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@storage/indexes/account_transactions_bootstrapper_test.go` around lines 146 - 153, The test after calling store.TransactionsByAddress should also assert that the transaction's BlockTimestamp is correctly populated; update the subtest that examines page.Transactions[0] (the result of TransactionsByAddress) to include an assertion comparing page.Transactions[0].BlockTimestamp to the expected timestamp value used when bootstrapping the test transaction, alongside the existing checks for TransactionID, BlockHeight, TransactionIndex, and Roles.storage/indexes/account_transactions.go (1)
60-62: Typo: "constuction" → "construction".📝 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 60 - 62, The comment above the account transactions index mentions a typo: replace "constuction" with "construction" in the comment that references storage.ErrNotBootstrapped and BootstrapAccountTransactions (located near the account transactions index initialization block); update the comment text to read "construction" so the message becomes: "If the index has not been initialized, construction will fail with [storage.ErrNotBootstrapped]. The caller should retry with `BootstrapAccountTransactions` passing the required initialization data."
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@access/backends/extended/backend_account_transactions.go`:
- Around line 117-127: The default error branch after calling
b.store.TransactionsByAddress currently calls irrecoverable.Throw(ctx,
fmt.Errorf(...)) but then returns the raw err; change that final return to a
gRPC Internal status so unexpected errors return codes.Internal as documented.
Concretely, keep the irrecoverable.Throw(ctx, fmt.Errorf("failed to get account
transactions: %w", err)) call, and replace the subsequent "return nil, err" with
a return that wraps err using status.Errorf(codes.Internal, "failed to get
account transactions: %v", err) so the function consistently returns a gRPC
status error for unexpected failures.
In `@engine/access/rest/common/http_request_handler.go`:
- Around line 101-106: The FailedPrecondition branch currently maps
codes.FailedPrecondition to HTTP 404; change it to return HTTP 400 by replacing
http.StatusNotFound with http.StatusBadRequest in the codes.FailedPrecondition
case where h.errorResponse is called (keep the same msg built from
se.Message()). Use the codes.FailedPrecondition branch (and
se.Message()/h.errorResponse) to send a Bad Request (400) response; if the
failure represents a transient "try again later" state later, consider switching
that branch to http.StatusServiceUnavailable and adding a Retry-After header
instead.
In `@engine/access/rest/router/router.go`:
- Line 130: The experimental handler is using the main LinkGenerator so nested
Build() calls (transaction.Build / result.Build) create links that resolve
against v1SubRouter (producing /v1/transactions/...) instead of the experimental
prefix; fix by ensuring the experimental router has the transaction routes
registered or by creating and passing a LinkGenerator bound to the experimental
subrouter into experimental.NewHandler (so LinkGenerator.TransactionLink and
TransactionResultLink resolve against the experimental/v1 routes). Update the
code that constructs the handler (the NewHandler call) to pass the
experimental-specific LinkGenerator (or register getTransactionByID and
getTransactionResultByID on the experimental subrouter) so expanded links point
to /experimental/v1/transactions/{id}.
In `@integration/localnet/AGENTS.md`:
- Around line 40-42: In AGENTS.md update the three fenced code blocks (the shell
command examples such as the curl, grpcurl, and docker-compose lines) to include
a shell language specifier (e.g., ```sh) so each block becomes ```sh ... ``` to
satisfy markdownlint MD040 and enable syntax highlighting; ensure you add the
specifier to the blocks around the curl command, the grpcurl command, and the
docker-compose command referenced in the comment.
- Around line 1-3: The markdown heading hierarchy is incorrect: the top-level
"AGENTS.md" (h1) is followed by "Localnet (Local Network Testing)" as an h3;
update the heading structure by promoting "### Localnet (Local Network Testing)"
to "## Localnet (Local Network Testing)" (or insert an appropriate h2 section
before the h3) so that the sequence goes h1 → h2 → h3 and satisfies MD001;
ensure the visible heading text ("Localnet (Local Network Testing)") remains
unchanged.
In `@integration/testnet/client.go`:
- Line 494: Fix the typo and formatting in the error return string: replace the
literal "failed cusnctruct create account transaction %w" with a corrected
message that includes a colon and space before the wrapped error, e.g. "failed
to construct create account transaction: %w"; update the return statement that
returns (sdk.Address{}, nil, fmt.Errorf(...)) to use this corrected string.
In `@integration/tests/access/cohort3/extended_indexing_test.go`:
- Around line 270-284: In verifyPagination, avoid indexing allPaginated with the
allUnpaginated loop index without checking lengths: first assert the counts
match (compare len(allUnpaginated.Transactions) and len(allPaginated)) and fail
the test if they differ, or iterate using the smaller length
(min(len(allUnpaginated.Transactions), len(allPaginated))) and then compare
corresponding entries; reference the variables allUnpaginated, allPaginated and
the helper methods fetchAccountTransactions and collectAllPages when making the
change.
In `@module/state_synchronization/indexer/extended/extended_indexer_test.go`:
- Around line 656-665: The function generateBlockFixtures performs height-1
which underflows for height==0; add an explicit guard at the start of
generateBlockFixtures to validate height > 0 and fail the test with a clear
message (e.g., using require.Greater or t.Fatalf) before computing parent :=
g.Blocks().Fixture(...), so you never perform the subtraction on a zero uint64
and the test fails with a helpful error.
In `@storage/indexes/account_transactions.go`:
- Around line 237-282: The computed fetchLimit (fetchLimit := limit + 1) can
overflow when limit == math.MaxUint32; change fetchLimit to a wider type and
update comparisons to avoid wrapping: declare fetchLimit as uint64(fetchLimit)
(e.g., fetchLimit := uint64(limit) + 1) and update the collection check
(currently using uint32(len(collected)) >= fetchLimit) to compare
uint64(len(collected)) against fetchLimit; update any subsequent uses that
assume fetchLimit is uint32 (e.g., determining whether there are more results)
so they use the uint64 value or a capped boolean instead (references:
fetchLimit, limit, collected, and the iteration inside operation.IterateKeys).
---
Outside diff comments:
In `@cmd/access/node_builder/access_node_builder.go`:
- Around line 2297-2336: When creating the extended backend (call to
extendedbackend.New) guard the path so that extendedIndexingEnabled is only
honored when execution data indexing/ExtendedStorage is initialized: change the
condition that currently checks builder.extendedIndexingEnabled to also require
builder.ExtendedStorage (or the flag executionDataIndexingEnabled) be
non-nil/true before calling extendedbackend.New, and return a clear error if
extendedIndexingEnabled is set without execution data indexing; apply the same
guard/mirroring in the observer builder initialization to avoid dereferencing an
uninitialized ExtendedStorage.
---
Nitpick comments:
In `@access/backends/extended/api.go`:
- Around line 23-31: The API interface references AccountTransactionFilter in
GetAccountTransactions but that type is defined in an implementation file,
creating a reversed dependency; move the AccountTransactionFilter definition
(and any re-exported AccountTransactionCursor) into the same public API file
where the interface is declared so the interface is self-contained, remove the
duplicate/old definition from the implementation file, and update imports/usages
accordingly; also add a method comment on GetAccountTransactions (or a doc
comment on AccountTransactionFilter) explicitly documenting the
zero-value/empty/nil semantics for the Roles slice (whether nil/empty means “no
filtering” vs “match nothing”) so callers know expected behavior.
In `@engine/access/rest/experimental/README.md`:
- Line 3: The README links the OpenAPI spec to a specific commit hash which will
go stale; update the URL string referencing
https://github.com/onflow/flow/blob/cf44613233910eade91759399f94fadfccd37b72/openapi/experimental/openapi.yaml
to use a branch-relative path (e.g., replace the commit hash with main or a
release tag) or add a short sentence explaining the pinned-commit is intentional
and must be updated during releases so the spec remains current.
In `@engine/access/rest/router/metrics.go`:
- Around line 56-78: MethodURLToRoute currently calls TrimPrefix("/v1") then
TrimPrefix("/experimental/v1"), which can misclassify routes when patterns
overlap; change the trimming logic to detect and remove the longest/more
specific known prefix first (e.g., check and strip "/experimental/v1" before
"/v1") or explicitly record which namespace prefix was matched and strip only
that so the remaining path fed to matchers preserves namespace semantics; update
the path normalization in MethodURLToRoute and ensure matchers are evaluated
against the correctly namespaced path (use the existing matchers slice and
m.method/m.re/m.name to locate where the change is applied).
In `@engine/access/rest/router/routes_experimental.go`:
- Around line 9-15: The doc comment currently says "Route defines an
experimental API route." but the type is unexported named experimentalRoute;
update the comment to match the identifier (e.g., change the comment to
"experimentalRoute defines an experimental API route.") or alternatively export
the type by renaming experimentalRoute to Route and adjust usages accordingly;
ensure the comment and the type name (experimentalRoute or Route) are consistent
and reflect whether the type is exported.
In `@integration/localnet/AGENTS.md`:
- Line 69: Replace deprecated V1 CLI invocations that use the hyphenated command
string "docker-compose -f docker-compose.nodes.yml ..." with the V2 plugin
syntax "docker compose -f docker-compose.nodes.yml ..." in the AGENTS.md entries
(the two occurrences referenced around line 69 and line 75); update both
commands so they use "docker compose" and verify the flags/order remain
identical to preserve behavior.
In `@integration/testnet/client.go`:
- Line 503: Update the fmt.Errorf calls that wrap errors to include a colon
before the %w so the wrapped error is clearly separated; specifically replace
messages like "failed to sign and send create account transaction %w" with
"failed to sign and send create account transaction: %w" (and do the same for
the other two similar error messages in the same function/method where create
account signing/sending occurs) so all fmt.Errorf usages use the "message: %w"
pattern.
In `@integration/testnet/experimental_client.go`:
- Around line 21-26: The NewExperimentalAPIClient function currently declares an
error return but never returns one; change its signature to return only
*ExperimentalAPIClient (remove the error) and update all call sites to match, or
alternatively add input validation (e.g., use net/url.Parse on baseURL inside
NewExperimentalAPIClient and return a non-nil error if parsing fails) — locate
the function NewExperimentalAPIClient and either remove the error from its
signature and return values or add URL validation before creating
swagger.NewConfiguration() so the error return is meaningfully used.
In `@storage/indexes/account_transactions_bootstrapper_test.go`:
- Around line 146-153: The test after calling store.TransactionsByAddress should
also assert that the transaction's BlockTimestamp is correctly populated; update
the subtest that examines page.Transactions[0] (the result of
TransactionsByAddress) to include an assertion comparing
page.Transactions[0].BlockTimestamp to the expected timestamp value used when
bootstrapping the test transaction, alongside the existing checks for
TransactionID, BlockHeight, TransactionIndex, and Roles.
In `@storage/indexes/account_transactions.go`:
- Around line 60-62: The comment above the account transactions index mentions a
typo: replace "constuction" with "construction" in the comment that references
storage.ErrNotBootstrapped and BootstrapAccountTransactions (located near the
account transactions index initialization block); update the comment text to
read "construction" so the message becomes: "If the index has not been
initialized, construction will fail with [storage.ErrNotBootstrapped]. The
caller should retry with `BootstrapAccountTransactions` passing the required
initialization data."
Comment on lines
+117
to
+127
| page, err := b.store.TransactionsByAddress(address, limit, cursor, filter.Filter()) | ||
| if err != nil { | ||
| switch { | ||
| case errors.Is(err, storage.ErrNotBootstrapped): | ||
| return nil, status.Errorf(codes.FailedPrecondition, "account transaction index not initialized: %v", err) | ||
| case errors.Is(err, storage.ErrHeightNotIndexed): | ||
| return nil, status.Errorf(codes.OutOfRange, "requested height not indexed: %v", err) | ||
| default: | ||
| irrecoverable.Throw(ctx, fmt.Errorf("failed to get account transactions: %w", err)) | ||
| return nil, err | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "status.Errorf|codes.Internal|FailedPrecondition|OutOfRange" access/backends/extended -g '*.go'Repository: onflow/flow-go
Length of output: 1846
🏁 Script executed:
# Find the irrecoverable package definition and understand Throw behavior
fd -n "irrecoverable" --type f -e go | head -20Repository: onflow/flow-go
Length of output: 288
🏁 Script executed:
# Find the irrecoverable package definition
find . -name "irrecoverable.go" -o -path "*/irrecoverable/*" -type f | grep -E "\.go$"Repository: onflow/flow-go
Length of output: 296
🏁 Script executed:
# Check the Throw function to understand its behavior
cat -n ./module/irrecoverable/irrecoverable.go | head -100Repository: onflow/flow-go
Length of output: 4201
🏁 Script executed:
# Check the test file to understand expected behavior for unexpected errors
cat -n ./access/backends/extended/backend_account_transactions_test.go | sed -n '140,180p'Repository: onflow/flow-go
Length of output: 2320
🏁 Script executed:
# Continue reading the unexpected error test
cat -n ./access/backends/extended/backend_account_transactions_test.go | sed -n '175,210p'Repository: onflow/flow-go
Length of output: 1131
🏁 Script executed:
# Let's look at the actual current implementation in the file to verify
cat -n ./access/backends/extended/backend_account_transactions.go | sed -n '115,130p'Repository: onflow/flow-go
Length of output: 787
Return a gRPC Internal status for unexpected errors.
The code documents that unexpected errors should return codes.Internal, but the default branch returns a raw error instead. While the irrecoverable.Throw() call makes the subsequent return statement unreachable in normal operation, the code should still return a properly typed gRPC status error to match the documented API contract and remain consistent with the error handling at line 136 of the same file.
✅ Proposed fix
default:
irrecoverable.Throw(ctx, fmt.Errorf("failed to get account transactions: %w", err))
- return nil, err
+ return nil, status.Errorf(codes.Internal, "failed to get account transactions: %v", err)
}📝 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
| page, err := b.store.TransactionsByAddress(address, limit, cursor, filter.Filter()) | |
| if err != nil { | |
| switch { | |
| case errors.Is(err, storage.ErrNotBootstrapped): | |
| return nil, status.Errorf(codes.FailedPrecondition, "account transaction index not initialized: %v", err) | |
| case errors.Is(err, storage.ErrHeightNotIndexed): | |
| return nil, status.Errorf(codes.OutOfRange, "requested height not indexed: %v", err) | |
| default: | |
| irrecoverable.Throw(ctx, fmt.Errorf("failed to get account transactions: %w", err)) | |
| return nil, err | |
| } | |
| page, err := b.store.TransactionsByAddress(address, limit, cursor, filter.Filter()) | |
| if err != nil { | |
| switch { | |
| case errors.Is(err, storage.ErrNotBootstrapped): | |
| return nil, status.Errorf(codes.FailedPrecondition, "account transaction index not initialized: %v", err) | |
| case errors.Is(err, storage.ErrHeightNotIndexed): | |
| return nil, status.Errorf(codes.OutOfRange, "requested height not indexed: %v", err) | |
| default: | |
| irrecoverable.Throw(ctx, fmt.Errorf("failed to get account transactions: %w", err)) | |
| return nil, status.Errorf(codes.Internal, "failed to get account transactions: %v", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@access/backends/extended/backend_account_transactions.go` around lines 117 -
127, The default error branch after calling b.store.TransactionsByAddress
currently calls irrecoverable.Throw(ctx, fmt.Errorf(...)) but then returns the
raw err; change that final return to a gRPC Internal status so unexpected errors
return codes.Internal as documented. Concretely, keep the
irrecoverable.Throw(ctx, fmt.Errorf("failed to get account transactions: %w",
err)) call, and replace the subsequent "return nil, err" with a return that
wraps err using status.Errorf(codes.Internal, "failed to get account
transactions: %v", err) so the function consistently returns a gRPC status error
for unexpected failures.
| router.Use(middleware.MetricsMiddleware(b.restCollector)) | ||
|
|
||
| for _, r := range ExperimentalRoutes { | ||
| h := experimental.NewHandler(b.logger, backend, r.Handler, b.LinkGenerator, chain, maxRequestSize, maxResponseSize) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, examine the router.go file to understand LinkGenerator initialization and usage
cd /
find . -name "router.go" -path "*/access/rest/router/*" -type f 2>/dev/nullRepository: onflow/flow-go
Length of output: 113
🏁 Script executed:
#!/bin/bash
# Get context around LinkGenerator initialization (lines 50-60) and AddExperimentalRoutes (lines 116-138)
head -160 engine/access/rest/router/router.go | tail -140Repository: onflow/flow-go
Length of output: 4339
🏁 Script executed:
#!/bin/bash
# Search for LinkGenerator usage in experimental code
rg -n "LinkGenerator" --type=go engine/access/rest/experimental/ -A 2 -B 2Repository: onflow/flow-go
Length of output: 2004
🏁 Script executed:
#!/bin/bash
# Check how experimental handler uses LinkGenerator parameter
rg -n "NewHandler" --type=go engine/access/rest/experimental/ -A 5 -B 2Repository: onflow/flow-go
Length of output: 679
🏁 Script executed:
#!/bin/bash
# Search for link generation in experimental endpoints
rg -n "link\." --type=go engine/access/rest/experimental/ -C 3Repository: onflow/flow-go
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check the Handler.ServeHTTP method and how it uses linkGenerator
rg -n "ServeHTTP\|linkGenerator" --type=go engine/access/rest/experimental/handler.go -A 10 -B 2Repository: onflow/flow-go
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check the common.HttpHandler to see if it uses linkGenerator
rg -n "linkGenerator\|LinkGenerator" --type=go engine/access/rest/common/ -A 5Repository: onflow/flow-go
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Look at experimental endpoint implementations to see what they return
head -100 engine/access/rest/experimental/get_account_transactions.go | tail -70Repository: onflow/flow-go
Length of output: 2518
🏁 Script executed:
#!/bin/bash
# Search more broadly for any link generation in experimental endpoints
rg -n "Link\|link\|URL\|url" --type=go engine/access/rest/experimental/ | head -30Repository: onflow/flow-go
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Look at the full method to see all LinkGenerator usage
sed -n '42,150p' engine/access/rest/experimental/get_account_transactions.goRepository: onflow/flow-go
Length of output: 3696
🏁 Script executed:
#!/bin/bash
# Check what the Build method does with LinkGenerator
rg -n "func.*Build" --type=go engine/access/rest/common/models/ -A 3Repository: onflow/flow-go
Length of output: 9567
🏁 Script executed:
#!/bin/bash
# Find LinkGenerator implementation to understand what paths it generates
rg -n "NewLinkGeneratorImpl\|type LinkGeneratorImpl" --type=go engine/access/rest/common/ -A 20Repository: onflow/flow-go
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Find LinkGenerator implementation
rg -n "type.*LinkGenerator\|NewLinkGeneratorImpl" --type=go engine/access/rest/common/models/ -A 15Repository: onflow/flow-go
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check what LinkGeneratorImpl actually does - look for its methods
rg -n "LinkGeneratorImpl" --type=go engine/access/rest/common/models/ -B 2 -A 8Repository: onflow/flow-go
Length of output: 4432
🏁 Script executed:
#!/bin/bash
# Check the full implementation of Transaction.Build to see if it uses LinkGenerator
sed -n '9,56p' engine/access/rest/common/models/transaction.goRepository: onflow/flow-go
Length of output: 1429
🏁 Script executed:
#!/bin/bash
# Check TransactionResult.Build to see if it uses LinkGenerator
sed -n '88,116p' engine/access/rest/common/models/transaction.goRepository: onflow/flow-go
Length of output: 820
🏁 Script executed:
#!/bin/bash
# Check what routes are defined in ExperimentalRoutes vs v1 routes
rg -n "ExperimentalRoutes\|var Routes" --type=go engine/access/rest/router/ -A 30Repository: onflow/flow-go
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check the route structure to understand naming
head -50 engine/access/rest/router/routes.goRepository: onflow/flow-go
Length of output: 152
🏁 Script executed:
#!/bin/bash
# Find where ExperimentalRoutes and Routes are defined
rg -n "ExperimentalRoutes\|^var Routes" --type=go engine/access/rest/ -A 10Repository: onflow/flow-go
Length of output: 40
🏁 Script executed:
#!/bin/bash
# List router-related files
find engine/access/rest/router -type f -name "*.go"Repository: onflow/flow-go
Length of output: 357
🏁 Script executed:
#!/bin/bash
# Check experimental routes
head -60 engine/access/rest/router/routes_experimental.goRepository: onflow/flow-go
Length of output: 573
🏁 Script executed:
#!/bin/bash
# Check main routes
head -60 engine/access/rest/router/routes_main.goRepository: onflow/flow-go
Length of output: 1517
🏁 Script executed:
#!/bin/bash
# Verify: when LinkGenerator tries to find route "getAccountTransactions",
# does it find it on v1SubRouter or the experimental subrouter?
# Check if "getAccountTransactions" route is registered on v1SubRouter
rg -n "getAccountTransactions" --type=go engine/access/rest/router/Repository: onflow/flow-go
Length of output: 230
🏁 Script executed:
#!/bin/bash
# Check the experimental subrouter creation and route registration
sed -n '116,138p' engine/access/rest/router/router.goRepository: onflow/flow-go
Length of output: 809
🏁 Script executed:
#!/bin/bash
# Check if gorillamux.Router.Get() returns nil or error when route not found
rg -n "mux.Router\|Get.*route" --type=go engine/access/rest/ | head -20Repository: onflow/flow-go
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check how LinkGenerator errors are handled in the Build methods
sed -n '1,25p' engine/access/rest/common/models/transaction.goRepository: onflow/flow-go
Length of output: 837
🏁 Script executed:
#!/bin/bash
# Look for tests that verify experimental endpoint link generation
rg -n "getAccountTransactions\|LinkGenerator" --type=go engine/access/rest/experimental/ -A 5 -B 2Repository: onflow/flow-go
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check if there's a test for experimental routes and link generation
find engine/access/rest -name "*test.go" -type f | xargs grep -l "experimental\|ExperimentalRoutes" 2>/dev/nullRepository: onflow/flow-go
Length of output: 164
🏁 Script executed:
#!/bin/bash
# Check the experimental test file
cat engine/access/rest/experimental/get_account_transactions_test.goRepository: onflow/flow-go
Length of output: 9619
🏁 Script executed:
#!/bin/bash
# Check router metrics test to see what it's testing
rg -n "getAccountTransactions" --type=go engine/access/rest/router/metrics_test.go -B 10 -A 10Repository: onflow/flow-go
Length of output: 742
🏁 Script executed:
#!/bin/bash
# Check if there are tests that expand transactions in the experimental endpoint
rg -n "Expands\|expand" --type=go engine/access/rest/experimental/ -B 3 -A 3Repository: onflow/flow-go
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check test expectations - do they have _links or _expand parameters?
grep -n "_expand\|_links" engine/access/rest/experimental/get_account_transactions_test.goRepository: onflow/flow-go
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Check if routes like "getTransactionByID" and "getTransactionResultByID" are registered on experimental subrouter
rg -n "getTransactionByID\|getTransactionResultByID" --type=go engine/access/rest/router/routes_experimental.goRepository: onflow/flow-go
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Verify: Does the experimental endpoint actually expand and use LinkGenerator for link generation?
# Check if GetAccountTransactions ever calls transaction.Build with expanded data
grep -n "r.Expands\|\.Build" engine/access/rest/experimental/get_account_transactions.goRepository: onflow/flow-go
Length of output: 341
🏁 Script executed:
#!/bin/bash
# Double-check: are the transaction routes (getTransactionByID, getTransactionResultByID)
# only on v1SubRouter or also on experimental subrouter?
rg -n "getTransactionByID\|getTransactionResultByID" --type=go engine/access/rest/router/routes_main.goRepository: onflow/flow-go
Length of output: 40
🏁 Script executed:
#!/bin/bash
# Let me verify: when LinkGenerator.TransactionResultLink is called,
# what does it actually generate? Trace through the code.
# First, check all routes in routes_main.go to see the route patterns
grep -n "Pattern\|Name" engine/access/rest/router/routes_main.go | head -40Repository: onflow/flow-go
Length of output: 1432
🏁 Script executed:
#!/bin/bash
# Verify that only v1 routes exist, no separate experimental transaction routes
rg -n "Pattern.*transaction" --type=go engine/access/rest/router/Repository: onflow/flow-go
Length of output: 534
🏁 Script executed:
#!/bin/bash
# Check what mux.Router.Get() returns when route is not found
# Look at how errors are handled in link generation
sed -n '84,94p' engine/access/rest/common/models/link.goRepository: onflow/flow-go
Length of output: 432
🏁 Script executed:
#!/bin/bash
# Check the actual behavior when LinkGenerator can't find a route
# Look for error handling in Build methods
grep -A 5 "SelfLink\|resultLink" engine/access/rest/common/models/transaction.goRepository: onflow/flow-go
Length of output: 500
Experimental endpoints generate incorrect links when expanding nested data.
When GetAccountTransactions expands transaction data (via _expand=transaction or _expand=result), it calls transaction.Build() and result.Build() with the LinkGenerator. These methods generate self-links using LinkGenerator.TransactionLink() and LinkGenerator.TransactionResultLink(), which resolve routes against the v1SubRouter. Since routes like getTransactionByID and getTransactionResultByID only exist on the /v1 subrouter (not on /experimental/v1), the generated links will be /v1/transactions/{id} instead of the correct /experimental/v1/transactions/{id}.
Verify that the experimental endpoint patterns include these transaction routes, or create a separate LinkGenerator for the experimental subrouter using the experimental router instance.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/access/rest/router/router.go` at line 130, The experimental handler is
using the main LinkGenerator so nested Build() calls (transaction.Build /
result.Build) create links that resolve against v1SubRouter (producing
/v1/transactions/...) instead of the experimental prefix; fix by ensuring the
experimental router has the transaction routes registered or by creating and
passing a LinkGenerator bound to the experimental subrouter into
experimental.NewHandler (so LinkGenerator.TransactionLink and
TransactionResultLink resolve against the experimental/v1 routes). Update the
code that constructs the handler (the NewHandler call) to pass the
experimental-specific LinkGenerator (or register getTransactionByID and
getTransactionResultByID on the experimental subrouter) so expanded links point
to /experimental/v1/transactions/{id}.
integration/localnet/AGENTS.md
Outdated
Comment on lines
40
to
42
| ``` | ||
| curl -s http://localhost:4004/v1/blocks?height=latest | ||
| ``` |
Contributor
There was a problem hiding this comment.
Add language specifiers to fenced code blocks (MD040)
Three code blocks lack a language tag, which suppresses syntax highlighting and is flagged by markdownlint. Suggested languages: sh (or bash) for shell commands.
📝 Proposed fix
-```
+```sh
curl -s http://localhost:4004/v1/blocks?height=latest
```diff
-```
+```sh
grpcurl -plaintext localhost:4001 list
```diff
-```
+```sh
docker-compose -f docker-compose.nodes.yml build access_1 && docker-compose -f docker-compose.nodes.yml up -d access_1
</details>
Also applies to: 45-47, 68-70
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.21.0)</summary>
[warning] 40-40: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<details>
<summary>🤖 Prompt for AI Agents</summary>
Verify each finding against the current code and only fix it if needed.
In @integration/localnet/AGENTS.md around lines 40 - 42, In AGENTS.md update the
three fenced code blocks (the shell command examples such as the curl, grpcurl,
and docker-compose lines) to include a shell language specifier (e.g., sh) so each block becomes sh ... ``` to satisfy markdownlint MD040 and enable syntax
highlighting; ensure you add the specifier to the blocks around the curl
command, the grpcurl command, and the docker-compose command referenced in the
comment.
</details>
<!-- fingerprinting:phantom:triton:churro -->
<!-- This is an auto-generated comment by CodeRabbit -->
Comment on lines
+656
to
+665
| // generateBlockFixtures creates internally consistent fixture data for a block at the given height. | ||
| // The returned blockFixtures contain complete data including user collections, system transactions, | ||
| // and events. | ||
| func generateBlockFixtures(t *testing.T, g *fixtures.GeneratorSuite, height uint64) *blockFixtures { | ||
| // CompleteFixture takes a parent block and creates a child, so use height-1 for the parent | ||
| // to get the resulting block at the desired height. | ||
| parent := g.Blocks().Fixture(fixtures.Block.WithHeight(height - 1)) | ||
| fixture := testutil.CompleteFixture(t, g, parent) | ||
| require.Equal(t, height, fixture.Block.Height, "generated block has unexpected height") | ||
|
|
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n "generateBlockFixtures\(" --type goRepository: onflow/flow-go
Length of output: 1112
🏁 Script executed:
# First, let's check the actual file and the function in question
head -680 module/state_synchronization/indexer/extended/extended_indexer_test.go | tail -40Repository: onflow/flow-go
Length of output: 1790
🏁 Script executed:
# Let's also search for the function definition
rg -B 5 -A 15 "func generateBlockFixtures" --type goRepository: onflow/flow-go
Length of output: 2677
🏁 Script executed:
# Check the loop context around line 267
sed -n '260,275p' module/state_synchronization/indexer/extended/extended_indexer_test.goRepository: onflow/flow-go
Length of output: 708
🏁 Script executed:
# Check the loop context around line 300
sed -n '295,310p' module/state_synchronization/indexer/extended/extended_indexer_test.goRepository: onflow/flow-go
Length of output: 764
🏁 Script executed:
# Check the context around line 245 to see what liveHeight is
sed -n '240,250p' module/state_synchronization/indexer/extended/extended_indexer_test.goRepository: onflow/flow-go
Length of output: 571
🏁 Script executed:
# Check semantics of block heights - look for constants or patterns
rg -n "liveHeight|genesis" module/state_synchronization/indexer/extended/extended_indexer_test.go | head -20Repository: onflow/flow-go
Length of output: 1316
🏁 Script executed:
# Let me check if there are any other test patterns or if height 1 is ever used (genesis block)
rg -n "generateBlockFixtures.*1\)" module/state_synchronization/indexer/extended/extended_indexer_test.goRepository: onflow/flow-go
Length of output: 40
🏁 Script executed:
# Check if there are any patterns where height could be derived or come from a variable that might be 0
rg -n "for h :=" module/state_synchronization/indexer/extended/extended_indexer_test.goRepository: onflow/flow-go
Length of output: 446
🏁 Script executed:
# Look at the broader pattern: check what Flow considers as genesis block height
rg -n "height.*0\|genesis" module/state_synchronization/indexer/extended/extended_indexer_test.go | head -15Repository: onflow/flow-go
Length of output: 40
Guard against height==0 to avoid uint64 underflow.
When height==0, subtracting 1 causes an unsigned integer underflow, producing a parent block at uint64 max height and masking the actual problem with a confusing downstream error. Add an explicit check before the subtraction.
Per coding guidelines, treat all inputs as potentially byzantine and validate accordingly.
Proposed fix
func generateBlockFixtures(t *testing.T, g *fixtures.GeneratorSuite, height uint64) *blockFixtures {
+ require.True(t, height > 0, "height must be > 0")
// CompleteFixture takes a parent block and creates a child, so use height-1 for the parent
// to get the resulting block at the desired height.
parent := g.Blocks().Fixture(fixtures.Block.WithHeight(height - 1))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@module/state_synchronization/indexer/extended/extended_indexer_test.go`
around lines 656 - 665, The function generateBlockFixtures performs height-1
which underflows for height==0; add an explicit guard at the start of
generateBlockFixtures to validate height > 0 and fail the test with a clear
message (e.g., using require.Greater or t.Fatalf) before computing parent :=
g.Blocks().Fixture(...), so you never perform the subtraction on a zero uint64
and the test fails with a helpful error.
Comment on lines
237
to
282
| // We fetch limit+1 to determine if there are more results beyond this page. | ||
| fetchLimit := limit + 1 | ||
|
|
||
| var collected []access.AccountTransaction | ||
| skipFirst := cursor != nil // skip the entry at the exact cursor position | ||
|
|
||
| err := operation.IterateKeys(reader, startKey, endKey, | ||
| func(keyCopy []byte, getValue func(any) error) (bail bool, err error) { | ||
| addr, height, txIndex, err := decodeAccountTxKey(keyCopy) | ||
| if err != nil { | ||
| return true, fmt.Errorf("could not decode key: %w", err) | ||
| } | ||
|
|
||
| // Skip entries at or before the cursor position (it was the last item of the previous page). | ||
| if skipFirst { | ||
| // no need to check height > cursor.BlockHeight since the iterator bounds already | ||
| // omit heights outside of the range. | ||
| if height == cursor.BlockHeight && txIndex <= cursor.TransactionIndex { | ||
| return false, nil | ||
| } | ||
| // Past the cursor position. Stop skipping. | ||
| skipFirst = false | ||
| } | ||
|
|
||
| var stored storedAccountTransaction | ||
| if err := getValue(&stored); err != nil { | ||
| return true, fmt.Errorf("could not unmarshal value: %w", err) | ||
| } | ||
|
|
||
| address, height, txIndex, err := decodeAccountTxKey(keyCopy) | ||
| if err != nil { | ||
| return true, fmt.Errorf("could not decode key: %w", err) | ||
| } | ||
|
|
||
| results = append(results, access.AccountTransaction{ | ||
| Address: address, | ||
| tx := access.AccountTransaction{ | ||
| Address: addr, | ||
| BlockHeight: height, | ||
| TransactionID: stored.TransactionID, | ||
| TransactionIndex: txIndex, | ||
| Roles: stored.Roles, | ||
| }) | ||
| } | ||
|
|
||
| if filter != nil && !filter(&tx) { | ||
| return false, nil | ||
| } | ||
|
|
||
| collected = append(collected, tx) | ||
|
|
||
| if uint32(len(collected)) >= fetchLimit { | ||
| return true, nil // bail after collecting enough | ||
| } |
Contributor
There was a problem hiding this comment.
fetchLimit overflows to 0 when limit == math.MaxUint32, causing an empty result.
fetchLimit := limit + 1 silently wraps to 0 for limit == math.MaxUint32. The >= fetchLimit check on line 280 then becomes >= 0 which is always true, so iteration bails immediately with zero results and no error.
While unlikely in practice, this is a latent correctness bug. Consider either capping the effective limit or promoting fetchLimit to uint64.
🛡️ Proposed fix
- // We fetch limit+1 to determine if there are more results beyond this page.
- fetchLimit := limit + 1
+ // We fetch limit+1 to determine if there are more results beyond this page.
+ // Use uint64 to avoid overflow when limit == math.MaxUint32.
+ fetchLimit := uint64(limit) + 1And update line 280 accordingly:
- if uint32(len(collected)) >= fetchLimit {
+ if uint64(len(collected)) >= fetchLimit {And line 293:
- if uint32(len(collected)) > limit {
+ if uint64(len(collected)) > uint64(limit) {🤖 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 - 282, The computed
fetchLimit (fetchLimit := limit + 1) can overflow when limit == math.MaxUint32;
change fetchLimit to a wider type and update comparisons to avoid wrapping:
declare fetchLimit as uint64(fetchLimit) (e.g., fetchLimit := uint64(limit) + 1)
and update the collection check (currently using uint32(len(collected)) >=
fetchLimit) to compare uint64(len(collected)) against fetchLimit; update any
subsequent uses that assume fetchLimit is uint32 (e.g., determining whether
there are more results) so they use the uint64 value or a capped boolean instead
(references: fetchLimit, limit, collected, and the iteration inside
operation.IterateKeys).
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
engine/access/rest/common/http_request_handler.go (1)
93-96: Misleading error message prefix forcodes.Internal(HTTP 500).The prefix
"Invalid Flow request: "implies the client sent a bad request, butcodes.Internal/ HTTP 500 indicates a server-side failure. Consider using a server-oriented prefix like"Internal server error: "to avoid confusing API consumers debugging 500 responses.Suggested fix
case codes.Internal: - msg := fmt.Sprintf("Invalid Flow request: %s", se.Message()) + msg := fmt.Sprintf("Internal server error: %s", se.Message()) h.errorResponse(w, http.StatusInternalServerError, msg, errorLogger)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/access/rest/common/http_request_handler.go` around lines 93 - 96, The error message for the gRPC-to-HTTP mapping using the codes.Internal branch currently uses a client-sided prefix "Invalid Flow request: " which is misleading; update the message construction in the codes.Internal case (where msg is set and h.errorResponse is called) to use a server-oriented prefix such as "Internal server error: " (or similar) before se.Message(), so the HTTP 500 response clearly indicates a server-side failure when h.errorResponse(w, http.StatusInternalServerError, msg, errorLogger) is invoked.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@engine/access/rest/common/http_request_handler.go`:
- Around line 101-106: The handler currently maps gRPC codes.FailedPrecondition
to HTTP 404 which is misleading; update the switch case handling
codes.FailedPrecondition in http_request_handler.go (the block that builds msg
and calls h.errorResponse) to return HTTP 400 (http.StatusBadRequest) instead of
http.StatusNotFound; if the semantics are instead "not ready, try later" change
to HTTP 503 (http.StatusServiceUnavailable) and set a Retry-After header before
calling h.errorResponse so clients can retry appropriately.
---
Nitpick comments:
In `@engine/access/rest/common/http_request_handler.go`:
- Around line 93-96: The error message for the gRPC-to-HTTP mapping using the
codes.Internal branch currently uses a client-sided prefix "Invalid Flow
request: " which is misleading; update the message construction in the
codes.Internal case (where msg is set and h.errorResponse is called) to use a
server-oriented prefix such as "Internal server error: " (or similar) before
se.Message(), so the HTTP 500 response clearly indicates a server-side failure
when h.errorResponse(w, http.StatusInternalServerError, msg, errorLogger) is
invoked.
fxamacker
reviewed
Feb 23, 2026
Member
fxamacker
left a comment
fxamacker
left a comment
There was a problem hiding this comment.
This is a large PR, so I left the first batch of comments and suggestions for now.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
access/backends/extended/backend_account_transactions.go (1)
110-131:⚠️ Potential issue | 🟠 MajorReturn gRPC Internal for unexpected errors after
irrecoverable.Throw.The function currently returns raw errors in the default branch and when enrichment fails, which can leak non-status errors to gRPC callers. Returning
codes.Internalkeeps the API contract consistent even if the irrecoverable path is reached.🛠️ Suggested fix
default: irrecoverable.Throw(ctx, fmt.Errorf("failed to get account transactions: %w", err)) - return nil, err + return nil, status.Errorf(codes.Internal, "failed to get account transactions: %v", err) } @@ if err != nil { err = fmt.Errorf("failed to populate details for transaction %s: %w", page.Transactions[i].TransactionID, err) irrecoverable.Throw(ctx, err) - return nil, err + return nil, status.Errorf(codes.Internal, "failed to populate details for transaction %s: %v", page.Transactions[i].TransactionID, err) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@access/backends/extended/backend_account_transactions.go` around lines 110 - 131, The code sometimes returns raw errors after calling irrecoverable.Throw (in the default branch after TransactionsByAddress and inside the loop after enrichTransaction), which may leak non-gRPC errors to callers; update both places to convert the error into a gRPC Internal status (e.g., return status.Errorf(codes.Internal, "internal error: %v", err)) instead of returning err directly, keeping irrecoverable.Throw calls but ensuring the function returns a gRPC Internal error for the default TransactionsByAddress branch and for the enrichment failure in the loop.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@access/backends/extended/backend_account_transactions.go`:
- Around line 110-131: The code sometimes returns raw errors after calling
irrecoverable.Throw (in the default branch after TransactionsByAddress and
inside the loop after enrichTransaction), which may leak non-gRPC errors to
callers; update both places to convert the error into a gRPC Internal status
(e.g., return status.Errorf(codes.Internal, "internal error: %v", err)) instead
of returning err directly, keeping irrecoverable.Throw calls but ensuring the
function returns a gRPC Internal error for the default TransactionsByAddress
branch and for the enrichment failure in the loop.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
access/backends/extended/backend_account_transactions.gomodel/access/account_transaction.go
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
storage/indexes/account_transactions.go (2)
283-290:NextCursorstill points to the last collected item (collected[limit-1]) rather than the peeked item (collected[limit]).Pointing
NextCursoratcollected[limit-1]requires theskipFirstclosure variable (line 231) and forces the next call to re-scan entries withincursor.BlockHeightbefore finding the first eligible entry. Pointing it instead at the already-retrieved peeked item (collected[limit]) eliminates the skip entirely and is cheaper when the cursor falls inside a block with many transactions.🤖 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 283 - 290, The NextCursor is being set to collected[limit-1] but should point to the peeked item collected[limit]; change the assignment so page.NextCursor uses collected[limit] (i.e., set BlockHeight/TransactionIndex from collected[limit]) to avoid relying on the skipFirst closure and re-scanning within cursor.BlockHeight — update the block that sets page.Transactions and page.NextCursor (referencing collected, limit, page.NextCursor, access.AccountTransactionCursor and the skipFirst behavior) so the cursor points to the peeked entry instead of the last returned entry.
227-228:fetchLimitstill overflows to 0 whenlimit == math.MaxUint32.
fetchLimit := limit + 1wraps to 0 forlimit == math.MaxUint32, making the>= fetchLimitguard on line 270 always true, so the iterator bails immediately with zero results. AlthoughvalidateLimitinTransactionsByAddressis expected to rejectmath.MaxUint32before reaching here,lookupAccountTransactionsis not protected internally and depends entirely on the caller's discipline.🤖 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 227 - 228, The fetchLimit calculation in lookupAccountTransactions can overflow for limit == math.MaxUint32 because fetchLimit := limit + 1 wraps to 0; update lookupAccountTransactions to avoid internal reliance on callers by handling the edge case: detect if limit == math.MaxUint32 (or convert limit to a wider type like uint64 before adding) and in that case set fetchLimit to uint64(limit) (or cap to math.MaxUint64) or use a bool/overflow-safe check so fetchLimit never wraps to 0; ensure the >= fetchLimit guard still works. Reference: lookupAccountTransactions, TransactionsByAddress, variable fetchLimit, and validateLimit.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@storage/indexes/account_transactions.go`:
- Around line 137-152: Update the TransactionsByAddress doc comment to include
storage.ErrInvalidQuery as a possible returned error (since validateLimit can
return errors.Join(storage.ErrInvalidQuery, ...)), and modify the return path
that forwards the validateCursorHeight error so it is wrapped with the same
contextual message used elsewhere (e.g., wrap the error returned from
validateCursorHeight when called in TransactionsByAddress) to keep error
wrapping consistent; reference symbols: TransactionsByAddress, validateLimit,
validateCursorHeight, storage.ErrInvalidQuery.
---
Duplicate comments:
In `@storage/indexes/account_transactions.go`:
- Around line 283-290: The NextCursor is being set to collected[limit-1] but
should point to the peeked item collected[limit]; change the assignment so
page.NextCursor uses collected[limit] (i.e., set BlockHeight/TransactionIndex
from collected[limit]) to avoid relying on the skipFirst closure and re-scanning
within cursor.BlockHeight — update the block that sets page.Transactions and
page.NextCursor (referencing collected, limit, page.NextCursor,
access.AccountTransactionCursor and the skipFirst behavior) so the cursor points
to the peeked entry instead of the last returned entry.
- Around line 227-228: The fetchLimit calculation in lookupAccountTransactions
can overflow for limit == math.MaxUint32 because fetchLimit := limit + 1 wraps
to 0; update lookupAccountTransactions to avoid internal reliance on callers by
handling the edge case: detect if limit == math.MaxUint32 (or convert limit to a
wider type like uint64 before adding) and in that case set fetchLimit to
uint64(limit) (or cap to math.MaxUint64) or use a bool/overflow-safe check so
fetchLimit never wraps to 0; ensure the >= fetchLimit guard still works.
Reference: lookupAccountTransactions, TransactionsByAddress, variable
fetchLimit, and validateLimit.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
storage/indexes/account_transactions.gostorage/indexes/helpers.go
✅ Files skipped from review due to trivial changes (1)
- storage/indexes/helpers.go
Comment on lines
137
to
152
| // Expected error returns during normal operations: | ||
| // - [storage.ErrHeightNotIndexed] if the requested range extends beyond indexed heights | ||
| // - [storage.ErrInvalidQuery] if the query is invalid | ||
| // - [storage.ErrHeightNotIndexed] if the cursor height extends beyond indexed heights | ||
| func (idx *AccountTransactions) TransactionsByAddress( | ||
| account flow.Address, | ||
| startHeight uint64, | ||
| endHeight uint64, | ||
| ) ([]access.AccountTransaction, error) { | ||
| latestHeight := idx.latestHeight.Load() | ||
| if startHeight > latestHeight { | ||
| return nil, fmt.Errorf("start height %d is greater than latest indexed height %d: %w", | ||
| startHeight, latestHeight, storage.ErrHeightNotIndexed) | ||
| } | ||
|
|
||
| if startHeight < idx.firstHeight { | ||
| return nil, fmt.Errorf("start height %d is before first indexed height %d: %w", | ||
| startHeight, idx.firstHeight, storage.ErrHeightNotIndexed) | ||
| limit uint32, | ||
| cursor *access.AccountTransactionCursor, | ||
| filter storage.IndexFilter[*access.AccountTransaction], | ||
| ) (access.AccountTransactionsPage, error) { | ||
| if err := validateLimit(limit); err != nil { | ||
| return access.AccountTransactionsPage{}, errors.Join(storage.ErrInvalidQuery, err) | ||
| } | ||
|
|
||
| if endHeight > latestHeight { | ||
| endHeight = latestHeight | ||
| } | ||
|
|
||
| if startHeight > endHeight { | ||
| return nil, fmt.Errorf("start height %d is greater than end height %d: %w", startHeight, endHeight, storage.ErrInvalidQuery) | ||
| latestHeight := idx.latestHeight.Load() | ||
| if cursor != nil { | ||
| if err := validateCursorHeight(cursor.BlockHeight, idx.firstHeight, latestHeight); err != nil { | ||
| return access.AccountTransactionsPage{}, err |
Contributor
There was a problem hiding this comment.
Doc comment is missing storage.ErrInvalidQuery as a possible error return; cursor validation error lacks context wrapping.
Two minor issues:
-
Doc omission: Lines 137–138 list only
storage.ErrHeightNotIndexed, but line 146 can returnerrors.Join(storage.ErrInvalidQuery, …)whenlimitis invalid. Callers relying solely on the documented error set will be surprised. -
Inconsistent error wrapping: Line 152 returns the
validateCursorHeighterror directly, while every other error path in this file wraps with a"could not …"context string. Either both or neither should be wrapped for consistency.
📝 Proposed patch
// Expected error returns during normal operations:
+// - [storage.ErrInvalidQuery] if limit is 0 or equals math.MaxUint32
// - [storage.ErrHeightNotIndexed] if the cursor height extends beyond indexed heights if err := validateCursorHeight(cursor.BlockHeight, idx.firstHeight, latestHeight); err != nil {
- return access.AccountTransactionsPage{}, err
+ return access.AccountTransactionsPage{}, fmt.Errorf("invalid cursor height: %w", err)
}🤖 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 137 - 152, Update the
TransactionsByAddress doc comment to include storage.ErrInvalidQuery as a
possible returned error (since validateLimit can return
errors.Join(storage.ErrInvalidQuery, ...)), and modify the return path that
forwards the validateCursorHeight error so it is wrapped with the same
contextual message used elsewhere (e.g., wrap the error returned from
validateCursorHeight when called in TransactionsByAddress) to keep error
wrapping consistent; reference symbols: TransactionsByAddress, validateLimit,
validateCursorHeight, storage.ErrInvalidQuery.
Co-authored-by: Faye Amacker <33205765+fxamacker@users.noreply.github.com>
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
fxamacker
reviewed
Feb 24, 2026
Member
fxamacker
left a comment
fxamacker
left a comment
There was a problem hiding this comment.
I only left one more comment.
Caveat: I am not familiar with node builder and changes to the integration tests, so maybe somebody else can review those.
Co-authored-by: Faye Amacker <33205765+fxamacker@users.noreply.github.com>
fxamacker
reviewed
Feb 24, 2026
Member
fxamacker
left a comment
fxamacker
left a comment
There was a problem hiding this comment.
Thanks for updating! I left a few more comments.
fxamacker
approved these changes
Feb 24, 2026
| if height > cursor.BlockHeight { // heights are descending | ||
| return false, nil | ||
| } | ||
| if height == cursor.BlockHeight && txIndex < cursor.TransactionIndex { |
Member
There was a problem hiding this comment.
unlike heights, txIndex is increasing, right? better add comments.
|
|
||
| // the cursor is the next entry to return. skip all entries before it. | ||
| if cursor != nil { | ||
| if height > cursor.BlockHeight { // heights are descending |
Member
There was a problem hiding this comment.
can we iterate from min(highestHeight, cursor.BlockHeight)? this could skip lots of pages.
Contributor
Author
There was a problem hiding this comment.
that's what it's doing now, I just included this check for completeness.
| for i, tx := range data.Transactions { | ||
| txIndex := uint32(i) | ||
| txID := tx.ID() | ||
| _, isSystemTx := a.systemCollections.SearchAll(txID) |
Member
There was a problem hiding this comment.
we don't have to search it anymore once isSystemChunk becomes true
|
|
||
| log = log.With().Str("component", "extended_indexer").Logger() | ||
| c := &ExtendedIndexer{ | ||
| log: log.With().Str("component", "extended_indexer").Logger(), |
Contributor
Author
There was a problem hiding this comment.
I moved it up so log can be reused
zhangchiqing
approved these changes
Feb 24, 2026
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.
Summary by CodeRabbit
New Features
Documentation
Bug Fixes