feat(keeper): add handling for pending payloads #47
Conversation
WalkthroughAdds PendingPayload proto and generated types, SubmitPayload (tx) and PendingPayload (query) RPCs with autocli wiring, keeper storage/expiration and payload SHA‑256 hashing, Msg/Query servers plus tests, BeginBlock expiration hook, and a Makefile lint-fix target. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant CLI as Client (CLI/SDK)
participant MsgSrv as MsgServer
participant K as Keeper
participant Exec as Executor
participant Fwd as Forwarder
participant Store as Pending Store
User->>CLI: SubmitPayload(signer, payload)
CLI->>MsgSrv: MsgSubmitPayload
MsgSrv->>MsgSrv: Unmarshal + validate
MsgSrv->>K: validatePayloadAgainstState(payload)
K->>Exec: Check pre-action pauses
Exec-->>K: OK/Paused
K->>Fwd: Check protocol / cross-chain paused
Fwd-->>K: OK/Paused
alt Valid
MsgSrv->>K: submit(payload)
K->>K: assign sequence & timestamp
K->>K: SHA-256(PendingPayload) -> hash
K->>Store: Put(hash -> PendingPayload)
Store-->>K: Stored
K-->>MsgSrv: hash
MsgSrv-->>CLI: MsgSubmitPayloadResponse{hash}
else Invalid/Paused
K-->>MsgSrv: error
MsgSrv-->>CLI: gRPC error
end
sequenceDiagram
autonumber
actor User
participant CLI as Client (CLI/SDK)
participant QrySrv as QueryServer
participant K as Keeper
participant Store as Pending Store
User->>CLI: Query PendingPayload(hash)
CLI->>QrySrv: QueryPendingPayloadRequest{hash}
QrySrv->>QrySrv: Validate hash
QrySrv->>K: pendingPayload(hash)
K->>Store: Get(hash)
alt Found
Store-->>K: PendingPayload
K-->>QrySrv: PendingPayload
QrySrv-->>CLI: QueryPendingPayloadResponse{payload}
else Not found / invalid
Store-->>K: nil
K-->>QrySrv: not found
QrySrv-->>CLI: gRPC error
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
MalteHerrmann
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (8)
api/core/v1/orbiter.pulsar.go (1)
2058-2096: PendingPayload reflection/descriptors look correctFast-reflection, msgTypes/goTypes/depIdxs, Exporter, and TypeBuilder counts are consistent.
Also ensure golangci-lint excludes generated files from license checks or inject a license header via the generator to satisfy “All Go files must include license headers”. As per coding guidelines
Also applies to: 2924-2994
proto/noble/orbiter/core/v1/orbiter.proto (1)
78-84: Proto addition looks goodMessage/fields match generated Go. Minor nit: “that will is registered” → “that is registered.”
types/expected_keepers.go (1)
58-64: Interface shape looks good; consider clearer method naming.Optionally rename PendingPayload(...) → GetPendingPayload(...) for clarity, and add brief method comments to aid implementers.
keeper/msg_server_test.go (1)
43-49: Optional: align testcase field naming with repo conventionConsider renaming errContains to expError to match the stated testCases pattern.
As per coding guidelines
keeper/payload_handler.go (2)
168-173: Comment contradicts behaviorThe comment says "no-op but does not return an error", but code returns an error when not found. Update the comment.
-// RemovePendingPayload removes the pending payload from the module state. -// If a payload is not found, it is a no-op but does not return an error. +// RemovePendingPayload removes the pending payload from the module state. +// If a payload is not found, an error is returned.
159-163: Optional: unify hash formatting in logsUse 0x-prefixed hex consistently for hashes to match other logs and responses.
k.Logger().Debug( "retrieved pending payload", - "hash", hex.EncodeToString(hash), + "hash", ethcommon.BytesToHash(hash).Hex(), "payload", payload.String(), ) @@ - k.Logger().Debug("removed pending payload", "hash", hex.EncodeToString(hash)) + k.Logger().Debug("removed pending payload", "hash", ethcommon.BytesToHash(hash).Hex())Also applies to: 187-189
keeper/payload_handler_test.go (2)
131-133: Consistency: use local var to avoid capturing outererrNot parallel here, but prefer a local assignment for clarity and to avoid accidental capture.
- err = k.Forwarder().Pause(ctx, core.PROTOCOL_CCTP, []string{cID}) - require.NoError(t, err, "failed to unpause cross-chain forwarding") + err := k.Forwarder().Pause(ctx, core.PROTOCOL_CCTP, []string{cID}) + require.NoError(t, err, "failed to pause cross-chain forwarding")
50-56: Optional: align testcase field to expError per repo conventionRename errContains to expError across tests to match the documented pattern. No behavior change.
As per coding guidelines
Also applies to: 198-204, 253-258
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
api/v1/query_grpc.pb.gois excluded by!**/*.pb.goapi/v1/tx_grpc.pb.gois excluded by!**/*.pb.gotypes/core/orbiter.pb.gois excluded by!**/*.pb.gotypes/query.pb.gois excluded by!**/*.pb.gotypes/query.pb.gw.gois excluded by!**/*.pb.gw.gotypes/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (21)
api/core/v1/orbiter.pulsar.go(5 hunks)api/v1/query.pulsar.go(2 hunks)api/v1/tx.pulsar.go(2 hunks)autocli.go(4 hunks)keeper/keeper.go(9 hunks)keeper/msg_server.go(1 hunks)keeper/msg_server_test.go(1 hunks)keeper/payload_handler.go(1 hunks)keeper/payload_handler_test.go(1 hunks)keeper/query_server.go(1 hunks)keeper/query_server_test.go(1 hunks)keeper/servers_registration.go(3 hunks)proto/noble/orbiter/core/v1/orbiter.proto(1 hunks)proto/noble/orbiter/v1/query.proto(1 hunks)proto/noble/orbiter/v1/tx.proto(1 hunks)types/autocli.go(1 hunks)types/codec.go(2 hunks)types/core/errors.go(1 hunks)types/core/keys.go(1 hunks)types/core/payload.go(1 hunks)types/expected_keepers.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*_test.go: Write unit tests using the testCases pattern
Name each test case as "success - " for success cases and "error - " for error cases; omit the prefix when no error is tested
For error-checking cases, include an expError string field in the test case and assert with require.ErrorContains(t, tC.expError, err)
Files:
keeper/query_server_test.gokeeper/payload_handler_test.gokeeper/msg_server_test.go
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Go code must be formatted and pass golangci-lint
All Go files must include license headers
Files:
keeper/query_server_test.gotypes/autocli.gotypes/expected_keepers.gotypes/core/errors.gokeeper/query_server.gotypes/core/payload.gokeeper/servers_registration.gotypes/core/keys.gotypes/codec.goapi/core/v1/orbiter.pulsar.goapi/v1/tx.pulsar.gokeeper/payload_handler_test.gokeeper/payload_handler.gokeeper/msg_server_test.goautocli.gokeeper/keeper.goapi/v1/query.pulsar.gokeeper/msg_server.go
proto/**/*.proto
📄 CodeRabbit inference engine (CLAUDE.md)
Protobuf files must be formatted and linted with buf
Files:
proto/noble/orbiter/core/v1/orbiter.protoproto/noble/orbiter/v1/query.protoproto/noble/orbiter/v1/tx.proto
🧠 Learnings (1)
📚 Learning: 2025-08-18T07:26:16.709Z
Learnt from: 0xstepit
PR: noble-assets/orbiter#15
File: autocli.go:26-35
Timestamp: 2025-08-18T07:26:16.709Z
Learning: In the orbiter codebase, service descriptors follow different naming conventions depending on their package location: API packages (api/*/v1) use Msg_ServiceDesc/Query_ServiceDesc (uppercase 'S'), while types packages (types/component/*) use Msg_serviceDesc/Query_serviceDesc (lowercase 's'). The autocli.go correctly uses both conventions depending on which service descriptors it's referencing.
Applied to files:
types/codec.go
🧬 Code graph analysis (14)
keeper/query_server_test.go (1)
testutil/mocks/orbiter/orbiter.go (1)
OrbiterKeeper(35-43)
types/core/errors.go (1)
types/core/keys.go (1)
ModuleName(30-30)
keeper/query_server.go (3)
api/v1/query_grpc.pb.go (1)
QueryServer(57-60)types/query.pb.go (7)
QueryServer(198-200)QueryPendingPayloadsRequest(32-35)QueryPendingPayloadsRequest(39-39)QueryPendingPayloadsRequest(40-42)QueryPendingPayloadsResponse(77-82)QueryPendingPayloadsResponse(86-86)QueryPendingPayloadsResponse(87-89)keeper/keeper.go (1)
Keeper(51-72)
types/core/payload.go (1)
types/core/errors.go (1)
ErrNilPointer(28-28)
types/codec.go (2)
api/v1/tx.pulsar.go (3)
MsgSubmitPayload(1928-1938)MsgSubmitPayload(1953-1953)MsgSubmitPayload(1956-1958)types/tx.pb.go (3)
MsgSubmitPayload(37-43)MsgSubmitPayload(47-47)MsgSubmitPayload(48-50)
api/core/v1/orbiter.pulsar.go (2)
types/core/orbiter.pb.go (15)
PendingPayload(234-239)PendingPayload(243-243)PendingPayload(244-246)Payload(128-135)Payload(139-139)Payload(140-142)Action(35-43)Action(47-47)Action(48-50)Forwarding(79-91)Forwarding(95-95)Forwarding(96-98)PayloadWrapper(187-191)PayloadWrapper(195-195)PayloadWrapper(196-198)api/core/v1/id.pulsar.go (8)
ActionID(500-500)ActionID(536-538)ActionID(540-542)ActionID(549-551)ProtocolID(556-556)ProtocolID(600-602)ProtocolID(604-606)ProtocolID(613-615)
api/v1/tx.pulsar.go (2)
api/core/v1/orbiter.pulsar.go (3)
Payload(2671-2682)Payload(2697-2697)Payload(2700-2702)types/core/orbiter.pb.go (3)
Payload(128-135)Payload(139-139)Payload(140-142)
keeper/payload_handler_test.go (9)
testutil/random.go (1)
RandomBytes(28-36)keeper/keeper.go (1)
Keeper(51-72)types/core/id.pb.go (2)
ACTION_FEE(36-36)PROTOCOL_CCTP(73-73)types/core/orbiter.go (1)
NewAction(39-49)types/controller/forwarding/cctp.go (1)
NewCCTPForwarding(69-81)testutil/mocks/orbiter/orbiter.go (1)
OrbiterKeeper(35-43)types/expected_keepers.go (1)
PendingPayloadsHandler(60-64)types/marshal_and_unmarshal.go (1)
MarshalJSON(36-38)testutil/accounts.go (1)
NewNobleAddress(45-47)
keeper/payload_handler.go (3)
types/expected_keepers.go (1)
PendingPayloadsHandler(60-64)keeper/keeper.go (1)
Keeper(51-72)types/core/errors.go (1)
ErrRemovePayload(37-37)
keeper/msg_server_test.go (4)
keeper/keeper.go (1)
Keeper(51-72)types/core/errors.go (1)
ErrSubmitPayload(36-36)testutil/mocks/orbiter/orbiter.go (1)
OrbiterKeeper(35-43)types/marshal_and_unmarshal.go (1)
MarshalJSON(36-38)
autocli.go (2)
types/tx.pb.go (1)
Msg_serviceDesc(360-360)types/autocli.go (2)
TxCommandOptions(25-37)QueryCommandOptions(39-47)
keeper/keeper.go (9)
types/component.go (1)
Authorizer(36-38)api/v1/tx_grpc.pb.go (1)
MsgServer(59-64)types/tx.pb.go (1)
MsgServer(324-328)api/v1/query_grpc.pb.go (1)
QueryServer(57-60)types/query.pb.go (1)
QueryServer(198-200)types/expected_keepers.go (2)
PendingPayloadsHandler(60-64)BankKeeper(33-36)types/core/orbiter.pb.go (3)
PendingPayload(234-239)PendingPayload(243-243)PendingPayload(244-246)types/core/keys.go (4)
PendingPayloadsSequencePrefix(129-129)PendingPayloadsSequenceName(124-124)PendingPayloadsPrefix(128-128)PendingPayloadsName(123-123)types/controller.go (3)
ForwardingController(30-33)ActionController(37-40)AdapterController(44-47)
api/v1/query.pulsar.go (1)
types/query.pb.go (6)
QueryPendingPayloadsRequest(32-35)QueryPendingPayloadsRequest(39-39)QueryPendingPayloadsRequest(40-42)QueryPendingPayloadsResponse(77-82)QueryPendingPayloadsResponse(86-86)QueryPendingPayloadsResponse(87-89)
keeper/msg_server.go (5)
api/v1/tx_grpc.pb.go (1)
MsgServer(59-64)types/tx.pb.go (7)
MsgServer(324-328)MsgSubmitPayload(37-43)MsgSubmitPayload(47-47)MsgSubmitPayload(48-50)MsgSubmitPayloadResponse(94-97)MsgSubmitPayloadResponse(101-101)MsgSubmitPayloadResponse(102-104)keeper/keeper.go (1)
Keeper(51-72)types/marshal_and_unmarshal.go (1)
UnmarshalJSON(30-32)types/core/errors.go (1)
ErrSubmitPayload(36-36)
🪛 GitHub Actions: Lint codebase
keeper/keeper.go
[error] 70-70: Line contains TODO/BUG/FIXME: "TODO: this is only exported to be able t..." (godox)
🪛 GitHub Check: lint
keeper/keeper.go
[failure] 70-70:
keeper/keeper.go:70: Line contains TODO/BUG/FIXME: "TODO: this is only exported to be able t..." (godox)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: prepare
- GitHub Check: build
🔇 Additional comments (8)
keeper/servers_registration.go (1)
42-47: Registering core Msg/Query servers on Keeper is correctAdds the module-level servers alongside component servers; looks good.
Confirm there’s no other place registering the same services to avoid duplicate registration panics.
Also applies to: 54-60
types/core/keys.go (1)
122-130: Prefix allocation is consistent and unique
New prefixes (50, 51) follow the existing scheme and have no duplicates across the repo.types/core/errors.go (1)
36-37: Error codes and usage verifiedErrSubmitPayload (11) and ErrRemovePayload (12) are referenced in keeper/msg_server.go and keeper/payload_handler.go, respectively.
types/core/payload.go (1)
39-46: Guard against nil inner payload to prevent NPE.p.Payload may be nil; calling Validate on it would panic.
Apply:
func (p *PendingPayload) Validate() error { if p == nil { return ErrNilPointer.Wrap("pending payload") } + if p.Payload == nil { + return ErrNilPointer.Wrap("pending payload: payload") + } return p.Payload.Validate() }⛔ Skipped due to learnings
Learnt from: 0xstepit PR: noble-assets/orbiter#10 File: keeper/component/dispatcher/dispatcher.go:151-155 Timestamp: 2025-08-14T15:11:47.828Z Learning: In the Orbiter codebase, the payload.Validate() method already handles nil pointer checks internally, so wrapper functions like ValidatePayload() don't need to add additional nil checks before calling payload.Validate().keeper/msg_server.go (1)
34-58: Approve SubmitPayload as permissionless
SubmitPayload is designed to be open to all callers and does not require authority checks; ignoring req.Signer is correct.types/autocli.go (1)
25-37: Verify signer population in Tx SubmitPayload CLIOnly
payloadis specified as a positional arg – TxCommandOptions omits thesignerfield. Confirm autocli auto-fillsSignerfrom the global--fromflag; if it doesn’t, add aFlagOptionsentry mappingProtoField: "signer"to the--fromflag.proto/noble/orbiter/v1/query.proto (1)
12-17: LGTM: Query RPC and messages are well definedService, HTTP mapping, and pagination fields look correct and buf-friendly.
keeper/payload_handler.go (1)
45-51: Nil payload guard is missingAcceptPayload dereferences payload without checking nil. Add a fast-fail.
func (k *Keeper) AcceptPayload( ctx context.Context, payload *core.Payload, ) ([]byte, error) { + if payload == nil { + return nil, errors.New("nil payload") + } if err := payload.Validate(); err != nil { return nil, errorsmod.Wrap(err, "invalid payload") }⛔ Skipped due to learnings
Learnt from: 0xstepit PR: noble-assets/orbiter#10 File: keeper/component/dispatcher/dispatcher.go:151-155 Timestamp: 2025-08-14T15:11:47.828Z Learning: In the Orbiter codebase, the payload.Validate() method already handles nil pointer checks internally, so wrapper functions like ValidatePayload() don't need to add additional nil checks before calling payload.Validate().
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
keeper/payload_handler_test.go (1)
86-97: Avoid mutating the sharedvalidPayloadfixture across cases.
p := *validPayloadonly copies the top-level struct;p.Payloadstill aliases the original pointer. Appending toPreActions(and similar tweaks in later cases) permanently mutatesvalidPayload, so subsequent testcases observe the modified payload. That tight coupling can hide regressions today and will turn into flakes if we ever addt.Parallel()to this table. Please create an isolated fixture per case (e.g. callcreateTestPendingPayloadWithSequenceinside eachpayloadfunc or deep-copy the nested structs before mutating).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
keeper/payload_handler_test.go(1 hunks)types/core/payload.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- types/core/payload.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*_test.go: Write unit tests using the testCases pattern
Name each test case as "success - " for success cases and "error - " for error cases; omit the prefix when no error is tested
For error-checking cases, include an expError string field in the test case and assert with require.ErrorContains(t, tC.expError, err)
Files:
keeper/payload_handler_test.go
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Go code must be formatted and pass golangci-lint
All Go files must include license headers
Files:
keeper/payload_handler_test.go
🧬 Code graph analysis (1)
keeper/payload_handler_test.go (9)
testutil/random.go (1)
RandomBytes(28-36)keeper/keeper.go (1)
Keeper(51-72)types/core/id.pb.go (2)
ACTION_FEE(36-36)PROTOCOL_CCTP(73-73)types/core/orbiter.go (1)
NewAction(39-49)types/controller/forwarding/cctp.go (1)
NewCCTPForwarding(69-81)testutil/mocks/orbiter/orbiter.go (1)
OrbiterKeeper(35-43)types/expected_keepers.go (1)
PendingPayloadsHandler(60-64)types/marshal_and_unmarshal.go (1)
MarshalJSON(36-38)testutil/accounts.go (1)
NewNobleAddress(45-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test (TestIBCToCCTP)
- GitHub Check: test (TestIBCToHyperlane)
MalteHerrmann
left a comment
MalteHerrmann
left a comment
There was a problem hiding this comment.
thanks for the review @0xstepit!! -- will address your comments and I've also left some explainers on some of the questions, pls check 💪
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (4)
keeper/payload_handler.go (1)
114-116: Fix formatting of enums and user-facing messageUse .String() for enums to avoid “%!s(MISSING=…)”, and prefer “protocol” wording in the error.
- if paused { - return fmt.Errorf("action %s is paused", action.ID()) - } + if paused { + return fmt.Errorf("action %s is paused", action.ID().String()) + } @@ - if err != nil { - return errorsmod.Wrap(err, "failed to check if forwarder is paused") - } + if err != nil { + return errorsmod.Wrap(err, "failed to check if protocol is paused") + } @@ - if paused { - return fmt.Errorf("protocol %s is paused", payload.Forwarding.ProtocolId) - } + if paused { + return fmt.Errorf("protocol %s is paused", payload.Forwarding.ProtocolId.String()) + }Also applies to: 121-121, 124-126
keeper/query_server_test.go (1)
78-81: Fix: cannot range over intReplace the invalid range with an indexed loop.
- for range tc.nPayloads { + for i := 0; i < tc.nPayloads; i++ { _, err := k.Submit(ctx, examplePayload) require.NoError(t, err, "failed to setup payloads") }keeper/keeper.go (2)
43-46: Compile-time interface assertions are fine; avoid duplicates across filesIf the same assertions exist elsewhere, keep them in one place to prevent drift.
63-69: Fix lint: remove TODO in exported field commentgolangci-lint fails (godox) on the TODO. Reword the comment to be neutral and keep the field as-is.
[ suggest_essential_refactor ]
Apply:
- // TODO: this is only exported to be able to set the sequence in tests -- make private again? + // NOTE: exported so tests can override the sequence; consider tightening once a test helper exists. PendingPayloadsSequence collections.SequenceAs per coding guidelines and pipeline failures.
🧹 Nitpick comments (6)
keeper/query_server_test.go (1)
50-53: Nit: singular phrasingUse “1 hash stored” for clarity.
- name: "success - 1 hashes stored", + name: "success - 1 hash stored",keeper/msg_server_test.go (1)
43-49: Align test case field with guidelines (use expError and require.ErrorContains)Rename errContains → expError and update assertions. As per coding guidelines.
@@ - testcases := []struct { + testcases := []struct { name string setup func(*testing.T, context.Context, *orbiterkeeper.Keeper) payload *core.Payload - errContains string + expError string expHash string }{ @@ - name: "error - invalid (empty) payload", - payload: &core.Payload{}, - errContains: "forwarding is not set: invalid nil pointer", + name: "error - invalid (empty) payload", + payload: &core.Payload{}, + expError: "forwarding is not set: invalid nil pointer", @@ - payload: validPayload.Payload, - errContains: core.ErrSubmitPayload.Error(), + payload: validPayload.Payload, + expError: core.ErrSubmitPayload.Error(), @@ - if tc.errContains == "" { + if tc.expError == "" { require.NoError(t, err, "failed to accept payload") require.Equal(t, tc.expHash, ethcommon.BytesToHash(res.Hash).String()) } else { - require.ErrorContains(t, err, tc.errContains, "expected different error") + require.ErrorContains(t, err, tc.expError, "expected different error") }Also applies to: 56-59, 75-76, 94-99
keeper/msg_server.go (1)
53-55: Avoid duplicate validation; keep validation in Keeper.SubmitDrop the msg server Validate to keep one source of truth (you already validate in Keeper.Submit). This matches earlier discussion and reduces divergence.
- if err := (&payload).Validate(); err != nil { - return nil, sdkerrors.ErrInvalidRequest.Wrap(err.Error()) - }keeper/payload_handler.go (3)
129-131: Wrap error with context when computing forwarding attributesImproves debuggability.
- cachedAttrs, err := payload.Forwarding.CachedAttributes() - if err != nil { - return err - } + cachedAttrs, err := payload.Forwarding.CachedAttributes() + if err != nil { + return errorsmod.Wrap(err, "failed to compute forwarding attributes") + }
75-79: Use typed SDK error for duplicate payload hashReturn a wrapped core.ErrSubmitPayload instead of a generic error.
- if found { - k.Logger().Error("payload hash already registered", "hash", hash.String()) - - return nil, errors.New("payload hash already registered") - } + if found { + k.Logger().Error("payload hash already registered", "hash", hash.String()) + return nil, core.ErrSubmitPayload.Wrap("payload hash already registered") + }
159-163: Standardize hex formatting in logs (use 0x-prefixed form)Prefer Ethereum-style 0x hex for consistency with other logs and responses.
- "hash", hex.EncodeToString(hash), + "hash", ethcommon.BytesToHash(hash).Hex(), @@ - k.Logger().Debug("removed pending payload", "hash", hex.EncodeToString(hash)) + k.Logger().Debug("removed pending payload", "hash", ethcommon.BytesToHash(hash).Hex())Also applies to: 187-187
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
keeper/keeper.go(9 hunks)keeper/msg_server.go(1 hunks)keeper/msg_server_test.go(1 hunks)keeper/payload_handler.go(1 hunks)keeper/payload_handler_test.go(1 hunks)keeper/query_server.go(1 hunks)keeper/query_server_test.go(1 hunks)keeper/servers_registration.go(3 hunks)types/interfaces.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- keeper/payload_handler_test.go
- keeper/servers_registration.go
- keeper/query_server.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*_test.go: Write unit tests using the testCases pattern
Name each test case as "success - " for success cases and "error - " for error cases; omit the prefix when no error is tested
For error-checking cases, include an expError string field in the test case and assert with require.ErrorContains(t, tC.expError, err)
Files:
keeper/query_server_test.gokeeper/msg_server_test.go
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Go code must be formatted and pass golangci-lint
All Go files must include license headers
Files:
keeper/query_server_test.gokeeper/msg_server_test.gokeeper/keeper.gokeeper/payload_handler.gokeeper/msg_server.gotypes/interfaces.go
🧬 Code graph analysis (5)
keeper/query_server_test.go (2)
testutil/mocks/orbiter/orbiter.go (1)
OrbiterKeeper(35-43)keeper/query_server.go (1)
NewQueryServer(41-43)
keeper/msg_server_test.go (5)
keeper/keeper.go (1)
Keeper(49-70)types/core/errors.go (1)
ErrSubmitPayload(36-36)testutil/mocks/orbiter/orbiter.go (1)
OrbiterKeeper(35-43)keeper/msg_server.go (1)
NewMsgServer(40-42)types/marshal_and_unmarshal.go (1)
MarshalJSON(36-38)
keeper/keeper.go (4)
types/component.go (1)
Authorizer(36-38)types/interfaces.go (1)
PendingPayloadsHandler(31-35)types/core/orbiter.pb.go (3)
PendingPayload(234-239)PendingPayload(243-243)PendingPayload(244-246)types/core/keys.go (4)
PendingPayloadsPrefix(128-128)PendingPayloadsName(123-123)PendingPayloadsSequencePrefix(129-129)PendingPayloadsSequenceName(124-124)
keeper/payload_handler.go (3)
types/interfaces.go (1)
PendingPayloadsHandler(31-35)keeper/keeper.go (1)
Keeper(49-70)types/core/errors.go (1)
ErrRemovePayload(37-37)
keeper/msg_server.go (5)
api/v1/tx_grpc.pb.go (1)
MsgServer(59-64)types/tx.pb.go (7)
MsgServer(324-328)MsgSubmitPayload(37-43)MsgSubmitPayload(47-47)MsgSubmitPayload(48-50)MsgSubmitPayloadResponse(94-97)MsgSubmitPayloadResponse(101-101)MsgSubmitPayloadResponse(102-104)keeper/keeper.go (1)
Keeper(49-70)types/marshal_and_unmarshal.go (1)
UnmarshalJSON(30-32)types/core/errors.go (1)
ErrSubmitPayload(36-36)
🪛 GitHub Actions: Lint codebase
keeper/keeper.go
[error] 68-68: golangci-lint (godox): Line contains TODO/BUG/FIXME: "TODO: this is only exported to be able t..."
🪛 GitHub Check: lint
keeper/keeper.go
[failure] 68-68:
keeper/keeper.go:68: Line contains TODO/BUG/FIXME: "TODO: this is only exported to be able t..." (godox)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: prepare
🔇 Additional comments (5)
types/interfaces.go (1)
29-35: LGTM: interface surface is clearMethods cover submit, fetch, and remove; naming consistent with keeper implementation.
keeper/keeper.go (4)
39-41: Alias switch to orbitertypes looks goodConsistent with the rest of the PR and clarifies ownership.
95-107: Collections wiring for pending payloads/sequence LGTMPrefixes/names/codecs align with core keys; initialization before sb.Build() is correct.
81-82: Type changes to orbitertypes. are consistent*Updated signatures for BankKeeper and controller types are coherent with the import alias.
Also applies to: 132-133, 256-257
204-214: Controller router wiring LGTMPattern is consistent across forwarder/executor/adapter; panicking on misconfiguration is acceptable during app wiring.
Also applies to: 216-226, 228-238
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
keeper/payload_handler.go (1)
168-169: Fix incorrect docstring.The docstring states "If a payload is not found, it is a no-op but does not return an error" but the implementation at line 180 returns an error when the payload is not found.
Apply this diff to correct the docstring:
// RemovePendingPayload removes the pending payload from the module state. -// If a payload is not found, it is a no-op but does not return an error. +// If a payload is not found, it returns an error. func (k *Keeper) RemovePendingPayload(
🧹 Nitpick comments (2)
keeper/payload_handler_test.go (2)
158-168: Consider using a simpler event assertion pattern.The event checking logic manually iterates and sets a boolean. Consider using testify helpers for cleaner assertions.
Apply this diff to simplify the event assertion:
- // ASSERT: expected event emitted - events := ctx.EventManager().Events() - require.Len(t, events, 1, "expected 1 event, got %d", len(events)) - - found := false - for _, e := range events { - if strings.Contains(e.Type, "EventPayloadSubmitted") { - require.False(t, found, "expected event to be emitted just once") - found = true - } - } - require.True(t, found, "expected event payload submitted to be found") + // ASSERT: expected event emitted + events := ctx.EventManager().Events() + require.Len(t, events, 1, "expected 1 event") + require.Contains(t, events[0].Type, "EventPayloadSubmitted")
298-307: Consider simplifying event assertion (same as TestSubmit).The event checking logic can be streamlined using testify helpers.
Apply this diff:
- // ASSERT: event was emitted. - found := false - for _, event := range ctx.EventManager().ABCIEvents() { - if strings.Contains(event.Type, "EventPayloadRemoved") { - require.False(t, found, "event should only be emitted once") - - found = true - } - } - require.True(t, found, "expected event to be emitted") + // ASSERT: event was emitted. + events := ctx.EventManager().ABCIEvents() + var found bool + for _, event := range events { + if strings.Contains(event.Type, "EventPayloadRemoved") { + found = true + break + } + } + require.True(t, found, "expected EventPayloadRemoved to be emitted")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
api/v1/tx_grpc.pb.gois excluded by!**/*.pb.gotypes/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (7)
api/v1/tx.pulsar.go(3 hunks)keeper/keeper.go(9 hunks)keeper/msg_server_test.go(1 hunks)keeper/payload_handler.go(1 hunks)keeper/payload_handler_test.go(1 hunks)proto/noble/orbiter/v1/tx.proto(1 hunks)types/core/payload.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- keeper/msg_server_test.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Go code must be formatted and pass golangci-lint
All Go files must include license headers
Files:
keeper/keeper.goapi/v1/tx.pulsar.gokeeper/payload_handler_test.gokeeper/payload_handler.gotypes/core/payload.go
proto/**/*.proto
📄 CodeRabbit inference engine (CLAUDE.md)
Protobuf files must be formatted and linted with buf
Files:
proto/noble/orbiter/v1/tx.proto
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*_test.go: Write unit tests using the testCases pattern
Name each test case as "success - " for success cases and "error - " for error cases; omit the prefix when no error is tested
For error-checking cases, include an expError string field in the test case and assert with require.ErrorContains(t, tC.expError, err)
Files:
keeper/payload_handler_test.go
🧬 Code graph analysis (5)
keeper/keeper.go (5)
types/component.go (1)
Authorizer(36-38)types/interfaces.go (1)
PendingPayloadsHandler(31-35)types/core/orbiter.pb.go (3)
PendingPayload(234-239)PendingPayload(243-243)PendingPayload(244-246)types/expected_keepers.go (1)
BankKeeper(31-34)types/core/keys.go (4)
PendingPayloadsPrefix(128-128)PendingPayloadsName(123-123)PendingPayloadsSequencePrefix(129-129)PendingPayloadsSequenceName(124-124)
api/v1/tx.pulsar.go (3)
types/tx.pb.go (6)
MsgSubmitPayload(35-41)MsgSubmitPayload(45-45)MsgSubmitPayload(46-48)MsgSubmitPayloadResponse(92-95)MsgSubmitPayloadResponse(99-99)MsgSubmitPayloadResponse(100-102)types/core/orbiter.pb.go (3)
Payload(128-135)Payload(139-139)Payload(140-142)api/core/v1/orbiter.pulsar.go (3)
Payload(2671-2682)Payload(2697-2697)Payload(2700-2702)
keeper/payload_handler_test.go (10)
testutil/random.go (1)
RandomBytes(28-36)keeper/keeper.go (1)
Keeper(49-68)types/core/id.pb.go (2)
ACTION_FEE(36-36)PROTOCOL_CCTP(73-73)types/core/orbiter.go (1)
NewAction(39-49)types/controller/forwarding/cctp.go (1)
NewCCTPForwarding(69-81)testutil/mocks/orbiter/orbiter.go (1)
OrbiterKeeper(35-43)types/interfaces.go (1)
PendingPayloadsHandler(31-35)keeper/msg_server.go (1)
NewMsgServer(40-42)types/marshal_and_unmarshal.go (1)
MarshalJSON(36-38)testutil/accounts.go (1)
NewNobleAddress(45-47)
keeper/payload_handler.go (3)
types/interfaces.go (1)
PendingPayloadsHandler(31-35)keeper/keeper.go (1)
Keeper(49-68)types/core/errors.go (1)
ErrRemovePayload(37-37)
types/core/payload.go (1)
types/core/errors.go (1)
ErrNilPointer(28-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: prepare
- GitHub Check: build
🔇 Additional comments (14)
types/core/payload.go (2)
28-39: LGTM! CONTRACT comment documents caller responsibility.The method correctly computes the Keccak256 hash after marshaling. The CONTRACT comment appropriately documents that validation is the caller's responsibility, which aligns with the design decision from previous review discussions.
41-48: LGTM! Proper nil handling and delegation.The validation method correctly guards against nil receivers and delegates to the nested Payload validation.
proto/noble/orbiter/v1/tx.proto (1)
15-39: LGTM! Well-structured protobuf definitions.The RPC and message definitions follow Cosmos SDK conventions with proper annotations (cosmos.msg.v1.signer, amino.name) and clear documentation.
keeper/payload_handler_test.go (3)
176-229: LGTM! Good test coverage.The test properly covers both success and error paths with appropriate parallel execution.
318-357: LGTM! Excellent test for sequence-based uniqueness.This test correctly verifies that identical payloads receive different hashes due to sequence incrementation.
359-408: LGTM! Clean test and helper implementation.The test correctly verifies sequence-based hash differentiation, and the helper function provides a deterministic test payload as intended.
api/v1/tx.pulsar.go (1)
1-1144: Generated code - no review needed.This file is generated by protoc-gen-go-pulsar. Manual review is not necessary.
keeper/keeper.go (4)
43-46: LGTM! Proper interface compliance declarations.The compile-time interface assertions clearly document that Keeper implements both Authorizer and PendingPayloadsHandler interfaces.
63-67: LGTM! Well-structured pending payload storage.The pending payload storage fields are properly defined with appropriate visibility and clear documentation.
93-104: LGTM! Correct collections initialization.The pending payload storage initialization properly configures collections.Map and collections.Sequence with appropriate prefixes, keys, and codecs.
79-79: LGTM! Consistent type path usage.The change from
types.BankKeepertoorbitertypes.BankKeeperaligns with the explicit import path approach used throughout the file.keeper/payload_handler.go (3)
37-96: LGTM! Comprehensive payload submission logic.The method correctly validates, sequences, hashes, checks for duplicates, stores, and emits events. Error handling is thorough at each step.
98-146: LGTM! Proper state validation.The method correctly checks for paused actions, protocols, and cross-chains with appropriate error messages. The use of
.String()on enum types ensures readable error messages.
148-166: LGTM! Clean payload retrieval.The method correctly retrieves the pending payload with appropriate error handling and debug logging.
…ss information for potential griefing
…d with given hash
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
keeper/payload_handler.go (1)
149-150: Docstring contradicts implementation.The comment states that the function "is a no-op but does not return an error" when a payload is not found, but lines 160-162 actually return an error in this case.
Apply this diff to fix the docstring:
-// RemovePendingPayload removes the pending payload from the module state. -// If a payload is not found, it is a no-op but does not return an error. +// RemovePendingPayload removes the pending payload from the module state. +// If a payload is not found, it returns an error.
🧹 Nitpick comments (5)
keeper/query_server_test.go (1)
57-63: Use shared JSON helper for consistencyPrefer orbitertypes.MarshalJSON over cdc.MarshalJSON to match other tests.
- bz, err := cdc.MarshalJSON(examplePayload.Payload) + bz, err := orbitertypes.MarshalJSON(cdc, examplePayload.Payload)keeper/payload_handler_test.go (1)
95-109: Nice: local err avoids race; tighten NotFound assertion
- Local err variable fixes the earlier parallel race.
- Optional: assert gRPC status code on the follow-up query for clarity.
- require.Error(t, err, "payload should not be present anymore") + require.Error(t, err, "payload should not be present anymore") + // Optionally assert gRPC code: + // st, _ := status.FromError(err) + // require.Equal(t, codes.NotFound, st.Code())keeper/msg_server_test.go (2)
90-92: Fix message typoIt pauses the protocol, not “unpause fee action”.
- require.NoError(t, err, "failed to unpause fee action") + require.NoError(t, err, "failed to pause protocol")
115-117: Avoid assigning to outer err in closureNot parallel here, but shadowing is clearer and safer.
- err = k.Forwarder().Pause(ctx, core.PROTOCOL_CCTP, []string{cID}) - require.NoError(t, err, "failed to unpause cross-chain forwarding") + err := k.Forwarder().Pause(ctx, core.PROTOCOL_CCTP, []string{cID}) + require.NoError(t, err, "failed to pause cross-chain forwarding")keeper/query_server.go (1)
42-62: Use pointer receiver for queryServer methodsAlign with msgServer and typical server types; avoids copying and keeps style consistent.
-func (s queryServer) PendingPayload( +func (s *queryServer) PendingPayload( ctx context.Context, req *orbitertypes.QueryPendingPayloadRequest, ) (*orbitertypes.QueryPendingPayloadResponse, error) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
api/v1/query_grpc.pb.gois excluded by!**/*.pb.gotypes/query.pb.gois excluded by!**/*.pb.gotypes/query.pb.gw.gois excluded by!**/*.pb.gw.go
📒 Files selected for processing (10)
api/v1/query.pulsar.go(2 hunks)keeper/keeper.go(9 hunks)keeper/msg_server.go(1 hunks)keeper/msg_server_test.go(1 hunks)keeper/payload_handler.go(1 hunks)keeper/payload_handler_test.go(1 hunks)keeper/query_server.go(1 hunks)keeper/query_server_test.go(1 hunks)proto/noble/orbiter/v1/query.proto(1 hunks)types/constants.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Go code must be formatted and pass golangci-lint
All Go files must include license headers
Files:
types/constants.gokeeper/keeper.gokeeper/payload_handler_test.gokeeper/msg_server.gokeeper/query_server.gokeeper/msg_server_test.gokeeper/query_server_test.goapi/v1/query.pulsar.gokeeper/payload_handler.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*_test.go: Write unit tests using the testCases pattern
Name each test case as "success - " for success cases and "error - " for error cases; omit the prefix when no error is tested
For error-checking cases, include an expError string field in the test case and assert with require.ErrorContains(t, tC.expError, err)
Files:
keeper/payload_handler_test.gokeeper/msg_server_test.gokeeper/query_server_test.go
proto/**/*.proto
📄 CodeRabbit inference engine (CLAUDE.md)
Protobuf files must be formatted and linted with buf
Files:
proto/noble/orbiter/v1/query.proto
🧬 Code graph analysis (8)
keeper/keeper.go (5)
types/component.go (1)
Authorizer(36-38)types/core/orbiter.pb.go (3)
PendingPayload(234-239)PendingPayload(243-243)PendingPayload(244-246)types/expected_keepers.go (1)
BankKeeper(31-34)types/core/keys.go (4)
PendingPayloadsPrefix(128-128)PendingPayloadsName(123-123)PendingPayloadsSequencePrefix(129-129)PendingPayloadsSequenceName(124-124)types/controller.go (3)
ForwardingController(30-33)ActionController(37-40)AdapterController(44-47)
keeper/payload_handler_test.go (5)
testutil/mocks/orbiter/orbiter.go (1)
OrbiterKeeper(35-43)keeper/msg_server.go (1)
NewMsgServer(41-43)keeper/query_server.go (1)
NewQueryServer(38-40)api/v1/query.pulsar.go (3)
QueryPendingPayloadRequest(888-895)QueryPendingPayloadRequest(910-910)QueryPendingPayloadRequest(913-915)types/query.pb.go (3)
QueryPendingPayloadRequest(33-36)QueryPendingPayloadRequest(40-40)QueryPendingPayloadRequest(41-43)
keeper/msg_server.go (4)
api/v1/tx_grpc.pb.go (1)
MsgServer(59-64)types/tx.pb.go (7)
MsgServer(203-207)MsgSubmitPayload(35-41)MsgSubmitPayload(45-45)MsgSubmitPayload(46-48)MsgSubmitPayloadResponse(92-95)MsgSubmitPayloadResponse(99-99)MsgSubmitPayloadResponse(100-102)keeper/keeper.go (1)
Keeper(46-65)types/marshal_and_unmarshal.go (1)
UnmarshalJSON(30-32)
keeper/query_server.go (3)
types/query.pb.go (7)
QueryServer(187-189)QueryPendingPayloadRequest(33-36)QueryPendingPayloadRequest(40-40)QueryPendingPayloadRequest(41-43)QueryPendingPayloadResponse(78-81)QueryPendingPayloadResponse(85-85)QueryPendingPayloadResponse(86-88)keeper/keeper.go (1)
Keeper(46-65)types/constants.go (1)
PayloadHashLength(24-24)
keeper/msg_server_test.go (9)
testutil/random.go (1)
RandomBytes(28-36)keeper/keeper.go (1)
Keeper(46-65)types/core/id.pb.go (2)
ACTION_FEE(36-36)PROTOCOL_CCTP(73-73)types/core/orbiter.go (1)
NewAction(39-49)types/controller/forwarding/cctp.go (1)
NewCCTPForwarding(69-81)testutil/mocks/orbiter/orbiter.go (1)
OrbiterKeeper(35-43)keeper/msg_server.go (1)
NewMsgServer(41-43)types/marshal_and_unmarshal.go (1)
MarshalJSON(36-38)testutil/accounts.go (1)
NewNobleAddress(45-47)
keeper/query_server_test.go (6)
types/marshal_and_unmarshal.go (1)
MarshalJSON(36-38)testutil/mocks/orbiter/orbiter.go (1)
OrbiterKeeper(35-43)keeper/msg_server.go (1)
NewMsgServer(41-43)keeper/query_server.go (1)
NewQueryServer(38-40)api/v1/query.pulsar.go (3)
QueryPendingPayloadRequest(888-895)QueryPendingPayloadRequest(910-910)QueryPendingPayloadRequest(913-915)types/query.pb.go (3)
QueryPendingPayloadRequest(33-36)QueryPendingPayloadRequest(40-40)QueryPendingPayloadRequest(41-43)
api/v1/query.pulsar.go (3)
types/query.pb.go (6)
QueryPendingPayloadRequest(33-36)QueryPendingPayloadRequest(40-40)QueryPendingPayloadRequest(41-43)QueryPendingPayloadResponse(78-81)QueryPendingPayloadResponse(85-85)QueryPendingPayloadResponse(86-88)types/core/orbiter.pb.go (3)
Payload(128-135)Payload(139-139)Payload(140-142)api/core/v1/orbiter.pulsar.go (3)
Payload(2671-2682)Payload(2697-2697)Payload(2700-2702)
keeper/payload_handler.go (2)
keeper/keeper.go (1)
Keeper(46-65)types/core/errors.go (1)
ErrRemovePayload(37-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: prepare
🔇 Additional comments (9)
types/constants.go (1)
23-24: LGTM: canonical hash length constant32-byte length matches Keccak256; good centralization.
keeper/query_server_test.go (1)
83-107: Good coverage and parallelizationCovers success, NotFound, and InvalidArgument; parallel subtests are safe.
keeper/msg_server.go (1)
45-81: LGTM: validation and status mappingNil-check, JSON unmarshal, payload validation, stateful checks, submission, and codes mapping look solid.
proto/noble/orbiter/v1/query.proto (1)
5-27: LGTM!The protobuf definitions are well-structured. The Query service, request/response messages, and HTTP annotations are properly configured.
keeper/keeper.go (2)
60-64: LGTM!The pending payload storage fields are properly defined using Cosmos SDK collections. The Map uses BytesKey for hash-based lookups, and the Sequence provides auto-incrementing IDs for payload tracking.
90-101: LGTM!The initialization of pending payload storage is correct. Collections are properly configured with schema builder, prefixes, and codec.
keeper/payload_handler.go (2)
38-77: LGTM!The submit function properly handles payload registration with sequence generation, hash computation, duplicate checking, and storage.
83-127: LGTM!The validation logic correctly checks pause states for actions, protocols, and cross-chains. Error messages use
.String()methods on enum types for proper formatting.api/v1/query.pulsar.go (1)
1-1085: Generated code - no review needed.This file is auto-generated by protoc-gen-go-pulsar and should not be manually edited or reviewed in detail.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
api/v1/query.pulsar.go (1)
1-16: Add required license headerPer project guidelines, every Go file must start with the license header. Please prepend the standard header to this generated file before committing.
🧹 Nitpick comments (7)
Makefile (1)
85-89: Addlint-fixto the.PHONYlist.If a file named
lint-fixever appears,makewould skip this recipe. Mark it phony to keep the target reliable.-.PHONY: tool-all license format lint vulncheck nancy +.PHONY: tool-all license format lint lint-fix vulncheck nancykeeper/payload_handler_test.go (1)
45-50: Align testcase field name with repo guideline (errContains -> expError).Tests use errContains instead of expError. Please rename to match the testCases pattern in coding_guidelines and update assertions accordingly.
- errContains string + expError string @@ - errContains: sdkerrors.ErrNotFound.Wrapf( + expError: sdkerrors.ErrNotFound.Wrapf( @@ - errContains: core.ErrNilPointer.Wrap("payload hash").Error(), + expError: core.ErrNilPointer.Wrap("payload hash").Error(), @@ - if tc.errContains == "" { + if tc.expError == "" { require.NoError(t, err, "failed to remove payload") // ASSERT: value with hash was removed. @@ - } else { - require.ErrorContains(t, err, tc.errContains, "expected different error") - } + } else { + require.ErrorContains(t, err, tc.expError, "expected different error") + }As per coding guidelines
Also applies to: 70-74, 76-80, 95-110
keeper/query_server_test.go (1)
27-36: Prefer asserting gRPC status codes over error substrings.Use status.Code(err) to assert codes.NotFound/InvalidArgument; keeps tests stable across message changes.
import ( "context" "testing" "github.com/stretchr/testify/require" "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" @@ - } else { - require.ErrorContains(t, err, tc.errContains, "expected different error") - } + } else { + require.Error(t, err) + require.Equal(t, tc.errContains, codes.Code(status.Code(err)).String(), "expected different grpc code") + }Alternatively, change tc.errContains to hold codes.Code and assert directly on status.Code(err). Based on learnings
Also applies to: 101-106
autocli.go (1)
111-116: Improve CLI UX: include argument in Use string.Show the expected [hash] arg in the query command usage.
- Use: "pending", + Use: "pending [hash]",Based on learnings
keeper/query_server.go (1)
23-31: Improve error mapping and receiver semantics.Differentiate not-found from internal errors; use pointer receiver; include hash in error for clarity.
import ( "context" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" + errorsmod "cosmossdk.io/errors" + sdkerrors "github.com/cosmos/cosmos-sdk/types/errors" orbitertypes "github.com/noble-assets/orbiter/types" "github.com/noble-assets/orbiter/types/core" ) @@ -func (s queryServer) PendingPayload( +func (s *queryServer) PendingPayload( ctx context.Context, req *orbitertypes.QueryPendingPayloadRequest, ) (*orbitertypes.QueryPendingPayloadResponse, error) { if req == nil { return nil, status.Error(codes.InvalidArgument, "empty request") } @@ - payload, err := s.pendingPayload(ctx, hash) - if err != nil { - return nil, status.Error(codes.NotFound, "payload not found") - } + payload, err := s.pendingPayload(ctx, hash) + if err != nil { + if errorsmod.IsOf(err, sdkerrors.ErrNotFound) { + return nil, status.Errorf(codes.NotFound, "payload %s not found", req.Hash) + } + return nil, status.Errorf(codes.Internal, "failed to retrieve payload %s", req.Hash) + }Also applies to: 43-64
keeper/msg_server_test.go (1)
48-54: Adopt testCases naming and minor cleanups.
- Rename errContains -> expError per guidelines.
- Fix typo in error message ("unpause" -> "pause").
- Prefer local err in setup to avoid outer var reuse.
- errContains string + expError string @@ - err := k.Forwarder().Pause(ctx, core.PROTOCOL_CCTP, nil) - require.NoError(t, err, "failed to unpause fee action") + err := k.Forwarder().Pause(ctx, core.PROTOCOL_CCTP, nil) + require.NoError(t, err, "failed to pause protocol") @@ - res, err := ms.SubmitPayload(ctx, &orbitertypes.MsgSubmitPayload{ + res, err := ms.SubmitPayload(ctx, &orbitertypes.MsgSubmitPayload{ Payload: string(payloadJSON), }) - if tc.errContains == "" { + if tc.expError == "" { require.NoError(t, err, "failed to submit payload") require.Equal(t, tc.expHash.String(), core.PayloadHash(res.Hash).String()) } else { - require.ErrorContains(t, err, tc.errContains, "expected different error") + require.ErrorContains(t, err, tc.expError, "expected different error") }As per coding guidelines
Also applies to: 129-132, 147-156
keeper/payload_handler.go (1)
158-175: Correct the RemovePendingPayload docstringComment claims absence is a no-op, yet the function returns
sdkerrors.ErrNotFound. Update the comment to match behavior.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
types/query.pb.gois excluded by!**/*.pb.gotypes/query.pb.gw.gois excluded by!**/*.pb.gw.gotypes/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (14)
Makefile(1 hunks)api/v1/query.pulsar.go(2 hunks)api/v1/tx.pulsar.go(3 hunks)autocli.go(4 hunks)keeper/keeper.go(9 hunks)keeper/msg_server_test.go(1 hunks)keeper/payload_handler.go(1 hunks)keeper/payload_handler_test.go(1 hunks)keeper/query_server.go(1 hunks)keeper/query_server_test.go(1 hunks)proto/noble/orbiter/v1/query.proto(1 hunks)proto/noble/orbiter/v1/tx.proto(1 hunks)types/autocli.go(1 hunks)types/core/payload.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- types/autocli.go
- keeper/keeper.go
🧰 Additional context used
📓 Path-based instructions (3)
proto/**/*.proto
📄 CodeRabbit inference engine (CLAUDE.md)
Protobuf files must be formatted and linted with buf
Files:
proto/noble/orbiter/v1/tx.protoproto/noble/orbiter/v1/query.proto
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*_test.go: Write unit tests using the testCases pattern
Name each test case as "success - " for success cases and "error - " for error cases; omit the prefix when no error is tested
For error-checking cases, include an expError string field in the test case and assert with require.ErrorContains(t, tC.expError, err)
Files:
keeper/query_server_test.gokeeper/payload_handler_test.gokeeper/msg_server_test.go
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Go code must be formatted and pass golangci-lint
All Go files must include license headers
Files:
keeper/query_server_test.gokeeper/payload_handler_test.goautocli.gokeeper/payload_handler.gokeeper/query_server.goapi/v1/query.pulsar.gokeeper/msg_server_test.goapi/v1/tx.pulsar.gotypes/core/payload.go
🧠 Learnings (2)
📚 Learning: 2025-08-18T07:26:16.709Z
Learnt from: 0xstepit
PR: noble-assets/orbiter#15
File: autocli.go:26-35
Timestamp: 2025-08-18T07:26:16.709Z
Learning: In the orbiter codebase, service descriptors follow different naming conventions depending on their package location: API packages (api/*/v1) use Msg_ServiceDesc/Query_ServiceDesc (uppercase 'S'), while types packages (types/component/*) use Msg_serviceDesc/Query_serviceDesc (lowercase 's'). The autocli.go correctly uses both conventions depending on which service descriptors it's referencing.
Applied to files:
autocli.go
📚 Learning: 2025-08-25T10:35:00.822Z
Learnt from: CR
PR: noble-assets/orbiter#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-25T10:35:00.822Z
Learning: Applies to **/*.go : Go code must be formatted and pass golangci-lint
Applied to files:
Makefile
🧬 Code graph analysis (9)
keeper/query_server_test.go (6)
types/tx.pb.go (4)
MsgServer(203-207)MsgSubmitPayload(35-41)MsgSubmitPayload(45-45)MsgSubmitPayload(46-48)types/marshal_and_unmarshal.go (1)
MarshalJSON(36-38)testutil/mocks/orbiter/orbiter.go (1)
OrbiterKeeper(35-43)keeper/msg_server.go (1)
NewMsgServer(41-43)keeper/query_server.go (1)
NewQueryServer(39-41)api/v1/query.pulsar.go (3)
QueryPendingPayloadRequest(886-893)QueryPendingPayloadRequest(908-908)QueryPendingPayloadRequest(911-913)
keeper/payload_handler_test.go (6)
types/tx.pb.go (4)
MsgServer(203-207)MsgSubmitPayload(35-41)MsgSubmitPayload(45-45)MsgSubmitPayload(46-48)types/core/errors.go (1)
ErrNilPointer(28-28)testutil/mocks/orbiter/orbiter.go (1)
OrbiterKeeper(35-43)keeper/msg_server.go (1)
NewMsgServer(41-43)keeper/query_server.go (1)
NewQueryServer(39-41)api/v1/query.pulsar.go (3)
QueryPendingPayloadRequest(886-893)QueryPendingPayloadRequest(908-908)QueryPendingPayloadRequest(911-913)
autocli.go (3)
types/tx.pb.go (1)
Msg_serviceDesc(239-239)types/autocli.go (2)
TxCommandOptions(25-40)QueryCommandOptions(42-54)types/query.pb.go (1)
Query_serviceDesc(221-221)
keeper/payload_handler.go (3)
keeper/keeper.go (1)
Keeper(46-65)types/core/payload.go (1)
PayloadHash(59-59)types/core/errors.go (1)
ErrNilPointer(28-28)
keeper/query_server.go (4)
api/v1/query_grpc.pb.go (1)
QueryServer(57-60)types/query.pb.go (7)
QueryServer(187-189)QueryPendingPayloadRequest(33-36)QueryPendingPayloadRequest(40-40)QueryPendingPayloadRequest(41-43)QueryPendingPayloadResponse(78-81)QueryPendingPayloadResponse(85-85)QueryPendingPayloadResponse(86-88)keeper/keeper.go (1)
Keeper(46-65)types/core/payload.go (1)
ParsePayloadHash(62-75)
api/v1/query.pulsar.go (2)
types/query.pb.go (6)
QueryPendingPayloadRequest(33-36)QueryPendingPayloadRequest(40-40)QueryPendingPayloadRequest(41-43)QueryPendingPayloadResponse(78-81)QueryPendingPayloadResponse(85-85)QueryPendingPayloadResponse(86-88)api/core/v1/orbiter.pulsar.go (3)
Payload(2671-2682)Payload(2697-2697)Payload(2700-2702)
keeper/msg_server_test.go (12)
testutil/random.go (1)
RandomBytes(28-36)keeper/keeper.go (1)
Keeper(46-65)types/core/payload.go (1)
PayloadHash(59-59)types/core/id.pb.go (2)
ACTION_FEE(36-36)PROTOCOL_CCTP(73-73)types/core/orbiter.go (1)
NewAction(39-49)types/controller/forwarding/cctp.go (1)
NewCCTPForwarding(69-81)testutil/mocks/orbiter/orbiter.go (1)
OrbiterKeeper(35-43)keeper/msg_server.go (1)
NewMsgServer(41-43)types/marshal_and_unmarshal.go (1)
MarshalJSON(36-38)api/v1/tx.pulsar.go (3)
MsgSubmitPayload(939-949)MsgSubmitPayload(964-964)MsgSubmitPayload(967-969)types/tx.pb.go (3)
MsgSubmitPayload(35-41)MsgSubmitPayload(45-45)MsgSubmitPayload(46-48)testutil/accounts.go (1)
NewNobleAddress(45-47)
api/v1/tx.pulsar.go (1)
types/tx.pb.go (6)
MsgSubmitPayload(35-41)MsgSubmitPayload(45-45)MsgSubmitPayload(46-48)MsgSubmitPayloadResponse(92-95)MsgSubmitPayloadResponse(99-99)MsgSubmitPayloadResponse(100-102)
types/core/payload.go (1)
types/core/errors.go (1)
ErrNilPointer(28-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
proto/noble/orbiter/v1/tx.proto (1)
15-18: Proto additions look good.Service/RPC and message definitions are clear and buf-lint friendly.
Also applies to: 20-39
api/v1/tx.pulsar.go (1)
1-1144: Generated code (no review required).Skipping manual review; changes should be managed via proto and codegen.
proto/noble/orbiter/v1/query.proto (1)
12-17: Query surface is well-defined and consistent.RPC, HTTP annotation, and messages align with server behavior.
Also applies to: 19-27
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
keeper/query_server_test.go (1)
112-114: Fix assertion: compare payloads, not wrapper messages.The assertion compares
tc.expPayload.String()(a*core.PendingPayload) withgot.String()(a*orbitertypes.QueryPendingPayloadResponse). This compares the string representations of different wrapper message types instead of the actual payload contents.Extract the payload from the response and compare it directly:
if tc.errContains == "" { require.NoError(t, err, "failed to get pending payload") - require.Equal(t, tc.expPayload.String(), got.String(), "expected different payload") + require.NotNil(t, got, "response should not be nil") + require.NotNil(t, got.Payload, "response payload should not be nil") + require.Equal(t, tc.expPayload.String(), got.Payload.String(), "expected different payload") } else {Alternatively, use proto equality for a more robust comparison:
if tc.errContains == "" { require.NoError(t, err, "failed to get pending payload") - require.Equal(t, tc.expPayload.String(), got.String(), "expected different payload") + require.NotNil(t, got, "response should not be nil") + require.NotNil(t, got.Payload, "response payload should not be nil") + require.True(t, proto.Equal(tc.expPayload, got.Payload), "expected different payload") } else {
🧹 Nitpick comments (2)
types/core/payload_test.go (1)
53-53: Consider renaming totestCasesfor consistency.The variable is named
testcases(lowercase 'c'), but Go convention typically uses camelCase for multi-word identifiers.Apply this diff:
- testcases := []struct { + testCases := []struct {And update the loop variable reference:
- for _, tc := range testcases { + for _, tc := range testCases {keeper/query_server_test.go (1)
73-73: Consider removing unusedexpPayloadfield in error test cases.The
expPayloadfield is set in the "error - hash not found" test case but is never used because the test only checks error conditions. The same pattern could apply to other error cases ifexpPayloadwere added there.Consider removing this field from error test cases to reduce clutter:
{ name: "error - hash not found", setup: nil, - expPayload: examplePayload, req: &orbitertypes.QueryPendingPayloadRequest{ Hash: exampleHash.String(), }, errContains: codes.NotFound.String(), },
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
keeper/query_server_test.go(1 hunks)types/core/payload.go(1 hunks)types/core/payload_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- types/core/payload.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*_test.go: Write unit tests using the testCases pattern
Name each test case as "success - " for success cases and "error - " for error cases; omit the prefix when no error is tested
For error-checking cases, include an expError string field in the test case and assert with require.ErrorContains(t, tC.expError, err)
Files:
types/core/payload_test.gokeeper/query_server_test.go
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Go code must be formatted and pass golangci-lint
All Go files must include license headers
Files:
types/core/payload_test.gokeeper/query_server_test.go
🧬 Code graph analysis (2)
types/core/payload_test.go (4)
types/core/payload.go (1)
ParsePayloadHash(55-68)types/controller/forwarding/cctp.go (1)
NewCCTPForwarding(69-81)testutil/random.go (1)
RandomBytes(28-36)types/core/orbiter.go (1)
NewPayload(249-263)
keeper/query_server_test.go (6)
types/tx.pb.go (4)
MsgServer(203-207)MsgSubmitPayload(35-41)MsgSubmitPayload(45-45)MsgSubmitPayload(46-48)types/query.pb.go (3)
QueryPendingPayloadRequest(33-36)QueryPendingPayloadRequest(40-40)QueryPendingPayloadRequest(41-43)types/marshal_and_unmarshal.go (1)
MarshalJSON(36-38)testutil/mocks/orbiter/orbiter.go (1)
OrbiterKeeper(35-43)keeper/msg_server.go (1)
NewMsgServer(41-43)keeper/query_server.go (1)
NewQueryServer(39-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: prepare
- GitHub Check: build
🔇 Additional comments (4)
types/core/payload_test.go (4)
1-20: LGTM!License header is properly included as required.
33-43: Test logic is sound.The round-trip test correctly validates SHA256 hashing and parsing workflows. The test creates a pending payload, hashes it, parses the hash string back, and asserts the round-trip produces identical results.
45-87: Well-structured test following coding guidelines.The test correctly follows the testCases pattern with proper naming ("success - valid hash", "error - invalid hash", "error - too short hash"), uses the
errContainsfield, and asserts errors withrequire.ErrorContains. Test cases align well with theParsePayloadHashimplementation, covering valid hex parsing and both error paths (invalid hex format and wrong byte length).As per coding guidelines.
89-107: Helper function is well-implemented.The helper correctly uses
t.Helper()to improve error reporting, constructs a validPendingPayloadviaforwarding.NewCCTPForwardingandcore.NewPayload, and properly handles errors during setup. Using random bytes formintRecipientis appropriate for test isolation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
keeper/query_server_test.go (1)
111-118: Add nil checks before accessing nested payload field.Before accessing
got.Payload.String(), verify that bothgotandgot.Payloadare non-nil. This prevents panics and provides clearer test failure messages if the response structure is unexpected.Apply this diff to add defensive nil checks:
if tc.errContains == "" { require.NoError(t, err, "failed to get pending payload") + require.NotNil(t, got, "response should not be nil") + require.NotNil(t, got.Payload, "response payload should not be nil") require.Equal( t, tc.expPayload.Payload.String(), got.Payload.String(), "expected different payload", )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
keeper/query_server_test.go(1 hunks)types/core/payload.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- types/core/payload.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*_test.go: Write unit tests using the testCases pattern
Name each test case as "success - " for success cases and "error - " for error cases; omit the prefix when no error is tested
For error-checking cases, include an expError string field in the test case and assert with require.ErrorContains(t, tC.expError, err)
Files:
keeper/query_server_test.go
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Go code must be formatted and pass golangci-lint
All Go files must include license headers
Files:
keeper/query_server_test.go
🧬 Code graph analysis (1)
keeper/query_server_test.go (7)
types/tx.pb.go (4)
MsgServer(203-207)MsgSubmitPayload(35-41)MsgSubmitPayload(45-45)MsgSubmitPayload(46-48)api/v1/query.pulsar.go (3)
QueryPendingPayloadRequest(886-893)QueryPendingPayloadRequest(908-908)QueryPendingPayloadRequest(911-913)types/query.pb.go (3)
QueryPendingPayloadRequest(33-36)QueryPendingPayloadRequest(40-40)QueryPendingPayloadRequest(41-43)types/marshal_and_unmarshal.go (1)
MarshalJSON(36-38)testutil/mocks/orbiter/orbiter.go (1)
OrbiterKeeper(35-43)keeper/msg_server.go (1)
NewMsgServer(41-43)keeper/query_server.go (1)
NewQueryServer(39-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: prepare
🔇 Additional comments (1)
keeper/query_server_test.go (1)
60-63: Add signer field to MsgSubmitPayload.The
MsgSubmitPayloadinitialization is missing theSignerfield. While proto3 makes fields optional by default, the message server may validate that a signer is provided. Best practice is to set all relevant fields in test setup.Apply this diff to add a signer:
_, err = ms.SubmitPayload(ctx, &orbitertypes.MsgSubmitPayload{ + Signer: "test-signer", Payload: string(bz), })If a valid address format is required, you may need to generate a proper SDK address instead of using a plain string.
0xstepit
left a comment
0xstepit
left a comment
There was a problem hiding this comment.
Amazing work here @MalteHerrmann 🚀 added more comment, but we are close to the final state!!
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
types/core/payload.go (2)
31-45: Value receiver prevents nil-receiver panics; CONTRACT respected.The value receiver (line 35) makes nil-receiver panics impossible, addressing previous nil-guard concerns. The CONTRACT comment (line 34) appropriately documents the validation prerequisite. The implementation correctly marshals, hashes, and converts to
*PayloadHash.One minor suggestion: returning
*PayloadHash(a pointer to a 32-byte array) instead ofPayloadHash(the array itself) adds a heap allocation. For value types like[32]byte, returning by value is typically more efficient unless there's a specific reason to return a pointer (e.g., nil-ability). Consider whether the pointer return is necessary or ifPayloadHash(value) would suffice.
54-67: Consider wrapping the length validation error for consistency.Lines 58 and 62 handle errors differently: hex-decode failures are wrapped with
errorsmod.Wrap, while length mismatches useerrors.Newdirectly. For consistency and better error context, consider wrapping the length error as well:if len(bz) != PayloadHashLength { - return nil, errors.New("malformed payload hash") + return nil, errorsmod.Wrap(errors.New("malformed payload hash"), "invalid length") }This ensures all errors from
NewPayloadHashhave consistent wrapping and contextual information.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
types/core/orbiter.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (7)
api/core/v1/orbiter.pulsar.go(5 hunks)keeper/msg_server_test.go(1 hunks)keeper/query_server.go(1 hunks)keeper/query_server_test.go(1 hunks)proto/noble/orbiter/core/v1/orbiter.proto(1 hunks)types/core/payload.go(1 hunks)types/core/payload_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
proto/**/*.proto
📄 CodeRabbit inference engine (CLAUDE.md)
Protobuf files must be formatted and linted with buf
Files:
proto/noble/orbiter/core/v1/orbiter.proto
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Go code must be formatted and pass golangci-lint
All Go files must include license headers
Files:
keeper/query_server.gotypes/core/payload_test.goapi/core/v1/orbiter.pulsar.gokeeper/query_server_test.gotypes/core/payload.gokeeper/msg_server_test.go
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*_test.go: Write unit tests using the testCases pattern
Name each test case as "success - " for success cases and "error - " for error cases; omit the prefix when no error is tested
For error-checking cases, include an expError string field in the test case and assert with require.ErrorContains(t, tC.expError, err)
Files:
types/core/payload_test.gokeeper/query_server_test.gokeeper/msg_server_test.go
🧠 Learnings (1)
📚 Learning: 2025-08-25T10:35:00.822Z
Learnt from: CR
PR: noble-assets/orbiter#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-25T10:35:00.822Z
Learning: Applies to **/*_test.go : Write unit tests using the testCases pattern
Applied to files:
keeper/query_server_test.gokeeper/msg_server_test.go
🧬 Code graph analysis (6)
keeper/query_server.go (4)
types/query.pb.go (6)
QueryPendingPayloadRequest(33-36)QueryPendingPayloadRequest(40-40)QueryPendingPayloadRequest(41-43)QueryPendingPayloadResponse(78-81)QueryPendingPayloadResponse(85-85)QueryPendingPayloadResponse(86-88)keeper/keeper.go (1)
Keeper(46-65)types/core/orbiter.pb.go (6)
PendingPayload(234-239)PendingPayload(243-243)PendingPayload(244-246)Payload(128-135)Payload(139-139)Payload(140-142)types/core/payload.go (1)
NewPayloadHash(55-68)
types/core/payload_test.go (5)
types/core/payload.go (1)
NewPayloadHash(55-68)types/core/orbiter.pb.go (6)
PendingPayload(234-239)PendingPayload(243-243)PendingPayload(244-246)Payload(128-135)Payload(139-139)Payload(140-142)types/controller/forwarding/cctp.go (1)
NewCCTPForwarding(69-81)testutil/random.go (1)
RandomBytes(28-36)types/core/orbiter.go (1)
NewPayload(249-263)
api/core/v1/orbiter.pulsar.go (2)
types/core/orbiter.pb.go (12)
Payload(128-135)Payload(139-139)Payload(140-142)Action(35-43)Action(47-47)Action(48-50)Forwarding(79-91)Forwarding(95-95)Forwarding(96-98)PayloadWrapper(187-191)PayloadWrapper(195-195)PayloadWrapper(196-198)api/core/v1/id.pulsar.go (8)
ActionID(500-500)ActionID(536-538)ActionID(540-542)ActionID(549-551)ProtocolID(556-556)ProtocolID(600-602)ProtocolID(604-606)ProtocolID(613-615)
keeper/query_server_test.go (6)
types/tx.pb.go (4)
MsgServer(203-207)MsgSubmitPayload(35-41)MsgSubmitPayload(45-45)MsgSubmitPayload(46-48)types/core/orbiter.pb.go (6)
PendingPayload(234-239)PendingPayload(243-243)PendingPayload(244-246)Payload(128-135)Payload(139-139)Payload(140-142)types/query.pb.go (3)
QueryPendingPayloadRequest(33-36)QueryPendingPayloadRequest(40-40)QueryPendingPayloadRequest(41-43)testutil/mocks/orbiter/orbiter.go (1)
OrbiterKeeper(35-43)keeper/msg_server.go (1)
NewMsgServer(41-43)keeper/query_server.go (1)
NewQueryServer(41-43)
types/core/payload.go (2)
api/core/v1/orbiter.pulsar.go (3)
PendingPayload(2759-2768)PendingPayload(2783-2783)PendingPayload(2786-2788)types/core/orbiter.pb.go (3)
PendingPayload(234-239)PendingPayload(243-243)PendingPayload(244-246)
keeper/msg_server_test.go (13)
testutil/random.go (1)
RandomBytes(28-36)keeper/keeper.go (1)
Keeper(46-65)api/core/v1/orbiter.pulsar.go (9)
Payload(2671-2682)Payload(2697-2697)Payload(2700-2702)Forwarding(2610-2626)Forwarding(2641-2641)Forwarding(2644-2646)PendingPayload(2759-2768)PendingPayload(2783-2783)PendingPayload(2786-2788)types/core/orbiter.pb.go (9)
Payload(128-135)Payload(139-139)Payload(140-142)Forwarding(79-91)Forwarding(95-95)Forwarding(96-98)PendingPayload(234-239)PendingPayload(243-243)PendingPayload(244-246)types/core/payload.go (1)
PayloadHash(52-52)types/core/id.pb.go (2)
ACTION_FEE(36-36)PROTOCOL_CCTP(73-73)types/core/orbiter.go (1)
NewAction(39-49)types/controller/forwarding/cctp.go (1)
NewCCTPForwarding(69-81)testutil/mocks/orbiter/orbiter.go (1)
OrbiterKeeper(35-43)keeper/msg_server.go (1)
NewMsgServer(41-43)types/marshal_and_unmarshal.go (1)
MarshalJSON(36-38)api/v1/tx.pulsar.go (3)
MsgSubmitPayload(939-949)MsgSubmitPayload(964-964)MsgSubmitPayload(967-969)testutil/accounts.go (1)
NewNobleAddress(45-47)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: prepare
- GitHub Check: build
🔇 Additional comments (13)
types/core/payload.go (2)
47-52: LGTM!The constant and type definition are appropriate for SHA-256 hashes (32 bytes).
70-76: LGTM!The helper methods correctly convert the hash to bytes and hex string representations.
api/core/v1/orbiter.pulsar.go (1)
2058-2539: LGTM! Generated protobuf code follows standard patterns.The generated fast-reflection scaffolding for
PendingPayloadcorrectly implements descriptor access, field operations (Has/Clear/Get/Set/Mutable), and protobuf methods (Size/Marshal/Unmarshal). The structure aligns with the proto definition and follows protobuf code generation conventions.proto/noble/orbiter/core/v1/orbiter.proto (1)
78-84: LGTM! Well-documented message definition.The
PendingPayloadmessage is clearly defined with appropriate field types. The comment on line 82 accurately describes the payload field's purpose.types/core/payload_test.go (3)
33-43: LGTM! Round-trip test validates hash consistency.The test correctly verifies that a hash can be serialized to string and parsed back, maintaining equality.
45-87: LGTM! Comprehensive test coverage for NewPayloadHash.The test cases follow coding guidelines with parallel execution and proper error validation. The three scenarios (success, invalid hex, invalid length) provide good coverage of the parsing logic.
89-107: LGTM! Clear helper function.The helper correctly constructs a test
PendingPayloadwith proper error handling.keeper/query_server_test.go (1)
38-124: LGTM! Comprehensive test coverage for PendingPayload query.The test follows coding guidelines with the
testCasespattern and parallel execution. All scenarios are well-covered:
- Success path with payload submission and retrieval
- NotFound when hash doesn't exist
- InvalidArgument for nil request and empty hash
The assertions correctly compare payload string representations (lines 113-118) after extracting from the response.
keeper/query_server.go (1)
45-66: LGTM! Proper input validation and error handling.The implementation correctly:
- Guards against nil requests (line 49-51)
- Validates hash format via
core.NewPayloadHash(line 53-56)- Returns appropriate gRPC status codes (InvalidArgument for validation, NotFound for missing data)
- Extracts the payload from
PendingPayloadfor the response (line 64)The error wrapping with
errorsmod.Wrapprovides good context for debugging.keeper/msg_server_test.go (4)
38-158: LGTM! Comprehensive test coverage for SubmitPayload.The test thoroughly validates:
- Success path with valid payload
- Rejection of paused actions, protocols, and cross-chains
- Invalid payload handling
The table-driven approach with setup functions (lines 50, 62-67, 85-90, 105-115) properly configures pause states. Hash comparison (line 152) correctly validates the returned hash.
163-202: LGTM! Validates sequence-based hash uniqueness.The test effectively demonstrates that identical payloads submitted sequentially generate different hashes due to sequence increment. This is critical for preventing hash collisions on duplicate submissions.
204-223: LGTM! Confirms sequence impact on hash generation.The test independently verifies that different sequence numbers produce different hashes, validating the hash uniqueness mechanism.
227-253: LGTM! Clean test helper with proper error handling.The helper function correctly constructs test
PendingPayloadinstances with specified sequences. The fixed recipient bytes (lines 233-234) ensure deterministic test data.
|
putting this into |
0xstepit
changed the title
This PR adds the required logic to clean up pending payloads after they have been submitted for a given lifespan period (currently set to 1 day). The clean up is done in the `BeginBlock` and is limited to 200 payloads per block to avoid running into timeouts or deadlocks after spam attacks through mass payload submissions. The iteration over the registered payloads is implemented by adding a new `collections.Index` for the block time of submission and using an `IndexedMap` instead of a regular `Map`. This makes it possible to call `k.pendingPayloads.Indexes.HashByTime.Walk(...)` which has a ranger implementation passed, to only iterate through those payloads that are expired.
There was a problem hiding this comment.
Actionable comments posted: 12
♻️ Duplicate comments (1)
keeper/payload_handler.go (1)
172-174: Docstring contradicts behavior; update to reflect NotFound error.Comment says no-op, but code returns sdkerrors.ErrNotFound on missing payload.
-// If a payload is not found, it is a no-op but does not return an error. +// If a payload is not found, it returns a not-found error.
🧹 Nitpick comments (8)
types/core/payload_test.go (1)
53-53: UsetestCasesinstead oftestcasesfor consistency.Following the codebase convention and coding guidelines, the test case slice should be named
testCases.As per coding guidelines
Apply this diff:
- testcases := []struct { + testCases := []struct {Don't forget to update the loop variable reference at line 74:
- for _, tc := range testcases { + for _, tc := range testCases {proto/noble/orbiter/v1/query.proto (1)
21-24: RPC surface looks good; minor REST path nitThe GET path reads “/payload/{hash}”. Consider plural (“/payloads/{hash}”) for consistency with resource naming, but it’s optional. Otherwise options and messages look correct and buf‑lint friendly.
keeper/msg_server_test.go (1)
119-121: Avoid capturing outer err in closureUse a local variable to prevent accidental sharing.
- err = k.Forwarder().Pause(ctx, core.PROTOCOL_CCTP, []string{cID}) - require.NoError(t, err, "failed to unpause cross-chain forwarding") + if err := k.Forwarder().Pause(ctx, core.PROTOCOL_CCTP, []string{cID}); err != nil { + require.NoError(t, err, "failed to pause cross-chain forwarding") + }keeper/msg_server.go (1)
45-81: Unwrap sdk.Context before keeper calls (verify signatures)If validatePayloadAgainstState/submit expect sdk.Context (common in Cosmos SDK), unwrap once and pass sdkCtx. Otherwise, ignore this.
func (s *msgServer) SubmitPayload( ctx context.Context, req *orbitertypes.MsgSubmitPayload, ) (*orbitertypes.MsgSubmitPayloadResponse, error) { + sdkCtx := sdk.UnwrapSDKContext(ctx) @@ - if err := s.validatePayloadAgainstState(ctx, &payload); err != nil { + if err := s.validatePayloadAgainstState(sdkCtx, &payload); err != nil { @@ - payloadHash, err := s.submit(ctx, &payload) + payloadHash, err := s.submit(sdkCtx, &payload)(Add:
sdk "github.com/cosmos/cosmos-sdk/types"import.)Also ensure registration uses the same MsgServer interface you implement. If you register against api/v1/tx_grpc.pb.go, you might need to embed UnimplementedMsgServer; otherwise your current compile-time assert is fine.
keeper/payload_handler.go (4)
121-124: Wrap CachedAttributes error for context consistency.Return context-rich errors like elsewhere in this method.
- cachedAttrs, err := payload.Forwarding.CachedAttributes() - if err != nil { - return err - } + cachedAttrs, err := payload.Forwarding.CachedAttributes() + if err != nil { + return errorsmod.Wrap(err, "failed to cache forwarding attributes") + }
154-161: Preserve underlying error vs always mapping to NotFound.If Get fails for reasons other than not-found, wrap and return the original error; only map collections.ErrNotFound to sdkerrors.ErrNotFound.
- payload, err := k.pendingPayloads.Get(ctx, hash.Bytes()) - if err != nil { - k.Logger().Error( - "failed to retrieve pending payload", - "hash", hash.String(), - ) - - return nil, sdkerrors.ErrNotFound.Wrapf("payload with hash %s", hash.String()) - } + payload, err := k.pendingPayloads.Get(ctx, hash.Bytes()) + if err != nil { + if errors.Is(err, collections.ErrNotFound) { + return nil, sdkerrors.ErrNotFound.Wrapf("payload with hash %s", hash.String()) + } + return nil, errorsmod.Wrap(err, "failed to retrieve pending payload") + }
78-82: Use SDK error types instead of plain errors.New for consistency.Prefer sdkerrors/errorsmod to preserve ABCI codes and consistent handling.
- return nil, errors.New("payload hash already registered") + return nil, sdkerrors.ErrInvalidRequest.Wrap("payload hash already registered")
210-236: Cleanup loop should tolerate benign races; don’t abort on NotFound.If a payload disappears between index scan and removal, continue instead of returning an error.
- if count > ExpiredPayloadsLimit { + if count > ExpiredPayloadsLimit { return true, nil } @@ - err = k.RemovePendingPayload(ctx, &h) - if err != nil { - k.Logger().Error( - "failed to remove pending payload", - "hash", h.String(), - "error", err.Error(), - ) - - return true, err - } + err = k.RemovePendingPayload(ctx, &h) + if err != nil { + if errors.Is(err, sdkerrors.ErrNotFound) { + // already removed; continue scanning + return false, nil + } + k.Logger().Error("failed to remove pending payload", "hash", h.String(), "error", err.Error()) + return true, err + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
api/v1/query_grpc.pb.gois excluded by!**/*.pb.gotypes/core/orbiter.pb.gois excluded by!**/*.pb.gotypes/query.pb.gois excluded by!**/*.pb.gotypes/query.pb.gw.gois excluded by!**/*.pb.gw.gotypes/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (22)
Makefile(1 hunks)api/core/v1/orbiter.pulsar.go(5 hunks)api/v1/query.pulsar.go(7 hunks)api/v1/tx.pulsar.go(1 hunks)autocli.go(5 hunks)keeper/abci.go(1 hunks)keeper/abci_test.go(1 hunks)keeper/keeper.go(10 hunks)keeper/msg_server.go(1 hunks)keeper/msg_server_test.go(1 hunks)keeper/payload_handler.go(1 hunks)keeper/payload_handler_test.go(1 hunks)keeper/query_server.go(2 hunks)keeper/query_server_test.go(1 hunks)module.go(2 hunks)proto/generate.sh(1 hunks)proto/noble/orbiter/core/v1/orbiter.proto(1 hunks)proto/noble/orbiter/v1/query.proto(3 hunks)proto/noble/orbiter/v1/tx.proto(1 hunks)simapp/app.yaml(1 hunks)types/core/keys.go(1 hunks)types/core/payload_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- types/core/keys.go
- Makefile
- keeper/query_server.go
- proto/noble/orbiter/v1/tx.proto
- api/v1/tx.pulsar.go
- proto/noble/orbiter/core/v1/orbiter.proto
🧰 Additional context used
📓 Path-based instructions (3)
proto/**/*.proto
📄 CodeRabbit inference engine (CLAUDE.md)
Protobuf files must be formatted and linted with buf
Files:
proto/noble/orbiter/v1/query.proto
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*_test.go: Write unit tests using the testCases pattern
Name each test case as "success - " for success cases and "error - " for error cases; omit the prefix when no error is tested
For error-checking cases, include an expError string field in the test case and assert with require.ErrorContains(t, tC.expError, err)
Files:
keeper/abci_test.gokeeper/msg_server_test.gokeeper/payload_handler_test.gokeeper/query_server_test.gotypes/core/payload_test.go
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Go code must be formatted and pass golangci-lint
All Go files must include license headers
Files:
keeper/abci_test.gomodule.gokeeper/msg_server_test.gokeeper/payload_handler_test.gokeeper/query_server_test.goautocli.gokeeper/msg_server.goapi/core/v1/orbiter.pulsar.gokeeper/keeper.gokeeper/abci.goapi/v1/query.pulsar.gotypes/core/payload_test.gokeeper/payload_handler.go
🧠 Learnings (3)
📚 Learning: 2025-08-25T10:35:00.822Z
Learnt from: CR
PR: noble-assets/orbiter#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-25T10:35:00.822Z
Learning: Applies to **/*_test.go : Write unit tests using the testCases pattern
Applied to files:
keeper/msg_server_test.gokeeper/query_server_test.go
📚 Learning: 2025-08-18T07:26:16.709Z
Learnt from: 0xstepit
PR: noble-assets/orbiter#15
File: autocli.go:26-35
Timestamp: 2025-08-18T07:26:16.709Z
Learning: In the orbiter codebase, service descriptors follow different naming conventions depending on their package location: API packages (api/*/v1) use Msg_ServiceDesc/Query_ServiceDesc (uppercase 'S'), while types packages (types/component/*) use Msg_serviceDesc/Query_serviceDesc (lowercase 's'). The autocli.go correctly uses both conventions depending on which service descriptors it's referencing.
Applied to files:
autocli.go
📚 Learning: 2025-08-18T07:26:16.709Z
Learnt from: 0xstepit
PR: noble-assets/orbiter#15
File: autocli.go:26-35
Timestamp: 2025-08-18T07:26:16.709Z
Learning: In the orbiter codebase, the service descriptors for AutoCLI configuration should be accessed from the types/component/* packages using the lowercase identifiers (Msg_serviceDesc, Query_serviceDesc), not from the api/component/*/v1 packages using the capitalized identifiers (Msg_ServiceDesc, Query_ServiceDesc).
Applied to files:
autocli.go
🧬 Code graph analysis (12)
keeper/abci_test.go (6)
testutil/mocks/orbiter/orbiter.go (1)
OrbiterKeeper(35-43)keeper/msg_server.go (1)
NewMsgServer(41-43)types/marshal_and_unmarshal.go (1)
MarshalJSON(36-38)api/core/v1/orbiter.pulsar.go (6)
Payload(2719-2730)Payload(2745-2745)Payload(2748-2750)PendingPayload(2807-2818)PendingPayload(2833-2833)PendingPayload(2836-2838)api/v1/tx.pulsar.go (3)
MsgSubmitPayload(937-947)MsgSubmitPayload(962-962)MsgSubmitPayload(965-967)keeper/abci.go (1)
PendingPayloadLifespan(30-30)
keeper/msg_server_test.go (9)
testutil/random.go (1)
RandomBytes(28-36)api/core/v1/orbiter.pulsar.go (9)
Payload(2719-2730)Payload(2745-2745)Payload(2748-2750)Forwarding(2658-2674)Forwarding(2689-2689)Forwarding(2692-2694)PendingPayload(2807-2818)PendingPayload(2833-2833)PendingPayload(2836-2838)types/core/id.pb.go (2)
ACTION_FEE(36-36)PROTOCOL_CCTP(73-73)types/core/orbiter.go (1)
NewAction(40-50)types/controller/forwarding/cctp.go (1)
NewCCTPForwarding(73-85)testutil/mocks/orbiter/orbiter.go (1)
OrbiterKeeper(35-43)keeper/msg_server.go (1)
NewMsgServer(41-43)types/marshal_and_unmarshal.go (1)
MarshalJSON(36-38)testutil/accounts.go (1)
NewNobleAddress(45-47)
keeper/payload_handler_test.go (11)
types/tx.pb.go (4)
MsgServer(203-207)MsgSubmitPayload(35-41)MsgSubmitPayload(45-45)MsgSubmitPayload(46-48)types/core/payload.go (2)
PayloadHash(52-52)NewPayloadHash(55-68)types/marshal_and_unmarshal.go (1)
MarshalJSON(36-38)api/core/v1/orbiter.pulsar.go (6)
Payload(2719-2730)Payload(2745-2745)Payload(2748-2750)PendingPayload(2807-2818)PendingPayload(2833-2833)PendingPayload(2836-2838)api/v1/tx.pulsar.go (3)
MsgSubmitPayload(937-947)MsgSubmitPayload(962-962)MsgSubmitPayload(965-967)types/core/errors.go (1)
ErrNilPointer(28-28)testutil/mocks/orbiter/orbiter.go (1)
OrbiterKeeper(35-43)keeper/msg_server.go (1)
NewMsgServer(41-43)keeper/query_server.go (1)
NewQueryServer(42-44)api/v1/query.pulsar.go (3)
QueryPendingPayloadRequest(3013-3020)QueryPendingPayloadRequest(3035-3035)QueryPendingPayloadRequest(3038-3040)keeper/payload_handler.go (1)
ExpiredPayloadsLimit(43-43)
keeper/query_server_test.go (7)
types/tx.pb.go (4)
MsgServer(203-207)MsgSubmitPayload(35-41)MsgSubmitPayload(45-45)MsgSubmitPayload(46-48)api/core/v1/orbiter.pulsar.go (6)
PendingPayload(2807-2818)PendingPayload(2833-2833)PendingPayload(2836-2838)Payload(2719-2730)Payload(2745-2745)Payload(2748-2750)api/v1/query.pulsar.go (3)
QueryPendingPayloadRequest(3013-3020)QueryPendingPayloadRequest(3035-3035)QueryPendingPayloadRequest(3038-3040)types/marshal_and_unmarshal.go (1)
MarshalJSON(36-38)testutil/mocks/orbiter/orbiter.go (1)
OrbiterKeeper(35-43)keeper/msg_server.go (1)
NewMsgServer(41-43)keeper/query_server.go (1)
NewQueryServer(42-44)
autocli.go (4)
api/v1/tx_grpc.pb.go (1)
Msg_ServiceDesc(118-129)types/tx.pb.go (1)
Msg_serviceDesc(239-239)types/autocli.go (2)
TxCommandOptions(25-40)QueryCommandOptions(42-54)types/query.pb.go (1)
Query_serviceDesc(465-465)
keeper/msg_server.go (5)
api/v1/tx_grpc.pb.go (1)
MsgServer(59-64)types/tx.pb.go (7)
MsgServer(203-207)MsgSubmitPayload(35-41)MsgSubmitPayload(45-45)MsgSubmitPayload(46-48)MsgSubmitPayloadResponse(92-95)MsgSubmitPayloadResponse(99-99)MsgSubmitPayloadResponse(100-102)keeper/keeper.go (1)
Keeper(46-65)types/core/orbiter.pb.go (3)
Payload(128-135)Payload(139-139)Payload(140-142)types/marshal_and_unmarshal.go (1)
UnmarshalJSON(30-32)
api/core/v1/orbiter.pulsar.go (1)
types/core/orbiter.pb.go (15)
PendingPayload(234-241)PendingPayload(245-245)PendingPayload(246-248)Payload(128-135)Payload(139-139)Payload(140-142)Action(35-43)Action(47-47)Action(48-50)Forwarding(79-91)Forwarding(95-95)Forwarding(96-98)PayloadWrapper(187-191)PayloadWrapper(195-195)PayloadWrapper(196-198)
keeper/keeper.go (6)
types/component.go (1)
Authorizer(36-38)api/core/v1/orbiter.pulsar.go (3)
PendingPayload(2807-2818)PendingPayload(2833-2833)PendingPayload(2836-2838)types/core/orbiter.pb.go (3)
PendingPayload(234-241)PendingPayload(245-245)PendingPayload(246-248)types/core/keys.go (6)
HashesByTimeIndexPrefix(125-125)HashesByTimeIndexName(118-118)PendingPayloadsPrefix(122-122)PendingPayloadsName(115-115)PendingPayloadsSequencePrefix(123-123)PendingPayloadsSequenceName(116-116)types/expected_keepers.go (1)
BankKeeper(31-34)types/controller.go (3)
ForwardingController(30-33)ActionController(37-40)AdapterController(44-47)
keeper/abci.go (1)
keeper/keeper.go (1)
Keeper(46-65)
api/v1/query.pulsar.go (2)
types/query.pb.go (6)
QueryPendingPayloadRequest(193-196)QueryPendingPayloadRequest(200-200)QueryPendingPayloadRequest(201-203)QueryPendingPayloadResponse(238-241)QueryPendingPayloadResponse(245-245)QueryPendingPayloadResponse(246-248)api/core/v1/orbiter.pulsar.go (3)
Payload(2719-2730)Payload(2745-2745)Payload(2748-2750)
types/core/payload_test.go (5)
types/core/payload.go (1)
NewPayloadHash(55-68)types/core/orbiter.pb.go (6)
PendingPayload(234-241)PendingPayload(245-245)PendingPayload(246-248)Payload(128-135)Payload(139-139)Payload(140-142)types/controller/forwarding/cctp.go (1)
NewCCTPForwarding(73-85)testutil/random.go (1)
RandomBytes(28-36)types/core/orbiter.go (1)
NewPayload(250-264)
keeper/payload_handler.go (5)
keeper/keeper.go (1)
Keeper(46-65)api/core/v1/orbiter.pulsar.go (9)
Payload(2719-2730)Payload(2745-2745)Payload(2748-2750)PendingPayload(2807-2818)PendingPayload(2833-2833)PendingPayload(2836-2838)Forwarding(2658-2674)Forwarding(2689-2689)Forwarding(2692-2694)types/core/orbiter.pb.go (9)
Payload(128-135)Payload(139-139)Payload(140-142)PendingPayload(234-241)PendingPayload(245-245)PendingPayload(246-248)Forwarding(79-91)Forwarding(95-95)Forwarding(96-98)types/core/payload.go (1)
PayloadHash(52-52)types/core/errors.go (1)
ErrNilPointer(28-28)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: prepare
- GitHub Check: build
🔇 Additional comments (21)
simapp/app.yaml (1)
7-7: LGTM!Adding
orbiterto thebegin_blockerslist correctly wires the module into the block lifecycle, enabling BeginBlock cleanup of expired pending payloads.proto/generate.sh (1)
16-16: LGTM!Removing the entire
github.comdirectory is appropriate after copying the generated code to the project root. This ensures a cleaner working tree.autocli.go (3)
31-31: LGTM!The import alias change to
orbitertypesimproves clarity and avoids potential confusion with component-specific type packages.
41-70: LGTM!The Tx descriptor correctly wires the
SubmitPayloadRPC to skip at the top level and organizes it under a dedicatedpayloadsubcommand. Service descriptors follow the correct naming conventions for API (uppercaseMsg_ServiceDesc) and types (lowercaseMsg_serviceDesc) packages.Based on learnings
86-134: LGTM!The Query descriptor correctly wires the
PendingPayloadRPC to skip at the top level and organizes it under a dedicatedpayloadsubcommand. Service descriptors follow the correct naming conventions (uppercase for API, lowercase for types packages).Based on learnings
module.go (2)
57-57: LGTM!The compile-time assertion correctly enforces that
AppModuleimplementsappmodule.HasBeginBlocker.
156-158: LGTM!The
BeginBlockmethod correctly delegates to the keeper, enabling block lifecycle integration for pending payload cleanup.keeper/abci_test.go (1)
35-79: LGTM!The test correctly exercises the BeginBlock cleanup logic by verifying that pending payloads remain during their lifespan and are removed after expiration. The time-progression scenario is well structured.
keeper/abci.go (2)
30-30: LGTM!The 24-hour lifespan for pending payloads is a reasonable default that balances storage concerns with cross-chain transaction settlement times.
32-41: LGTM!The BeginBlock implementation correctly computes the expiration cutoff and delegates cleanup to
RemoveExpiredPayloads. The logic properly removes payloads older than 24 hours.types/core/payload_test.go (3)
33-43: LGTM!The round-trip test correctly verifies that payload hashing and hash parsing are inverse operations.
45-87: LGTM!The table-driven test correctly exercises valid and invalid hash parsing scenarios. Test case naming follows the required "success - " and "error - " prefix convention, and error assertions use
require.ErrorContainsas specified in the guidelines.As per coding guidelines
89-107: LGTM!The helper function correctly constructs a test
PendingPayloadand follows best practices by callingt.Helper().keeper/query_server_test.go (1)
39-132: LGTM!The table-driven test correctly exercises the
PendingPayloadquery across success and error scenarios. Test case naming follows the required "success - " and "error - " prefix convention, assertions properly compare payload contents (not wrapper messages), and error checking usesrequire.ErrorContainsas specified in the guidelines.As per coding guidelines
proto/noble/orbiter/v1/query.proto (1)
39-47: Request/response shape aligns with use-casesHex string input and returning core.v1.Payload are appropriate. No blockers.
api/core/v1/orbiter.pulsar.go (1)
2058-2071: Generated: PendingPayload wiring looks consistentDescriptors, fastReflection, rawDesc, and NumMessages are coherent. No manual changes needed.
Also applies to: 2810-2860, 2951-3056
keeper/payload_handler_test.go (2)
131-135: Use sdk.Context in setup signatureSetup receives an sdk.Context in this test environment.
- setup func(sdk.Context, codec.Codec, orbitertypes.MsgServer) ([]string, error) + setup func(sdk.Context, codec.Codec, orbitertypes.MsgServer) ([]string, error)(Adjust existing literal signatures accordingly where defined.)
Likely an incorrect or invalid review comment.
259-281: Review comment is incorrect:b.Loop()is valid and preferred in Go 1.24+The code uses the modern benchmark pattern introduced in Go 1.24. Since Go 1.24,
testing.B.Loopis the preferred method for benchmarking—simpler, safer against dead-code elimination and auto-manages timers, making the suggested replacement withfor i := 0; i < b.N; i++outdated.However, the timing scope suggestion has merit: setup operations (
SubmitPayload,NewPayloadHash) should not be timed. Consider moving the payload submission outside the loop and only timing theRemovePendingPayloadoperation.The
sdk.WrapSDKContextwrapping is unsupported by evidence in the codebase—no uses exist, and otherSubmitPayloadcalls passctxdirectly.Likely an incorrect or invalid review comment.
keeper/keeper.go (1)
60-65: LGTM: IndexedMap + time index wiring for pending payloads is sound.Schema, index key types, and initialization look correct and align with types/core keys. Nice separation via HashToTimeIndex.
If not needed outside this package, consider making HashToTimeIndex unexported to reduce surface area. Also verify golangci-lint passes for unused exports.
Also applies to: 67-84, 101-126
api/v1/query.pulsar.go (1)
2023-2068: Generated API types for PendingPayload query look correct.Request/response messages, fastReflection methods, and service wiring to v1.Payload are consistent. No manual edits needed here.
Please re-run proto generation in CI to ensure descriptors match the .proto sources: make proto (or your project’s generator target).
Also applies to: 2443-2699, 3013-3084, 3189-3216
keeper/payload_handler.go (1)
53-66: The review comment is based on incorrect assumptions about SHA256Hash and should be disregarded.The SHA256Hash() method explicitly states "To guarantee uniqueness the sequence number is included", meaning the hash computation includes the Sequence and Timestamp fields through protobuf marshaling, not just the Payload. Test code confirms that changing the Sequence produces different hashes—when Sequence is modified on the same payload, the resulting hash differs.
The proposed refactoring to compute the hash before allocating a sequence would be incorrect because:
- Creating a temporary PendingPayload with only the Payload field (Sequence=0) produces a hash that won't match stored payloads (which have actual Sequence values)
- Duplicate detection would fail: the computed temporary hash would never match the stored hash
- Each submission intentionally gets a unique sequence AND a unique hash by design
The current implementation is correct and requires no changes.
Likely an incorrect or invalid review comment.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
keeper/payload_handler.go (1)
172-173: Fix docstring to match implementation.The docstring states "If a payload is not found, it is a no-op but does not return an error" but the implementation actually returns an error at lines 187-189. Update the comment to accurately reflect the behavior.
Apply this diff:
-// RemovePendingPayload removes the pending payload from the module state. -// If a payload is not found, it is a no-op but does not return an error. +// RemovePendingPayload removes the pending payload from the module state. +// If a payload is not found, it returns an error.
🧹 Nitpick comments (2)
keeper/abci.go (1)
32-41: Consider simplifying the cutoff time calculation.The current implementation correctly calculates the cutoff time, but it can be simplified for better readability.
Apply this diff to simplify the calculation:
func (k *Keeper) BeginBlock(ctx context.Context) error { - blockTime := sdk.UnwrapSDKContext(ctx).BlockTime().UnixNano() - - cutoff := time.Unix( - 0, - blockTime-PendingPayloadLifespan.Nanoseconds(), - ) + blockTime := sdk.UnwrapSDKContext(ctx).BlockTime() + cutoff := blockTime.Add(-PendingPayloadLifespan) return k.RemoveExpiredPayloads(ctx, cutoff) }api/v1/query.pulsar.go (1)
1-3315: LGTM! Generated code appears correct.This is protobuf-generated code that properly implements the QueryPendingPayload request/response messages. The fast reflection scaffolding, marshaling logic, type registration, and Go struct definitions all follow standard protobuf patterns and are consistent with the existing messages in this file.
Note: The file is missing a license header per the coding guidelines. However, generated files are typically excluded from license header requirements. Consider verifying whether your project's license tooling exempts generated
.pulsar.gofiles.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
api/v1/query_grpc.pb.gois excluded by!**/*.pb.gotypes/core/orbiter.pb.gois excluded by!**/*.pb.gotypes/query.pb.gois excluded by!**/*.pb.gotypes/query.pb.gw.gois excluded by!**/*.pb.gw.gotypes/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (22)
Makefile(1 hunks)api/core/v1/orbiter.pulsar.go(5 hunks)api/v1/query.pulsar.go(7 hunks)api/v1/tx.pulsar.go(1 hunks)autocli.go(5 hunks)keeper/abci.go(1 hunks)keeper/abci_test.go(1 hunks)keeper/keeper.go(10 hunks)keeper/msg_server.go(1 hunks)keeper/msg_server_test.go(1 hunks)keeper/payload_handler.go(1 hunks)keeper/payload_handler_test.go(1 hunks)keeper/query_server.go(2 hunks)keeper/query_server_test.go(1 hunks)module.go(2 hunks)proto/generate.sh(1 hunks)proto/noble/orbiter/core/v1/orbiter.proto(1 hunks)proto/noble/orbiter/v1/query.proto(3 hunks)proto/noble/orbiter/v1/tx.proto(1 hunks)simapp/app.yaml(1 hunks)types/core/keys.go(1 hunks)types/core/payload_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- types/core/payload_test.go
- keeper/msg_server_test.go
- keeper/msg_server.go
- api/v1/tx.pulsar.go
- keeper/query_server_test.go
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Go code must be formatted and pass golangci-lint
All Go files must include license headers
Files:
module.gokeeper/abci.goapi/core/v1/orbiter.pulsar.gokeeper/payload_handler.goautocli.gotypes/core/keys.goapi/v1/query.pulsar.gokeeper/keeper.gokeeper/abci_test.gokeeper/payload_handler_test.gokeeper/query_server.go
proto/**/*.proto
📄 CodeRabbit inference engine (CLAUDE.md)
Protobuf files must be formatted and linted with buf
Files:
proto/noble/orbiter/core/v1/orbiter.protoproto/noble/orbiter/v1/tx.protoproto/noble/orbiter/v1/query.proto
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*_test.go: Write unit tests using the testCases pattern
Name each test case as "success - " for success cases and "error - " for error cases; omit the prefix when no error is tested
For error-checking cases, include an expError string field in the test case and assert with require.ErrorContains(t, tC.expError, err)
Files:
keeper/abci_test.gokeeper/payload_handler_test.go
🧠 Learnings (3)
📚 Learning: 2025-08-18T07:26:16.709Z
Learnt from: 0xstepit
PR: noble-assets/orbiter#15
File: autocli.go:26-35
Timestamp: 2025-08-18T07:26:16.709Z
Learning: In the orbiter codebase, service descriptors follow different naming conventions depending on their package location: API packages (api/*/v1) use Msg_ServiceDesc/Query_ServiceDesc (uppercase 'S'), while types packages (types/component/*) use Msg_serviceDesc/Query_serviceDesc (lowercase 's'). The autocli.go correctly uses both conventions depending on which service descriptors it's referencing.
Applied to files:
autocli.go
📚 Learning: 2025-08-18T07:26:16.709Z
Learnt from: 0xstepit
PR: noble-assets/orbiter#15
File: autocli.go:26-35
Timestamp: 2025-08-18T07:26:16.709Z
Learning: In the orbiter codebase, the service descriptors for AutoCLI configuration should be accessed from the types/component/* packages using the lowercase identifiers (Msg_serviceDesc, Query_serviceDesc), not from the api/component/*/v1 packages using the capitalized identifiers (Msg_ServiceDesc, Query_ServiceDesc).
Applied to files:
autocli.go
📚 Learning: 2025-08-25T10:35:00.822Z
Learnt from: CR
PR: noble-assets/orbiter#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-25T10:35:00.822Z
Learning: Applies to **/*.go : Go code must be formatted and pass golangci-lint
Applied to files:
Makefile
🧬 Code graph analysis (9)
keeper/abci.go (1)
keeper/keeper.go (1)
Keeper(46-65)
api/core/v1/orbiter.pulsar.go (1)
types/core/orbiter.pb.go (12)
PendingPayload(234-241)PendingPayload(245-245)PendingPayload(246-248)Payload(128-135)Payload(139-139)Payload(140-142)Action(35-43)Action(47-47)Action(48-50)Forwarding(79-91)Forwarding(95-95)Forwarding(96-98)
keeper/payload_handler.go (3)
keeper/keeper.go (1)
Keeper(46-65)types/core/payload.go (1)
PayloadHash(52-52)types/core/errors.go (1)
ErrNilPointer(28-28)
autocli.go (2)
api/v1/tx_grpc.pb.go (1)
Msg_ServiceDesc(118-129)types/autocli.go (2)
TxCommandOptions(25-40)QueryCommandOptions(42-54)
api/v1/query.pulsar.go (3)
types/query.pb.go (6)
QueryPendingPayloadRequest(193-196)QueryPendingPayloadRequest(200-200)QueryPendingPayloadRequest(201-203)QueryPendingPayloadResponse(238-241)QueryPendingPayloadResponse(245-245)QueryPendingPayloadResponse(246-248)api/core/v1/orbiter.pulsar.go (3)
Payload(2719-2730)Payload(2745-2745)Payload(2748-2750)types/core/orbiter.pb.go (3)
Payload(128-135)Payload(139-139)Payload(140-142)
keeper/keeper.go (6)
types/component.go (1)
Authorizer(36-38)api/core/v1/orbiter.pulsar.go (3)
PendingPayload(2807-2818)PendingPayload(2833-2833)PendingPayload(2836-2838)types/core/orbiter.pb.go (3)
PendingPayload(234-241)PendingPayload(245-245)PendingPayload(246-248)types/core/keys.go (6)
HashesByTimeIndexPrefix(125-125)HashesByTimeIndexName(118-118)PendingPayloadsPrefix(122-122)PendingPayloadsName(115-115)PendingPayloadsSequencePrefix(123-123)PendingPayloadsSequenceName(116-116)types/expected_keepers.go (1)
BankKeeper(31-34)types/controller.go (3)
ForwardingController(30-33)ActionController(37-40)AdapterController(44-47)
keeper/abci_test.go (6)
testutil/mocks/orbiter/orbiter.go (1)
OrbiterKeeper(35-43)keeper/msg_server.go (1)
NewMsgServer(41-43)types/marshal_and_unmarshal.go (1)
MarshalJSON(36-38)api/core/v1/orbiter.pulsar.go (6)
Payload(2719-2730)Payload(2745-2745)Payload(2748-2750)PendingPayload(2807-2818)PendingPayload(2833-2833)PendingPayload(2836-2838)api/v1/tx.pulsar.go (3)
MsgSubmitPayload(937-947)MsgSubmitPayload(962-962)MsgSubmitPayload(965-967)keeper/abci.go (1)
PendingPayloadLifespan(30-30)
keeper/payload_handler_test.go (10)
types/tx.pb.go (4)
MsgServer(203-207)MsgSubmitPayload(35-41)MsgSubmitPayload(45-45)MsgSubmitPayload(46-48)types/core/payload.go (2)
PayloadHash(52-52)NewPayloadHash(55-68)types/marshal_and_unmarshal.go (1)
MarshalJSON(36-38)api/core/v1/orbiter.pulsar.go (6)
Payload(2719-2730)Payload(2745-2745)Payload(2748-2750)PendingPayload(2807-2818)PendingPayload(2833-2833)PendingPayload(2836-2838)types/core/errors.go (1)
ErrNilPointer(28-28)testutil/mocks/orbiter/orbiter.go (1)
OrbiterKeeper(35-43)keeper/msg_server.go (1)
NewMsgServer(41-43)keeper/query_server.go (1)
NewQueryServer(42-44)api/v1/query.pulsar.go (3)
QueryPendingPayloadRequest(3013-3020)QueryPendingPayloadRequest(3035-3035)QueryPendingPayloadRequest(3038-3040)keeper/payload_handler.go (1)
ExpiredPayloadsLimit(43-43)
keeper/query_server.go (3)
api/core/v1/orbiter.pulsar.go (6)
PendingPayload(2807-2818)PendingPayload(2833-2833)PendingPayload(2836-2838)Payload(2719-2730)Payload(2745-2745)Payload(2748-2750)api/v1/query.pulsar.go (6)
QueryPendingPayloadRequest(3013-3020)QueryPendingPayloadRequest(3035-3035)QueryPendingPayloadRequest(3038-3040)QueryPendingPayloadResponse(3049-3056)QueryPendingPayloadResponse(3071-3071)QueryPendingPayloadResponse(3074-3076)types/core/payload.go (1)
NewPayloadHash(55-68)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test (TestIBCToCCTP)
- GitHub Check: test (TestIBCToHyperlane)
🔇 Additional comments (23)
simapp/app.yaml (1)
7-7: Verify the begin_blockers ordering is correct.The addition of
orbiterto the begin_blockers list looks good and enables the new BeginBlock lifecycle introduced in this PR. However, ensure that placingorbiteraftercapability,staking, andibcis the intended order, especially if the orbiter module depends on state or initialization from these modules.Makefile (1)
85-89: LGTM!The new
lint-fixtarget provides a convenient way to automatically fix linting issues using golangci-lint's--fixflag. The implementation follows the existing pattern and properly uses the same configuration file.types/core/keys.go (1)
110-126: LGTM!The new Keeper storage keys and prefixes for pending payload management are well-defined. The prefix numbers (50, 51, 52) are properly sequenced after the existing AdapterParamsPrefix (40), and the naming convention is consistent with the existing codebase patterns.
keeper/abci.go (1)
30-30: LGTM!The 24-hour lifespan for pending payloads is reasonable and provides a good balance between allowing time for payload processing and preventing indefinite storage accumulation.
module.go (2)
57-57: LGTM!The interface assertion properly verifies at compile-time that
AppModuleimplements theHasBeginBlockerinterface, following the existing pattern in this file.
156-158: LGTM!The BeginBlock implementation correctly delegates to the keeper's BeginBlock method and follows the standard pattern for module lifecycle hooks.
proto/noble/orbiter/core/v1/orbiter.proto (1)
78-86: LGTM!The
PendingPayloadmessage is well-structured with clear, descriptive comments for each field. The field types are appropriate for their purposes:
sequenceas uint64 for unique identificationpayloadreferencing the existing Payload typetimestampas int64 with explicit nanosecond precision documentationkeeper/abci_test.go (1)
35-79: LGTM with excellent lifecycle coverage!The test thoroughly validates the BeginBlock functionality across the complete pending payload lifecycle:
- Submission and hash verification
- Query and retrieval
- Time advancement within lifespan (payload persists)
- Expiration after lifespan (payload removed)
The test structure is clear, uses proper assertions with descriptive messages, and follows the testify patterns as per coding guidelines.
proto/generate.sh (1)
16-16: No issues found—the broader directory removal is safe.The proto files all declare
option go_package = "github.com/noble-assets/orbiter/...", and buf generates code only under that single namespace. External dependencies (cosmos, google, gogoproto) are resolved by buf's built-in types and are not written to the github.com directory. After the generated files are copied out (line 11), the entire github.com directory contains only temporary artifacts that should be removed. Removing the entire directory is actually safer and cleaner than targeting a specific nested path, as it prevents any leftover fragments.proto/noble/orbiter/v1/tx.proto (1)
1-39: LGTM! Proto definitions are well-structured.The RPC and message definitions follow protobuf conventions, include proper annotations, and incorporate feedback from previous reviews.
keeper/query_server.go (1)
88-109: LGTM! Query method properly implements nil checks and error handling.The implementation correctly guards against nil requests and uses appropriate gRPC status codes, addressing past review feedback.
autocli.go (2)
41-49: LGTM! Proper CLI command organization.Correctly skips the SubmitPayload RPC at the top level to place it under the
payloadsubcommand for better navigation.
66-70: LGTM! Payload subcommands properly configured.The new
payloadsubcommands for Tx and Query follow the established pattern and correctly reference service descriptors from the types package per project learnings.Based on learnings
Also applies to: 130-134
keeper/payload_handler_test.go (2)
41-122: LGTM! Tests follow coding guidelines.The test properly uses the testCases pattern with descriptive names ("success - ..." and "error - ...") and includes expError fields for error checking as specified in the coding guidelines.
As per coding guidelines
126-206: LGTM! Comprehensive coverage of expiration scenarios.The test covers multiple scenarios including partial expiration, no expiration, and empty state, providing good coverage of the bulk removal logic.
proto/noble/orbiter/v1/query.proto (1)
21-24: LGTM! Query proto definitions are well-structured.The PendingPayload RPC and message definitions follow protobuf conventions with appropriate HTTP annotations and field documentation.
Also applies to: 39-47
api/core/v1/orbiter.pulsar.go (1)
2058-2587: LGTM! Generated protobuf code.This is auto-generated code from protoc-gen-go-pulsar that provides the reflection and serialization infrastructure for the PendingPayload message. No manual changes needed.
Also applies to: 2806-2859
keeper/keeper.go (2)
60-84: LGTM! Well-designed storage with efficient indexing.The indexed map with timestamp-based secondary index enables efficient expiration queries. The HashToTimeIndex design properly leverages collections.Multi for time-based lookups needed by the expiration logic.
109-125: LGTM! Proper collections initialization.The initialization correctly wires up the indexed map and sequence using the schema builder, following cosmossdk.io/collections patterns.
keeper/payload_handler.go (4)
45-91: LGTM! Robust payload submission logic.The implementation properly:
- Generates unique sequence numbers
- Computes SHA256 hashes
- Prevents duplicate hash registration
- Wraps errors with context
93-141: LGTM! Comprehensive state validation.The validation properly checks:
- Action pause status for all pre-actions
- Protocol pause status
- Cross-chain pause status
The enum formatting issue mentioned in past reviews has been correctly addressed with
.String()calls.
143-170: LGTM! Clean payload retrieval logic.The method properly validates nil input and returns appropriate NotFound errors when payloads are missing.
200-239: LGTM! Efficient expiration with DoS protection.The implementation leverages the time-based index for efficient queries and limits the number of removals per block to prevent spam attacks from degrading block processing performance.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
keeper/payload_handler.go (1)
172-174: Update docstring to match actual behavior.The docstring states "If a payload is not found, it is a no-op but does not return an error," but Line 188 returns an error when the payload is not found.
Apply this diff to correct the docstring:
// RemovePendingPayload removes the pending payload from the module state. -// If a payload is not found, it is a no-op but does not return an error. +// If a payload is not found, it returns an error.
🧹 Nitpick comments (12)
autocli.go (1)
31-31: CLI wiring/naming aligns with conventions; good split underpayload.Aliasing and descriptor picks look consistent with prior patterns; skipping at top-level to nest under
payloadimproves UX. Consider updating CLI docs/examples accordingly.Based on learnings
Also applies to: 41-49, 66-70, 86-91, 95-95, 130-134
keeper/keeper.go (1)
67-84: Time index design: confirm scan order for expiry.
HashToTimeIndexindexes byint64timestamp. Ensure the BeginBlock cleanup iterates oldest→newest and short-circuits oncetimestamp > nowto keep work bounded.keeper/abci.go (2)
32-41: Simplify cutoff computation using time.Add.Cleaner, less error‑prone than manual UnixNano math.
Apply:
-func (k *Keeper) BeginBlock(ctx context.Context) error { - blockTime := sdk.UnwrapSDKContext(ctx).BlockTime().UnixNano() - - cutoff := time.Unix( - 0, - blockTime-PendingPayloadLifespan.Nanoseconds(), - ) - return k.RemoveExpiredPayloads(ctx, cutoff) +func (k *Keeper) BeginBlock(ctx context.Context) error { + now := sdk.UnwrapSDKContext(ctx).BlockTime() + cutoff := now.Add(-PendingPayloadLifespan) + return k.RemoveExpiredPayloads(ctx, cutoff) }
30-30: Consider making lifespan configurable.Expose PendingPayloadLifespan via params or app config to avoid hardcoding 24h.
If params are already present elsewhere, align with that mechanism for consistency.
types/core/payload_test.go (2)
33-43: Enable parallelism for this test, too.Add t.Parallel() at the start for symmetry with other tests.
func TestPayloadRoundTrip(t *testing.T) { + t.Parallel() pp := createPendingPayload(t)
53-87: Add a couple of edge cases for hash parsing.
- Uppercase hex should parse identically.
- Too-long hex should error.
Example additions:
@@ testcases := []struct { @@ { name: "error - too short hash", input: "0123ab", expError: "malformed payload hash", }, + { + name: "success - uppercase hex", + input: strings.ToUpper(hash.String()), + }, + { + name: "error - too long hash", + input: hash.String() + "00", + expError: "malformed payload hash", + }, }Remember to import strings.
proto/noble/orbiter/core/v1/orbiter.proto (1)
78-86: Consider google.protobuf.Timestamp for timestamp.Using Timestamp improves clarity/interop over raw int64 nanos; JSON encoding also becomes standard.
If you keep int64, document overflow limits and ensure all callers treat it as UTC nanos.
keeper/abci_test.go (1)
72-79: Prefer asserting a typed not-found error if exposed.If the query returns a sentinel/type (e.g., sdkerrors.ErrNotFound), assert on that to avoid brittle string matches.
If no sentinel exists, keep ErrorContains as-is.
keeper/query_server_test.go (1)
119-126: Add NotNil guards before dereferencing response.Prevent accidental panics; improves failure messages.
- require.NoError(t, err, "failed to get pending payload") - require.Equal( + require.NoError(t, err, "failed to get pending payload") + require.NotNil(t, got) + require.NotNil(t, got.Payload) + require.Equal( t, tc.expPayload.Payload.String(), got.Payload.String(), "expected different payload", )proto/noble/orbiter/v1/query.proto (1)
21-24: API shape: ensure omission of sequence/timestamp is intentional.Returning only Payload drops useful metadata for ops/debug. If acceptable, ignore; otherwise consider adding
uint64 sequenceandint64 timestampto the response.keeper/msg_server_test.go (2)
119-121: Fix assertion message and err assignment style.Use short var decl for local
errand correct message text.- err = k.Forwarder().Pause(ctx, core.PROTOCOL_CCTP, []string{cID}) - require.NoError(t, err, "failed to unpause cross-chain forwarding") + err := k.Forwarder().Pause(ctx, core.PROTOCOL_CCTP, []string{cID}) + require.NoError(t, err, "failed to pause cross-chain forwarding")
155-161: Confirm Signer omission is intentional in table tests.Direct MsgServer call may ignore Signer, but adding it keeps parity with other tests and future-proofs API changes.
- res, err := ms.SubmitPayload(ctx, &orbitertypes.MsgSubmitPayload{ - Payload: string(payloadJSON), - }) + res, err := ms.SubmitPayload(ctx, &orbitertypes.MsgSubmitPayload{ + Signer: testutil.NewNobleAddress(), + Payload: string(payloadJSON), + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (5)
api/v1/query_grpc.pb.gois excluded by!**/*.pb.gotypes/core/orbiter.pb.gois excluded by!**/*.pb.gotypes/query.pb.gois excluded by!**/*.pb.gotypes/query.pb.gw.gois excluded by!**/*.pb.gw.gotypes/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (22)
Makefile(1 hunks)api/core/v1/orbiter.pulsar.go(5 hunks)api/v1/query.pulsar.go(7 hunks)api/v1/tx.pulsar.go(1 hunks)autocli.go(5 hunks)keeper/abci.go(1 hunks)keeper/abci_test.go(1 hunks)keeper/keeper.go(10 hunks)keeper/msg_server.go(1 hunks)keeper/msg_server_test.go(1 hunks)keeper/payload_handler.go(1 hunks)keeper/payload_handler_test.go(1 hunks)keeper/query_server.go(2 hunks)keeper/query_server_test.go(1 hunks)module.go(2 hunks)proto/generate.sh(1 hunks)proto/noble/orbiter/core/v1/orbiter.proto(1 hunks)proto/noble/orbiter/v1/query.proto(3 hunks)proto/noble/orbiter/v1/tx.proto(1 hunks)simapp/app.yaml(1 hunks)types/core/keys.go(1 hunks)types/core/payload_test.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- keeper/query_server.go
- keeper/payload_handler_test.go
- Makefile
- proto/noble/orbiter/v1/tx.proto
🧰 Additional context used
📓 Path-based instructions (3)
proto/**/*.proto
📄 CodeRabbit inference engine (CLAUDE.md)
Protobuf files must be formatted and linted with buf
Files:
proto/noble/orbiter/v1/query.protoproto/noble/orbiter/core/v1/orbiter.proto
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*_test.go: Write unit tests using the testCases pattern
Name each test case as "success - " for success cases and "error - " for error cases; omit the prefix when no error is tested
For error-checking cases, include an expError string field in the test case and assert with require.ErrorContains(t, tC.expError, err)
Files:
types/core/payload_test.gokeeper/query_server_test.gokeeper/msg_server_test.gokeeper/abci_test.go
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Go code must be formatted and pass golangci-lint
All Go files must include license headers
Files:
types/core/payload_test.gokeeper/abci.gokeeper/keeper.goautocli.gokeeper/payload_handler.gokeeper/msg_server.gotypes/core/keys.gomodule.gokeeper/query_server_test.gokeeper/msg_server_test.goapi/core/v1/orbiter.pulsar.goapi/v1/query.pulsar.gokeeper/abci_test.goapi/v1/tx.pulsar.go
🧠 Learnings (2)
📚 Learning: 2025-08-18T07:26:16.709Z
Learnt from: 0xstepit
PR: noble-assets/orbiter#15
File: autocli.go:26-35
Timestamp: 2025-08-18T07:26:16.709Z
Learning: In the orbiter codebase, service descriptors follow different naming conventions depending on their package location: API packages (api/*/v1) use Msg_ServiceDesc/Query_ServiceDesc (uppercase 'S'), while types packages (types/component/*) use Msg_serviceDesc/Query_serviceDesc (lowercase 's'). The autocli.go correctly uses both conventions depending on which service descriptors it's referencing.
Applied to files:
autocli.go
📚 Learning: 2025-08-25T10:35:00.822Z
Learnt from: CR
PR: noble-assets/orbiter#0
File: CLAUDE.md:0-0
Timestamp: 2025-08-25T10:35:00.822Z
Learning: Applies to **/*_test.go : Write unit tests using the testCases pattern
Applied to files:
keeper/query_server_test.gokeeper/msg_server_test.go
🧬 Code graph analysis (12)
types/core/payload_test.go (5)
types/core/payload.go (1)
NewPayloadHash(55-68)api/core/v1/orbiter.pulsar.go (6)
PendingPayload(2807-2818)PendingPayload(2833-2833)PendingPayload(2836-2838)Payload(2719-2730)Payload(2745-2745)Payload(2748-2750)types/controller/forwarding/cctp.go (1)
NewCCTPForwarding(73-85)testutil/random.go (1)
RandomBytes(28-36)types/core/orbiter.go (1)
NewPayload(250-264)
keeper/abci.go (1)
keeper/keeper.go (1)
Keeper(46-65)
keeper/keeper.go (6)
types/component.go (1)
Authorizer(36-38)api/core/v1/orbiter.pulsar.go (3)
PendingPayload(2807-2818)PendingPayload(2833-2833)PendingPayload(2836-2838)types/core/orbiter.pb.go (3)
PendingPayload(234-241)PendingPayload(245-245)PendingPayload(246-248)types/core/keys.go (6)
HashesByTimeIndexPrefix(125-125)HashesByTimeIndexName(118-118)PendingPayloadsPrefix(122-122)PendingPayloadsName(115-115)PendingPayloadsSequencePrefix(123-123)PendingPayloadsSequenceName(116-116)types/expected_keepers.go (1)
BankKeeper(31-34)types/controller.go (3)
ForwardingController(30-33)ActionController(37-40)AdapterController(44-47)
autocli.go (4)
api/v1/tx_grpc.pb.go (1)
Msg_ServiceDesc(118-129)types/tx.pb.go (1)
Msg_serviceDesc(239-239)types/autocli.go (2)
TxCommandOptions(25-40)QueryCommandOptions(42-54)types/query.pb.go (1)
Query_serviceDesc(465-465)
keeper/payload_handler.go (5)
keeper/keeper.go (1)
Keeper(46-65)api/core/v1/orbiter.pulsar.go (9)
Payload(2719-2730)Payload(2745-2745)Payload(2748-2750)PendingPayload(2807-2818)PendingPayload(2833-2833)PendingPayload(2836-2838)Forwarding(2658-2674)Forwarding(2689-2689)Forwarding(2692-2694)types/core/orbiter.pb.go (9)
Payload(128-135)Payload(139-139)Payload(140-142)PendingPayload(234-241)PendingPayload(245-245)PendingPayload(246-248)Forwarding(79-91)Forwarding(95-95)Forwarding(96-98)types/core/payload.go (1)
PayloadHash(52-52)types/core/errors.go (1)
ErrNilPointer(28-28)
keeper/msg_server.go (5)
api/v1/tx_grpc.pb.go (1)
MsgServer(59-64)types/tx.pb.go (7)
MsgServer(203-207)MsgSubmitPayload(35-41)MsgSubmitPayload(45-45)MsgSubmitPayload(46-48)MsgSubmitPayloadResponse(92-95)MsgSubmitPayloadResponse(99-99)MsgSubmitPayloadResponse(100-102)keeper/keeper.go (1)
Keeper(46-65)api/core/v1/orbiter.pulsar.go (3)
Payload(2719-2730)Payload(2745-2745)Payload(2748-2750)types/marshal_and_unmarshal.go (1)
UnmarshalJSON(30-32)
keeper/query_server_test.go (7)
types/tx.pb.go (4)
MsgServer(203-207)MsgSubmitPayload(35-41)MsgSubmitPayload(45-45)MsgSubmitPayload(46-48)api/core/v1/orbiter.pulsar.go (6)
PendingPayload(2807-2818)PendingPayload(2833-2833)PendingPayload(2836-2838)Payload(2719-2730)Payload(2745-2745)Payload(2748-2750)api/v1/query.pulsar.go (3)
QueryPendingPayloadRequest(3013-3020)QueryPendingPayloadRequest(3035-3035)QueryPendingPayloadRequest(3038-3040)types/marshal_and_unmarshal.go (1)
MarshalJSON(36-38)testutil/mocks/orbiter/orbiter.go (1)
OrbiterKeeper(35-43)keeper/msg_server.go (1)
NewMsgServer(41-43)keeper/query_server.go (1)
NewQueryServer(42-44)
keeper/msg_server_test.go (6)
testutil/random.go (1)
RandomBytes(28-36)keeper/keeper.go (1)
Keeper(46-65)types/controller/forwarding/cctp.go (1)
NewCCTPForwarding(73-85)testutil/mocks/orbiter/orbiter.go (1)
OrbiterKeeper(35-43)keeper/msg_server.go (1)
NewMsgServer(41-43)types/marshal_and_unmarshal.go (1)
MarshalJSON(36-38)
api/core/v1/orbiter.pulsar.go (1)
types/core/orbiter.pb.go (6)
PendingPayload(234-241)PendingPayload(245-245)PendingPayload(246-248)Payload(128-135)Payload(139-139)Payload(140-142)
api/v1/query.pulsar.go (1)
api/core/v1/orbiter.pulsar.go (3)
Payload(2719-2730)Payload(2745-2745)Payload(2748-2750)
keeper/abci_test.go (8)
testutil/mocks/orbiter/orbiter.go (1)
OrbiterKeeper(35-43)keeper/msg_server.go (1)
NewMsgServer(41-43)types/marshal_and_unmarshal.go (1)
MarshalJSON(36-38)api/core/v1/orbiter.pulsar.go (6)
Payload(2719-2730)Payload(2745-2745)Payload(2748-2750)PendingPayload(2807-2818)PendingPayload(2833-2833)PendingPayload(2836-2838)types/core/orbiter.pb.go (6)
Payload(128-135)Payload(139-139)Payload(140-142)PendingPayload(234-241)PendingPayload(245-245)PendingPayload(246-248)api/v1/tx.pulsar.go (3)
MsgSubmitPayload(937-947)MsgSubmitPayload(962-962)MsgSubmitPayload(965-967)types/tx.pb.go (3)
MsgSubmitPayload(35-41)MsgSubmitPayload(45-45)MsgSubmitPayload(46-48)keeper/abci.go (1)
PendingPayloadLifespan(30-30)
api/v1/tx.pulsar.go (1)
types/tx.pb.go (6)
MsgSubmitPayload(35-41)MsgSubmitPayload(45-45)MsgSubmitPayload(46-48)MsgSubmitPayloadResponse(92-95)MsgSubmitPayloadResponse(99-99)MsgSubmitPayloadResponse(100-102)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test (TestIBCToCCTP)
- GitHub Check: test (TestIBCToHyperlane)
🔇 Additional comments (20)
simapp/app.yaml (1)
7-7: Begin blocker wiring looks good; verify runtime cost.Adding
orbitertobegin_blockersaligns with the newBeginBlockhook. Ensure the pruning pass is idempotent and remains O(pruned) per block.module.go (1)
57-58: BeginBlock integration is correct; confirm keeper semantics.Interface assertion + delegation look right. Please ensure
keeper.BeginBlock(ctx)is:
- idempotent,
- bounded in work per block,
- returns errors instead of panicking.
Also applies to: 156-158
keeper/keeper.go (2)
95-96: BankKeeper type switch toorbitertypes.BankKeeperlooks correct.Signatures and wiring updated consistently in ctor, validation, and components setup.
Also applies to: 151-152, 281-282
223-263: Controller setters: pattern LGTM.Router mutation +
SetRouterper component is fine; errors are wrapped with context.api/core/v1/orbiter.pulsar.go (2)
2077-2587: PendingPayload fast-reflection and descriptor wiring look consistent.Message count bumped to 5, exporter added, depIdxs/goTypes updated correctly. No functional concerns in the generated block.
Please ensure protoc/protoc-gen-go(-pulsar) versions are pinned in CI so regenerated artifacts are stable across contributors.
Also applies to: 2951-3056
2058-2063: Review comment is incorrect—generated .pulsar.go files are intentionally excluded from SPDX header requirements.The repo's established practice shows all 28 .pulsar.go files lack SPDX headers, while all hand-written .go files include them (BUSL-1.1). Notably, the verification script provided in the review itself explicitly excludes .pulsar.go files with
-g '!**/*.pulsar.go', confirming these generated artifacts are not subject to the SPDX header guideline. The coding guideline applies to hand-written Go files, not machine-generated protobuf outputs.Likely an incorrect or invalid review comment.
keeper/abci_test.go (1)
35-56: Verify context type expectations for Msg/Query servers.Calls pass sdk.Context to server methods; if these expect context.Context, ensure wrappers (Wrap/Unwrap) are used consistently to avoid accidental mismatches.
Would you confirm the server interfaces accept sdk.Context in this codebase? If not, I can suggest a small adapter.
proto/noble/orbiter/core/v1/orbiter.proto (1)
1-11: Proto file already passes buf format/lint checks.Verification confirms that
proto/noble/orbiter/core/v1/orbiter.protopasses bothbuf formatandbuf lintwith no issues. The file is compliant with the coding guidelines requirement for protobuf files.types/core/keys.go (1)
114-126: ---Verified: prefixes 50–52 and map names are unique across the codebase.
The verification confirms no other component reuses these prefix IDs or map names. The changes are safe and consistent with the existing keying scheme.
proto/noble/orbiter/v1/query.proto (1)
39-47: Proto hygiene verified: buf lint and format both pass.The new
QueryPendingPayloadRequestandQueryPendingPayloadResponsemessage definitions comply with buf formatting and linting standards. No issues found.api/v1/tx.pulsar.go (1)
1-3: Review comment is incorrect—generated file is already excluded from linting.The file
api/v1/tx.pulsar.gois explicitly excluded from all linting checks via.golangci.yaml:exclusions: paths: - .*\.pb\.go - .*\.pulsar\.go - api/The file matches both the
.*\.pulsar\.goandapi/patterns, so it is not subject to header validation or any other linting rules. Additionally, no header linter is enabled in the configuration. The file is correct as-is with the standard "Code generated by protoc-gen-go-pulsar. DO NOT EDIT." header.Likely an incorrect or invalid review comment.
keeper/msg_server.go (3)
1-31: LGTM!License header is present and imports are appropriate.
33-43: LGTM!The msgServer structure and constructor are clean. The compile-time assertion ensures interface compliance.
45-81: LGTM!The SubmitPayload implementation is well-structured with appropriate validation layers and error handling. The validation flow (nil check → unmarshal → validate → state checks → submit) is logical, and gRPC status codes are used correctly.
keeper/payload_handler.go (5)
1-43: LGTM!License header is present, imports are appropriate, and the ExpiredPayloadsLimit constant is well-documented with a clear rationale for the chosen value.
45-91: LGTM!The submit method is well-implemented with proper sequencing, duplicate detection, and logging. The CONTRACT comment clearly states preconditions.
93-141: LGTM!The validatePayloadAgainstState method correctly checks pause states for actions, protocols, and cross-chains. Error messages are clear and contextual. Note that the enum types are correctly converted to strings via .String() calls (lines 108, 118, 137).
143-170: LGTM!The pendingPayload method has appropriate nil checks, error handling, and logging.
200-239: LGTM!The RemoveExpiredPayloads method is well-implemented with proper batch limiting, error handling, and logging. The use of ExpiredPayloadsLimit prevents performance degradation from spam attacks.
api/v1/query.pulsar.go (1)
8-8: Generated code — no review needed.This file contains protobuf-generated code for QueryPendingPayloadRequest and QueryPendingPayloadResponse types. The generated scaffolding follows standard protoc-gen-go-pulsar patterns.
Also applies to: 2023-3083
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
keeper/payload_handler_test.go (5)
109-114: Wrap sdk.Context for Query server call.
QueryServer.PendingPayloadexpectscontext.Context, butctxhere issdk.Context. Wrap it withsdk.WrapSDKContext(ctx).Apply this diff:
gotPayload, err := qs.PendingPayload( - ctx, + sdk.WrapSDKContext(ctx), &orbitertypes.QueryPendingPayloadRequest{ Hash: tc.hash.String(), }, )
227-229: Wrap sdk.Context for Msg server call in setup helper.
MsgServer.SubmitPayloadexpectscontext.Context, butctxissdk.Context. Wrap it withsdk.WrapSDKContext(ctx).Apply this diff:
-res, err := ms.SubmitPayload(ctx, &orbitertypes.MsgSubmitPayload{ +res, err := ms.SubmitPayload(sdk.WrapSDKContext(ctx), &orbitertypes.MsgSubmitPayload{ Payload: string(payloadBz), })
182-184: Wrap sdk.Context for Query server calls in "outdated" assertion.Same as above: wrap
ctxwithsdk.WrapSDKContext(ctx)when callingqs.PendingPayload.Apply this diff:
_, err = qs.PendingPayload( - ctx, + sdk.WrapSDKContext(ctx), &orbitertypes.QueryPendingPayloadRequest{Hash: hash}, )
196-198: Wrap sdk.Context for Query server calls in "active" assertion.Same issue in the "active" payloads verification loop.
Apply this diff:
_, err = qs.PendingPayload( - ctx, + sdk.WrapSDKContext(ctx), &orbitertypes.QueryPendingPayloadRequest{Hash: hash}, )
291-291: Replaceb.Loop()with standardb.Nloop.Same issue: use the standard
b.Npattern instead ofb.Loop().Apply this diff:
-for b.Loop() { +for i := 0; i < b.N; i++ { if _, err := setupPayloadsInState(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
keeper/payload_handler_test.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*_test.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*_test.go: Write unit tests using the testCases pattern
Name each test case as "success - " for success cases and "error - " for error cases; omit the prefix when no error is tested
For error-checking cases, include an expError string field in the test case and assert with require.ErrorContains(t, tC.expError, err)
Files:
keeper/payload_handler_test.go
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Go code must be formatted and pass golangci-lint
All Go files must include license headers
Files:
keeper/payload_handler_test.go
🧬 Code graph analysis (1)
keeper/payload_handler_test.go (10)
types/tx.pb.go (4)
MsgServer(203-207)MsgSubmitPayload(35-41)MsgSubmitPayload(45-45)MsgSubmitPayload(46-48)types/core/payload.go (2)
PayloadHash(52-52)NewPayloadHash(55-68)types/marshal_and_unmarshal.go (1)
MarshalJSON(36-38)types/core/orbiter.pb.go (6)
Payload(128-135)Payload(139-139)Payload(140-142)PendingPayload(234-241)PendingPayload(245-245)PendingPayload(246-248)types/core/errors.go (1)
ErrNilPointer(28-28)testutil/mocks/orbiter/orbiter.go (1)
OrbiterKeeper(35-43)keeper/msg_server.go (1)
NewMsgServer(41-43)keeper/query_server.go (1)
NewQueryServer(42-44)api/v1/query.pulsar.go (3)
QueryPendingPayloadRequest(3013-3020)QueryPendingPayloadRequest(3035-3035)QueryPendingPayloadRequest(3038-3040)keeper/payload_handler.go (1)
ExpiredPayloadsLimit(43-43)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: prepare
- GitHub Check: build
|
|
||
| testcases := []struct { | ||
| name string | ||
| setup func(*testing.T, context.Context, codec.Codec, orbitertypes.MsgServer) |
There was a problem hiding this comment.
Fix type mismatch in setup function signature.
The setup function declares context.Context as its second parameter, but it receives sdk.Context from mockorbiter.OrbiterKeeper(t) at line 101. Since the test uses sdk.Context throughout and passes it to keeper methods, the signature should match.
Apply this diff to fix the signature:
- setup func(*testing.T, context.Context, codec.Codec, orbitertypes.MsgServer)
+ setup func(*testing.T, sdk.Context, codec.Codec, orbitertypes.MsgServer)🤖 Prompt for AI Agents
In keeper/payload_handler_test.go around line 54, the setup function currently
declares its second parameter as context.Context but the test supplies an
sdk.Context from mockorbiter.OrbiterKeeper(t) (used at ~line 101) and passes
that sdk.Context to keeper methods; change the setup function signature to
accept sdk.Context as the second parameter (replace context.Context with
sdk.Context) so the types match and all calls compile.
|
|
||
| b.ResetTimer() | ||
|
|
||
| for b.Loop() { |
There was a problem hiding this comment.
Replace b.Loop() with standard b.N loop.
b.Loop() is a Go 1.24 feature that may not be available yet. Use the standard benchmark pattern with b.N.
Apply this diff:
-for b.Loop() {
+for i := 0; i < b.N; i++ {
res, err := ms.SubmitPayload(ctx, &orbitertypes.MsgSubmitPayload{📝 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.
| for b.Loop() { | |
| for i := 0; i < b.N; i++ { | |
| res, err := ms.SubmitPayload(ctx, &orbitertypes.MsgSubmitPayload{ |
🤖 Prompt for AI Agents
In keeper/payload_handler_test.go around line 260, the benchmark uses Go 1.24's
b.Loop(), which may be unavailable; replace the b.Loop() usage with the standard
benchmark pattern: iterate using for i := 0; i < b.N; i++ { ... } and move the
benchmarked logic into that loop so the test runs correctly across Go versions.
Ensure any per-iteration setup/teardown is done inside the loop and remove the
b.Loop() call.
|
Setting this to draft again (and potentially closing) with the recent switch to preferring to pass payload bytes directly as calldata. |
This PR adds the logic to handle incoming payloads that will be handled in a separate message.
To test, run the local image:
make build && ./local.sh -rThen, submit a pending payload for the Orbiter:
After successful submission we can query the payload pending by its submission hash.
Cleanup
In addition to the submission and queries of pending payloads, the corresponding logic to clean up pending payloads that may have gone stale is included. This was worked on in its own PR: #52.
Summary by CodeRabbit