test: add bridge call unit test #951
Conversation
WalkthroughThis pull request introduces comprehensive modifications to the crosschain bridge call functionality across multiple files. The changes focus on enhancing token handling, fee management, and testing for native coins, ERC20 tokens, and IBC tokens. The modifications span testing frameworks, contract implementations, and keeper logic, with a particular emphasis on improving the flexibility and robustness of bridge call operations. Changes
Sequence DiagramsequenceDiagram
participant Sender
participant CrosschainContract
participant BridgeFeeQuote
participant TokenContract
participant CrosschainModule
Sender->>CrosschainContract: bridgeCall()
CrosschainContract->>BridgeFeeQuote: Get fee quote
BridgeFeeQuote-->>CrosschainContract: Return fee details
alt Fee token is origin token
CrosschainContract->>CrosschainContract: Validate msg.value
else Fee token is different
CrosschainContract->>TokenContract: Transfer fee
end
CrosschainContract->>CrosschainModule: Execute bridge call
Possibly related PRs
Suggested labels
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 (6)
precompiles/vm_caller.go (2)
50-58: Enhanced error handling for EVM revertsThis block successfully unpacks the revert message for better error visibility. As an enhancement, consider logging or preserving the original error message when unpacking fails, to provide full debugging context. Otherwise, this approach significantly improves error transparency.
74-74: Clarify and refine the commentThe comment “// the error handling is done in the caller” is correct. For improved readability, you might add more context about how errors are captured in the returned struct and processed downstream.
x/crosschain/keeper/bridge_fee.go (1)
Line range hint
31-39: Enhance error handling and observability in TransferBridgeFee.Consider adding structured logging for important operations and improving error context:
func (k Keeper) TransferBridgeFee(ctx sdk.Context, caller contract.Caller, from, to common.Address, bridgeFee *big.Int, bridgeTokenName string) error { + k.Logger(ctx).Debug("transferring bridge fee", + "from", from.String(), + "to", to.String(), + "amount", bridgeFee.String(), + "token", bridgeTokenName) if bridgeTokenName == fxtypes.DefaultDenom { fees := sdk.NewCoins(sdk.NewCoin(fxtypes.DefaultDenom, sdkmath.NewIntFromBigInt(bridgeFee))) - return k.bankKeeper.SendCoins(ctx, from.Bytes(), to.Bytes(), fees) + if err := k.bankKeeper.SendCoins(ctx, from.Bytes(), to.Bytes(), fees); err != nil { + return fmt.Errorf("failed to send native coins: %w", err) + } + return nil } erc20Token, err := k.erc20Keeper.GetERC20Token(ctx, bridgeTokenName) if err != nil { - return err + return fmt.Errorf("failed to get ERC20 token %s: %w", bridgeTokenName, err) } erc20TokenKeeper := contract.NewERC20TokenKeeper(caller) _, err = erc20TokenKeeper.Transfer(ctx, erc20Token.GetERC20Contract(), from, to, bridgeFee) - return err + if err != nil { + return fmt.Errorf("failed to transfer ERC20 token %s: %w", bridgeTokenName, err) + } + return nil }solidity/contracts/test/CrosschainTest.sol (1)
88-98: Handle ETH-based fees with caution.
Subtracting the fee amount frommsgValueis logically sound if the user has sent enough native currency to cover it. However, if the logic or contract address changes, code modifications may be required. Additional checks or explicit event logging may help diagnose user errors (e.g., not sending sufficient funds).x/crosschain/keeper/bridge_call_in_test.go (1)
63-63: Check existence of ERC20 token.
The subsequent call toGetERC20Tokenensures the token was properly registered byAddBridgeTokenExecuted. If your unit tests only validate happy paths, consider tests for failure scenarios where the token might not register correctly.tests/contract/crosschain_test.sol.go (1)
205-220: Consider adding test coverage for the new BRIDGE_FEE_ADDRESS function.
This function has been introduced to retrieve a fee address. Ensure it’s invoked and validated in your test suite to confirm correct behavior when bridging tokens, especially under various fee scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
precompiles/crosschain/bridge_call_test.go(2 hunks)precompiles/crosschain/contract_test.go(3 hunks)precompiles/crosschain/crosschain_test.go(3 hunks)precompiles/vm_caller.go(2 hunks)solidity/contracts/test/CrosschainTest.sol(2 hunks)tests/contract/crosschain_test.sol.go(2 hunks)testutil/helpers/erc20_token_suite.go(2 hunks)x/crosschain/keeper/bridge_call_in_test.go(1 hunks)x/crosschain/keeper/bridge_fee.go(1 hunks)x/evm/keeper/contract_code_test.go(1 hunks)x/evm/keeper/msg_server_test.go(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (36)
precompiles/vm_caller.go (4)
41-42: Parameter rename tocABI: Good for clarityRenaming
abitocABIhelps avoid overshadowing the imported package name. This change clarifies the code and is consistent with common Go naming practices.
46-49: Check for potential nil behavior when returningres, errWhen
v.ExecuteEVMfails, the code returns bothresanderr. Ensure thatresis always non-nil in error scenarios so the caller can safely inspect any partial state. Ifrescould be nil, consider returning(nil, err)or ensuringExecuteEVMnever returns a nil response.
59-59: Clean return aligns with caller’s error handlingReturning
res, nilcentralizes error handling in the caller, which is a clean way to manage EVM execution results. This is consistent with the preceding condition checks.
79-79: Return signature is consistent with delegated error handlingAlways returning
nilerror here ensures that any VM errors are encapsulated in theMsgEthereumTxResponse. This design elegantly defers all error handling to the caller.x/crosschain/keeper/bridge_fee.go (2)
31-31: Verify case sensitivity implications for bridge token comparison.The removal of
strings.ToUpper()makes the comparison betweenbridgeTokenNameandfxtypes.DefaultDenomcase-sensitive. This could potentially break existing functionality ifbridgeTokenNameis provided in a different case.Let's verify the case sensitivity of
fxtypes.DefaultDenomand its usage:
Line range hint
31-39: Ensure comprehensive test coverage for fee handling paths.Since this PR focuses on adding bridge call unit tests, ensure test coverage for:
- Case-sensitive token name handling
- Failed fee transfers for both native and ERC20 tokens
- Quote validation edge cases
- Fee collector address validation
Let's verify the test coverage:
Also applies to: 66-77
✅ Verification successful
Test coverage for fee handling paths is comprehensive
The codebase includes thorough test coverage for:
- Fee transfers across all token types (native, ERC20, IBC)
- Quote validation and fee collection
- Token name handling and validation
- Fee collector address verification
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check test coverage for bridge fee handling # Find all test files related to bridge fees echo "=== Bridge fee test files ===" fd -e go "bridge.*test" -x echo "=== {} ===" \; -x cat {} # Find test cases for fee transfers echo -e "\n=== Fee transfer test cases ===" rg --type go "Test.*Fee.*Transfer|Test.*Transfer.*Fee" -A 5 # Find test cases for quote handling echo -e "\n=== Quote handling test cases ===" rg --type go "Test.*Quote" -A 5Length of output: 22781
solidity/contracts/test/CrosschainTest.sol (5)
8-8: Ensure interface compatibility.
The new import statement forIBridgeFeeQuotelooks fine. Verify that the interface declarations match actual contract implementation inIBridgeFee.solto avoid compilation or runtime issues.
16-17: Validate constant address correctness.
DefiningBRIDGE_FEE_ADDRESSat compile time is acceptable, but ensure that this address is correct for the target environment, and that referencing it from external calls is secure. If the fee contract isn’t deployed at this address in all environments (e.g. dev, test, production), consider a configurable approach.
83-87: Retrieve fee quote carefully.
Storing the returned quote directly intoinfois fine. However, consider validating the quote (e.g., token name) to ensure it isn’t empty or malformed before proceeding, preventing potential out-of-bounds or unexpected behavior later in the function.
99-106: Avoid potential approval confusion.
This block transfers ERC20 tokens from the user for fee purposes. Confirm that a priorapprovecall by the user for this contract is enforced. Otherwise, this transfer will revert. If approvals are managed behind the scenes, ensure good documentation to prevent user confusion.
109-109: Finalize bridge call with adjusted msgValue.
UsingmsgValueinstead of the originalmsg.valueensures the correct amount is forwarded. This appears correct; just ensure that no underflow conditions can arise.x/crosschain/keeper/bridge_call_in_test.go (1)
56-61: Structured token claim usage.
Switching toAddBridgeTokenExecutedwith a structuredMsgBridgeTokenClaimis more robust, encapsulating properties likeName,Symbol, and so on. Confirm that existing tests validate corner cases, such as conflicting token contracts or invalid decimals.x/evm/keeper/msg_server_test.go (1)
51-51: Expanded Mint signature.
The extra parameter forMintclarifies the “from” address origin. Ensure that the chosen “from” address is valid (e.g., an owner or minter role in the ERC20 contract). Add tests verifying reverts if the “from” address lacks permission.testutil/helpers/erc20_token_suite.go (2)
118-119: Check for consistent usage offromparameter.
This new parameter aligns with the refactoring where the mint initiator is explicitly specified. Usage appears correct; ensure all invocations in the codebase pass the correctfromaddress.
164-164: Clarify intent of minting from the signer to itself.
Minting tokens from and to the same address is unusual. Confirm this is valid for unit tests and does not cause logical inconsistencies.precompiles/crosschain/crosschain_test.go (5)
18-18: Approved import addition.
No issues identified with bringing incrosschaintypes.
Line range hint
32-69: Thorough test for native coin bridging.
This test method covers the minting, approval, crosschain call, and final balance checks. The logic looks consistent, ensuring end-to-end coverage.
71-107: Solid coverage for bridging native ERC20 tokens.
The test ensures that newly deployed ERC20 tokens can be recognized and transferred via the crosschain mechanism. Looks robust.
109-147: Comprehensive IBC token bridging test.
This function extends coverage to IBC tokens, verifying mint, approval, and crosschain bridging. All checks adhere to the same pattern established in prior tests.
Line range hint
150-169: Origin token bridging test logic.
The test effectively demonstrates bridging of a default symbol token, confirming that fee calculation and final balances match expectations. Code is consistent and clear.precompiles/crosschain/contract_test.go (8)
32-33: Additional fields in CrosschainPrecompileTestSuite.
The newbridgeFeeSuiteanderc20TokenSuitefields provide complementary helpers, enhancing test organization.
63-63: Initialization of ERC20TokenSuite.
No concerns here; instantiatingerc20TokenSuiteis consistent with existing patterns for suite-based tests.
105-109:GetERC20Tokenmethod.
Retrieves an ERC20 token correctly, handling potential errors. Straightforward approach.
111-115:GetBridgeTokenmethod.
Fetches a bridge token with error checks. Implementation is aligned with the existing keeper usage.
117-132: EnhancedAddBridgeTokenmethod.
Supports both native and IBC tokens with the optionalisIBCargument. This modular approach allows for flexible token registration. Ensure that extended usage is thoroughly tested.
136-145:AddNativeERC20ToEVMmethod.
Mints tokens to the signer's address if the token is recognized as native ERC20, or else uses the module minter. Straightforward logic, well aligned with test flow.
147-156:AddNativeCoinToEVMmethod.
Adds native coins to EVM by callingAddNativeERC20ToEVMthen minting tokens for the crosschain module if not IBC. This method elegantly centralizes multiple steps.
158-176:NewBridgeCallArgsmethod.
Constructs valid arguments for a bridge call, with checks for zero addresses or nil amounts. This param builder approach improves clarity and reduces duplication.precompiles/crosschain/bridge_call_test.go (5)
20-20: Minor import addition.
No concerns with addingcrosschaintypes.
183-223: Test bridging native coins viaTestContract_BridgeCall_NativeCoin.
Validates approval amounts, bridging fees, and final balances. Methodically checks all relevant modules and logs. Good coverage.
225-265: Test bridging native ERC20 withTestContract_BridgeCall_NativeERC20.
Ensures deployment, minting, approval, and bridging remain consistent with the new flows. The test is thorough.
267-306: Test bridging IBC tokens withTestContract_BridgeCall_IBCToken.
Mirrors logic in the native coin tests while addressing IBC nuances. The code consistently checks final balances and fees.
312-329: Coverage for origin token bridging inTestContract_BridgeCall_OriginToken.
Handles default symbol bridging, verifying fees and final coin distribution. The approach is consistent and adequately tested.x/evm/keeper/contract_code_test.go (1)
68-68: Validate the identical 'from' and 'to' addresses in Mint call.
This call uses the same address (erc20Suite.HexAddress()) for both the ‘from’ and ‘to’ parameters, which can be confusing. Confirm that this usage is intentional and verify it does not introduce logical errors in mint tracking or ledger updates.tests/contract/crosschain_test.sol.go (2)
34-35: Ensure ABI and bytecode consistency with the updated Solidity contract.
The newly introducedBRIDGE_FEE_ADDRESSfunction must match the contract’s actual implementation. Confirm that the updated ABI and bytecode accurately reflect any parameter changes or new functions for correct contract interactions.
222-235: Same function is repeated in session contexts.
This appears to be the session-based variant of theBRIDGE_FEE_ADDRESS()logic, mirroring the existing pattern. No issues are apparent; the duplication is consistent with standard session binding.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Changes