test: crosschain origin token #932
Conversation
WalkthroughThis pull request introduces several modifications across multiple files in the codebase, focusing on crosschain functionality, precompile implementations, and testing utilities. The changes primarily involve enhancing error handling, updating method signatures, and refactoring initialization processes. Key areas of modification include crosschain transaction handling, bridge token management, and test suite improvements. The modifications aim to provide more flexible and robust implementation of crosschain operations and associated testing mechanisms. Changes
Sequence DiagramsequenceDiagram
participant Client
participant CrosschainPrecompile
participant Keeper
participant BridgeModule
Client->>CrosschainPrecompile: Initiate Crosschain Transaction
CrosschainPrecompile->>Keeper: Apply Contract
Keeper->>BridgeModule: Build Outgoing Tx Batch
BridgeModule-->>Keeper: Return Batch Result
Keeper-->>CrosschainPrecompile: Return Transaction Response
CrosschainPrecompile-->>Client: Confirm Transaction
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
precompiles/crosschain/crosschain_test.go (1)
39-41: Add test cases with different amount and fee combinations.The test uses hardcoded values (amount=1, fee=1). Consider adding test cases with:
- Zero amount/fee
- Large numbers
- Amount less than fee
contract/crosschain_precompile.go (1)
79-86: Consider adding input validation.The
Crosschainmethod could benefit from additional validation:
- Check for nil pointers in
valueandargs- Validate that amount and fee are not negative
- Add specific error types for different failure scenarios
Example validation:
func (k CrosschainPrecompileKeeper) Crosschain(ctx context.Context, value *big.Int, from common.Address, args CrosschainArgs) (*evmtypes.MsgEthereumTxResponse, error) { + if value == nil || args.Amount == nil || args.Fee == nil { + return nil, sdkerrors.ErrInvalidRequest.Wrap("nil values not allowed") + } + if args.Amount.Sign() < 0 || args.Fee.Sign() < 0 { + return nil, sdkerrors.ErrInvalidRequest.Wrap("negative values not allowed") + } res, err := k.ApplyContract(ctx, from, k.contractAddr, value, k.abi, "crossChain", args.Token, args.Receipt, args.Amount, args.Fee, args.Target, args.Memo) if err != nil { return nil, err } return unpackRetIsOk(k.abi, "crossChain", res) }precompiles/crosschain/contract_test.go (1)
95-107: Document the special case handling for default symbol.The method includes special handling for the default symbol, setting
isNativeOrOrigintofalse. This behavior should be documented to prevent confusion.func (suite *CrosschainPrecompileTestSuite) AddBridgeToken(symbolOrAddr string, isNativeOrOrigin bool) common.Address { + // For default symbol, isNativeOrOrigin is always set to false regardless of input keeper := suite.App.Erc20Keeper var erc20Token erc20types.ERC20Token var err error
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
app/init.go(1 hunks)client/client_test.go(2 hunks)contract/crosschain_precompile.go(1 hunks)precompiles/crosschain/contract_test.go(1 hunks)precompiles/crosschain/crosschain_test.go(2 hunks)testutil/helpers/base_suite.go(1 hunks)testutil/helpers/crosschain_precompile_suite.go(1 hunks)x/crosschain/keeper/bridge_call_out.go(1 hunks)x/crosschain/keeper/outgoing_tx.go(1 hunks)x/erc20/keeper/genesis.go(0 hunks)
💤 Files with no reviewable changes (1)
- x/erc20/keeper/genesis.go
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (9)
precompiles/crosschain/crosschain_test.go (2)
48-50: Verify module address balance change.The test correctly verifies balance changes but could be more explicit about the expected state:
- Signer's balance decreases by 2 (amount + fee)
- Module receives the full transfer amount
37-38: Consider using a valid token address instead of empty address.Using an empty address (
common.Address{}) for the token parameter might not cover all edge cases. Consider testing with both empty and valid token addresses to ensure robust functionality.testutil/helpers/crosschain_precompile_suite.go (1)
73-77: Implementation follows established patterns.The new
Crosschainmethod maintains consistency with other methods in the suite:
- Proper error handling using
requireError- Clean parameter passing
- Clear return type
precompiles/crosschain/contract_test.go (1)
108-108: Verify external address generation.The method uses
helpers.GenExternalAddr(suite.chainName)for the bridge token. Ensure this generates valid addresses for all supported chains.✅ Verification successful
External address generation implementation verified
The
GenExternalAddrimplementation properly handles address generation for different chain types through a router system, with specific implementations for Ethereum and Tron addresses. The generated addresses are cryptographically secure and properly formatted according to chain-specific requirements.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check external address generation across supported chains rg "GenExternalAddr" -A 2Length of output: 11183
Script:
#!/bin/bash # Check implementation of GenExternalAddr and related functions ast-grep --pattern 'func GenExternalAddr($$$) $$$' ast-grep --pattern 'func GenHexAddress($$$) $$$' rg "ExternalAddrToStr" -A 2Length of output: 3696
x/crosschain/keeper/outgoing_tx.go (1)
22-22: Improved error handling with descriptive message.The error wrapping provides better context about the failure point in the bridge fee quote retrieval.
testutil/helpers/base_suite.go (1)
183-183: LGTM! Using string comparison for balance assertions.Converting amounts to strings before comparison is a safer approach to handle large numbers and prevent precision loss.
x/crosschain/keeper/bridge_call_out.go (1)
245-248: Simplified error handling in deprecated function.The change improves error propagation by directly returning the error from
BuildOutgoingTxBatch. This aligns with Go's idiomatic error handling.client/client_test.go (1)
192-192: Verify the account sequence number changes.The test expectations for account sequence numbers have been decremented:
- Line 192: Changed from
21+ito20+i- Line 227: Changed from
24to23Please ensure these changes align with any modifications in transaction processing logic.
Also applies to: 227-227
✅ Verification successful
Test expectations correctly adjusted to match account numbering
The sequence number changes align with the documented zero-based account numbering scheme (accounts 0-12) and don't reflect changes in transaction processing logic. This is a test correction to match the actual system behavior.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any changes in transaction processing that might affect sequence numbers # Search for sequence number handling in transaction processing rg -A 5 "GetSequence|sequence" --type go # Look for any recent changes in account handling git log -p -- "**/account*.go"Length of output: 6741
Script:
#!/bin/bash # Get the complete test case and surrounding context rg -B 10 -A 10 "NewBaseAccount\(toAddress, nil, uint64\(20\+i\)" client/client_test.go # Check recent changes in the test file git log -p client/client_test.goLength of output: 859
app/init.go (1)
28-33:⚠️ Potential issueAdd validation and error handling for chain names.
The current implementation has several potential issues:
MustStrToByte32could panic if chain names exceed 32 bytes.- Using the same
DefaultDenomfor all chains might not be appropriate if different chains require different denominations.- No validation of chain names from
GetSupportChains().Consider this safer implementation:
bridgeDenoms := make([]contract.BridgeDenoms, 0) for _, chainName := range fxtypes.GetSupportChains() { + // Validate chain name length + if len(chainName) > 32 { + return fmt.Errorf("chain name %s exceeds 32 bytes", chainName) + } + + // Get chain-specific denom (assuming you add this function) + denom, err := fxtypes.GetChainDenom(chainName) + if err != nil { + return fmt.Errorf("failed to get denom for chain %s: %w", chainName, err) + } + bridgeDenoms = append(bridgeDenoms, contract.BridgeDenoms{ - ChainName: contract.MustStrToByte32(chainName), - Denoms: []common.Hash{contract.MustStrToByte32(fxtypes.DefaultDenom)}, + ChainName: contract.StrToByte32(chainName), + Denoms: []common.Hash{contract.StrToByte32(denom)}, }) }Let's verify the chain names and their lengths:
Summary by CodeRabbit
Based on the comprehensive summary, here are the release notes:
New Features
Bug Fixes
Tests
Refactor