Conversation
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
engine/thorchain/thorchain_test.go (1)
219-255: Check resource path consistency forthorchain_swaprulesIn
TestThorchain_Evaluate_Success_ThorchainSwapthe rule resource is"thorchain.thorchain_swap", whereas other swap tests use"thorchain.thorchain_swap.swap". Ifutil.ParseResourceexpects a fixed<chain>.<protocol>.<function>shape (as suggested by"thorchain.send.rune"elsewhere), this shorter resource might be relying on undefined or inconsistent parsing behavior.Consider standardizing this test to the same resource shape as the others (or documenting that 2‑segment resources are intentionally supported) to avoid subtle mismatches between rule resources and protocol/function parsing.
sdk/thorchain/thorchain_test.go (2)
20-29: MockRPCClient correctly models the RPC interfaceThe mock cleanly mirrors the
RPCClientinterface and integrates with testify’smock.Mock, which keeps Broadcast tests straightforward. Just be aware thatargs.Get(0).(*coretypes.ResultBroadcastTx)will panic if a future test usesReturn(nil, err)with an untypednil; using a typed nil in such cases will keep it safe.
30-44: Unusedhigh_s_valuevector could be removed or exercised
testSignatureVectors["high_s_value"]isn’t used in any test. Consider either:
- adding a test that verifies
normalizeLowSor signing behavior for a high‑S input, or- removing this vector to avoid confusion about untested cases.
sdk/thorchain/thorchain.go (2)
99-184: Signing flow and low‑S handling look correct; only minor polish opportunitiesThe
Signmethod:
- enforces exactly one signature,
- validates signer info structure,
- parses hex R/S (with optional
0xprefix),- enforces 32‑byte R/S, validates R in
[1, N-1], and normalizes S into low‑S form in[1, N-1],- constructs raw
R||Sand encodes viaTxConfig/authtx.WrapTx.This matches Cosmos expectations for
SIGN_MODE_DIRECTsecp256k1 signatures and should be robust.Two minor improvements you might consider:
- Use
signing.SignMode_SIGN_MODE_DIRECT(with an extra import) invalidateSignerInfoinstead of comparingsingle.Mode.String()to"SIGN_MODE_DIRECT".- Optionally check S with
validateCurveOrderValuefor symmetry with R, even thoughnormalizeLowSalready enforces the same bounds.
281-327:validateSignerInfoenforces the expected THORChain signer shapeThe signer validation correctly checks:
- presence of
AuthInfoandSignerInfos,- exactly one signer,
- non‑nil
PublicKeywith secp256k1 type URL, and- non‑nil
ModeInfowith single mode andSIGN_MODE_DIRECT.Only minor nit: comparing
single.Mode.String()to"SIGN_MODE_DIRECT"is slightly brittle versus comparing to the enum constant, but behavior is otherwise sound.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
engine/thorchain/thorchain.go(2 hunks)engine/thorchain/thorchain_test.go(5 hunks)sdk/thorchain/thorchain.go(1 hunks)sdk/thorchain/thorchain_test.go(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: neavra
Repo: vultisig/recipes PR: 123
File: engine/xrpl/xrpl.go:41-51
Timestamp: 2025-09-16T15:18:29.097Z
Learning: XRPL engine should support multiple functions including "send" and "thorchain_swap" rather than being restricted to just "send".
📚 Learning: 2025-07-28T17:33:10.162Z
Learnt from: webpiratt
Repo: vultisig/recipes PR: 50
File: ethereum/ethereum.go:165-166
Timestamp: 2025-07-28T17:33:10.162Z
Learning: The Protocol interface in types/protocol.go includes a ChainID() string method, so calling ChainID() on any Protocol implementation (like NewETH()) is valid and will not cause compilation errors.
Applied to files:
engine/thorchain/thorchain.go
🧬 Code graph analysis (3)
engine/thorchain/thorchain.go (1)
sdk/thorchain/thorchain.go (1)
MakeCodec(72-81)
sdk/thorchain/thorchain_test.go (1)
sdk/thorchain/thorchain.go (2)
NewSDK(47-55)NewCometBFTRPCClient(58-67)
engine/thorchain/thorchain_test.go (1)
sdk/thorchain/thorchain.go (1)
MakeCodec(72-81)
🔇 Additional comments (21)
engine/thorchain/thorchain.go (1)
15-29: Centralizing codec creation viathorchain_sdk.MakeCodecis a good refactorUsing the shared codec factory removes duplicated registration logic, keeps engine/tests/SDK aligned, and reduces the risk of interface‑registry drift. No issues spotted with this change.
engine/thorchain/thorchain_test.go (4)
13-16: Tests now using sharedMakeCodeckeep them aligned with runtime behaviorSwitching the MsgDeposit helper to
thorchain_sdk.MakeCodec()means test transactions are encoded with the exact same interface registry as the engine/SDK, which avoids subtle Any/Msg registration mismatches. Looks good.Also applies to: 323-372
375-412:createValidMsgSendTransactioncodec change is consistent and correctUsing
thorchain_sdk.MakeCodec()here mirrors the runtime codec used byNewThorchainand the SDK, so MsgSend‑based txs in tests will deserialize identically to production. No issues with the refactor.
425-471:TestThorchain_parseTransactionnow uses the standardized codec – good alignmentCreating the protobuf transaction with
thorchain_sdk.MakeCodec()ensures the bytes under test are exactly what the engine’s codec expects, tightening the realism of this test and preventing registry drift.
549-554: Protobuf parse test usingMakeCodecis consistent with the new codec strategyThe dedicated protobuf parse test now reuses
thorchain_sdk.MakeCodec(), which keeps it in sync with both engine and SDK codec wiring. This is the right direction.sdk/thorchain/thorchain_test.go (10)
46-112:buildRealisticTHORChainTxis a solid end‑to‑end fixtureBuilding a real
MsgSend+AuthInfo(with secp256k1 pubkey and SIGN_MODE_DIRECT) and encoding it viaNewSDK(nil).codecgives you highly realistic unsigned tx bytes that exercise the same codec path as production. This is an excellent foundation for the signing and hashing tests.
135-163: Happy‑path signing test matches SDK expectations
TestSDK_Sign_WithRealisticTransactionvalidates unmarshalling, signer validation, signature injection, and that the body/memo survive signing intact. This directly exercises the core Sign logic and looks correct.
176-188: Multi‑signature rejection behavior is well‑specified
TestSDK_Sign_MultipleSignaturesconfirms the SDK fails when more than one TSS signature is provided, matching the single‑signer assumption. This is a useful guardrail and aligns with the Sign implementation.
206-225: Broadcast tests correctly assert RPC wiring and error propagation
TestSDK_BroadcastandTestSDK_Broadcast_NoRPCClientensure successful broadcasts are passed through and that missing RPC configuration is surfaced as a clear error. The mock expectations and assertions look sound.
238-261:Sendtests validate the sign‑then‑broadcast orchestration
TestSDK_Send_Successand the corresponding signing‑error test correctly verify that Send:
- uses Sign internally,
- propagates signing errors, and
- does not call
BroadcastTxSyncwhen signing fails.This nicely pins down the high‑level API behavior.
282-324: MessageHash tests thoroughly cover determinism and input sensitivity
TestSDK_MessageHashchecks hash length, determinism for identical inputs, and variance across account number, sequence, and transaction body changes. This gives strong confidence thatMessageHashis behaving as a proper SignDoc hash builder.
326-367: Raw signature format tests ensure flexible hex handling and 64‑byte output
TestSDK_Sign_RawSignatureFormatconfirms that signatures with and without0xprefixes decode correctly and always result in 64‑byteR||Ssignatures in the transaction. This is a valuable compatibility check against different TSS outputs.
369-391: Integration test is cautious and non‑disruptive
TestSDK_Integration_Testnetrespectstesting.Short()and skips cleanly when the testnet endpoint is unreachable. It avoids broadcasting, limiting itself to connectivity + hashing. This is a balanced approach for an integration‑style test.
393-460: Edge‑case signing tests align with curve‑order and length checks
TestSDK_Sign_EdgeCasesexercises:
- zero R/S,
- oversized R/S (near all‑ones),
- valid mixed patterns, and
- short R values,
matching the constraints enforced in
validateCurveOrderValueandnormalizeLowS. The expected pass/fail outcomes look consistent with the implementation.
462-540: Signer‑info validation tests closely mirror the SDK’s error paths
TestSDK_SignerInfoValidationsystematically mutates AuthInfo/SignerInfos/PublicKey/ModeInfo to confirm thatvalidateSignerInfoemits the expected errors. This tightly couples tests to the validation logic and should catch regressions if that logic is changed.sdk/thorchain/thorchain.go (6)
35-55: SDK struct andNewSDKconstructor are well‑structuredThe SDK cleanly encapsulates RPC client, codec, and TxConfig, and uses the shared
MakeCodec()plusauthtx.NewTxConfig, which is the right way to align encoding/signing behavior with Cosmos tooling. No issues here.
69-81:MakeCodecis a good single source of truth for THORChain codec setupCentralizing interface registration (crypto, bank,
MsgDeposit) intoMakeCodecand reusing it across SDK/engine/tests significantly reduces the risk of Any/unpacking mismatches. This design looks solid.
83-97: Broadcast wrapper correctly surfaces CometBFT errors and ABCI codes
CometBFTRPCClient.BroadcastTxSyncwraps network errors and rejects non‑zero ABCI codes with a detailed error while still returning the raw result. This is a sensible default behavior.
186-206:BroadcastandSendcompose cleanly and handle signing errors correctly
Broadcastguards against a nil RPC client, andSendproperly propagates signing errors and only calls Broadcast when signing succeeds. This matches the test expectations and is a clear high‑level API.
253-279: Curve‑order validation helper is straightforward and correct
validateCurveOrderValuecleanly enforces non‑zero and< Nbounds for secp256k1 values using the globalsecpN. This matches the needs for validating R (and potentially S) and looks correct.
329-349: Low‑S normalization logic appears correct and preserves 32‑byte encoding
normalizeLowS:
- rejects empty or out‑of‑range S,
- reflects S into the lower half of the curve order when needed (
S' = N - S),- left‑pads results to 32 bytes.
This matches standard low‑S normalization practices and should prevent malleability while keeping signatures in fixed‑width
R||Sformat.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
sdk/thorchain/thorchain.go (2)
211-260: Chain ID still hard‑coded; consider making it configurable
MessageHashis now safe (no panics) and correctly builds aSignDocand SHA‑256 hash. However,ChainIdis still set via the hard‑codedthorchainChainID = "thorchain-1". This locks the SDK to mainnet and makes reuse on stagenet/testnet or any fork require code changes.Consider:
- Adding a
ChainID stringfield onSDKand initializing it inNewSDK(defaulting to"thorchain-1"), or- Accepting
chainIDas an argument toMessageHash.This would address the earlier concern while keeping the current default behavior.
262-269: Hex cleaning and marshal helpers are safe and address previous panic concerns
cleanHexis simple and safe for trimming optional0xprefixes, and the newmarshalTxBody/marshalAuthInfohelpers return errors instead of panicking on nil inputs or marshal failures. This directly addresses the earlier panic risk in public API paths while keeping the call sites (MessageHash) clean.Also applies to: 362-384
🧹 Nitpick comments (5)
sdk/thorchain/thorchain.go (5)
189-209: Broadcast/Send flow is straightforward but could add more context on failure
BroadcastandSendare minimal and correct:SendcomposesSign+Broadcast, andBroadcastenforces thatrpcClientis configured.If you want slightly richer errors, you could wrap the broadcast error with context (e.g.,
"failed to broadcast signed transaction"), but this is optional and not a blocker.
110-186: Minor naming nit: receiversdkvs importedsdkaliasUsing
sdkas the method receiver name on*SDKwhile also importingsdk "github.com/cosmos/cosmos-sdk/types"can be a bit confusing, especially for future edits that might need the Cosmossdkalias in these methods.Optional: rename the receiver to something like
sortsdkto avoid shadowing and improve readability.Also applies to: 190-209, 211-260
229-243: Clarify sequence contract betweenMessageHashandSign
MessageHashmutatesunsignedTx.AuthInfo.SignerInfos[0].Sequencein-memory, but this doesn't modify the originalunsignedTxBytesparameter (byte slices are immutable in Go). WhenSignis later called with those sameunsignedTxBytes, it uses whatever sequence is embedded in them—not the value set inMessageHash.The
Signdoc comment requires "SignerInfos[0].Sequence matching the sequence used in MessageHash()", which is only satisfied if the caller ensures the bytes are already correct upfront. The mutation inMessageHashis therefore misleading and doesn't "fix up" the tx.Consider either: (1) tightening the
MessageHashdoc to clarify that thesequenceparameter must already match what's inunsignedTxBytes, with the mutation being for the hashing computation only, or (2) removing the mutation and just validating equality, making the dependency explicit.
290-336: Use enum constant for SignMode comparison instead of String() representationThe test suite already demonstrates the correct pattern at line 84 of
thorchain_test.go:signing.SignMode_SIGN_MODE_DIRECT. The code invalidateSignerInfoshould follow this same approach.Change the comparison from
single.Mode.String() != "SIGN_MODE_DIRECT"tosingle.Mode != signing.SignMode_SIGN_MODE_DIRECTby:
- Adding import:
"github.com/cosmos/cosmos-sdk/types/tx/signing"- Replacing the string comparison with direct enum comparison
This aligns with the Cosmos SDK's protobuf design where
SignModevalues are stable numeric identifiers, making enum comparison the canonical and future-proof approach.
102-187: Use enum constant for SignMode comparison; consider symmetric R/S padding handlingThe signing pipeline is sound, but one best-practice improvement is confirmed by Cosmos SDK conventions:
- Line 328: Replace
if single.Mode.String() != "SIGN_MODE_DIRECT"withif single.Mode != signingtypes.SignMode_SIGN_MODE_DIRECT. Enum comparison is type-safe, faster, and not dependent on string formatting.Additionally, note the asymmetry in R/S validation: S values are left-padded to 32 bytes in
normalizeLowS(lines 352–356), but R values hard-fail if not exactly 32 bytes. If TSS guarantees are documented as always providing 32-byte values, this is acceptable; otherwise, consider applying symmetric padding to both R and S for robustness.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sdk/thorchain/thorchain.go(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-17T22:17:33.321Z
Learnt from: webpiratt
Repo: vultisig/recipes PR: 129
File: engine/solana/assert.go:77-94
Timestamp: 2025-09-17T22:17:33.321Z
Learning: In engine/solana/assert.go, the functions assertFuncSelector and assertArgs correctly accept solana.Base58 parameters, not []byte. The instruction data (inst.Data) passed to these functions is already of the appropriate solana.Base58 type, so no type conversion is needed.
Applied to files:
sdk/thorchain/thorchain.go
🔇 Additional comments (3)
sdk/thorchain/thorchain.go (3)
35-40: SDK construction & shared codec/TxConfig look solidThe
SDKstruct,NewSDK, andMakeCodecgive a clear, reusable setup for Tx encoding and type registration (includingMsgDeposit). Centralizing codec/TxConfig this way should keep signing, hashing, and broadcasting consistent across engine and plugins.Also applies to: 49-58, 72-84
30-33: CometBFTRPCClient and RPCClient abstraction are well-shapedThe
RPCClientinterface andCometBFTRPCClientwrapper cleanly separate transport from the SDK logic, andBroadcastTxSyncsurfaces node-side rejection via a descriptive error while still returning the rawResultBroadcastTx. That’s a good balance for higher-level callers.Also applies to: 60-70, 86-100
338-358: Curve-order checks and low‑S normalization look correct
validateCurveOrderValueandnormalizeLowScorrectly enforceR,S ∈ [1, N-1]and adjustSinto the lower half of the curve order, with proper left‑padding to 32 bytes. This matches standard secp256k1 anti‑malleability practice and should interoperate cleanly with Cosmos/THORChain signature verification.Also applies to: 271-288
1 participant
Summary by CodeRabbit
New Features
Refactor
Tests