test: precompile bridge call #889
Conversation
WalkthroughThis pull request introduces enhancements to the cross-chain bridge call testing infrastructure across multiple files. The changes primarily focus on expanding test capabilities for bridge call functionality, modifying contract interfaces, and updating test suite initialization methods. Key modifications include adding a new test function for bridge calls, changing function visibility in Solidity contracts, updating contract metadata, and streamlining test suite initialization processes. Changes
Sequence DiagramsequenceDiagram
participant Sender
participant CrosschainContract
participant BridgeToken
participant ModuleKeeper
Sender->>CrosschainContract: Initiate Bridge Call
CrosschainContract->>BridgeToken: Verify Token
BridgeToken-->>CrosschainContract: Token Verified
CrosschainContract->>ModuleKeeper: Mint/Transfer Tokens
ModuleKeeper-->>CrosschainContract: Token Transaction Complete
CrosschainContract-->>Sender: Bridge Call Response
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: 2
🧹 Nitpick comments (2)
precompiles/crosschain/bridge_call_test.go (2)
188-190: Verify block height initializationSetting last observed block height to 100. Consider parameterizing this value if it's significant for the test.
205-207: Consider adding more balance checksWhile the final balance check is good, consider:
- Adding initial balance verification before the bridge call
- Verifying the module's token balance
func (suite *CrosschainPrecompileTestSuite) TestContract_BridgeCall() { // ... existing setup ... + initialBalance, err := erc20TokenKeeper.BalanceOf(suite.Ctx, tokenAddr, suite.GetSender()) + suite.Require().NoError(err) + suite.Require().Equal(big.NewInt(100), initialBalance) + txResponse := suite.BridgeCall(...) // ... existing verification ... + moduleAddress := common.BytesToAddress(authtypes.NewModuleAddress(erc20types.ModuleName).Bytes()) + moduleBalance, err := erc20TokenKeeper.BalanceOf(suite.Ctx, tokenAddr, moduleAddress) + suite.Require().NoError(err) + suite.Require().Equal(big.NewInt(101), moduleBalance) // 100 initial + 1 transferred
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
precompiles/crosschain/bridge_call_test.go(2 hunks)precompiles/crosschain/contract_test.go(5 hunks)solidity/contracts/test/CrosschainTest.sol(1 hunks)tests/contract/crosschain_test.sol.go(2 hunks)tests/integration/integration_test.go(1 hunks)testutil/helpers/bank_precompile_suite.go(1 hunks)testutil/helpers/crosschain_precompile_suite.go(1 hunks)testutil/helpers/staking_precompile_suite.go(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- testutil/helpers/staking_precompile_suite.go
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: test
- GitHub Check: Mergify Merge Protections
- GitHub Check: Summary
🔇 Additional comments (11)
testutil/helpers/bank_precompile_suite.go (1)
21-21: Clean initialization pattern!Good improvement in combining the ContractBaseSuite creation with WithContract call. This makes the code more concise while maintaining immutability.
testutil/helpers/crosschain_precompile_suite.go (1)
22-22: Consistent initialization pattern applied!Good to see the same clean initialization pattern being consistently applied across different precompile suites. This maintains code consistency and improves readability across the codebase.
tests/contract/crosschain_test.sol.go (2)
34-35: Verify the updated ABI and binary dataThe
ABIandBinfields inCrosschainTestMetaDatahave been updated to include the newbridgeCallfunction. Ensure that the generated ABI and binary accurately reflect the changes in the Solidity contract to prevent any mismatches between the contract and its Go bindings.
329-349: NewBridgeCallmethods addedThe
BridgeCallmethods have been added to theCrosschainTestTransactor,CrosschainTestSession, andCrosschainTestTransactorSessiontypes. The method signatures align with the contract's ABI, and the implementations appear correct.solidity/contracts/test/CrosschainTest.sol (1)
81-81:⚠️ Potential issueAssess the change in function visibility from
internaltoexternalThe
bridgeCallfunction's visibility has been changed frominternaltoexternal. Exposing this function externally allows it to be called from outside the contract. Ensure this change is intentional and does not introduce security risks. Consider implementing access control modifiers, such asonlyOwneror custom modifiers, to restrict access if necessary.precompiles/crosschain/contract_test.go (2)
32-34: Initialize test suite with specifiedcrosschainAddrThe test suite is now initialized with an explicit
crosschainAddr, improving clarity and consistency in the test setup.
66-76: Enhance code readability with new helper methodsThe addition of
GetSender()andIsCallPrecompile()methods encapsulates logic for determining the sender and checking if the call is to a precompile. This improves code readability and maintainability.tests/integration/integration_test.go (1)
109-111: Verify test dependencies and stabilityThe previously disabled staking contract tests have been re-enabled. Please ensure:
- These tests don't have dependencies on the preceding tests in the execution sequence
- The tests are stable and don't introduce flakiness to the test suite
Run this script to check for potential test dependencies and stability issues:
✅ Verification successful
Tests are properly isolated and independent
The re-enabled tests are well-designed with:
- Independent test state using fresh keys and accounts for each test
- Proper cleanup of shared resources
- No dependencies on test execution order
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential test dependencies and stability issues # Look for shared state setup/teardown and test ordering requirements # Search for test setup/teardown methods echo "Checking test setup/teardown methods:" rg -A 5 "func.*Setup|func.*TearDown" tests/integration/ # Search for potential shared state echo "Checking for shared state usage:" rg "suite\.(StakingContractTest|StakingSharesContractTest|StakingPrecompileRedelegateByContractTest)" tests/integration/Length of output: 1397
Script:
#!/bin/bash # Check test implementations and state management # Look for the test implementations echo "Test implementations:" rg -B 2 -A 10 "func.*\(suite \*IntegrationTest\) (StakingContractTest|StakingSharesContractTest|StakingPrecompileRedelegateByContractTest)" tests/integration/ # Check for shared state variables echo -e "\nShared state variables:" rg "suite\." --context 2 tests/integration/integration_test.go | grep -v "func" # Check for network state modifications echo -e "\nNetwork state modifications:" rg "suite\.network\." tests/integration/integration_test.goLength of output: 7226
precompiles/crosschain/bridge_call_test.go (3)
8-18: LGTM! Dependencies are properly organized.The new imports are correctly added and organized by their respective domains (SDK, Ethereum, and internal packages).
191-204: LGTM! Bridge call test is well structured.The test properly:
- Sets up the test scenario with necessary parameters
- Executes the bridge call
- Verifies the transaction response and logs
180-187: Verify token minting behaviorThe test mints tokens both via ERC20TokenKeeper and MintTokenToModule. Please ensure:
- Both minting operations are necessary
- The amounts (100 units) are sufficient for the test scenarios
✅ Verification successful
Both minting operations are necessary and amounts are appropriate
The dual minting is correct as it maintains parity between ERC20 tokens and native coins, which is essential for bridge operations. The 100-unit amount provides sufficient balance for the test's bridge call that transfers 1 unit, with adequate buffer for potential additional test scenarios.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check token minting patterns across tests # Look for similar dual minting patterns that might indicate redundancy # Search for similar minting patterns echo "Checking minting patterns:" rg "MintTokenToModule.*NewCoin.*usdt" precompiles/ rg "erc20TokenKeeper.*Mint" precompiles/Length of output: 433
Script:
#!/bin/bash # Get broader context of the test function echo "=== Test function context ===" rg -B 10 -A 20 "MintTokenToModule.*NewCoin.*usdt" precompiles/crosschain/bridge_call_test.go echo -e "\n=== Other bridge tests with similar patterns ===" # Look for bridge-related test files fd "bridge.*test" precompiles/ --exec rg -l "MintTokenToModule|erc20TokenKeeper.*Mint" {}Length of output: 1633
Summary by CodeRabbit
New Features
Tests
Improvements