fix: apply changes from external audit - #42
Conversation
WalkthroughAdds pagination to dispatcher and forwarder query surfaces and generated APIs; tightens protocol/action/counterparty ID parsing and validation; converts many error paths to gRPC/status or errorsmod; several keeper/controller methods now return errors; CI Nancy scan accepts secrets; some proto service declarations removed; tests and e2e helpers updated. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Client
participant Q as Dispatcher QueryServer
participant K as Dispatcher Keeper
participant S as Store (Collections)
Client->>Q: QueryDispatchedAmountsByProtocolID(req{protocol_id,pagination})
Q->>Q: core.NewProtocolIDFromString(req.ProtocolId)
alt invalid protocol_id
Q-->>Client: gRPC InvalidArgument
else valid
Q->>K: GetDispatchedAmountsBySourceProtocolID(ctx,pid,pageReq)
K->>S: query.CollectionPaginate(pairPrefix(pid), pageReq)
S-->>K: entries, pageRes
K-->>Q: entries, pageRes
Q-->>Client: response{amounts, pagination}
end
sequenceDiagram
autonumber
actor Client
participant QF as Forwarder QueryServer
participant F as Forwarder Keeper
participant S as Store (Collections)
Client->>QF: QueryPausedCrossChains(req{protocol_id,pagination})
QF->>QF: core.NewProtocolIDFromString(req.ProtocolId)
alt invalid protocol_id
QF-->>Client: gRPC InvalidArgument
else valid
QF->>F: GetPaginatedPausedCrossChains(ctx,pid,pageReq)
F->>S: query.CollectionPaginate(pairPrefix(pid), pageReq)
S-->>F: counterpartyIDs, pageRes
F-->>QF: ids, pageRes
QF-->>Client: response{counterparty_ids, pagination}
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
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 |
8cf02f9 to
433509d
Compare
MalteHerrmann
left a comment
MalteHerrmann
left a comment
There was a problem hiding this comment.
great work so far! I agree with your suggestions for WONTFIXs in the Notion doc so this looks good for me -- did leave some nits here and there 🙌
c2182cb to
cc08e8c
Compare
cc08e8c to
b1f02bf
Compare
|
Thanks for the amazing review @MalteHerrmann. I did some other changes to the code for reduce repetition, have symmetric responses between forwarder and executor, and improve query responses. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (5)
keeper/component/dispatcher/genesis_test.go (1)
73-135: Consider adding ID format validation tests.While the current test coverage is adequate, consider adding test cases that validate counterparty ID format requirements (e.g., IBC channels must use "channel-" prefix, handling of empty/malformed IDs). This would strengthen the test suite and document expected ID formats.
types/core/orbiter_test.go (1)
175-255: Good test coverage for Payload validation.The new test function comprehensively covers validation scenarios including nil payloads, repeated actions, invalid IDs, missing forwarding, and successful cases. The test structure follows the coding guidelines correctly (testCases pattern, naming conventions, error assertions with
require.ErrorContains).Consider making the "error - invalid ID" test case (lines 198-206) more specific. Currently it creates both an Action with
Id = 0(unsupported) and a Forwarding withProtocolId = 0(unsupported), making it ambiguous which validation path triggers the error. While the test is valid as-is, you could improve precision by testing invalid Action IDs and invalid Forwarding ProtocolIds separately:{ - name: "error - invalid ID", + name: "error - invalid action ID", payload: &core.Payload{ PreActions: []*core.Action{ {}, }, - Forwarding: &core.Forwarding{}, + Forwarding: &core.Forwarding{ + ProtocolId: core.PROTOCOL_IBC, + Attributes: &codectypes.Any{}, + }, }, expError: "ID is not supported", }, +{ + name: "error - invalid forwarding protocol ID", + payload: &core.Payload{ + PreActions: []*core.Action{}, + Forwarding: &core.Forwarding{}, + }, + expError: "ID is not supported", +},controller/adapter/ibc_test.go (1)
153-195: Consider validating parsed attribute values.The test case correctly validates that parsing succeeds with incomplete CCTP attributes (addressing the audit finding). However, it only checks the TypeUrl and doesn't verify that the
mint_recipientfield was actually parsed into the CCTPAttributes.Consider adding an assertion to verify the parsed content for more thorough validation:
require.NoError(t, err, "expected no error parsing the payload") if tc.expectIsOrbiter { require.NotNil(t, payload.Forwarding) require.Equal(t, tc.expectPayload.Forwarding.ProtocolId, payload.Forwarding.ProtocolId, "expected different id") require.Equal(t, tc.expectPayload.Forwarding.Attributes.TypeUrl, payload.Forwarding.Attributes.TypeUrl, "expected different forwarding attributes type url") // Optionally verify the parsed CCTPAttributes var attrs forwardingtypes.CCTPAttributes err = encCfg.Codec.Unmarshal(payload.Forwarding.Attributes.Value, &attrs) require.NoError(t, err) require.NotEmpty(t, attrs.MintRecipient, "expected mint_recipient to be parsed")keeper/component/adapter/adapter.go (1)
216-222: LGTM with suggestion to clarify comment.The error handling implements a safe fallback strategy where
GetParamsfailure results inmaxSize = 0, allowing only transfers without passthrough payloads. This is a reasonable defensive approach.Consider enhancing the comment to be more explicit:
- // If we obtain an error, we assume 0 allowed payload size so - // we can execute the transfer if no payload is specified. + // If we obtain an error retrieving params, we use the zero-value (maxSize = 0) + // which only allows transfers without passthrough payloads. This safe fallback + // prevents processing payloads when the configuration state is uncertain. params, err := a.GetParams(ctx) if err != nil { a.logger.Error("getting params returned an error", "err", err.Error()) }keeper/component/forwarder/state_test.go (1)
42-104: Pass the subtesttintopostChecksThe closures capture the outer
t, so failures get reported against the parent test and risk concurrency issues if subtests ever run in parallel. Thread the subtest handle through the callback instead.- testCases := []struct { + testCases := []struct { name string protocolID core.ProtocolID pagination *query.PageRequest expLen int - postChecks func(counterparties []string, pageResp *query.PageResponse) + postChecks func(t *testing.T, counterparties []string, pageResp *query.PageResponse) }{ @@ - postChecks: func(counterparties []string, pageResp *query.PageResponse) { + postChecks: func(t *testing.T, counterparties []string, pageResp *query.PageResponse) { @@ - postChecks: func(counterparties []string, pageResp *query.PageResponse) { + postChecks: func(t *testing.T, counterparties []string, pageResp *query.PageResponse) { @@ - if tC.postChecks != nil { - tC.postChecks(counterparties, pageResp) + if tC.postChecks != nil { + tC.postChecks(t, counterparties, pageResp)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (10)
e2e/go.sumis excluded by!**/*.sumgo.sumis excluded by!**/*.sumsimapp/go.sumis excluded by!**/*.sumtool/go.sumis excluded by!**/*.sumtypes/component/dispatcher/query.pb.gois excluded by!**/*.pb.gotypes/component/dispatcher/query.pb.gw.gois excluded by!**/*.pb.gw.gotypes/component/forwarder/query.pb.gois excluded by!**/*.pb.gotypes/component/forwarder/query.pb.gw.gois excluded by!**/*.pb.gw.gotypes/query.pb.gois excluded by!**/*.pb.gotypes/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (53)
.github/workflows/vuln_nancy.yaml(1 hunks)Makefile(1 hunks)api/component/dispatcher/v1/query.pulsar.go(53 hunks)api/component/forwarder/v1/query.pulsar.go(29 hunks)controller/action/fee.go(1 hunks)controller/action/fee_test.go(3 hunks)controller/adapter/ibc_test.go(4 hunks)depinject.go(3 hunks)e2e/go.mod(11 hunks)e2e/ibc_to_cctp_test.go(4 hunks)e2e/utils_queries.go(2 hunks)go.mod(1 hunks)keeper/component/adapter/adapter.go(1 hunks)keeper/component/adapter/genesis.go(1 hunks)keeper/component/adapter/genesis_test.go(2 hunks)keeper/component/adapter/query_server.go(2 hunks)keeper/component/adapter/state.go(1 hunks)keeper/component/dispatcher/dispatcher.go(0 hunks)keeper/component/dispatcher/genesis_test.go(2 hunks)keeper/component/dispatcher/pagination.go(1 hunks)keeper/component/dispatcher/query_server.go(7 hunks)keeper/component/dispatcher/state.go(3 hunks)keeper/component/dispatcher/state_test.go(8 hunks)keeper/component/dispatcher/stats.go(4 hunks)keeper/component/dispatcher/stats_test.go(9 hunks)keeper/component/executor/msg_server.go(3 hunks)keeper/component/executor/msg_server_test.go(2 hunks)keeper/component/executor/query_server.go(3 hunks)keeper/component/forwarder/forwarder.go(2 hunks)keeper/component/forwarder/genesis_test.go(1 hunks)keeper/component/forwarder/msg_server.go(6 hunks)keeper/component/forwarder/query_server.go(5 hunks)keeper/component/forwarder/state.go(2 hunks)keeper/component/forwarder/state_test.go(1 hunks)keeper/keeper.go(1 hunks)proto/noble/orbiter/component/dispatcher/v1/query.proto(3 hunks)proto/noble/orbiter/component/forwarder/v1/query.proto(2 hunks)proto/noble/orbiter/v1/query.proto(0 hunks)proto/noble/orbiter/v1/tx.proto(0 hunks)simapp/go.mod(2 hunks)tool/go.mod(8 hunks)types/codec.go(0 hunks)types/component/dispatcher/dispatcher.go(1 hunks)types/component/forwarder/genesis_test.go(1 hunks)types/controller/action/fee.go(3 hunks)types/controller/action/fee_test.go(2 hunks)types/controller/forwarding/cctp.go(2 hunks)types/controller/forwarding/internal.go(1 hunks)types/core/id.go(4 hunks)types/core/id_test.go(3 hunks)types/core/keys.go(1 hunks)types/core/orbiter.go(2 hunks)types/core/orbiter_test.go(2 hunks)
💤 Files with no reviewable changes (4)
- types/codec.go
- proto/noble/orbiter/v1/query.proto
- keeper/component/dispatcher/dispatcher.go
- proto/noble/orbiter/v1/tx.proto
🧰 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:
controller/action/fee_test.gokeeper/component/forwarder/genesis_test.gotypes/controller/action/fee_test.gokeeper/component/adapter/genesis_test.gokeeper/component/dispatcher/genesis_test.gotypes/core/id_test.gocontroller/adapter/ibc_test.gotypes/core/orbiter_test.gokeeper/component/executor/msg_server_test.gokeeper/component/dispatcher/stats_test.goe2e/ibc_to_cctp_test.gotypes/component/forwarder/genesis_test.gokeeper/component/dispatcher/state_test.gokeeper/component/forwarder/state_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:
controller/action/fee_test.gokeeper/component/forwarder/genesis_test.gotypes/component/dispatcher/dispatcher.gotypes/controller/action/fee.gokeeper/component/forwarder/msg_server.gotypes/controller/action/fee_test.gokeeper/keeper.gokeeper/component/adapter/state.gokeeper/component/adapter/genesis_test.gokeeper/component/dispatcher/genesis_test.gokeeper/component/adapter/genesis.gotypes/core/id_test.gocontroller/adapter/ibc_test.gokeeper/component/dispatcher/stats.gotypes/core/orbiter_test.gotypes/core/keys.gokeeper/component/adapter/query_server.gokeeper/component/dispatcher/state.gokeeper/component/forwarder/query_server.gotypes/core/orbiter.gokeeper/component/executor/msg_server_test.gokeeper/component/forwarder/forwarder.gotypes/controller/forwarding/cctp.gotypes/core/id.gokeeper/component/dispatcher/query_server.gocontroller/action/fee.gokeeper/component/executor/query_server.goe2e/utils_queries.gokeeper/component/forwarder/state.gokeeper/component/dispatcher/stats_test.gokeeper/component/dispatcher/pagination.goapi/component/dispatcher/v1/query.pulsar.godepinject.goe2e/ibc_to_cctp_test.gokeeper/component/adapter/adapter.gotypes/controller/forwarding/internal.gotypes/component/forwarder/genesis_test.gokeeper/component/executor/msg_server.gokeeper/component/dispatcher/state_test.gokeeper/component/forwarder/state_test.goapi/component/forwarder/v1/query.pulsar.go
proto/**/*.proto
📄 CodeRabbit inference engine (CLAUDE.md)
Protobuf files must be formatted and linted with buf
Files:
proto/noble/orbiter/component/forwarder/v1/query.protoproto/noble/orbiter/component/dispatcher/v1/query.proto
🧠 Learnings (5)
📚 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:
controller/adapter/ibc_test.go
📚 Learning: 2025-07-22T12:31:35.398Z
Learnt from: 0xstepit
PR: noble-assets/orbiter#1
File: types/interfaces/adapter.go:32-34
Timestamp: 2025-07-22T12:31:35.398Z
Learning: In the PayloadAdapter interface, the ParsePayload method returns (bool, *types.Payload, error) where the boolean indicates whether the payload is targeting the orbiter system, not success/failure. This allows distinguishing between "payload not for orbiter" (false, nil, nil) versus "payload for orbiter but parsing failed" (true/false, nil/payload, error).
Applied to files:
controller/adapter/ibc_test.go
📚 Learning: 2025-08-14T15:11:47.828Z
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().
Applied to files:
types/core/orbiter_test.gotypes/core/orbiter.go
📚 Learning: 2025-08-18T07:30:08.537Z
Learnt from: 0xstepit
PR: noble-assets/orbiter#15
File: keeper/component/forwarder/msg_server.go:94-102
Timestamp: 2025-08-18T07:30:08.537Z
Learning: In the orbiter codebase, when looking up protocol IDs in core.ProtocolID_value map, the default/zero value represents an unsupported protocol ID that is correctly handled by downstream validation logic, so explicit validation before the lookup is not necessary.
Applied to files:
types/core/id.go
📚 Learning: 2025-08-18T07:30:12.691Z
Learnt from: 0xstepit
PR: noble-assets/orbiter#15
File: keeper/component/forwarder/msg_server.go:112-120
Timestamp: 2025-08-18T07:30:12.691Z
Learning: The user prefers defensive validation when converting string protocol IDs to typed core.ProtocolID values in the orbiter forwarder msg_server.go. They want to check if the protocol_id exists in the ProtocolID_value map before using it, rather than assuming it's valid.
Applied to files:
keeper/component/dispatcher/query_server.go
🧬 Code graph analysis (31)
controller/action/fee_test.go (1)
types/controller/action/fee.go (1)
BPSNormalizer(35-35)
keeper/component/forwarder/genesis_test.go (1)
types/core/id.pb.go (2)
PROTOCOL_CCTP(73-73)PROTOCOL_HYPERLANE(75-75)
types/component/dispatcher/dispatcher.go (2)
api/component/dispatcher/v1/dispatcher.pulsar.go (3)
DispatchCountEntry(1846-1854)DispatchCountEntry(1869-1869)DispatchCountEntry(1872-1874)types/component/dispatcher/dispatcher.pb.go (3)
DispatchCountEntry(145-149)DispatchCountEntry(153-153)DispatchCountEntry(154-156)
keeper/component/forwarder/msg_server.go (4)
types/core/id.go (1)
NewProtocolIDFromString(80-91)types/core/errors.go (2)
ErrUnableToPause(33-33)ErrUnableToUnpause(34-34)types/component/forwarder/events.pb.go (6)
EventProtocolPaused(26-28)EventProtocolPaused(32-32)EventProtocolPaused(33-35)EventProtocolUnpaused(70-72)EventProtocolUnpaused(76-76)EventProtocolUnpaused(77-79)types/core/keys.go (1)
MaxTargetCounterparties(53-53)
types/controller/action/fee_test.go (3)
types/controller/action/fee.go (2)
BPSNormalizer(35-35)MaxFeeRecipients(38-38)testutil/codec.go (1)
SetSDKConfig(37-40)types/core/errors.go (1)
ErrNilPointer(28-28)
keeper/keeper.go (3)
testutil/mocks/controllers.go (1)
ForwardingController(33-35)types/controller.go (3)
ForwardingController(30-33)ActionController(37-40)AdapterController(44-47)types/router/router.go (1)
Router(44-47)
keeper/component/adapter/state.go (3)
keeper/component/adapter/adapter.go (1)
Adapter(45-54)api/component/adapter/v1/adapter.pulsar.go (3)
Params(434-442)Params(457-457)Params(460-462)types/component/adapter/adapter.pb.go (3)
Params(27-31)Params(35-35)Params(36-38)
keeper/component/adapter/genesis_test.go (2)
types/core/errors.go (1)
ErrNilPointer(28-28)testutil/mocks/adapter.go (1)
NewAdapterComponent(34-64)
keeper/component/dispatcher/genesis_test.go (3)
types/core/id.pb.go (2)
PROTOCOL_IBC(71-71)PROTOCOL_CCTP(73-73)api/component/dispatcher/v1/dispatcher.pulsar.go (3)
DispatchCountEntry(1846-1854)DispatchCountEntry(1869-1869)DispatchCountEntry(1872-1874)types/component/dispatcher/dispatcher.pb.go (3)
DispatchCountEntry(145-149)DispatchCountEntry(153-153)DispatchCountEntry(154-156)
keeper/component/adapter/genesis.go (4)
api/component/adapter/v1/genesis.pulsar.go (3)
GenesisState(465-471)GenesisState(486-486)GenesisState(489-491)types/component/adapter/genesis.pb.go (3)
GenesisState(27-29)GenesisState(33-33)GenesisState(34-36)api/component/adapter/v1/adapter.pulsar.go (3)
Params(434-442)Params(457-457)Params(460-462)types/component/adapter/adapter.pb.go (3)
Params(27-31)Params(35-35)Params(36-38)
types/core/id_test.go (2)
types/core/id.pb.go (4)
ProtocolID(64-64)ProtocolID(100-102)PROTOCOL_CCTP(73-73)PROTOCOL_IBC(71-71)types/core/id.go (2)
MaxCounterpartyIDLength(34-34)ValidateCounterpartyID(152-183)
controller/adapter/ibc_test.go (8)
types/core/attributes.go (1)
ForwardingAttributes(35-41)api/controller/forwarding/v1/cctp.pulsar.go (3)
CCTPAttributes(568-582)CCTPAttributes(597-597)CCTPAttributes(600-602)types/controller/forwarding/cctp.pb.go (3)
CCTPAttributes(29-39)CCTPAttributes(43-43)CCTPAttributes(44-46)testutil/payload.go (1)
CreateValidIBCPacketData(36-46)types/core/keys.go (1)
ModuleAddress(37-37)api/core/v1/orbiter.pulsar.go (6)
Payload(2188-2199)Payload(2214-2214)Payload(2217-2219)Forwarding(2127-2143)Forwarding(2158-2158)Forwarding(2161-2163)types/core/orbiter.pb.go (6)
Payload(128-135)Payload(139-139)Payload(140-142)Forwarding(79-91)Forwarding(95-95)Forwarding(96-98)types/core/id.pb.go (2)
PROTOCOL_CCTP(73-73)PROTOCOL_IBC(71-71)
keeper/component/dispatcher/stats.go (1)
keeper/component/dispatcher/dispatcher.go (1)
New(53-103)
types/core/orbiter_test.go (5)
testutil/testdata/testdata.pb.go (3)
TestActionAttr(75-77)TestActionAttr(81-81)TestActionAttr(82-84)types/core/orbiter.go (1)
NewAction(41-51)types/core/id.pb.go (2)
ACTION_FEE(36-36)PROTOCOL_IBC(71-71)api/core/v1/orbiter.pulsar.go (9)
Payload(2188-2199)Payload(2214-2214)Payload(2217-2219)Action(2078-2090)Action(2105-2105)Action(2108-2110)Forwarding(2127-2143)Forwarding(2158-2158)Forwarding(2161-2163)types/core/orbiter.pb.go (9)
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/component/dispatcher/state.go (3)
types/component/dispatcher/dispatcher.pb.go (9)
DispatchedAmountEntry(75-80)DispatchedAmountEntry(84-84)DispatchedAmountEntry(85-87)AmountDispatched(33-38)AmountDispatched(42-42)AmountDispatched(43-45)DispatchCountEntry(145-149)DispatchCountEntry(153-153)DispatchCountEntry(154-156)keeper/component/dispatcher/pagination.go (1)
WithCollectionPaginationQuadPrefix(28-35)keeper/component/dispatcher/dispatcher.go (1)
Dispatcher(41-50)
keeper/component/forwarder/query_server.go (4)
types/core/id.go (2)
NewProtocolIDFromString(80-91)NewCrossChainID(122-137)keeper/component/forwarder/forwarder.go (1)
Forwarder(47-57)api/component/forwarder/v1/query.pulsar.go (3)
QueryPausedCrossChainsResponse(3811-3820)QueryPausedCrossChainsResponse(3835-3835)QueryPausedCrossChainsResponse(3838-3840)types/component/forwarder/query.pb.go (3)
QueryPausedCrossChainsResponse(172-177)QueryPausedCrossChainsResponse(181-181)QueryPausedCrossChainsResponse(182-184)
keeper/component/forwarder/forwarder.go (1)
types/core/id.go (1)
ValidateCounterpartyID(152-183)
types/core/id.go (1)
types/core/id.pb.go (12)
ActionID(29-29)ActionID(57-59)ActionID_value(47-51)ACTION_UNSUPPORTED(34-34)ProtocolID(64-64)ProtocolID(100-102)ProtocolID_value(88-94)PROTOCOL_UNSUPPORTED(69-69)PROTOCOL_IBC(71-71)PROTOCOL_CCTP(73-73)PROTOCOL_HYPERLANE(75-75)PROTOCOL_INTERNAL(77-77)
keeper/component/dispatcher/query_server.go (3)
types/core/id.go (2)
NewProtocolIDFromString(80-91)NewCrossChainID(122-137)api/component/dispatcher/v1/query.pulsar.go (6)
QueryDispatchedCountsResponse(3574-3582)QueryDispatchedCountsResponse(3597-3597)QueryDispatchedCountsResponse(3600-3602)QueryDispatchedAmountsResponse(3738-3746)QueryDispatchedAmountsResponse(3761-3761)QueryDispatchedAmountsResponse(3764-3766)types/component/dispatcher/query.pb.go (6)
QueryDispatchedCountsResponse(163-167)QueryDispatchedCountsResponse(171-171)QueryDispatchedCountsResponse(172-174)QueryDispatchedAmountsResponse(358-362)QueryDispatchedAmountsResponse(366-366)QueryDispatchedAmountsResponse(367-369)
controller/action/fee.go (1)
types/controller/action/fee.go (1)
BPSNormalizer(35-35)
keeper/component/executor/query_server.go (1)
types/core/id.go (1)
NewActionIDFromString(43-54)
keeper/component/forwarder/state.go (2)
keeper/component/forwarder/forwarder.go (1)
Forwarder(47-57)types/component.go (1)
Forwarder(42-48)
keeper/component/dispatcher/stats_test.go (4)
keeper/component/dispatcher/dispatcher.go (1)
Dispatcher(41-50)types/core/id.pb.go (2)
PROTOCOL_IBC(71-71)PROTOCOL_CCTP(73-73)types/packet.go (2)
TransferAttributes(36-43)NewTransferAttributes(47-69)types/component/dispatcher/dispatcher.pb.go (3)
AmountDispatched(33-38)AmountDispatched(42-42)AmountDispatched(43-45)
api/component/dispatcher/v1/query.pulsar.go (2)
types/component/dispatcher/query.pb.go (12)
QueryDispatchedCountsByProtocolIDRequest(106-111)QueryDispatchedCountsByProtocolIDRequest(117-117)QueryDispatchedCountsByProtocolIDRequest(118-120)QueryDispatchedCountsResponse(163-167)QueryDispatchedCountsResponse(171-171)QueryDispatchedCountsResponse(172-174)QueryDispatchedAmountsByProtocolIDRequest(299-304)QueryDispatchedAmountsByProtocolIDRequest(312-312)QueryDispatchedAmountsByProtocolIDRequest(313-315)QueryDispatchedAmountsResponse(358-362)QueryDispatchedAmountsResponse(366-366)QueryDispatchedAmountsResponse(367-369)api/component/dispatcher/v1/dispatcher.pulsar.go (6)
DispatchCountEntry(1846-1854)DispatchCountEntry(1869-1869)DispatchCountEntry(1872-1874)DispatchedAmountEntry(1785-1794)DispatchedAmountEntry(1809-1809)DispatchedAmountEntry(1812-1814)
e2e/ibc_to_cctp_test.go (4)
e2e/utils_queries.go (1)
GetIBCTotalEscrow(121-140)e2e/setup.go (2)
IBC(59-65)Suite(67-87)e2e/utils.go (3)
OneE6(42-42)GetTxsResult(183-202)SearchEvents(206-229)types/core/keys.go (1)
ModuleAddress(37-37)
types/controller/forwarding/internal.go (3)
types/core/attributes.go (1)
ForwardingAttributes(35-41)api/controller/forwarding/v1/internal.pulsar.go (3)
InternalAttributes(452-459)InternalAttributes(474-474)InternalAttributes(477-479)types/controller/forwarding/internal.pb.go (3)
InternalAttributes(29-32)InternalAttributes(36-36)InternalAttributes(37-39)
types/component/forwarder/genesis_test.go (1)
types/core/id.pb.go (2)
PROTOCOL_IBC(71-71)PROTOCOL_CCTP(73-73)
keeper/component/executor/msg_server.go (2)
types/core/id.go (1)
NewActionIDFromString(43-54)types/core/errors.go (2)
ErrUnableToPause(33-33)ErrUnableToUnpause(34-34)
keeper/component/dispatcher/state_test.go (1)
types/core/id.pb.go (3)
PROTOCOL_IBC(71-71)PROTOCOL_HYPERLANE(75-75)PROTOCOL_CCTP(73-73)
keeper/component/forwarder/state_test.go (3)
testutil/mocks/forwarder.go (1)
NewForwarderComponent(33-52)keeper/component/forwarder/forwarder.go (1)
Forwarder(47-57)types/component.go (1)
Forwarder(42-48)
api/component/forwarder/v1/query.pulsar.go (1)
types/component/forwarder/query.pb.go (6)
QueryPausedCrossChainsRequest(117-122)QueryPausedCrossChainsRequest(126-126)QueryPausedCrossChainsRequest(127-129)QueryPausedCrossChainsResponse(172-177)QueryPausedCrossChainsResponse(181-181)QueryPausedCrossChainsResponse(182-184)
🪛 GitHub Actions: Scan Vulnerabilities
Makefile
[error] 96-96: nancy vulnerability scan detected 1 vulnerable package (github.com/mitchellh/mapstructure) CVE-2025-11065. Exit code 1 from 'make nancy'.
🪛 OSV Scanner (2.2.3)
e2e/go.mod
[HIGH] 19-19: github.com/cosmos/cosmos-sdk 0.50.13: The x/crisis package does not cause chain halt in github.com/cosmos/cosmos-sdk
(GO-2023-1821)
[HIGH] 19-19: github.com/cosmos/cosmos-sdk 0.50.13: The x/crisis package does not charge ConstantFee in github.com/cosmos/cosmos-sdk
(GO-2023-1881)
[HIGH] 19-19: github.com/cosmos/cosmos-sdk 0.50.13: Slashing evasion in github.com/cosmos/cosmos-sdk
(GO-2024-2584)
[HIGH] 19-19: github.com/cosmos/cosmos-sdk 0.50.13: Integer Overflow vulnerability in its Validator Rewards pool can cause a chain halt in github.com/cosmos/cosmos-sdk
(GO-2025-3803)
[HIGH] 19-19: github.com/cosmos/cosmos-sdk 0.50.13: Cosmos SDK's Integer Overflow vulnerability in its Validator Rewards pool can cause a chain halt
🔇 Additional comments (68)
types/component/forwarder/genesis_test.go (1)
46-47: LGTM! Test data now uses protocol-accurate identifiers.The updated CounterpartyId values align with actual protocol conventions: "channel-1" follows IBC's standard channel identifier format, and "2" represents a typical CCTP numeric domain identifier. This improves test realism without changing behavior.
keeper/component/dispatcher/genesis_test.go (4)
95-98: LGTM! IBC channel naming convention applied correctly.The updated counterparty IDs correctly use the "channel-" prefix for IBC protocol identifiers, which aligns with IBC standards. The CCTP protocol appropriately uses plain numeric identifiers.
102-105: LGTM! Consistent with dispatched amounts test data.The counterparty ID updates maintain consistency with the DispatchedAmounts entries, correctly applying protocol-specific naming conventions.
151-154: LGTM! Export test data aligns with import test.The DispatchedAmounts test data correctly mirrors the corresponding entries in TestInitGenesis, ensuring consistent round-trip testing.
157-160: LGTM! Export test data aligns with import test.The DispatchedCounts test data correctly mirrors the corresponding entries in TestInitGenesis, maintaining consistency for round-trip testing.
types/core/orbiter_test.go (1)
125-125: LGTM! Typo fix improves clarity.The correction from "an non nil" to "and non nil" makes the test case name grammatically correct and aligns with the feedback from previous reviews about expressive naming.
controller/adapter/ibc_test.go (2)
24-24: LGTM!The import changes are appropriate for the new test case requirements.
Also applies to: 34-34
207-207: LGTM!The enhanced assertion messages improve test clarity and will make debugging failures easier.
Also applies to: 211-220
types/component/dispatcher/dispatcher.go (1)
69-71: LGTM!The
IsPositive()helper method is well-implemented and follows the same pattern as the existingAmountDispatched.IsPositive()method, improving code consistency and readability.types/core/orbiter.go (1)
274-280: LGTM!The duplicate action ID detection is correctly implemented. The logic uses a map to efficiently track visited IDs and reports duplicates before proceeding to per-action validation, which is the right order of operations. This addresses the validation requirement mentioned in past review comments.
types/controller/forwarding/cctp.go (2)
30-32: LGTM!Relocating the
CCTPNobleDomainconstant from the core package to the forwarding package improves cohesion by keeping CCTP-specific configuration close to its usage.
62-62: LGTM!The validation correctly references the new local
CCTPNobleDomainconstant, maintaining the same validation logic while using the relocated constant.types/controller/forwarding/internal.go (2)
30-31: LGTM!Extracting the counterparty ID into a named constant improves maintainability by eliminating the magic string and making the value's purpose explicit.
36-36: LGTM!The method correctly returns the new
CounterpartyIDconstant, maintaining the same behavior while improving code clarity.types/controller/action/fee.go (3)
32-39: LGTM!The constants are well-defined and appropriately documented:
BPSNormalizer(10,000) provides the basis points denominator for fee calculationsMaxFeeRecipients(5) sets a reasonable upper bound on the number of fee recipientsRelocating these from the core package to the action package improves cohesion by keeping fee-related configuration with fee logic.
63-69: LGTM!The validation correctly enforces the maximum number of fee recipients using the new
MaxFeeRecipientsconstant, preventing potential abuse or performance issues from excessive recipient lists.
85-90: LGTM!The validation correctly uses the local
BPSNormalizerconstant and provides a clear error message. The logic ensures basis points are within the valid range (0 < BasisPoints ≤ 10,000).types/core/keys.go (1)
53-53: LGTM!The
MaxTargetCounterpartiesconstant (100) establishes a reasonable upper bound for the number of counterparty IDs that can be targeted in operations like pause/unpause, helping prevent resource exhaustion or abuse.go.mod (1)
183-183: Verify replace directive consistency and document fork rationale
Both go.mod files replace cosmossdk.io/collections with v0.0.0-20250923134424-edd65694c2f7; confirm this fork provides required functionality beyond upstream and record its rationale in the README or CONTRIBUTING guide.simapp/go.mod (1)
228-229: Verify necessity of fork and plan upstream contribution
Cannot automatically confirm that upstream SDK lacks “iterate raw” support. Please manually check and ensure:
- Upstream
cosmossdk.io/collectionstruly missing a raw-iteration API.- This replace is a temporary workaround; consider opening an upstream issue or PR.
- Pseudo-version
edd65694c2f7corresponds to a stable commit in the fork.keeper/component/forwarder/msg_server.go (2)
59-78: LGTM: Improved ID parsing and error handling.The refactor to use
core.NewProtocolIDFromStringprovides better validation and consistent error handling. The error messages are clear and properly wrapped.
122-127: LGTM: Essential validation added.The validation ensuring no more than
core.MaxTargetCounterpartiescounterparties can be paused in a single transaction prevents potential DoS vectors and excessive gas consumption.keeper/component/dispatcher/pagination.go (1)
28-35: LGTM: Clean pagination helper.The generic pagination helper follows the standard Cosmos SDK pattern and provides type-safe prefix filtering for Quad-key collections.
keeper/component/dispatcher/stats_test.go (2)
108-132: LGTM: Important overflow test added.The new test case properly validates that dispatched count overflow is detected and returns an error when attempting to increment beyond
math.MaxUint64. This prevents potential state corruption from integer overflow.
25-30: LGTM: Proper migration to sdkmath.The migration from an external math package to
cosmossdk.io/math(aliased assdkmath) aligns with Cosmos SDK best practices for deterministic on-chain arithmetic and overflow protection.Based on learnings.
Also applies to: 45-47
tool/go.mod (1)
1-221: No issues identified.The tooling dependency updates appear routine. These are dev-only dependencies with no runtime impact.
keeper/component/executor/msg_server.go (2)
55-76: LGTM: Consistent ID parsing pattern.The refactor to use
core.NewActionIDFromStringmirrors the pattern in the forwarder component, providing consistent validation and error handling across the codebase.
87-107: LGTM: Proper error handling.The error handling correctly uses
core.ErrUnableToUnpausefor the unpause operation, and error messages are clear and properly wrapped.types/core/id.go (3)
33-35: LGTM: Reasonable length constraint.The 32-character maximum for counterparty IDs provides a sensible upper bound while accommodating standard chain/channel identifiers.
43-54: LGTM: Improved ID construction with validation.The new
NewActionIDFromStringandNewProtocolIDFromStringconstructors provide clear error messages when IDs don't exist in the enum or aren't supported, improving debuggability over direct map lookups.Also applies to: 80-91
152-183: LGTM: Protocol-specific validation.The updated
ValidateCounterpartyIDproperly enforces protocol-specific constraints:
- IBC: validates channel ID format
- CCTP/Hyperlane: requires numeric IDs
- Internal: allows any format
- Unsupported: rejects
This prevents invalid IDs from being accepted based on protocol type.
types/core/id_test.go (1)
130-190: LGTM: Comprehensive validation tests.The new
TestValidateCounterpartyIDtest cases cover:
- Empty IDs
- Length limits
- Protocol-specific format requirements (numeric for CCTP, channel format for IBC)
- Default/unsupported protocol handling
This provides good coverage of the validation logic.
e2e/go.mod (3)
303-303: Document and verifycollectionsfork (e2e/go.mod:303)
No documentation exists for thecosmossdk.io/collectionsreplace directive. Confirm the fork’s compatibility with the current Cosmos SDK version and update project documentation (e.g., README or docs/) with the rationale for using this fork.
8-8: Confirm hyperlane-cosmos version
Integration is covered in e2e tests (utils_queries.go, setup.go, ibc_to_hyperlane_test.go); ensure v1.0.1 is the intended version.
1-300: Follow up: upgrade Cosmos SDK to address critical vulnerabilitiesStatic analysis flagged high severity issues in cosmos-sdk v0.50.13:
- x/crisis: GO-2023-1821, GO-2023-1881
- Slashing evasion: GO-2024-2584
- Validator rewards integer overflow: GO-2025-3803, GHSA-p22h-3m2v-cmgh
Track and remediate in a follow-up PR by upgrading to the latest patched v0.50.x release. Please verify the correct patched version and update accordingly.
keeper/component/executor/msg_server_test.go (1)
57-57: LGTM!The test expectations correctly reflect the new error message format from
core.NewActionIDFromString, which provides more precise error messages for invalid action IDs.Also applies to: 133-133
keeper/component/adapter/query_server.go (1)
53-56: LGTM!Proper error handling using gRPC status codes. The use of
codes.Internalis appropriate for server-side parameter retrieval failures.controller/action/fee.go (1)
243-243: LGTM!The constant reference has been correctly updated to use
actiontypes.BPSNormalizer, aligning with the refactoring that moves domain-specific constants to the action package.keeper/component/forwarder/genesis_test.go (1)
105-106: LGTM!The test data has been correctly updated to use the new cross-chain ID format with numeric counterparty identifiers, aligning with the pagination refactoring.
keeper/component/forwarder/query_server.go (2)
54-57: LGTM!Proper input validation using
core.NewProtocolIDFromStringwith appropriate gRPC status codes. The use ofcodes.InvalidArgumentfor validation failures is correct.Also applies to: 96-99
130-141: LGTM!Pagination support has been correctly integrated into the
PausedCrossChainsquery. The response now includes both the counterparty IDs and pagination metadata, following standard Cosmos SDK query patterns.types/controller/action/fee_test.go (2)
57-57: LGTM!The constant reference has been correctly updated to use
actiontypes.BPSNormalizer, consistent with the refactoring in other files.
100-159: LGTM!Excellent test coverage for
FeeAttributesvalidation. The test properly validates nil attributes, boundary cases, and the maximum recipients constraint usingactiontypes.MaxFeeRecipients.keeper/component/executor/query_server.go (4)
26-27: LGTM: gRPC status imports added.The addition of gRPC status and codes imports aligns with the migration from cosmossdk.io/errors to standard gRPC error handling in query servers.
54-56: Good: Defensive action ID validation.The switch from
core.NewActionID(core.ActionID_value[req.ActionId])tocore.NewActionIDFromString(req.ActionId)provides defensive validation that checks both existence and support before construction. The gRPC status codeInvalidArgumentis appropriate for invalid input.Based on learnings
59-62: Good: Appropriate error code for internal failures.Using
status.Error(codes.Internal, ...)for state retrieval failures correctly signals server-side issues to clients.
78-81: Good: Consistent gRPC error handling.The PausedActions query method correctly uses
codes.Internalfor internal state retrieval errors, maintaining consistency with the error handling pattern in IsActionPaused.keeper/component/adapter/genesis_test.go (2)
36-37: Good: Correct expectation for uninitialized state.Based on the past review discussion, expecting an error from
GetParamsbefore genesis initialization is correct behavior, as the underlying state has not been populated yet.
43-44: Good: Proper error handling for GetParams.All calls to
GetParamsnow correctly handle the two-value return signature, with appropriate error checks where needed.Also applies to: 59-64
proto/noble/orbiter/component/forwarder/v1/query.proto (2)
6-6: LGTM: Pagination import added.The cosmos base query pagination import is correctly added to support the new pagination fields in the query messages.
54-55: Good: Consistent pagination support.Pagination fields are correctly added to both the request (PageRequest) and response (PageResponse) messages for the PausedCrossChains query, following standard Cosmos SDK patterns.
Also applies to: 63-64
keeper/component/forwarder/forwarder.go (2)
138-150: Good: Inline validation improves clarity.The inline protocol and per-counterparty validation in
Pauseis clearer than the previousValidateCrossChainsapproach. Each counterparty ID is validated against the protocol ID usingcore.ValidateCounterpartyID, with appropriate error wrapping.
161-175: Good: Symmetric validation in Unpause.The validation logic in
Unpausemirrors that inPause, ensuring consistency across pause/unpause operations.e2e/ibc_to_cctp_test.go (4)
67-68: Good: Pre-test escrow verification.Adding the initial escrow check before running subtests establishes a baseline for escrow invariant verification.
70-76: Good: Improved test structure with descriptive names.The split into subtests with clear names (
FailingParsingWithoutForwardingandFailingAfterParsingInvalidForwarding) makes the test intent explicit and improves maintainability.
87-175: Good: Escrow invariant checks added.The test now properly captures initial and final escrow amounts (lines 103, 173-174) and verifies neutrality, ensuring the failing transaction doesn't leave orphaned funds.
177-274: Good: Comprehensive test for post-parsing errors.The new test function covers the scenario where forwarding parsing succeeds but validation fails in the CCTP handler. Key assertions include:
- Orbiter middleware error in the ACK (lines 244-249)
- Exactly one IBC tx recorded (line 253)
- No DepositForBurn CCTP event emitted (line 258)
- Counterparty balance restored (lines 266-271)
- Escrow invariant maintained (lines 272-273)
proto/noble/orbiter/component/dispatcher/v1/query.proto (2)
5-5: LGTM: Pagination import added.The cosmos base query pagination import is correctly added to support the new pagination fields in dispatcher query messages.
69-70: Good: Comprehensive pagination support.Pagination fields are consistently added to all by-protocol query messages (both counts and amounts), enabling efficient querying of large result sets. The pattern follows standard Cosmos SDK conventions.
Also applies to: 77-78, 100-101, 108-109
keeper/component/dispatcher/query_server.go (7)
26-27: LGTM: gRPC status imports added.The addition of gRPC status and codes imports supports the migration from cosmossdk.io/errors to standard gRPC error handling in query servers.
53-66: Good: Defensive protocol ID validation.Both source and destination protocol IDs use
NewProtocolIDFromStringfor defensive validation, with contextual error messages distinguishing between source and destination failures. The cross-chain ID construction error handling is also appropriate.Based on learnings
77-89: Good: Existence check with clear error code.The
IsPositive()check on the retrieved counts provides a clear existence test, and returningcodes.NotFoundwith a descriptive message when no data exists is the correct gRPC pattern. SettingPaginationtonilfor single-entry responses is appropriate.
100-117: Good: Paginated query implementation.The
DispatchedCountsByDestinationProtocolIDquery correctly:
- Validates the protocol ID using
NewProtocolIDFromString- Passes the pagination request to the state layer
- Returns both counts and pagination response
- Uses
codes.Internalfor state retrieval errors
128-141: Good: Symmetric implementation for source protocol queries.The
DispatchedCountsBySourceProtocolIDquery mirrors the destination query implementation, maintaining consistency across the API surface.
152-193: Good: Complete validation for amounts query.The
DispatchedAmountsquery includes all necessary validations:
- Empty denom check (line 152-154)
- Source and destination protocol ID validation
- Source and destination cross-chain ID validation
- Existence check using
IsPositive()- Contextual error messages
203-220: Good: Consistent pagination for amounts queries.Both
DispatchedAmountsByDestinationProtocolIDandDispatchedAmountsBySourceProtocolIDfollow the same pagination pattern as the counts queries, providing a uniform API experience.Also applies to: 231-248
keeper/keeper.go (2)
194-205: All callers handle SetActionControllers errors. Verified that the only invocation in depinject.go checks the returned error and panics on failure.
180-191: All callers handleSetForwardingControllerserror Verified the invocation indepinject.go:141checks the returned error.
MalteHerrmann
left a comment
MalteHerrmann
left a comment
There was a problem hiding this comment.
looks great @0xstepit; awesome work!! 💪
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
types/core/id.go (2)
43-54: Refine error message to avoid redundancy.The error message at line 50 includes "action ID" prefix when
err.Error()(from line 71) already contains "action ID:", resulting in redundant text like "action ID action ID: ACTION_UNSUPPORTED is not supported".Apply this diff to improve clarity:
actionID, err := NewActionID(val) if err != nil { - return ACTION_UNSUPPORTED, fmt.Errorf("action ID %s is not supported", err.Error()) + return ACTION_UNSUPPORTED, err }Or alternatively, if you want to add context:
actionID, err := NewActionID(val) if err != nil { - return ACTION_UNSUPPORTED, fmt.Errorf("action ID %s is not supported", err.Error()) + return ACTION_UNSUPPORTED, fmt.Errorf("invalid action ID: %w", err) }
80-91: Refine error message to avoid redundancy.The error message at line 87 has the same redundancy issue as
NewActionIDFromString— it includes "protocol ID" prefix whenerr.Error()(from line 107) already contains "protocol ID:", resulting in redundant text.Apply this diff to improve clarity:
protocolID, err := NewProtocolID(val) if err != nil { - return PROTOCOL_UNSUPPORTED, fmt.Errorf("protocol ID %s is not supported", err.Error()) + return PROTOCOL_UNSUPPORTED, err }Or alternatively:
protocolID, err := NewProtocolID(val) if err != nil { - return PROTOCOL_UNSUPPORTED, fmt.Errorf("protocol ID %s is not supported", err.Error()) + return PROTOCOL_UNSUPPORTED, fmt.Errorf("invalid protocol ID: %w", err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
types/core/id.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/id.go
🧠 Learnings (1)
📚 Learning: 2025-08-18T07:30:08.537Z
Learnt from: 0xstepit
PR: noble-assets/orbiter#15
File: keeper/component/forwarder/msg_server.go:94-102
Timestamp: 2025-08-18T07:30:08.537Z
Learning: In the orbiter codebase, when looking up protocol IDs in core.ProtocolID_value map, the default/zero value represents an unsupported protocol ID that is correctly handled by downstream validation logic, so explicit validation before the lookup is not necessary.
Applied to files:
types/core/id.go
🧬 Code graph analysis (1)
types/core/id.go (2)
api/core/v1/id.pulsar.go (10)
ActionID(500-500)ActionID(536-538)ActionID(540-542)ActionID(549-551)ActionID_value(519-523)ProtocolID(556-556)ProtocolID(600-602)ProtocolID(604-606)ProtocolID(613-615)ProtocolID_value(581-587)types/core/id.pb.go (12)
ActionID(29-29)ActionID(57-59)ActionID_value(47-51)ACTION_UNSUPPORTED(34-34)ProtocolID(64-64)ProtocolID(100-102)ProtocolID_value(88-94)PROTOCOL_UNSUPPORTED(69-69)PROTOCOL_IBC(71-71)PROTOCOL_CCTP(73-73)PROTOCOL_HYPERLANE(75-75)PROTOCOL_INTERNAL(77-77)
⏰ 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 (3)
types/core/id.go (3)
30-31: LGTM!The import of
channeltypesfrom ibc-go v8 is correct and necessary for the IBC channel ID validation added in line 167. Based on learnings, v8.6.1 is the current stable patch release with security fixes.
33-35: LGTM!The
MaxCounterpartyIDLengthconstant provides a reasonable upper bound for counterparty IDs and adds defense-in-depth against unbounded string inputs.
152-191: LGTM!The per-protocol validation logic is well-designed and addresses audit findings:
- Max length check (lines 157-162): Prevents unbounded strings
- Protocol-specific rules (lines 164-176):
PROTOCOL_IBC: Uses ibc-go's channel ID validationPROTOCOL_CCTP,PROTOCOL_HYPERLANE: Requires numeric IDsPROTOCOL_INTERNAL: Flexible for internal use- Correctly rejects unsupported protocols
- Helper function (lines 185-191):
isIntegeris safe given the 32-character max length constraintThe validation correctly handles the zero/default value case for
PROTOCOL_UNSUPPORTEDas indicated in the learnings.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
types/core/id.go (1)
185-191: Consider using ParseUint for domain ID validation.The function correctly validates integer strings. However, for CCTP/HYPERLANE domain IDs (typically unsigned 32-bit values),
strconv.ParseUint(s, 10, 32)might be more precise thanstrconv.Atoi.func isInteger(s string) bool { - _, err := strconv.Atoi(s) + _, err := strconv.ParseUint(s, 10, 32) return err == nil }This would:
- Reject negative numbers (which are invalid for domain IDs)
- Enforce 32-bit range explicitly (matching typical domain ID size)
- Be more self-documenting of the expected input range
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
types/core/id.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/id.go
🧠 Learnings (1)
📚 Learning: 2025-08-18T07:30:08.537Z
Learnt from: 0xstepit
PR: noble-assets/orbiter#15
File: keeper/component/forwarder/msg_server.go:94-102
Timestamp: 2025-08-18T07:30:08.537Z
Learning: In the orbiter codebase, when looking up protocol IDs in core.ProtocolID_value map, the default/zero value represents an unsupported protocol ID that is correctly handled by downstream validation logic, so explicit validation before the lookup is not necessary.
Applied to files:
types/core/id.go
🧬 Code graph analysis (1)
types/core/id.go (2)
api/core/v1/id.pulsar.go (10)
ActionID(500-500)ActionID(536-538)ActionID(540-542)ActionID(549-551)ActionID_value(519-523)ProtocolID(556-556)ProtocolID(600-602)ProtocolID(604-606)ProtocolID(613-615)ProtocolID_value(581-587)types/core/id.pb.go (12)
ActionID(29-29)ActionID(57-59)ActionID_value(47-51)ACTION_UNSUPPORTED(34-34)ProtocolID(64-64)ProtocolID(100-102)ProtocolID_value(88-94)PROTOCOL_UNSUPPORTED(69-69)PROTOCOL_IBC(71-71)PROTOCOL_CCTP(73-73)PROTOCOL_HYPERLANE(75-75)PROTOCOL_INTERNAL(77-77)
⏰ 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). (4)
- GitHub Check: build
- GitHub Check: prepare
- GitHub Check: unit-test
- GitHub Check: lint
🔇 Additional comments (3)
types/core/id.go (3)
30-35: LGTM! Security constraint added.The addition of
MaxCounterpartyIDLengthconstant and the ibc-go channeltypes import are appropriate security improvements that prevent unbounded string lengths and enable proper IBC channel ID validation.
152-183: Excellent per-protocol validation logic!The updated validation provides protocol-specific rules:
- IBC channels validated using official ibc-go validation
- CCTP/HYPERLANE domain IDs validated as integers
- Length constraint prevents DoS attacks
- Unsupported protocols explicitly rejected
This is a strong security improvement that aligns with audit findings.
141-150: LGTM! Proper integration of per-protocol validation.The updated call to
ValidateCounterpartyIDcorrectly passes the protocol parameter, enabling the per-protocol validation logic.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
keeper/component/forwarder/state_test.go (1)
55-88: Fix typos in test case names.All four case titles still say “portocol”. Please spell it “protocol” so the descriptions read cleanly.
- name: "success - all paused cross-chain for portocol ID 3 with no pagination", + name: "success - all paused cross-chain for protocol ID 3 with no pagination", @@ - name: "success - all paused cross-chain for portocol ID 3 with pagination", + name: "success - all paused cross-chain for protocol ID 3 with pagination", @@ - name: "success - all paused cross-chain for portocol ID 3 with pagination reversed", + name: "success - all paused cross-chain for protocol ID 3 with pagination reversed",types/core/id.go (1)
43-51: Fix%wusage—passingerr.Error()breaks compilation.
fmt.Errorfwith the%wverb expects anerror, buterr.Error()is astring. This does not compile and also loses the original error chain. Pass the error itself.- return ACTION_UNSUPPORTED, fmt.Errorf("action ID %s is not supported: %w", id, err.Error()) + return ACTION_UNSUPPORTED, fmt.Errorf("action ID %s is not supported: %w", id, err)keeper/component/dispatcher/state.go (1)
191-194: Thanks for fixing the source/destination context in the error.Message now correctly refers to “source protocol ID.”
🧹 Nitpick comments (6)
keeper/component/dispatcher/state.go (3)
176-189: Avoid redundant store reads in paginate mappers (amounts).Mapper re-fetches each value via Get. Use the provided v to build entries and save I/O.
Apply within these ranges:
- func(k DispatchedAmountsKey, v dispatchertypes.AmountDispatched) (*dispatchertypes.DispatchedAmountEntry, error) { - entry, err := d.getDispatchedAmountEntryFromKey(ctx, k) - if err != nil { - return nil, err - } - return &entry, nil - }, + func(k DispatchedAmountsKey, v dispatchertypes.AmountDispatched) (*dispatchertypes.DispatchedAmountEntry, error) { + entry, err := d.makeDispatchedAmountEntryFromKeyValue(k, v) + if err != nil { + return nil, err + } + return &entry, nil + },Add helper (outside the ranges):
func (d *Dispatcher) makeDispatchedAmountEntryFromKeyValue( k DispatchedAmountsKey, v dispatchertypes.AmountDispatched, ) (dispatchertypes.DispatchedAmountEntry, error) { var entry dispatchertypes.DispatchedAmountEntry sourceID, err := core.NewCrossChainID(core.ProtocolID(k.K1()), k.K2()) if err != nil { return entry, errorsmod.Wrap(err, "failed to create source cross-chain ID") } destID, err := core.ParseCrossChainID(k.K3()) if err != nil { return entry, errorsmod.Wrap(err, "failed to parse destination cross-chain ID") } entry.SourceId = &sourceID entry.DestinationId = &destID entry.Denom = k.K4() entry.AmountDispatched = v return entry, nil }Also applies to: 205-218
413-425: Avoid redundant store reads in paginate mappers (counts).Same pattern: build entries from key+value to avoid an extra Get per item.
Apply within these ranges:
- func(k DispatchedCountsKey, v uint64) (*dispatchertypes.DispatchCountEntry, error) { - entry, err := d.getDispatchCountEntryFromKey(ctx, k) - if err != nil { - return nil, err - } - return &entry, nil - }, + func(k DispatchedCountsKey, v uint64) (*dispatchertypes.DispatchCountEntry, error) { + entry, err := d.makeDispatchCountEntryFromKeyValue(k, v) + if err != nil { + return nil, err + } + return &entry, nil + },Add helper (outside the ranges):
func (d *Dispatcher) makeDispatchCountEntryFromKeyValue( k DispatchedCountsKey, v uint64, ) (dispatchertypes.DispatchCountEntry, error) { var entry dispatchertypes.DispatchCountEntry sourceID, err := core.NewCrossChainID(core.ProtocolID(k.K1()), k.K2()) if err != nil { return entry, errorsmod.Wrap(err, "failed to create source cross-chain ID") } destID, err := core.NewCrossChainID(core.ProtocolID(k.K3()), k.K4()) if err != nil { return entry, errorsmod.Wrap(err, "failed to create destination cross-chain ID") } entry.SourceId = &sourceID entry.DestinationId = &destID entry.Count = v return entry, nil }Also applies to: 442-455
408-412: Parameter naming consistency.Consider renaming parameter “id” to “protocolID” to match other getters.
Also applies to: 437-441
controller/adapter/ibc_test.go (3)
47-55: Use expError field name per repo’s testCases guideline.Rename errorContains to expError and update its usage.
As per coding guidelines
expectIsOrbiter bool expectPayload *core.Payload - expectError bool - errorContains string + expectError bool + expError stringAnd where asserted (see Line 217 below):
- require.ErrorContains(t, err, tc.errorContains, "expected a different error") + require.ErrorContains(t, err, tc.expError, "expected a different error")
57-72: Align test names; avoid "skip -" prefix.Prefer “success - …” or omit the prefix for non-error cases to keep naming consistent across tests.
As per coding guidelines
- name: "skip - not ics20 packet", + name: "success - not ics20 packet", ... - name: "skip - receiver is not orbiter module", + name: "success - receiver is not orbiter module",
206-219: Assertions cleanup reads well; tiny message nit (optional).Messages improved; consider a clearer message for the isOrbiter flag.
- require.Equal( + require.Equal( t, tc.expectIsOrbiter, isOrbiterPayload, - "expected payload to be orbiter payload", + "unexpected isOrbiterPayload value", )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
controller/adapter/ibc_test.go(4 hunks)keeper/component/dispatcher/state.go(3 hunks)keeper/component/forwarder/msg_server.go(6 hunks)keeper/component/forwarder/state_test.go(1 hunks)types/core/id.go(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/component/forwarder/msg_server.gocontroller/adapter/ibc_test.gokeeper/component/dispatcher/state.gokeeper/component/forwarder/state_test.gotypes/core/id.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:
controller/adapter/ibc_test.gokeeper/component/forwarder/state_test.go
🧠 Learnings (4)
📚 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:
controller/adapter/ibc_test.go
📚 Learning: 2025-07-22T12:31:35.398Z
Learnt from: 0xstepit
PR: noble-assets/orbiter#1
File: types/interfaces/adapter.go:32-34
Timestamp: 2025-07-22T12:31:35.398Z
Learning: In the PayloadAdapter interface, the ParsePayload method returns (bool, *types.Payload, error) where the boolean indicates whether the payload is targeting the orbiter system, not success/failure. This allows distinguishing between "payload not for orbiter" (false, nil, nil) versus "payload for orbiter but parsing failed" (true/false, nil/payload, error).
Applied to files:
controller/adapter/ibc_test.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 : Name each test case as "success - <DESCRIPTION>" for success cases and "error - <DESCRIPTION>" for error cases; omit the prefix when no error is tested
Applied to files:
keeper/component/forwarder/state_test.go
📚 Learning: 2025-08-18T07:30:08.537Z
Learnt from: 0xstepit
PR: noble-assets/orbiter#15
File: keeper/component/forwarder/msg_server.go:94-102
Timestamp: 2025-08-18T07:30:08.537Z
Learning: In the orbiter codebase, when looking up protocol IDs in core.ProtocolID_value map, the default/zero value represents an unsupported protocol ID that is correctly handled by downstream validation logic, so explicit validation before the lookup is not necessary.
Applied to files:
types/core/id.go
🧬 Code graph analysis (5)
keeper/component/forwarder/msg_server.go (4)
types/core/id.go (1)
NewProtocolIDFromString(80-95)types/core/errors.go (2)
ErrUnableToPause(33-33)ErrUnableToUnpause(34-34)types/component/forwarder/events.pb.go (6)
EventProtocolPaused(26-28)EventProtocolPaused(32-32)EventProtocolPaused(33-35)EventProtocolUnpaused(70-72)EventProtocolUnpaused(76-76)EventProtocolUnpaused(77-79)types/core/keys.go (1)
MaxTargetCounterparties(53-53)
controller/adapter/ibc_test.go (7)
types/core/attributes.go (1)
ForwardingAttributes(35-41)api/controller/forwarding/v1/cctp.pulsar.go (3)
CCTPAttributes(568-582)CCTPAttributes(597-597)CCTPAttributes(600-602)types/controller/forwarding/cctp.pb.go (3)
CCTPAttributes(29-39)CCTPAttributes(43-43)CCTPAttributes(44-46)testutil/payload.go (1)
CreateValidIBCPacketData(36-46)types/core/keys.go (1)
ModuleAddress(37-37)types/core/orbiter.pb.go (6)
Payload(128-135)Payload(139-139)Payload(140-142)Forwarding(79-91)Forwarding(95-95)Forwarding(96-98)types/core/id.pb.go (2)
PROTOCOL_CCTP(73-73)PROTOCOL_IBC(71-71)
keeper/component/dispatcher/state.go (2)
keeper/component/dispatcher/pagination.go (1)
WithCollectionPaginationQuadPrefix(28-35)keeper/component/dispatcher/dispatcher.go (1)
Dispatcher(41-50)
keeper/component/forwarder/state_test.go (3)
testutil/mocks/forwarder.go (1)
NewForwarderComponent(33-52)keeper/component/forwarder/forwarder.go (1)
Forwarder(47-57)types/component.go (1)
Forwarder(42-48)
types/core/id.go (2)
api/core/v1/id.pulsar.go (10)
ActionID(500-500)ActionID(536-538)ActionID(540-542)ActionID(549-551)ActionID_value(519-523)ProtocolID(556-556)ProtocolID(600-602)ProtocolID(604-606)ProtocolID(613-615)ProtocolID_value(581-587)types/core/id.pb.go (12)
ActionID(29-29)ActionID(57-59)ActionID_value(47-51)ACTION_UNSUPPORTED(34-34)ProtocolID(64-64)ProtocolID(100-102)ProtocolID_value(88-94)PROTOCOL_UNSUPPORTED(69-69)PROTOCOL_IBC(71-71)PROTOCOL_CCTP(73-73)PROTOCOL_HYPERLANE(75-75)PROTOCOL_INTERNAL(77-77)
⏰ 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). (4)
- GitHub Check: prepare
- GitHub Check: build
- GitHub Check: lint
- GitHub Check: unit-test
🔇 Additional comments (3)
keeper/component/dispatcher/state.go (1)
1-20: License header present.Header exists and matches policy.
As per coding guidelines
controller/adapter/ibc_test.go (2)
24-24: LGTM on new imports.Both encoding/json and forwardingtypes are used; should pass lint.
Also applies to: 35-35
153-194: Enforce CCTPAttributes validation in execution
Parser accepts incomplete CCTPAttributes; confirm controller/forwarding/cctp.go’s ValidateAttributes (invoked in executeForwarding) runs and rejects missing fields (e.g. DestinationDomain) before use.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
controller/adapter/ibc_test.go (1)
184-184: Critical: Extra closing brace causes syntax error.Line 184 contains an extra
}that doesn't match any opening brace. The if-else block starting at line 170 is already properly closed at line 183, making this brace extraneous.Apply this diff to remove the extra brace:
} } } - } }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
controller/action/fee.go(1 hunks)controller/adapter/ibc_test.go(5 hunks)keeper/component/adapter/adapter.go(1 hunks)keeper/component/forwarder/forwarder.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- keeper/component/adapter/adapter.go
- keeper/component/forwarder/forwarder.go
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Go code must be formatted and pass golangci-lint
All Go files must include license headers
Files:
controller/action/fee.gocontroller/adapter/ibc_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:
controller/adapter/ibc_test.go
🧬 Code graph analysis (2)
controller/action/fee.go (1)
types/controller/action/fee.go (1)
BPSNormalizer(35-35)
controller/adapter/ibc_test.go (5)
types/core/attributes.go (1)
ForwardingAttributes(35-41)types/controller/forwarding/cctp.pb.go (3)
CCTPAttributes(29-39)CCTPAttributes(43-43)CCTPAttributes(44-46)testutil/payload.go (1)
CreateValidIBCPacketData(36-46)types/core/orbiter.pb.go (6)
Payload(128-135)Payload(139-139)Payload(140-142)Forwarding(79-91)Forwarding(95-95)Forwarding(96-98)types/core/id.pb.go (1)
PROTOCOL_CCTP(73-73)
🪛 GitHub Actions: Lint codebase
controller/adapter/ibc_test.go
[error] 184-184: syntax error: unexpected newline in argument list; possibly missing comma or )
⏰ 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). (3)
- GitHub Check: prepare
- GitHub Check: build
- GitHub Check: unit-test
🔇 Additional comments (3)
controller/adapter/ibc_test.go (2)
167-167: LGTM: Improved error assertion.Using
require.NoErrorwith a descriptive message is clearer than the previous assertion, especially since no error is expected at parser creation.
178-182: LGTM: Cleaner PreActions validation logic.The refactored validation is more straightforward:
- Unconditionally checks length first
- Only validates content when expected PreActions exist
This is more maintainable than the previous nested nil/non-nil checks.
controller/action/fee.go (1)
243-243: Approve –BPSNormalizerconsistently migrated toactiontypes
Search confirms removal of allcore.BPSNormalizerreferences and uniform use ofactiontypes.BPSNormalizer = 10000in code and tests.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
types/core/id_test.go (1)
162-165: Consider making the default protocol test more explicit.The test case relies on the zero value of
protocolIDto test the default/unspecified protocol behavior. While this is correct (zero value falls through to the default case in the switch statement), explicitly setting it would make the test intent clearer.Consider applying this diff for clarity:
{ - name: "error - with default protocol ID", - id: "12345", - expError: "invalid counterparty ID", + name: "error - with default protocol ID", + id: "12345", + protocolID: core.ProtocolID(0), // or core.PROTOCOL_UNSPECIFIED if defined + expError: "invalid counterparty ID", },keeper/component/dispatcher/state.go (4)
180-187: Avoid second store read in pagination callback (use v).You already have the value; avoid re-get for better performance.
Apply:
- func(k DispatchedAmountsKey, v dispatchertypes.AmountDispatched) (*dispatchertypes.DispatchedAmountEntry, error) { - entry, err := d.getDispatchedAmountEntryFromKey(ctx, k) - if err != nil { - return nil, err - } - return &entry, nil - }, + func(k DispatchedAmountsKey, v dispatchertypes.AmountDispatched) (*dispatchertypes.DispatchedAmountEntry, error) { + sourceID, err := core.NewCrossChainID(core.ProtocolID(k.K1()), k.K2()) + if err != nil { + return nil, errorsmod.Wrap(err, "failed to create source cross-chain ID") + } + destID, err := core.ParseCrossChainID(k.K3()) + if err != nil { + return nil, errorsmod.Wrap(err, "failed to parse destination cross-chain ID") + } + entry := dispatchertypes.DispatchedAmountEntry{ + SourceId: &sourceID, + DestinationId: &destID, + Denom: k.K4(), + AmountDispatched: v, + } + return &entry, nil + },
417-424: Avoid second store read in counts pagination callback (use v).Use the provided value; skip an extra Get per row.
Apply:
- func(k DispatchedCountsKey, v uint64) (*dispatchertypes.DispatchCountEntry, error) { - entry, err := d.getDispatchCountEntryFromKey(ctx, k) - if err != nil { - return nil, err - } - return &entry, nil - }, + func(k DispatchedCountsKey, v uint64) (*dispatchertypes.DispatchCountEntry, error) { + sourceID, err := core.NewCrossChainID(core.ProtocolID(k.K1()), k.K2()) + if err != nil { + return nil, errorsmod.Wrap(err, "failed to create source cross-chain ID") + } + destID, err := core.NewCrossChainID(core.ProtocolID(k.K3()), k.K4()) + if err != nil { + return nil, errorsmod.Wrap(err, "failed to create destination cross-chain ID") + } + entry := dispatchertypes.DispatchCountEntry{ + SourceId: &sourceID, + DestinationId: &destID, + Count: v, + } + return &entry, nil + },
78-79: Naming consistency: use “cross_chain_id” instead of “orbit_id”.For index name consistency with prefixes/constants, prefer cross-chain terminology.
- core.DispatchedAmountsName+"_by_destination_orbit_id", + core.DispatchedAmountsName+"_by_destination_cross_chain_id",
248-251: Include the underlying error in logs (observability).Add err to logs and tighten messages.
- d.logger.Error("error in dispatched amounts walking all values") + d.logger.Error("error walking all dispatched amounts", "error", err)- d.logger.Error("error in dispatchedCounts walking all values") + d.logger.Error("error walking all dispatched counts", "error", err)Also applies to: 399-401
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
controller/adapter/ibc_test.go(5 hunks)keeper/component/dispatcher/state.go(3 hunks)types/core/id_test.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- controller/adapter/ibc_test.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/id_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/id_test.gokeeper/component/dispatcher/state.go
🧬 Code graph analysis (2)
types/core/id_test.go (2)
types/core/id.pb.go (4)
ProtocolID(64-64)ProtocolID(100-102)PROTOCOL_CCTP(73-73)PROTOCOL_IBC(71-71)types/core/id.go (2)
MaxCounterpartyIDLength(34-34)ValidateCounterpartyID(152-183)
keeper/component/dispatcher/state.go (3)
types/component/dispatcher/dispatcher.pb.go (9)
DispatchedAmountEntry(75-80)DispatchedAmountEntry(84-84)DispatchedAmountEntry(85-87)AmountDispatched(33-38)AmountDispatched(42-42)AmountDispatched(43-45)DispatchCountEntry(145-149)DispatchCountEntry(153-153)DispatchCountEntry(154-156)keeper/component/dispatcher/pagination.go (1)
WithCollectionPaginationQuadPrefix(28-35)keeper/component/dispatcher/dispatcher.go (1)
Dispatcher(41-50)
⏰ 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). (4)
- GitHub Check: build
- GitHub Check: prepare
- GitHub Check: unit-test
- GitHub Check: lint
🔇 Additional comments (6)
types/core/id_test.go (3)
25-25: LGTM!The
stringsimport is appropriately added to support the new test case that generates long counterparty IDs usingstrings.Repeat.
97-99: LGTM!The test case correctly validates that IDs with multiple colons are invalid. The descriptive name clearly indicates what's being tested, which addresses the previous review feedback about accuracy.
130-190: Well-structured test function with comprehensive coverage.The test follows all coding guidelines:
- Uses testCases pattern correctly
- Follows naming conventions ("error - ...", "success - ...")
- Includes expError field for error cases
- Uses require.ErrorContains for assertions
The test coverage is thorough, including edge cases like maximum length boundaries, empty IDs, format validation for different protocols, and both success and error scenarios.
keeper/component/dispatcher/state.go (3)
30-31: Import OKNeeded for pagination APIs. LGTM.
205-218: Index-based pagination usage LGTMCorrect use of index and pair prefix for destination protocol ID.
442-454: Destination counts pagination LGTMIndex + pair prefix look correct; error messages are clear.
|
Closes ENG-522 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
entrypoint/ibc_middleware.go (1)
142-142: Consider adding context to error messages.The error messages on lines 142 and 162 could be more helpful for debugging by including the denom and chain information. Line 150-154 demonstrates a better pattern by including the actual values.
- return "", errors.New("coin is native of source chain") + return "", fmt.Errorf("coin %q is native of source chain %s/%s", denom, sourcePort, sourceChannel)- return "", errors.New("orbiter supports only native tokens") + return "", fmt.Errorf("orbiter supports only native tokens, got non-native denom trace: %q", unprefixedDenom)Also applies to: 162-162
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
entrypoint/ibc_middleware.go(5 hunks)keeper/component/adapter/adapter.go(2 hunks)keeper/component/dispatcher/dispatcher.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- keeper/component/dispatcher/dispatcher.go
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (CLAUDE.md)
**/*.go: Go code must be formatted and pass golangci-lint
All Go files must include license headers
Files:
entrypoint/ibc_middleware.gokeeper/component/adapter/adapter.go
🧬 Code graph analysis (1)
entrypoint/ibc_middleware.go (3)
types/core/id.go (1)
NewCrossChainID(122-137)types/core/id.pb.go (1)
PROTOCOL_IBC(71-71)types/core/errors.go (1)
ErrNonNativeCoin(36-36)
⏰ 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 (4)
keeper/component/adapter/adapter.go (2)
264-264: LGTM!The rename from
clearOrbiterBalances(plural) toclearOrbiterBalance(singular) accurately reflects that the method operates on a single denomination. This improves code clarity and consistency.Also applies to: 271-271, 275-275
233-238: Approve fail-safe GetParams error handling — The logging-and-default-zero-size pattern aligns with genesis.go, preserving empty-payload transfers and rejecting non-empty payloads on errors.entrypoint/ibc_middleware.go (2)
85-90: LGTM! Proper IBC counterparty identifier construction.The change to derive
ibcCounterpartyIDusingGetDenomPrefixfollows the IBC denom prefix pattern correctly. Using the destination port and channel is appropriate since this represents the source chain's channel identifier on Noble.
96-96: LGTM! Clear documentation of sentinel error behavior.The comment effectively documents the special handling of
ErrNoOrbiterPacket, making the control flow more understandable.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
keeper/component/dispatcher/dispatcher.go (1)
231-244: Consider harmonizing error handling patterns.While the structured logging and error wrapping here are appropriate, there's an inconsistency with
dispatchActions(lines 169-186), which wraps and returns errors without logging them first.If forwarding errors are considered more critical than action errors and warrant Error-level logging at this layer, the current approach is fine. Otherwise, consider applying the same error handling pattern to both methods for consistency.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
keeper/component/dispatcher/dispatcher.go(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/component/dispatcher/dispatcher.go
⏰ 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). (4)
- GitHub Check: prepare
- GitHub Check: build
- GitHub Check: lint
- GitHub Check: unit-test
🔇 Additional comments (1)
keeper/component/dispatcher/dispatcher.go (1)
204-217: LGTM! Improved error handling with structured logging.The addition of structured logging with key-value pairs and error wrapping using
errorsmod.Wrapfimproves observability and provides better context for debugging. The protocol ID is properly included in both the log entry and the wrapped error message.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
entrypoint/ibc_middleware.go (1)
140-166: Prefer typed/sentinel errors in recoverNativeDenom for consistent classification.Currently returns plain errors/fmt.Errorf. Consider using errorsmod with sentinels (e.g., core.ErrNonNativeCoin, an “invalid denom/prefix” sentinel) so upstream can use errors.Is/As and so acknowledgements carry consistent context across the stack.
Example (adjust sentinel names to your taxonomy):
- return "", errors.New("coin is native of source chain") + return "", errorsmod.Wrap(core.ErrNonNativeCoin, "coin is native of source chain") - return "", fmt.Errorf( - "denom %q missing expected IBC prefix %q", - denom, - voucherPrefix, - ) + return "", errorsmod.Wrapf(core.ErrInvalidDenom, "denom %q missing expected IBC prefix %q", denom, voucherPrefix) - return "", errors.New("orbiter supports only native tokens") + return "", errorsmod.Wrap(core.ErrNonNativeCoin, "orbiter supports only native tokens")Also consider aligning the message phrasing with the wrapper at Line 112 (“non-native coin”) for clarity.
I can help introduce/standardize the sentinels in core and update call sites if desired.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
entrypoint/ibc_middleware.go(5 hunks)types/core/errors.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.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/errors.goentrypoint/ibc_middleware.go
🧠 Learnings (1)
📚 Learning: 2025-10-13T11:49:11.257Z
Learnt from: MalteHerrmann
PR: noble-assets/orbiter#49
File: controller/adapter/ibc.go:74-81
Timestamp: 2025-10-13T11:49:11.257Z
Learning: In the orbiter codebase, when packet parsing determines a packet is not intended for the orbiter module (e.g., invalid ICS-20 data or wrong receiver), return a typed sentinel error `core.ErrNoOrbiterPacket` instead of (nil, nil). Callers should check with `errors.Is(err, core.ErrNoOrbiterPacket)` to implement fallback behavior, avoiding nil pointer panics and making intent explicit.
Applied to files:
types/core/errors.go
🧬 Code graph analysis (2)
types/core/errors.go (1)
types/core/keys.go (1)
ModuleName(30-30)
entrypoint/ibc_middleware.go (2)
types/core/id.go (1)
NewCrossChainID(122-137)types/core/id.pb.go (1)
PROTOCOL_IBC(71-71)
⏰ 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). (4)
- GitHub Check: build
- GitHub Check: prepare
- GitHub Check: unit-test
- GitHub Check: lint
🔇 Additional comments (4)
types/core/errors.go (1)
36-36: Confirm no external dependencies on changed error code. Internal checks useerrors.Isand no numeric code comparisons were found; verify that no external clients, indexers, or monitoring tools depend on the previous code (12) forErrNoOrbiterPacket.entrypoint/ibc_middleware.go (3)
96-103: Good: fall through to ICS20 app on ErrNoOrbiterPacket.Clear, symmetric behavior; wrapped errors continue to be handled via errors.Is.
111-113: Good: preserve error chain when surfacing “non-native coin”.Switching to errorsmod.Wrap avoids losing the original error chain (addresses earlier feedback).
85-91: Verify need for trailing-sllash normalization in CounterpartyId
I didn’t findNewCrossChainIDor aValidate()enforcingCounterpartyIdformat—please confirm if the trailing “/” fromtransfertypes.GetDenomPrefixmust be trimmed. Example normalization:ibcCounterpartyID := strings.TrimSuffix( transfertypes.GetDenomPrefix(packet.DestinationPort, packet.DestinationChannel), "/", )
2 participants
This PR applies changes to fix findings reported by Halborn audit.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Breaking Changes
Chores
Closes ENG-522