[Access] Add endpoints for getting receipts#8480
Open
peterargue wants to merge 1 commit intomasterfrom
Open
Conversation
Contributor
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.OpenSSF Scorecard
Scanned Files
|
Contributor
📝 WalkthroughWalkthroughThis pull request introduces a new public API surface for accessing execution receipts via two new methods: GetExecutionReceiptsByBlockID and GetExecutionReceiptsByResultID. The changes span API interfaces, gRPC/REST implementations, backend business logic, mock utilities, and integration tests. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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 |
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
🧹 Nitpick comments (3)
engine/access/rpc/backend/backend.go (1)
315-315: Fail fast ifExecutionReceiptsis missing inNew.Since the new receipt APIs dereference this dependency, adding constructor validation avoids nil-deref failures at runtime.
Suggested patch
func New(params Params) (*Backend, error) { + if params.ExecutionReceipts == nil { + return nil, fmt.Errorf("missing required dependency: ExecutionReceipts") + } + loggedScripts, err := lru.New[[md5.Size]byte, time.Time](common.DefaultLoggedScriptsCacheSize)As per coding guidelines
Use dependency injection patterns for component composition and implement proper interfaces before concrete types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/access/rpc/backend/backend.go` at line 315, The constructor New in backend.go should validate that params.ExecutionReceipts is non-nil and return an error immediately if missing to avoid nil-dereference later; update the New function (the code that sets receipts: params.ExecutionReceipts) to check params.ExecutionReceipts at the start, return a descriptive error (e.g., "missing ExecutionReceipts") instead of constructing the backend, and keep the rest of the initialization unchanged so callers must inject a valid ExecutionReceipts implementation.integration/tests/access/cohort1/access_api_test.go (1)
1472-1480: Make the receipt set key explicit to avoid ambiguous concatenation.Using direct string concatenation for composite identity is brittle; add a delimiter (or use a small struct key) for clearer intent and safer matching.
♻️ Small clarity refactor
- key := convert.MessageToIdentifier(receipt.Meta.ExecutorId).String() + - convert.MessageToIdentifier(receipt.Meta.ResultId).String() + key := convert.MessageToIdentifier(receipt.Meta.ExecutorId).String() + ":" + + convert.MessageToIdentifier(receipt.Meta.ResultId).String()- key := convert.MessageToIdentifier(receipt.Meta.ExecutorId).String() + - convert.MessageToIdentifier(receipt.Meta.ResultId).String() + key := convert.MessageToIdentifier(receipt.Meta.ExecutorId).String() + ":" + + convert.MessageToIdentifier(receipt.Meta.ResultId).String()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@integration/tests/access/cohort1/access_api_test.go` around lines 1472 - 1480, The composite key built in the loops (variable key in the receipts iteration that uses convert.MessageToIdentifier(...) for receipt.Meta.ExecutorId and receipt.Meta.ResultId) is formed by naive string concatenation which can be ambiguous; change the key to include an explicit delimiter (e.g. executorId + "|" + resultId) or replace the string key with a small struct type (e.g. type receiptKey struct { Executor, Result string } used in byBlockSet map) so byBlockSet, receiptsByBlock and receiptsByResult comparisons are unambiguous.engine/access/rest/http/routes/execution_receipts_test.go (1)
57-125: Add malformed-ID negative tests for both receipt endpoints.Coverage is good for success and NotFound, but request parsing for malformed
block_idand malformed{resultID}is not asserted. Adding those cases will protect the validation paths from regressions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@engine/access/rest/http/routes/execution_receipts_test.go` around lines 57 - 125, Add negative subtests to TestGetExecutionReceiptsByBlockID and TestGetExecutionReceiptsByResultID that exercise malformed IDs: call getReceiptsByBlockIDReq with an invalid block_id string (e.g. "not-a-valid-id") and call getReceiptsByResultIDReq with an invalid result id path segment, then assert the router returns HTTP 400 and the appropriate JSON error body for invalid parameters. Place these new subtests alongside the existing cases in TestGetExecutionReceiptsByBlockID and TestGetExecutionReceiptsByResultID; they do not need backend mocks (do not set expectations on mock.API) and should validate request parsing/validation for getReceiptsByBlockIDReq and getReceiptsByResultIDReq.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@engine/access/apiproxy/access_api_proxy.go`:
- Around line 441-451: Both GetExecutionReceiptsByBlockID and
GetExecutionReceiptsByResultID always call the upstream, bypassing the local
index; update them to check the router's index flag and use the local index
handler when enabled. In each function (GetExecutionReceiptsByBlockID and
GetExecutionReceiptsByResultID) branch on h.useIndex (or the existing
local-index boolean/field) and call the corresponding index method (e.g.,
h.index.GetExecutionReceiptsByBlockID / h.index.GetExecutionReceiptsByResultID)
when true, otherwise fall back to h.upstream. Ensure you keep the same logging
pattern (h.log with UpstreamApiService or an IndexApiService tag) and return the
response and error as before.
In `@engine/access/rest/common/models/execution_receipt.go`:
- Around line 17-23: The Build method on ExecutionReceipt dereferences receipt
and link immediately which will panic on nil inputs; add defensive checks at the
start of ExecutionReceipt.Build to validate receipt and link (and any required
nested fields like receipt.ExecutionResult or receipt.ExecutorID) and return a
descriptive error (e.g., wrapped or typed) when they are nil or invalid instead
of proceeding; locate the Build function and insert nil checks for the receipt
parameter and the link parameter and validate
receipt.ExecutionResult/receipt.ExecutorID before using them, returning a clear,
classified error to the caller.
In `@engine/access/rest/http/request/get_execution_receipt.go`:
- Line 20: The comment saying "No errors are expected during normal operation."
is inaccurate because the request builders validate inputs and explicitly return
errors for missing or malformed execution IDs; update the comment(s) in
get_execution_receipt.go to state that the builders (e.g.,
GetExecutionReceiptRequestBuilder and its Build/Validate methods) will return
errors if required IDs are missing or malformed so the documented error contract
matches the actual behavior (also update the duplicate comment near the second
builder mentioned).
In `@engine/access/rpc/backend/backend_execution_results.go`:
- Around line 56-58: Replace fmt.Errorf(...) usage with
irrecoverable.NewExceptionf(...) when constructing exceptions before calling
irrecoverable.Throw. Specifically, in the function that calls
irrecoverable.Throw(ctx, err) (the blocks currently using fmt.Errorf("failed to
get execution receipts by block ID: %w", err) and the two other similar blocks
at the later commented ranges), change those fmt.Errorf calls to err =
irrecoverable.NewExceptionf("failed to get execution receipts by block ID: %v",
err) (or the matching message used in the other two locations) and leave the
subsequent irrecoverable.Throw(ctx, err) call unchanged; use
irrecoverable.NewExceptionf and irrecoverable.Throw together everywhere instead
of fmt.Errorf.
- Around line 94-96: Guard against nil receipt entries before dereferencing by
checking receipt and its ExecutionResult before calling
receipt.ExecutionResult.ID(): inside the loop over allReceipts add a defensive
if receipt == nil || receipt.ExecutionResult == nil { continue } so you skip nil
entries and avoid panics when comparing to resultID; update any related logic
that assumes non-nil receipts accordingly (look for the loop using allReceipts
and the comparison receipt.ExecutionResult.ID() == resultID in
backend_execution_results.go).
In `@engine/access/rpc/handler.go`:
- Line 1120: Validate incoming request ID payloads before calling
convert.MessageToIdentifier: check req.GetBlockId() (and the other ID used at
the second call) for nil/empty and run explicit validation/parse logic (e.g.,
base64/UUID/expected format decode) and return a context-appropriate error if
validation fails instead of passing malformed data into
convert.MessageToIdentifier; update the handler function around the uses of
convert.MessageToIdentifier to perform these checks and short-circuit with a
clear client-facing error (including which field failed) so invalid IDs are
rejected upstream rather than silently coerced.
In `@engine/common/rpc/convert/execution_results.go`:
- Around line 151-170: ExecutionReceiptToMessage currently dereferences receipt
and its nested fields (e.g., receipt.ExecutorID, receipt.ExecutionResult,
receipt.Spocks, receipt.ExecutorSignature) and can panic on nil inputs; add
defensive nil guards at the start of ExecutionReceiptToMessage to return a clear
error if receipt is nil and also verify receipt.ExecutionResult is non-nil
before calling ExecutionResultToMessage when includeResult is true (return a
descriptive error like "nil execution result in receipt"); ensure any sub-field
dereferences (ExecutorID, Spocks, ExecutorSignature) are safe or documented as
required so the function fails closed with explicit errors rather than
panicking.
In `@integration/tests/access/cohort2/observer_indexer_enabled_test.go`:
- Around line 685-696: Update the SetupTest in observer_indexer_enabled_test.go
to classify the new receipt endpoints the same way as the base observer tests:
add "getExecutionReceiptsByBlockID" and "getExecutionReceiptsByResultID" to the
localRest map used in SetupTest so they are treated as locally handled REST
endpoints (match how observer_test.go does for
getExecutionReceiptsByBlockID/getExecutionReceiptsByResultID); also inspect the
localRpc map in SetupTest and add or verify the receipt RPC entries there if RPC
handling differs from the base suite so localRpc and localRest classifications
remain consistent with getRPCs/getRestEndpoints and the other observer tests.
In `@integration/tests/access/cohort2/observer_test.go`:
- Around line 73-81: The localRpc map is missing the RPC method names for
execution receipts, causing inconsistent handling vs localRest; update the
localRpc initialization (the map named localRpc) to include
"GetExecutionReceiptsByBlockID" and "GetExecutionReceiptsByResultID" (exact
strings) so RPC calls are treated as locally handled by the observer like the
REST endpoints (which contain
getExecutionReceiptsByBlockID/getExecutionReceiptsByResultID); ensure you add
those two keys to the same map literal where other RPC methods are listed.
---
Nitpick comments:
In `@engine/access/rest/http/routes/execution_receipts_test.go`:
- Around line 57-125: Add negative subtests to TestGetExecutionReceiptsByBlockID
and TestGetExecutionReceiptsByResultID that exercise malformed IDs: call
getReceiptsByBlockIDReq with an invalid block_id string (e.g. "not-a-valid-id")
and call getReceiptsByResultIDReq with an invalid result id path segment, then
assert the router returns HTTP 400 and the appropriate JSON error body for
invalid parameters. Place these new subtests alongside the existing cases in
TestGetExecutionReceiptsByBlockID and TestGetExecutionReceiptsByResultID; they
do not need backend mocks (do not set expectations on mock.API) and should
validate request parsing/validation for getReceiptsByBlockIDReq and
getReceiptsByResultIDReq.
In `@engine/access/rpc/backend/backend.go`:
- Line 315: The constructor New in backend.go should validate that
params.ExecutionReceipts is non-nil and return an error immediately if missing
to avoid nil-dereference later; update the New function (the code that sets
receipts: params.ExecutionReceipts) to check params.ExecutionReceipts at the
start, return a descriptive error (e.g., "missing ExecutionReceipts") instead of
constructing the backend, and keep the rest of the initialization unchanged so
callers must inject a valid ExecutionReceipts implementation.
In `@integration/tests/access/cohort1/access_api_test.go`:
- Around line 1472-1480: The composite key built in the loops (variable key in
the receipts iteration that uses convert.MessageToIdentifier(...) for
receipt.Meta.ExecutorId and receipt.Meta.ResultId) is formed by naive string
concatenation which can be ambiguous; change the key to include an explicit
delimiter (e.g. executorId + "|" + resultId) or replace the string key with a
small struct type (e.g. type receiptKey struct { Executor, Result string } used
in byBlockSet map) so byBlockSet, receiptsByBlock and receiptsByResult
comparisons are unambiguous.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9bacf739-74f9-4d83-8b76-ccff09cf5608
⛔ Files ignored due to path filters (3)
go.sumis excluded by!**/*.suminsecure/go.sumis excluded by!**/*.sumintegration/go.sumis excluded by!**/*.sum
📒 Files selected for processing (28)
access/api.goaccess/mock/api.goaccess/mock/receipts_api.goaccess/unimplemented.goengine/access/apiproxy/access_api_proxy.goengine/access/mock/access_api_client.goengine/access/mock/access_api_server.goengine/access/rest/common/models/execution_receipt.goengine/access/rest/common/models/model_execution_receipt.goengine/access/rest/common/models/model_execution_receipt__expandable.goengine/access/rest/http/request/get_execution_receipt.goengine/access/rest/http/routes/execution_receipts.goengine/access/rest/http/routes/execution_receipts_test.goengine/access/rest/router/metrics_test.goengine/access/rest/router/routes_main.goengine/access/rpc/backend/backend.goengine/access/rpc/backend/backend_execution_results.goengine/access/rpc/backend/backend_execution_results_test.goengine/access/rpc/backend/backend_test.goengine/access/rpc/handler.goengine/common/rpc/convert/execution_results.goengine/common/rpc/convert/execution_results_test.gogo.modinsecure/go.modintegration/go.modintegration/tests/access/cohort1/access_api_test.gointegration/tests/access/cohort2/observer_indexer_enabled_test.gointegration/tests/access/cohort2/observer_test.go
Comment on lines
+441
to
+451
| func (h *FlowAccessAPIRouter) GetExecutionReceiptsByBlockID(context context.Context, req *access.GetExecutionReceiptsByBlockIDRequest) (*access.ExecutionReceiptsResponse, error) { | ||
| res, err := h.upstream.GetExecutionReceiptsByBlockID(context, req) | ||
| h.log(UpstreamApiService, "GetExecutionReceiptsByBlockID", err) | ||
| return res, err | ||
| } | ||
|
|
||
| func (h *FlowAccessAPIRouter) GetExecutionReceiptsByResultID(context context.Context, req *access.GetExecutionReceiptsByResultIDRequest) (*access.ExecutionReceiptsResponse, error) { | ||
| res, err := h.upstream.GetExecutionReceiptsByResultID(context, req) | ||
| h.log(UpstreamApiService, "GetExecutionReceiptsByResultID", err) | ||
| return res, err | ||
| } |
Contributor
There was a problem hiding this comment.
useIndex is bypassed for new receipts reads.
On Line 441 and Line 447, both methods always route upstream. This makes useIndex ineffective for receipts and can add unnecessary upstream dependency/latency when local indexing is enabled.
🔧 Suggested routing parity fix
func (h *FlowAccessAPIRouter) GetExecutionReceiptsByBlockID(context context.Context, req *access.GetExecutionReceiptsByBlockIDRequest) (*access.ExecutionReceiptsResponse, error) {
+ if h.useIndex {
+ res, err := h.local.GetExecutionReceiptsByBlockID(context, req)
+ h.log(LocalApiService, "GetExecutionReceiptsByBlockID", err)
+ return res, err
+ }
+
res, err := h.upstream.GetExecutionReceiptsByBlockID(context, req)
h.log(UpstreamApiService, "GetExecutionReceiptsByBlockID", err)
return res, err
}
func (h *FlowAccessAPIRouter) GetExecutionReceiptsByResultID(context context.Context, req *access.GetExecutionReceiptsByResultIDRequest) (*access.ExecutionReceiptsResponse, error) {
+ if h.useIndex {
+ res, err := h.local.GetExecutionReceiptsByResultID(context, req)
+ h.log(LocalApiService, "GetExecutionReceiptsByResultID", err)
+ return res, err
+ }
+
res, err := h.upstream.GetExecutionReceiptsByResultID(context, req)
h.log(UpstreamApiService, "GetExecutionReceiptsByResultID", err)
return res, err
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/access/apiproxy/access_api_proxy.go` around lines 441 - 451, Both
GetExecutionReceiptsByBlockID and GetExecutionReceiptsByResultID always call the
upstream, bypassing the local index; update them to check the router's index
flag and use the local index handler when enabled. In each function
(GetExecutionReceiptsByBlockID and GetExecutionReceiptsByResultID) branch on
h.useIndex (or the existing local-index boolean/field) and call the
corresponding index method (e.g., h.index.GetExecutionReceiptsByBlockID /
h.index.GetExecutionReceiptsByResultID) when true, otherwise fall back to
h.upstream. Ensure you keep the same logging pattern (h.log with
UpstreamApiService or an IndexApiService tag) and return the response and error
as before.
Comment on lines
+17
to
+23
| func (e *ExecutionReceipt) Build( | ||
| receipt *flow.ExecutionReceipt, | ||
| link LinkGenerator, | ||
| expand map[string]bool, | ||
| ) error { | ||
| e.ExecutorId = receipt.ExecutorID.String() | ||
| e.ResultId = receipt.ExecutionResult.ID().String() |
Contributor
There was a problem hiding this comment.
Guard Build inputs to prevent panic paths.
Build dereferences receipt and link immediately; nil inputs will panic instead of returning a classified error.
🛡️ Proposed hardening
import (
+ "errors"
"fmt"
"github.com/onflow/flow-go/engine/access/rest/util"
"github.com/onflow/flow-go/model/flow"
)
@@
func (e *ExecutionReceipt) Build(
receipt *flow.ExecutionReceipt,
link LinkGenerator,
expand map[string]bool,
) error {
+ if receipt == nil {
+ return errors.New("execution receipt is nil")
+ }
+ if link == nil {
+ return errors.New("link generator is nil")
+ }
+
e.ExecutorId = receipt.ExecutorID.String()
e.ResultId = receipt.ExecutionResult.ID().String()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/access/rest/common/models/execution_receipt.go` around lines 17 - 23,
The Build method on ExecutionReceipt dereferences receipt and link immediately
which will panic on nil inputs; add defensive checks at the start of
ExecutionReceipt.Build to validate receipt and link (and any required nested
fields like receipt.ExecutionResult or receipt.ExecutorID) and return a
descriptive error (e.g., wrapped or typed) when they are nil or invalid instead
of proceeding; locate the Build function and insert nil checks for the receipt
parameter and the link parameter and validate
receipt.ExecutionResult/receipt.ExecutorID before using them, returning a clear,
classified error to the caller.
| // GetExecutionReceiptsByBlockIDRequest extracts necessary variables from the provided request, | ||
| // builds a GetExecutionReceiptsByBlockID instance, and validates it. | ||
| // | ||
| // No errors are expected during normal operation. |
Contributor
There was a problem hiding this comment.
Commented error contract does not match implementation.
Line 20 and Line 49 say “No errors are expected during normal operation,” but these builders explicitly error on missing/malformed IDs. Please align the comments with the real behavior.
Also applies to: 49-49
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/access/rest/http/request/get_execution_receipt.go` at line 20, The
comment saying "No errors are expected during normal operation." is inaccurate
because the request builders validate inputs and explicitly return errors for
missing or malformed execution IDs; update the comment(s) in
get_execution_receipt.go to state that the builders (e.g.,
GetExecutionReceiptRequestBuilder and its Build/Validate methods) will return
errors if required IDs are missing or malformed so the documented error contract
matches the actual behavior (also update the duplicate comment near the second
builder mentioned).
Comment on lines
+56
to
+58
| err = fmt.Errorf("failed to get execution receipts by block ID: %w", err) | ||
| irrecoverable.Throw(ctx, err) | ||
| return nil, err |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cd /tmp/codebase && wc -l engine/access/rpc/backend/backend_execution_results.goRepository: onflow/flow-go
Length of output: 121
🏁 Script executed:
cd /tmp/codebase && cat -n engine/access/rpc/backend/backend_execution_results.go | sed -n '50,100p'Repository: onflow/flow-go
Length of output: 121
🏁 Script executed:
cd /tmp/codebase && rg -n 'irrecoverable\.' engine/access/rpc/backend/backend_execution_results.go | head -20Repository: onflow/flow-go
Length of output: 121
🏁 Script executed:
cd /tmp/codebase && rg -n 'NewExceptionf' --type go | head -10Repository: onflow/flow-go
Length of output: 121
🏁 Script executed:
wc -l engine/access/rpc/backend/backend_execution_results.goRepository: onflow/flow-go
Length of output: 116
🏁 Script executed:
cat -n engine/access/rpc/backend/backend_execution_results.go | sed -n '50,100p'Repository: onflow/flow-go
Length of output: 2607
🏁 Script executed:
rg -n 'irrecoverable\.' engine/access/rpc/backend/backend_execution_results.goRepository: onflow/flow-go
Length of output: 165
🏁 Script executed:
rg -n 'NewExceptionf' --type go | head -20Repository: onflow/flow-go
Length of output: 2634
🏁 Script executed:
fd -t f 'irrecoverable' | grep -E '\.go$' | head -5Repository: onflow/flow-go
Length of output: 227
🏁 Script executed:
rg -n 'func NewExceptionf' --type goRepository: onflow/flow-go
Length of output: 146
🏁 Script executed:
cat -n module/irrecoverable/irrecoverable.go | head -50Repository: onflow/flow-go
Length of output: 1775
🏁 Script executed:
rg -B2 -A2 'irrecoverable\.Throw.*err' engine/ --type go | head -40Repository: onflow/flow-go
Length of output: 3238
Use irrecoverable.NewExceptionf() instead of fmt.Errorf() for exception construction.
Storage failures that are not ErrNotFound are treated as irrecoverable, but the current code wraps them with fmt.Errorf() before passing to irrecoverable.Throw(). Per the project guidelines, use irrecoverable.NewExceptionf() to construct the exception directly.
Suggested patch
- err = fmt.Errorf("failed to get execution receipts by block ID: %w", err)
- irrecoverable.Throw(ctx, err)
- return nil, err
+ exception := irrecoverable.NewExceptionf("failed to get execution receipts by block ID: %w", err)
+ irrecoverable.Throw(ctx, exception)
+ return nil, exception- err = fmt.Errorf("failed to get execution result: %w", err)
- irrecoverable.Throw(ctx, err)
- return nil, err
+ exception := irrecoverable.NewExceptionf("failed to get execution result: %w", err)
+ irrecoverable.Throw(ctx, exception)
+ return nil, exception- err = fmt.Errorf("failed to get execution receipts by result ID: %w", err)
- irrecoverable.Throw(ctx, err)
- return nil, err
+ exception := irrecoverable.NewExceptionf("failed to get execution receipts by result ID: %w", err)
+ irrecoverable.Throw(ctx, exception)
+ return nil, exceptionAlso applies to: lines 75-77, 86-88
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/access/rpc/backend/backend_execution_results.go` around lines 56 - 58,
Replace fmt.Errorf(...) usage with irrecoverable.NewExceptionf(...) when
constructing exceptions before calling irrecoverable.Throw. Specifically, in the
function that calls irrecoverable.Throw(ctx, err) (the blocks currently using
fmt.Errorf("failed to get execution receipts by block ID: %w", err) and the two
other similar blocks at the later commented ranges), change those fmt.Errorf
calls to err = irrecoverable.NewExceptionf("failed to get execution receipts by
block ID: %v", err) (or the matching message used in the other two locations)
and leave the subsequent irrecoverable.Throw(ctx, err) call unchanged; use
irrecoverable.NewExceptionf and irrecoverable.Throw together everywhere instead
of fmt.Errorf.
Comment on lines
+94
to
+96
| for _, receipt := range allReceipts { | ||
| if receipt.ExecutionResult.ID() == resultID { | ||
| receipts = append(receipts, receipt) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, examine the full context of the function containing lines 94-96
wc -l engine/access/rpc/backend/backend_execution_results.goRepository: onflow/flow-go
Length of output: 116
🏁 Script executed:
#!/bin/bash
# Get context around the flagged lines
sed -n '80,110p' engine/access/rpc/backend/backend_execution_results.goRepository: onflow/flow-go
Length of output: 727
🏁 Script executed:
#!/bin/bash
# Identify the function name and full signature
sed -n '1,100p' engine/access/rpc/backend/backend_execution_results.go | grep -B20 'for _, receipt := range allReceipts' | head -40Repository: onflow/flow-go
Length of output: 859
🏁 Script executed:
#!/bin/bash
# Find the receipts field definition and its type
rg -n 'receipts\s+' engine/access/rpc/backend/backend_execution_results.go | head -5Repository: onflow/flow-go
Length of output: 462
🏁 Script executed:
#!/bin/bash
# Search for the type of b.receipts and the ByBlockID method
fd -e go backend.go | head -5Repository: onflow/flow-go
Length of output: 278
🏁 Script executed:
#!/bin/bash
# Look at the backend structure definition
rg -n 'type.*Backend\s*struct' engine/access/rpc/backend/ -A30Repository: onflow/flow-go
Length of output: 2171
🏁 Script executed:
#!/bin/bash
# Search for ByBlockID method definition
rg -n 'func.*ByBlockID' engine/access/rpc/backend/ -A5Repository: onflow/flow-go
Length of output: 41036
🏁 Script executed:
#!/bin/bash
# Find storage.ExecutionReceipts interface definition
fd -e go storage | head -20Repository: onflow/flow-go
Length of output: 986
🏁 Script executed:
#!/bin/bash
# Search for ExecutionReceipts interface in storage
rg -n 'type ExecutionReceipts' --type goRepository: onflow/flow-go
Length of output: 1082
🏁 Script executed:
#!/bin/bash
# Search for ByBlockID method signature and documentation in storage
rg -n 'ByBlockID.*ExecutionReceipt' --type go -A3 -B1Repository: onflow/flow-go
Length of output: 27304
🏁 Script executed:
#!/bin/bash
# Find flow.ExecutionReceiptList type definition
rg -n 'type ExecutionReceiptList' --type goRepository: onflow/flow-go
Length of output: 139
🏁 Script executed:
#!/bin/bash
# Get full implementation of byBlockID method
sed -n '110,150p' storage/store/receipts.goRepository: onflow/flow-go
Length of output: 346
🏁 Script executed:
#!/antml:bash
# Look for the complete byBlockID implementation in receipts.go
cat -n storage/store/receipts.go | sed -n '100,200p'Repository: onflow/flow-go
Length of output: 833
🏁 Script executed:
#!/bin/bash
# Search for the actual byBlockID implementation (lowercase)
rg -n 'func.*byBlockID' storage/store/receipts.go -A20Repository: onflow/flow-go
Length of output: 881
🏁 Script executed:
#!/bin/bash
# Also check the ExecutionReceipt type definition to understand dereferences
rg -n 'type ExecutionReceipt struct' model/flow/execution_receipt.go -A10Repository: onflow/flow-go
Length of output: 507
🏁 Script executed:
#!/bin/bash
# Check if there are any nil guards in the filtered loop in tests
rg -n 'for.*receipt.*range.*allReceipts' --type go -A5Repository: onflow/flow-go
Length of output: 561
🏁 Script executed:
#!/bin/bash
# Search for similar patterns where code iterates over slices and dereferences without nil guards
rg -n 'for.*range.*\[\]\*.*{' --type go -A3 | grep -A3 'ExecutionReceipt\|receipt\.' | head -40Repository: onflow/flow-go
Length of output: 163
🏁 Script executed:
#!/bin/bash
# Check if there are explicit nil checks for receipt entries elsewhere in the codebase
rg -n 'if.*receipt.*==.*nil' --type go | head -20Repository: onflow/flow-go
Length of output: 152
🏁 Script executed:
#!/bin/bash
# Verify the full function context including error handling to understand the expected invariant
sed -n '70,100p' engine/access/rpc/backend/backend_execution_results.goRepository: onflow/flow-go
Length of output: 1281
🏁 Script executed:
#!/bin/bash
# Check the storage.ExecutionReceipts interface definition for documentation
cat -n storage/receipts.go | sed -n '20,35p'Repository: onflow/flow-go
Length of output: 824
🏁 Script executed:
#!/bin/bash
# Check if there are any other methods that iterate over ExecutionReceiptList with nil guards
rg -n 'ExecutionReceiptList' --type go -B2 -A8 | grep -A8 'for.*range'Repository: onflow/flow-go
Length of output: 8514
🏁 Script executed:
#!/bin/bash
# Examine the ByID implementation to confirm whether it can return nil pointer in normal operation
sed -n '50,80p' storage/store/receipts.goRepository: onflow/flow-go
Length of output: 946
Guard against nil receipt entries before dereferencing.
At line 95, receipt.ExecutionResult.ID() could panic if any entry in allReceipts is nil. While the current byBlockID implementation handles errors properly and prevents nil entries in normal operation, add a defensive guard aligned with coding guidelines for treating all inputs as potentially unsafe.
Suggested patch
for _, receipt := range allReceipts {
+ if receipt == nil {
+ exception := irrecoverable.NewExceptionf("nil execution receipt while filtering by result ID: %x", resultID)
+ irrecoverable.Throw(ctx, exception)
+ return nil, exception
+ }
if receipt.ExecutionResult.ID() == resultID {
receipts = append(receipts, receipt)
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/access/rpc/backend/backend_execution_results.go` around lines 94 - 96,
Guard against nil receipt entries before dereferencing by checking receipt and
its ExecutionResult before calling receipt.ExecutionResult.ID(): inside the loop
over allReceipts add a defensive if receipt == nil || receipt.ExecutionResult ==
nil { continue } so you skip nil entries and avoid panics when comparing to
resultID; update any related logic that assumes non-nil receipts accordingly
(look for the loop using allReceipts and the comparison
receipt.ExecutionResult.ID() == resultID in backend_execution_results.go).
| return nil, err | ||
| } | ||
|
|
||
| blockID := convert.MessageToIdentifier(req.GetBlockId()) |
Contributor
There was a problem hiding this comment.
Validate request IDs before converting to internal identifiers.
Line 1120 and Line 1149 use convert.MessageToIdentifier(...), which does not reject malformed ID payloads. This can silently coerce invalid client input and misclassify request errors downstream.
🔧 Suggested fix
func (h *Handler) GetExecutionReceiptsByBlockID(ctx context.Context, req *accessproto.GetExecutionReceiptsByBlockIDRequest) (*accessproto.ExecutionReceiptsResponse, error) {
metadata, err := h.buildMetadataResponse()
if err != nil {
return nil, err
}
- blockID := convert.MessageToIdentifier(req.GetBlockId())
+ blockID, err := convert.BlockID(req.GetBlockId())
+ if err != nil {
+ return nil, status.Errorf(codes.InvalidArgument, "invalid block id: %v", err)
+ }
receipts, err := h.api.GetExecutionReceiptsByBlockID(ctx, blockID)
if err != nil {
return nil, err
}
@@
func (h *Handler) GetExecutionReceiptsByResultID(ctx context.Context, req *accessproto.GetExecutionReceiptsByResultIDRequest) (*accessproto.ExecutionReceiptsResponse, error) {
metadata, err := h.buildMetadataResponse()
if err != nil {
return nil, err
}
- resultID := convert.MessageToIdentifier(req.GetResultId())
+ resultID, err := convert.BlockID(req.GetResultId()) // or a dedicated validating identifier parser
+ if err != nil {
+ return nil, status.Errorf(codes.InvalidArgument, "invalid result id: %v", err)
+ }
receipts, err := h.api.GetExecutionReceiptsByResultID(ctx, resultID)
if err != nil {
return nil, err
}As per coding guidelines, “Treat all inputs as potentially byzantine and classify errors in a context-dependent manner; no code path is safe unless explicitly proven and documented”.
Also applies to: 1149-1149
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/access/rpc/handler.go` at line 1120, Validate incoming request ID
payloads before calling convert.MessageToIdentifier: check req.GetBlockId() (and
the other ID used at the second call) for nil/empty and run explicit
validation/parse logic (e.g., base64/UUID/expected format decode) and return a
context-appropriate error if validation fails instead of passing malformed data
into convert.MessageToIdentifier; update the handler function around the uses of
convert.MessageToIdentifier to perform these checks and short-circuit with a
clear client-facing error (including which field failed) so invalid IDs are
rejected upstream rather than silently coerced.
Comment on lines
+151
to
+170
| func ExecutionReceiptToMessage(receipt *flow.ExecutionReceipt, includeResult bool) (*entities.ExecutionReceipt, error) { | ||
| msg := &entities.ExecutionReceipt{ | ||
| Meta: &entities.ExecutionReceiptMeta{ | ||
| ExecutorId: IdentifierToMessage(receipt.ExecutorID), | ||
| ResultId: IdentifierToMessage(receipt.ExecutionResult.ID()), | ||
| Spocks: SignaturesToMessages(receipt.Spocks), | ||
| ExecutorSignature: MessageToSignature(receipt.ExecutorSignature), | ||
| }, | ||
| } | ||
|
|
||
| if includeResult { | ||
| result, err := ExecutionResultToMessage(&receipt.ExecutionResult) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("could not convert execution result: %w", err) | ||
| } | ||
| msg.ExecutionResult = result | ||
| } | ||
|
|
||
| return msg, nil | ||
| } |
Contributor
There was a problem hiding this comment.
Add nil guards to prevent panic on malformed receipt inputs.
On Line 154/Line 179 paths, a nil receipt will panic during field access. These converters should fail closed with explicit errors.
🛡️ Defensive conversion fix
func ExecutionReceiptToMessage(receipt *flow.ExecutionReceipt, includeResult bool) (*entities.ExecutionReceipt, error) {
+ if receipt == nil {
+ return nil, fmt.Errorf("execution receipt is nil")
+ }
+
msg := &entities.ExecutionReceipt{
Meta: &entities.ExecutionReceiptMeta{
ExecutorId: IdentifierToMessage(receipt.ExecutorID),
ResultId: IdentifierToMessage(receipt.ExecutionResult.ID()),
Spocks: SignaturesToMessages(receipt.Spocks),
ExecutorSignature: MessageToSignature(receipt.ExecutorSignature),
},
}
@@
func ExecutionReceiptsToMessages(receipts []*flow.ExecutionReceipt, includeResult bool) ([]*entities.ExecutionReceipt, error) {
msgs := make([]*entities.ExecutionReceipt, len(receipts))
for i, receipt := range receipts {
+ if receipt == nil {
+ return nil, fmt.Errorf("execution receipt at index %d is nil", i)
+ }
msg, err := ExecutionReceiptToMessage(receipt, includeResult)
if err != nil {
return nil, fmt.Errorf("could not convert execution receipt at index %d: %w", i, err)
}
msgs[i] = msg
}
return msgs, nil
}As per coding guidelines "Treat all inputs as potentially byzantine and classify errors in a context-dependent manner; no code path is safe unless explicitly proven and documented".
Also applies to: 176-186
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@engine/common/rpc/convert/execution_results.go` around lines 151 - 170,
ExecutionReceiptToMessage currently dereferences receipt and its nested fields
(e.g., receipt.ExecutorID, receipt.ExecutionResult, receipt.Spocks,
receipt.ExecutorSignature) and can panic on nil inputs; add defensive nil guards
at the start of ExecutionReceiptToMessage to return a clear error if receipt is
nil and also verify receipt.ExecutionResult is non-nil before calling
ExecutionResultToMessage when includeResult is true (return a descriptive error
like "nil execution result in receipt"); ensure any sub-field dereferences
(ExecutorID, Spocks, ExecutorSignature) are safe or documented as required so
the function fails closed with explicit errors rather than panicking.
Comment on lines
+685
to
+696
| {name: "GetExecutionReceiptsByBlockID", call: func(ctx context.Context, client accessproto.AccessAPIClient) error { | ||
| _, err := client.GetExecutionReceiptsByBlockID(ctx, &accessproto.GetExecutionReceiptsByBlockIDRequest{ | ||
| BlockId: make([]byte, 32), | ||
| }) | ||
| return err | ||
| }}, | ||
| {name: "GetExecutionReceiptsByResultID", call: func(ctx context.Context, client accessproto.AccessAPIClient) error { | ||
| _, err := client.GetExecutionReceiptsByResultID(ctx, &accessproto.GetExecutionReceiptsByResultIDRequest{ | ||
| ResultId: make([]byte, 32), | ||
| }) | ||
| return err | ||
| }}, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Inspect how localRpc/localRest influence observer test expectations.
echo "=== Searching for localRpc/localRest usage patterns ==="
rg -n --type=go -C3 '\blocalRpc\b|\blocalRest\b|testedRPCs|testedRestEndpoints|RPCTest|RestEndpointTest|StopContainerByName' integration/tests/access/cohort2
# 2) Check placement of newly added receipt endpoint names.
echo ""
echo "=== Checking receipt endpoint names placement ==="
rg -n --type=go -C2 '"GetExecutionReceiptsByBlockID"|"GetExecutionReceiptsByResultID"|"getExecutionReceiptsByBlockID"|"getExecutionReceiptsByResultID"' \
integration/tests/access/cohort2/observer_indexer_enabled_test.go \
integration/tests/access/cohort2/observer_test.goRepository: onflow/flow-go
Length of output: 21089
🏁 Script executed:
sed -n '51,71p' integration/tests/access/cohort2/observer_indexer_enabled_test.goRepository: onflow/flow-go
Length of output: 816
Add missing endpoint classifications to SetupTest.
The receipt endpoints were added to getRPCs and getRestEndpoints but are not classified in localRest and localRpc maps. This creates a classification mismatch: observer_test.go marks getExecutionReceiptsByBlockID and getExecutionReceiptsByResultID as locally handled (lines 79–80), but observer_indexer_enabled_test.go does not. Since these endpoints are tested identically in both suites, they should be consistently classified.
Add the following to SetupTest in observer_indexer_enabled_test.go:
- "getExecutionReceiptsByBlockID" and "getExecutionReceiptsByResultID" to the
localRestmap (lines 72–78) - Verify that receipt RPC endpoints are handled appropriately in
localRpcif they differ from the base suite
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@integration/tests/access/cohort2/observer_indexer_enabled_test.go` around
lines 685 - 696, Update the SetupTest in observer_indexer_enabled_test.go to
classify the new receipt endpoints the same way as the base observer tests: add
"getExecutionReceiptsByBlockID" and "getExecutionReceiptsByResultID" to the
localRest map used in SetupTest so they are treated as locally handled REST
endpoints (match how observer_test.go does for
getExecutionReceiptsByBlockID/getExecutionReceiptsByResultID); also inspect the
localRpc map in SetupTest and add or verify the receipt RPC entries there if RPC
handling differs from the base suite so localRpc and localRest classifications
remain consistent with getRPCs/getRestEndpoints and the other observer tests.
Comment on lines
73
to
81
| s.localRest = map[string]struct{}{ | ||
| "getBlocksByIDs": {}, | ||
| "getBlocksByHeight": {}, | ||
| "getBlockPayloadByID": {}, | ||
| "getNetworkParameters": {}, | ||
| "getNodeVersionInfo": {}, | ||
| "getBlocksByIDs": {}, | ||
| "getBlocksByHeight": {}, | ||
| "getBlockPayloadByID": {}, | ||
| "getNetworkParameters": {}, | ||
| "getNodeVersionInfo": {}, | ||
| "getExecutionReceiptsByBlockID": {}, | ||
| "getExecutionReceiptsByResultID": {}, | ||
| } |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if observer implements ReceiptsAPI methods locally or proxies them
# Search for observer-specific implementation of GetExecutionReceiptsByBlockID
echo "=== Searching for observer GetExecutionReceiptsByBlockID implementation ==="
rg -n -C5 'GetExecutionReceiptsByBlockID' --type go -g '*observer*'
# Check if there's a receipts backend or handler for observer
echo ""
echo "=== Searching for observer receipts handling ==="
rg -n -C3 'Receipts' --type go -g '*observer*'Repository: onflow/flow-go
Length of output: 14438
🏁 Script executed:
#!/bin/bash
# Get the localRpc map definition and surrounding context
echo "=== localRpc map definition ==="
sed -n '61,71p' integration/tests/access/cohort2/observer_test.go
echo ""
echo "=== testedRPCs assignment and related context ==="
sed -n '83,95p' integration/tests/access/cohort2/observer_test.go
echo ""
echo "=== Check the RPC test structure to understand what s.testedRPCs contains ==="
sed -n '380,415p' integration/tests/access/cohort2/observer_test.goRepository: onflow/flow-go
Length of output: 2857
🏁 Script executed:
#!/bin/bash
# Find the test logic that uses localRpc and localRest maps
echo "=== Search for how localRpc is used in test logic ==="
rg -n 'localRpc' integration/tests/access/cohort2/observer_test.go -A3 -B1
echo ""
echo "=== Search for how localRest is used in test logic ==="
rg -n 'localRest' integration/tests/access/cohort2/observer_test.go -A3 -B1
echo ""
echo "=== Search for HandledByObserver and HandledByUpstream logic ==="
rg -n 'HandledBy|isLocalEndpoint' integration/tests/access/cohort2/observer_test.go -B2 -A2Repository: onflow/flow-go
Length of output: 4276
Add execution receipt RPC methods to localRpc map for consistency with REST endpoints.
The new execution receipt endpoints are handled differently between REST and RPC:
- REST endpoints
getExecutionReceiptsByBlockIDandgetExecutionReceiptsByResultIDare inlocalRest(lines 79-80) - RPC methods
GetExecutionReceiptsByBlockIDandGetExecutionReceiptsByResultIDare NOT inlocalRpc(lines 61-71)
This means REST endpoints will be tested as locally handled (HandledByObserver test), while RPC methods will be tested as proxied to upstream (HandledByUpstream test). Since the observer has ExecutionReceipts storage available (cmd/observer/node_builder/observer_builder.go:2130), both should be handled consistently. Add the RPC methods to localRpc to match the REST behavior, unless there's a specific reason these should be proxied differently.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@integration/tests/access/cohort2/observer_test.go` around lines 73 - 81, The
localRpc map is missing the RPC method names for execution receipts, causing
inconsistent handling vs localRest; update the localRpc initialization (the map
named localRpc) to include "GetExecutionReceiptsByBlockID" and
"GetExecutionReceiptsByResultID" (exact strings) so RPC calls are treated as
locally handled by the observer like the REST endpoints (which contain
getExecutionReceiptsByBlockID/getExecutionReceiptsByResultID); ensure you add
those two keys to the same map literal where other RPC methods are listed.
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
2 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.
Proto/openapi spec: onflow/flow#1698
This PR adds new methods for getting execution receipts from the Access API.
It adds grpc methods:
GetExecutionReceiptsByBlockIDGetExecutionReceiptsByResultIDand rest endpoints:
/v1/execution_receipts/v1/execution_receipts/results/{id}This is needed for indexing which execution nodes produced receipts for a given block. It will also make it easier to debug when the network encounters an execution fork.
Summary by CodeRabbit
Release Notes
GET /v1/execution_receiptsandGET /v1/execution_receipts/results/{id}for querying execution receipts.