Skip to content

feat: Add Thorchain support with RUNE and TCY transfer functionality#26

Open
SunshineRider wants to merge 14 commits intomainfrom
add_thorchain
Open

feat: Add Thorchain support with RUNE and TCY transfer functionality#26
SunshineRider wants to merge 14 commits intomainfrom
add_thorchain

Conversation

@SunshineRider
Copy link
Contributor

@SunshineRider SunshineRider commented Jun 14, 2025

This PR adds Thorchain blockchain support to the Vultisig recipes system, enabling RUNE and TCY token transfers.

Core Thorchain Implementation

  • New Chain Support: Added thorchain as a supported blockchain in the chain registry
  • Dual Token Support: Native RUNE and TCY token transfer protocols
  • Transaction Processing: Cosmos SDK-compatible transaction parsing and hash computation

Functional Implementations

  • RUNE: Native RUNE token transfers with recipient, amount, and memo parameters
  • TCY: TCY token transfers with the same parameter structure

[EDIT]
Additional Changes

  • Refactor Protocol to use factory methods to generate token protocol
  • Add memo parser (for future functionalities)
  • Update testdata
# PASS - Exact limit 100 RUNE
# From: thor1jkndhpfauwmtn2uk7ytmruu0yywz5e66mq94e2
# To: thor1c3xl04vgw7f7q9xrmqc4llr6s9l8p7qmgqwzsl
# Amount: 10000000000 rune (100.00000000 RUNE)
go run cmd/vulticheck/*.go  --policy testdata/payroll.json \
           --tx "0x0aa8010a8f010a1c2f636f736d6f732e62616e6b2e763162657461312e4d736753656e64126f0a2b74686f72316a6b6e646870666175776d746e32756b3779746d727575307979777a356536366d7139346532122b74686f72316333786c3034766777376637713978726d7163346c6c723673396c387037716d6771777a736c1a130a0472756e65120b313030303030303030303012144d617820616c6c6f776564207472616e73666572121b0a02180112150a0f0a0472756e6512073230303030303010c09a0c" \
           --chain thorchain


# FAIL - Amount exceeds 150 RUNE
# From: thor1jkndhpfauwmtn2uk7ytmruu0yywz5e66mq94e2
# To: thor1c3xl04vgw7f7q9xrmqc4llr6s9l8p7qmgqwzsl
# Amount: 15000000000 rune (150.00000000 RUNE)   
go run cmd/vulticheck/*.go  --policy testdata/payroll.json \
           --tx "0x0aa1010a8f010a1c2f636f736d6f732e62616e6b2e763162657461312e4d736753656e64126f0a2b74686f72316a6b6e646870666175776d746e32756b3779746d727575307979777a356536366d7139346532122b74686f72316333786c3034766777376637713978726d7163346c6c723673396c387037716d6771777a736c1a130a0472756e65120b3135303030303030303030120d45786365656473206c696d6974121b0a02180312150a0f0a0472756e6512073230303030303010c09a0c" \
           --chain thorchain
           
# FAIL - Wrong recipient
# From: thor1jkndhpfauwmtn2uk7ytmruu0yywz5e66mq94e2
# To: thor1wrongaddress000000000000000000000000
# Amount: 5000000000 rune (50.00000000 RUNE)
go run cmd/vulticheck/*.go  --policy testdata/payroll.json \
           --tx "0x0aa0010a8c010a1c2f636f736d6f732e62616e6b2e763162657461312e4d736753656e64126c0a2b74686f72316a6b6e646870666175776d746e32756b3779746d727575307979777a356536366d7139346532122974686f723177726f6e67616464726573733030303030303030303030303030303030303030303030301a120a0472756e65120a35303030303030303030120f57726f6e6720726563697069656e74121b0a02180412150a0f0a0472756e6512073230303030303010c09a0c" \
           --chain thorchain

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Added support for Thorchain blockchain with integration into the chain and protocol registries.
    • Introduced protocols for RUNE and TCY token transfers with detailed parameter validation and constraint handling.
    • Enabled parsing, validation, and hashing of Thorchain transactions following Cosmos SDK protobuf standards.
    • Added comprehensive validation for Thorchain addresses, memos, and transfer amounts.
    • Added resource declarations for Thorchain RUNE and TCY transfer policies.
    • Integrated Thorchain support into transaction generation and policy evaluation tooling.
  • Tests

    • Added extensive unit and integration tests covering Thorchain transaction parsing, protocol matching, validation, and policy enforcement.
    • Extended existing test suites to include Thorchain-specific test vectors and scenarios.
  • Chores

    • Updated ignore rules to exclude the thornode-develop directory from version control.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Jun 14, 2025

Warning

Rate limit exceeded

@neavra has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 15 minutes and 25 seconds before requesting another review.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

📥 Commits

Reviewing files that changed from the base of the PR and between b68ddae and 85fad6a.

📒 Files selected for processing (1)
  • testdata/payroll_schema.json (1 hunks)

Walkthrough

The changes introduce native Thorchain blockchain support, including its registration in the global chain registry, implementation of the Chain interface, transaction decoding, protocol and function definitions, and comprehensive validation logic. Extensive unit and integration tests are added to verify Thorchain's integration, transaction parsing, protocol functionality, and address/memo/amount validation.

Changes

File(s) Change Summary
.gitignore Added thornode-develop/* to exclude this directory from version control.
chain/registry.go Imported Thorchain package and registered Thorchain in the default chain registry.
chain/thorchain_integration_test.go Added integration tests for Thorchain registration, protocols, transaction parsing, and functions.
thorchain/chain.go Implemented Thorchain struct as a Chain, with transaction parsing, protocol retrieval, and hashing.
thorchain/decode.go Added Thorchain transaction decoding, validation, and hash computation logic.
thorchain/decode_test.go Introduced unit tests for transaction decoding, validation, and hash computation.
thorchain/protocol.go Defined RUNE and TCY protocols, transfer functions, and parameter constraint logic.
thorchain/thorchain_test.go Added unit tests for Thorchain chain, protocols, and protocol functions.
thorchain/validator.go Implemented ThorchainValidator for validating transfer parameters.
thorchain/memo.go Added memo parsing and representation for Thorchain memos including swap and transfer types.
go.mod Added direct dependency on Cosmos SDK v0.50.4 and numerous indirect dependencies related to Cosmos ecosystem.
RESOURCES.md Added resource declarations for Thorchain RUNE and TCY transfer functions with parameters and example policies.
cmd/generator/main.go Registered Thorchain instance in chain generator registration.
protocol/registry.go Registered Thorchain RUNE and TCY protocols in the default protocol registry.
engine/engine_test.go Added Thorchain test vectors for policy evaluation including valid and invalid transfer cases.
testdata/thorchain_test_helper.go Added utilities and test cases for generating Thorchain transaction hex strings for testing.
testdata/payroll.json Added policy rule allowing RUNE transfers to a specific Thorchain address with amount constraints.

Sequence Diagram(s)

sequenceDiagram
    participant App
    participant ChainRegistry
    participant Thorchain
    participant Protocol
    participant Validator

    App->>ChainRegistry: GetChain("thorchain")
    ChainRegistry->>Thorchain: Instantiate Thorchain
    App->>Thorchain: ParseTransaction(txHex)
    Thorchain->>Thorchain: ParseThorchainTransaction(txHex)
    Thorchain-->>App: DecodedTransaction

    App->>Thorchain: GetProtocol("rune")
    Thorchain->>Protocol: Return RUNE Protocol
    App->>Protocol: MatchFunctionCall(decodedTx, policyMatcher)
    Protocol->>Validator: ValidateTransfer(params)
    Validator-->>Protocol: Validation Result
    Protocol-->>App: Match Result
Loading

Poem

In burrows deep, a rabbit cheers,
For Thorchain’s code now reappears!
With hashes, memos, and RUNE to send,
Protocols and tests that never end.
From address checks to transfer flows,
This blockchain garden grows and grows!
🐇✨


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.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Explain this complex logic.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai explain this code block.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and explain its main purpose.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 8

🔭 Outside diff range comments (1)
chain/registry.go (1)

26-33: 🛠️ Refactor suggestion

Warn on accidental chain-ID overwrites

Register silently overwrites an existing entry when the same chain ID is registered twice.
Returning an error (and letting RegisterChain bubble it up) would prevent subtle bugs when multiple inits accidentally use the same ID.

 func (r *Registry) Register(chain types.Chain) error {
 	r.mu.Lock()
 	defer r.mu.Unlock()

-	r.chains[chain.ID()] = chain
+	if _, exists := r.chains[chain.ID()]; exists {
+		return fmt.Errorf("chain with ID %q already registered", chain.ID())
+	}
+	r.chains[chain.ID()] = chain
 	return nil
 }
🧹 Nitpick comments (8)
.gitignore (1)

35-37: Prefer directory patterns over *-suffix for clarity
Using bin/* and thornode-develop/* ignores their contents but not the directories themselves. Recommend ignoring the directories directly and anchoring to the repo root:

- bin/*
+ /bin/

- thornode-develop/*
+ /thornode-develop/
thorchain/validator.go (1)

16-54: Consider a typed request struct instead of map[string]interface{}

Using a plain map hides compile-time guarantees and forces a lot of type assertions. A small struct (e.g. TransferParams) would improve readability and safety.

Optional, but worth keeping in mind.

thorchain/thorchain_test.go (1)

11-24: Test style nit – mix of assert and require

Inside a single test it’s clearer to stick to one style; mixing them can lead to continuing after a fatal condition. Here assert after require is fine, but for consistency consider using require for property checks that must hold before later assertions.

Minor style only.

thorchain/decode_test.go (1)

132-138: Inefficient long-memo construction

The loop rebuilds the string O(n²) times. A simpler constant-time approach is clearer:

longMemo := strings.Repeat("a", MaxMemoLength+1)

Not a performance issue in a test, but keeps the code tidy.

thorchain/chain.go (1)

71-72: Pre-allocation off by ~31 bytes per signature

len(sigs)*96 assumes every signature is 96 bytes, yet you only append 65 bytes (32 + 32 + 1).
The slice will be re-allocated anyway, negating the intended optimisation.

-signedTxData := make([]byte, 0, len(txBytes)+len(sigs)*96)
+const sigLen = 65 // 32 R + 32 S + 1 recID
+signedTxData := make([]byte, 0, len(txBytes)+len(sigs)*sigLen)
thorchain/protocol.go (1)

257-260: Redundant dummy receiver

(&RUNE{}).evaluateBasicConstraints works but creates an unnecessary object each
call. Extract the function to a standalone helper and reuse it for both
protocols for clarity.

thorchain/decode.go (2)

148-152: Hash casing may differ from Tendermint RPC

computeThorchainTxHash upper-cases the hex string; Tendermint RPCs usually
return lower-case hashes. Consider returning lower-case to avoid unnecessary
string conversions downstream.


192-218: Hard-coded max amounts are magic numbers

The 1 B RUNE / 500 M TCY limits are hidden constants. Expose them as named
const values or derive from supply caps if available to aid maintenance.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 9763991 and 6179012.

📒 Files selected for processing (9)
  • .gitignore (1 hunks)
  • chain/registry.go (2 hunks)
  • chain/thorchain_integration_test.go (1 hunks)
  • thorchain/chain.go (1 hunks)
  • thorchain/decode.go (1 hunks)
  • thorchain/decode_test.go (1 hunks)
  • thorchain/protocol.go (1 hunks)
  • thorchain/thorchain_test.go (1 hunks)
  • thorchain/validator.go (1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (6)
chain/registry.go (1)
thorchain/chain.go (1)
  • NewThorchain (103-105)
thorchain/decode_test.go (1)
thorchain/decode.go (6)
  • ParseThorchainTransaction (115-146)
  • ParsedThorchainTransaction (22-36)
  • ValidateThorchainAddress (155-181)
  • ValidateMemo (184-189)
  • MaxMemoLength (18-18)
  • ValidateAmount (192-220)
chain/thorchain_integration_test.go (2)
chain/registry.go (2)
  • GetChain (74-76)
  • DefaultRegistry (62-62)
protocol/registry.go (1)
  • GetProtocol (92-94)
thorchain/validator.go (1)
thorchain/decode.go (3)
  • ValidateThorchainAddress (155-181)
  • ValidateAmount (192-220)
  • ValidateMemo (184-189)
thorchain/chain.go (4)
types/transaction.go (1)
  • DecodedTransaction (7-26)
thorchain/decode.go (1)
  • ParseThorchainTransaction (115-146)
thorchain/protocol.go (2)
  • NewRUNE (71-103)
  • NewTCY (111-143)
types/chain.go (1)
  • Chain (6-31)
thorchain/decode.go (1)
types/transaction.go (1)
  • DecodedTransaction (7-26)
🔇 Additional comments (2)
chain/registry.go (1)

78-84: Init looks good – Thorchain is now globally discoverable

Import and registration of Thorchain integrate cleanly with the existing pattern. No cycle risk observed.

chain/thorchain_integration_test.go (1)

50-63: Integration test passes with synthetic hex – OK

The dummy hex string is sufficient for hashing/decoding sanity checks and avoids external dependencies. Looks good.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (2)
thorchain/decode.go (2)

131-138: Still no explicit check for empty txHex.
Decoding an empty string succeeds and only fails later with “transaction too short”, which is misleading. Add a quick if txHex == "" { return nil, err } before hex.DecodeString.


242-268: 🛠️ Refactor suggestion

Bech32 checksum still not validated.

The function checks prefix/length/charset but ignores the checksum, repeating the issue flagged earlier. Integrate a Bech32 decoder (e.g. btcsuite/btcutil/bech32) for full validation.

🧹 Nitpick comments (2)
thorchain/decode.go (2)

187-213: Parses only the first message – multi-message Tx support needed.

Thorchain deposits are often batched. Scanning just Body.Messages[0] can miss transfers or produce incorrect totals. Iterate over all messages or at least validate that len(Messages)==1 and fail otherwise.


288-304: Hard-coded max amounts & ignored decimals.

The upper bounds are magic numbers and assume 1e8 precision. Expose them as named constants and document the reasoning. Consider validating decimal places instead of raw integer magnitude.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6179012 and d5932aa.

⛔ Files ignored due to path filters (1)
  • go.sum is excluded by !**/*.sum
📒 Files selected for processing (4)
  • chain/thorchain_integration_test.go (1 hunks)
  • go.mod (1 hunks)
  • thorchain/decode.go (1 hunks)
  • thorchain/decode_test.go (1 hunks)
✅ Files skipped from review due to trivial changes (1)
  • go.mod
🚧 Files skipped from review as they are similar to previous changes (2)
  • chain/thorchain_integration_test.go
  • thorchain/decode_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
thorchain/decode.go (1)
types/transaction.go (1)
  • DecodedTransaction (7-26)
⏰ Context from checks skipped due to timeout of 90000ms (1)
  • GitHub Check: build

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (2)
thorchain/decode.go (1)

189-201: 🛠️ Refactor suggestion

Gas-price calculation ignores multi-denom fees and silently falls back to nil

Only the first fee coin is considered and you silently skip unsupported denoms, leaving gasPrice == nil.
Down-stream policy code may interpret this as “free gas”.

  1. Iterate over all fee coins and pick the one matching the native denom (rune) or explicitly error on mixed denoms.
  2. Return an error when the selected denom is unsupported instead of swallowing the case.
- if len(cosmosSDKTx.AuthInfo.Fee.Amount) > 0 && parsed.gasLimit > 0 {
-     feeCoin := cosmosSDKTx.AuthInfo.Fee.Amount[0]
-     if feeCoin.Denom == "rune" || feeCoin.Denom == "tcy" {
+ for _, feeCoin := range cosmosSDKTx.AuthInfo.Fee.Amount {
+     if feeCoin.Denom == "rune" || feeCoin.Denom == "tcy" {
          ...
          parsed.gasPrice = new(big.Int).Div(feeAmount, gasLimit)
+         break
+     } else {
+         return nil, fmt.Errorf("unsupported fee denom %s", feeCoin.Denom)
      }
- }
+ }
thorchain/protocol.go (1)

200-213: Same issue here – verify the denom is tcy before accepting the match

Replicate the logic suggested for the RUNE matcher to avoid misclassification.

🧹 Nitpick comments (3)
thorchain/decode.go (2)

141-144: Drop the arbitrary 10-byte size gate

A valid Cosmos-SDK tx can be < 10 bytes once amino is removed (e.g. TxRaw{}).
The check adds no security and will reject legitimate edge-cases while not protecting against malformed input.
Recommend removing or replacing with a structural check (e.g. ensure BodyBytes / AuthInfoBytes are non-empty).


210-240: Parser drops information when multiple messages are present

ParseThorchainTransaction only inspects Body.Messages[0].
Multisend or combined MsgSend + Memo patterns are common on Thorchain; ignoring the remaining messages leads to incomplete or incorrect decoding.

At minimum iterate over all messages and surface:

  • every MsgSend (aggregate amounts per recipient?)
  • any message carrying the memo you care about.

Fail fast (or warn) if >1 distinct recipients/denoms are seen and the higher-level logic cannot cope.

thorchain/protocol.go (1)

145-187: Consider factoring the nearly identical RUNE / TCY matchers into one helper

Apart from the denom check the two blocks are copy-paste identical.
Moving the common logic to matchTransfer(decodedTx, expectedDenom string, …) avoids divergence bugs and eases future maintenance.

Also applies to: 189-231

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d5932aa and 484b262.

📒 Files selected for processing (5)
  • thorchain/chain.go (1 hunks)
  • thorchain/decode.go (1 hunks)
  • thorchain/decode_test.go (1 hunks)
  • thorchain/protocol.go (1 hunks)
  • thorchain/validator.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • thorchain/validator.go
  • thorchain/chain.go
  • thorchain/decode_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
thorchain/decode.go (1)
types/transaction.go (1)
  • DecodedTransaction (7-26)

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (5)
cmd/generator/main.go (1)

11-12: Keep import groups goimports-friendly

github.com/vultisig/recipes/thorchain is an external repository path just like the other three, but it sits in the middle of the “internal” grouping (internal/generator). A quick run of goimports will reorder and de-duplicate imports automatically – run it to avoid noisy diffs later.

thorchain/thorchain_test.go (4)

12-25: Enable parallel execution for faster tests

None of these tests share mutable state; mark them t.Parallel() to cut CI time and surface data-race issues sooner.

-func TestThorchainChain(t *testing.T) {
+func TestThorchainChain(t *testing.T) {
+    t.Parallel()

Apply the same one-liner at the start of every test function in this file.


27-48: Ditto – make sub-tests parallel

Replicate the t.Parallel() addition here (and in the remaining test functions) for consistency and speed.


50-95: Minor duplication – collapse lookup loop

The for loop finding "transfer" is duplicated in several tests. Extract a helper to keep the focus on assertions:

func findFunction(fns []*types.Function, id string) *types.Function {
    for _, fn := range fns {
        if fn.ID == id {
            return fn
        }
    }
    return nil
}

Usage:

transferFunc := findFunction(functions, "transfer")
require.NotNil(t, transferFunc)

151-228: Use constants to avoid magic numbers

100000000 and 200000000 appear in multiple places; define them once to avoid mismatches:

const (
    oneRune  = "100000000"
    twoRune  = "200000000"
)

Then reference oneRune, twoRune throughout.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 484b262 and dd4add7.

📒 Files selected for processing (5)
  • RESOURCES.md (1 hunks)
  • cmd/generator/main.go (2 hunks)
  • protocol/registry.go (2 hunks)
  • thorchain/protocol.go (1 hunks)
  • thorchain/thorchain_test.go (1 hunks)
✅ Files skipped from review due to trivial changes (2)
  • protocol/registry.go
  • RESOURCES.md
🚧 Files skipped from review as they are similar to previous changes (1)
  • thorchain/protocol.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
cmd/generator/main.go (2)
thorchain/chain.go (1)
  • NewThorchain (131-133)
chain/registry.go (1)
  • RegisterChain (65-71)

Comment on lines +51 to +54
// Register Thorchain
thorchainChain := thorchain.NewThorchain()
chain.RegisterChain(thorchainChain)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

RegisterChain can panic – bubble the error instead

chain.RegisterChain panics on failure; wrapping it inside a helper that already returns error defeats the purpose of the error return from registerChains.
Consider registering directly and propagating the error (mirrors what you already do for ethereum.InitEthereum):

-// Register Thorchain
-thorchainChain := thorchain.NewThorchain()
-chain.RegisterChain(thorchainChain)
+// Register Thorchain
+if err := chain.DefaultRegistry.Register(thorchain.NewThorchain()); err != nil {
+    return fmt.Errorf("error registering Thorchain: %w", err)
+}

This keeps the CLI from crashing unexpectedly and lets callers decide how to handle failures.

📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Register Thorchain
thorchainChain := thorchain.NewThorchain()
chain.RegisterChain(thorchainChain)
// Register Thorchain
if err := chain.DefaultRegistry.Register(thorchain.NewThorchain()); err != nil {
return fmt.Errorf("error registering Thorchain: %w", err)
}
🤖 Prompt for AI Agents
In cmd/generator/main.go around lines 51 to 54, the call to chain.RegisterChain
can panic on failure, but currently it is called directly without error
handling. Modify the code to call RegisterChain in a way that captures any error
it produces instead of panicking, then propagate this error up by returning it
from the registerChains function. This approach aligns with how
ethereum.InitEthereum errors are handled and prevents the CLI from crashing
unexpectedly, allowing callers to handle errors gracefully.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (2)
thorchain/memo.go (2)

12-14: Consider the purpose of commented-out memo types.

The commented-out memo types (ADD, WITHDRAW, DONATE) suggest incomplete functionality. If these are planned for future implementation, consider adding a TODO comment explaining the roadmap.

 const (
 	MemoTypeSwap MemoType = "SWAP"
-	// MemoTypeAdd      MemoType = "ADD"
-	// MemoTypeWithdraw MemoType = "WITHDRAW"
-	// MemoTypeDonate   MemoType = "DONATE"
+	// TODO: Implement additional memo types in future versions
+	// MemoTypeAdd      MemoType = "ADD"
+	// MemoTypeWithdraw MemoType = "WITHDRAW"
+	// MemoTypeDonate   MemoType = "DONATE"
 	MemoTypeTransfer MemoType = "TRANSFER"
 	MemoTypeUnknown  MemoType = "UNKNOWN"
 )

92-99: Consider expanding validation for production use.

The current validation only checks for missing assets in swap memos. Consider adding validation for other critical fields like destination addresses for swap operations.

 func (p *ParsedMemo) Validate() error {
 	if p.Type == MemoTypeSwap && p.Asset == "" {
 		return fmt.Errorf("swap memo missing target asset")
 	}
+	
+	if p.Type == MemoTypeSwap && p.DestAddr == "" {
+		return fmt.Errorf("swap memo missing destination address")
+	}

 	return nil
 }
📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between c9d8da9 and c389e45.

📒 Files selected for processing (3)
  • thorchain/decode.go (1 hunks)
  • thorchain/decode_test.go (1 hunks)
  • thorchain/memo.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • thorchain/decode_test.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
thorchain/decode.go (2)
thorchain/memo.go (4)
  • ParsedMemo (20-30)
  • MemoType (8-8)
  • MemoTypeTransfer (15-15)
  • ParseThorchainMemo (33-61)
types/transaction.go (1)
  • DecodedTransaction (7-26)
🔇 Additional comments (10)
thorchain/memo.go (2)

32-61: Memo parsing logic is well-structured and handles edge cases gracefully.

The parsing logic correctly handles empty memos, validates structure, and gracefully degrades for unknown operations. The default type assignment to MemoTypeTransfer for empty memos is appropriate.


64-80: Parsing logic is robust with proper bounds checking.

The parseSwapMemo method properly validates array bounds before accessing elements, preventing potential panics from malformed memo strings.

thorchain/decode.go (8)

21-26: Network configuration constants are well-defined.

The constants provide clear bounds for address validation and memo length limits, which align with Thorchain network specifications.


28-44: Comprehensive transaction structure with proper field organization.

The ParsedThorchainTransaction struct effectively captures all necessary transaction details including Cosmos-specific fields (sequence, accountNum) and Thorchain-specific fields (parsedMemo, denom).


142-156: Good input validation and error handling.

The function properly handles hex prefix removal, validates input format, and checks minimum transaction size. This addresses previous concerns about empty hex string validation.


164-189: Proper Cosmos SDK transaction decoding implementation.

The implementation correctly uses tx.TxRaw for initial decoding and then properly unmarshals the body and auth info components. This addresses the previous issue with incorrect protobuf target usage.


201-214: Robust gas price calculation with proper validation.

The gas price calculation includes proper validation for division by zero and denomination verification, addressing previous concerns about brittle gas price computation.


280-313: Comprehensive address validation using bech32 library.

The address validation now properly uses the bech32 library for checksum validation, addressing the previous concern about ignoring bech32 checksum validation.


324-352: Well-designed amount validation with reasonable bounds.

The validation includes proper nil checks, positivity verification, and reasonable maximum limits per denomination. The approach of setting different limits for different tokens is appropriate.


221-254: No Action Needed: Memo Parsing Integration Verified

The ParseThorchainMemo function is invoked on cosmosSDKTx.Body.Memo and assigned to the parsed.parsedMemo field. The ParsedThorchainTransaction struct declares parsedMemo *ParsedMemo and exposes it via GetParsedMemo(). All wiring between decode.go and memo.go is correct, so no changes are required.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
testdata/thorchain_test_helper.go (1)

121-218: Consider improving error handling in transaction generation functions.

While the transaction generation functions are well-implemented and follow the existing pattern, the panic behavior could be improved for better robustness in testing environments.

Consider refactoring to return errors instead of panicking:

func ValidRuneTransferMaxTxHex() string {
	params := TxParams{
		FromAddr:  "thor1jkndhpfauwmtn2uk7ytmruu0yywz5e66mq94e2",
		ToAddr:    "thor1c3xl04vgw7f7q9xrmqc4llr6s9l8p7qmgqwzsl",
		Amount:    10000000000, // 100 RUNE
		Denom:     "rune",
		Memo:      "Max allowed transfer",
		GasLimit:  200000,
		FeeAmount: 2000000,
		Sequence:  1,
	}

	txHex, err := GenerateThorchainTx(params)
	if err != nil {
-		panic(fmt.Sprintf("Failed to generate valid RUNE transfer tx: %v", err))
+		// Log error and return a fallback or handle gracefully
+		return ""
	}
	return txHex
}

However, if maintaining consistency with existing patterns is preferred, the current implementation is acceptable.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 49b709d and 6419ff3.

📒 Files selected for processing (4)
  • engine/engine_test.go (1 hunks)
  • testdata/payroll.json (1 hunks)
  • testdata/thorchain_test_helper.go (1 hunks)
  • thorchain/memo.go (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
  • thorchain/memo.go
🧰 Additional context used
🧬 Code Graph Analysis (1)
engine/engine_test.go (1)
testdata/thorchain_test_helper.go (5)
  • ValidRuneTransferMaxTxHex (121-138)
  • ValidRuneTransferUnderLimitTxHex (141-158)
  • ExceedAmountRuneTransferTxHex (161-178)
  • InvalidRecipientRuneTransferTxHex (181-198)
  • InvalidDenomTCYTransferTxHex (201-218)
🔇 Additional comments (6)
engine/engine_test.go (1)

99-129: Excellent comprehensive test coverage for Thorchain integration.

The new test vectors provide thorough coverage of Thorchain RUNE transfer scenarios, including both positive and negative test cases that align perfectly with the policy constraints in payroll.json. The implementation follows the established pattern and integrates well with the existing test framework.

The test cases cover all critical scenarios:

  • Valid transfers at the maximum limit (100 RUNE)
  • Valid transfers under the limit (50 RUNE)
  • Invalid transfers exceeding the limit (150 RUNE)
  • Invalid recipient addresses
  • Invalid token denominations (TCY instead of RUNE)
testdata/payroll.json (1)

55-80: Well-structured policy rule for RUNE transfers.

The new policy rule is properly formatted and follows the established pattern of existing rules in the file. The constraints are well-defined:

  • Fixed recipient address ensures transfers only go to the intended recipient
  • Maximum amount of 10,000,000,000 base units (100 RUNE) provides reasonable spending limits
  • The resource identifier "thorchain.rune.transfer" follows the expected naming convention

The rule integrates seamlessly with the existing policy structure and aligns perfectly with the test scenarios in the engine tests.

testdata/thorchain_test_helper.go (4)

17-36: Well-designed data structures for test parameter management.

The TxParams and TestCase structs provide a clean and comprehensive way to manage test scenarios. The structure allows for easy parameterization of different test cases while maintaining clarity about what each test is validating.


37-116: Comprehensive test case coverage with clear documentation.

The predefined test cases provide excellent coverage of different scenarios and align perfectly with the policy constraints in payroll.json. Each test case is well-documented with clear descriptions of the expected behavior.

The test scenarios cover:

  • Boundary conditions (exactly at limit, under limit, over limit)
  • Invalid recipient addresses
  • Invalid token denominations

This thorough coverage will help ensure the Thorchain integration is robust and handles edge cases properly.


220-281: Robust transaction generation using proper Cosmos SDK patterns.

The GenerateThorchainTx function demonstrates excellent use of the Cosmos SDK for transaction construction. The implementation correctly:

  • Sets up the codec with proper interface registration
  • Creates valid MsgSend messages with proper coin types
  • Constructs complete transaction structures with body, auth info, and fees
  • Handles encoding and hex conversion properly

The transaction structure will be compatible with Cosmos SDK-based parsing and validation logic.


283-301: Useful debugging utilities for test development.

The PrintTestCase and formatAmount helper functions provide valuable debugging capabilities that will be helpful during development and troubleshooting. The formatted output includes all relevant transaction details and makes it easy to understand what each test case is validating.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants