test(e2e): extend staking precompile tests #2830
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughWalkthroughThe changes introduce enhancements to the staking functionality within the codebase, including the addition of a new staking smart contract, improved end-to-end tests for staking through contracts, and modifications to existing staking methods. The updates aim to enhance the testing coverage and streamline interactions with the staking module, ensuring better integration and functionality for users. Changes
Assessment against linked issues
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
Documentation and Community
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2830 +/- ##
===========================================
+ Coverage 66.68% 66.94% +0.25%
===========================================
Files 370 370
Lines 20967 20965 -2
===========================================
+ Hits 13982 14035 +53
+ Misses 6345 6290 -55
Partials 640 640
|
skosito
requested review from
brewmaster012,
kingpinXD,
lumtis,
ws4charlie,
swift1337 and
fbac
as code owners
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 18
Outside diff range, codebase verification and nitpick comments (11)
e2e/contracts/teststaking/TestStaking.sol (2)
19-42: Interface Definition: IStakingThe interface methods are clearly defined, covering essential staking operations. Ensure that the interface is fully implemented in the contract that interacts with this precompile, and consider adding comments to each function to explain their purpose and usage further.
Consider adding detailed comments for each method in the
IStakinginterface to enhance code readability and maintainability.
44-47: Lack of Error Handling in WZETA Method ImplementationsThe
depositWZETAandwithdrawWZETAfunctions ine2e/contracts/teststaking/TestStaking.soldo not include error handling mechanisms. Consider implementing checks for successful execution or usingtry/catchto handle potential failures in these external calls. This will ensure more robust and reliable contract behavior.
e2e/contracts/teststaking/TestStaking.sol: Lines wheredepositWZETAandwithdrawWZETAare implemented.Analysis chain
Interface Definition: WZETA
The
WZETAinterface methods for deposit and withdrawal are succinct. Ensure that error handling is adequately addressed in the implementations of these methods to prevent issues related to token transfers.Verify the implementation of
WZETAmethods to ensure robust error handling:Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check the implementation of WZETA methods for error handling. # Test: Search for the implementation of WZETA methods. Expect: Proper error handling. rg --type solidity -A 5 $'function deposit' 'function withdraw'Length of output: 97
Script:
#!/bin/bash # Description: Check the implementation of WZETA methods for error handling. # Test: Search for the implementation of WZETA methods in Solidity files. Expect: Proper error handling. rg --type-add 'solidity:*.sol' --type solidity -A 5 'function deposit' rg --type-add 'solidity:*.sol' --type solidity -A 5 'function withdraw'Length of output: 8878
pkg/rpc/clients.go (1)
34-35: Addition of Staking client to Clients struct.The addition of the
Stakingfield to theClientsstruct is correctly implemented and aligns with the existing structure. However, there is a minor typo in the comment on line 34, which incorrectly refers to "Bank" instead of "Staking".Please correct the comment to accurately reflect the field it describes:
- // Bank is a github.com/cosmos/cosmos-sdk/x/staking/types QueryClient + // Staking is a github.com/cosmos/cosmos-sdk/x/staking/types QueryClientcontrib/localnet/orchestrator/start-zetae2e.sh (2)
Line range hint
11-21: Enhance error handling in version fetching.The
get_zetacored_versionfunction could benefit from more robust error handling to ensure that transient network issues or unexpected responses do not cause the script to fail silently or exit prematurely.Consider adding more detailed error logging and possibly a mechanism to handle unexpected responses more gracefully:
+ echo "Error fetching node info, attempt $attempt" + echo "Response: $node_info"
Line range hint
31-39: Improve error handling in Ether funding.The
fund_ethfunction currently does not handle potential errors from thegethcommand execution. It would be beneficial to add error handling to ensure that any issues during the transaction are caught and handled appropriately.Add error checking for the
gethcommand execution to improve reliability:+ if ! geth --exec "eth.sendTransaction({from: eth.coinbase, to: '${address}', value: web3.toWei(${ether}, 'ether')})" attach http://eth:8545 > /dev/null; then + echo "Error funding address $address" + return 1 + fie2e/contracts/teststaking/TestStaking.json (1)
19-24: ReviewdepositWZETAFunctionThe function is marked as
payablewhich is suitable for functions handling Ether transactions. However, it would benefit from additional comments explaining its purpose and behavior, especially since it has no outputs.cmd/zetae2e/local/local.go (1)
314-314: Missing Implementation forTestPrecompilesStakingThroughContractNameThe test
TestPrecompilesStakingThroughContractNameis referenced in the codebase but lacks an actual function implementation. This could be an oversight or an indication of an incomplete feature. Please verify whether this test is intended for future implementation or if it should be removed. If the test is necessary, ensure that it is implemented to maintain comprehensive test coverage.
- Location of Reference:
e2e/e2etests/e2etests.go- Action Required: Implement the missing test function or remove the reference if not needed.
Analysis chain
Approved: Addition of new staking precompile test.
The addition of
e2etests.TestPrecompilesStakingThroughContractNameto theprecompiledContractTestsslice correctly extends the testing coverage for staking functionality. Ensure that the implementation of this new test in thee2etestspackage is correct and integrates well with the existing testing framework.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the new staking precompile test. # Test: Search for the implementation of `TestPrecompilesStakingThroughContractName`. Expect: Implementation details in the `e2etests` package. ast-grep --lang go --pattern $'func TestPrecompilesStakingThroughContractName($_, $_) { $$$ }'Length of output: 97
Script:
#!/bin/bash # Search for the function name `TestPrecompilesStakingThroughContractName` across the entire codebase. rg 'TestPrecompilesStakingThroughContractName' -A 5Length of output: 1030
precompiles/staking/staking_test.go (4)
Line range hint
32-84: Refactor thesetupfunction for clarity and modularity.The
setupfunction has been significantly expanded to include new parameters for EVM and contract simulation. While this is necessary for the new testing capabilities, the function has become quite lengthy and complex. Consider refactoring it into smaller, more manageable functions. This could involve separating the creation of the mock EVM and contract into their own functions, which would improve readability and maintainability.Here's a proposed refactor:
+func createMockEVM(ctx sdk.Context, sdkKeepers keeper.SDKKeepers) *vm.EVM { + return vm.NewEVM( + vm.BlockContext{}, + vm.TxContext{}, + statedb.New(ctx, sdkKeepers.EvmKeeper, statedb.TxConfig{}), + ¶ms.ChainConfig{}, + vm.Config{}, + ) +} + +func createMockContract(address common.Address, contractAddress common.Address) *vm.Contract { + return vm.NewContract( + contractRef{address: address}, + contractRef{address: contractAddress}, + big.NewInt(0), + 0, + ) +} + func setup(t *testing.T) (sdk.Context, *Contract, abi.ABI, keeper.SDKKeepers, *vm.EVM, *vm.Contract) { var encoding ethermint.EncodingConfig appCodec := encoding.Codec ... - mockEVM := vm.NewEVM( - vm.BlockContext{}, - vm.TxContext{}, - statedb.New(ctx, sdkKeepers.EvmKeeper, statedb.TxConfig{}), - ¶ms.ChainConfig{}, - vm.Config{}, - ) - mockVMContract := vm.NewContract( - contractRef{address: common.Address{}}, - contractRef{address: ContractAddress}, - big.NewInt(0), - 0, - ) + mockEVM := createMockEVM(ctx, sdkKeepers) + mockVMContract := createMockContract(common.Address{}, ContractAddress) return ctx, contract, abi, sdkKeepers, mockEVM, mockVMContract }
Line range hint
101-123: Ensure consistent error handling in test cases.Several test cases use the pattern of setting up the test environment, performing actions, and then asserting conditions. It's crucial to ensure that error handling is consistent across all tests. For example, the test
Test_IStakingContractchecks various properties of the ABI but does not always assert the outcomes of actions likecontract.RequiredGas. Consistency in error handling and assertions will improve the reliability of the tests.Consider adding more detailed assertions and error checks where missing to ensure that each test case robustly verifies the expected outcomes.
Line range hint
223-320: Review error messages for clarity and accuracy.In the test cases for staking, such as
Test_Stake, the error messages should clearly reflect the expected conditions and outcomes. For instance, when a test expects a failure due to a missing validator, the error message should explicitly state this condition. This helps in understanding test failures quickly during debugging and maintenance.Review and update the error messages in the test cases to ensure they are descriptive and accurate, reflecting the specific failure conditions tested.
Line range hint
488-1232: Improve test setup reuse and error handling inTest_UnstakeandTest_MoveStake.The test cases for unstaking and moving stakes have a lot of repeated setup code, which could be refactored into helper functions to reduce duplication and improve maintainability. Additionally, the error handling in these tests should be reviewed to ensure that it is consistent and that all possible error conditions are appropriately tested.
Consider creating helper functions for common setup tasks and improving error handling as follows:
+func prepareStakingTest(t *testing.T, methodID abi.Method) (sdk.Context, *Contract, abi.ABI, keeper.SDKKeepers, *vm.EVM, *vm.Contract, []interface{}) { + ctx, contract, abi, sdkKeepers, mockEVM, mockVMContract := setup(t) + staker := sample.Bech32AccAddress() + stakerEthAddr := common.BytesToAddress(staker.Bytes()) + coins := sample.Coins() + err := sdkKeepers.BankKeeper.MintCoins(ctx, fungibletypes.ModuleName, coins) + require.NoError(t, err) + err = sdkKeepers.BankKeeper.SendCoinsFromModuleToAccount(ctx, fungibletypes.ModuleName, staker, coins) + require.NoError(t, err) + args := []interface{}{stakerEthAddr, sample.Validator(t, rand.New(rand.NewSource(42))).OperatorAddress, coins.AmountOf(config.BaseDenom).BigInt()} + return ctx, contract, abi, sdkKeepers, mockEVM, mockVMContract, args +} -ctx, contract, abi, sdkKeepers, mockEVM, mockVMContract := setup(t) -staker := sample.Bech32AccAddress() -stakerEthAddr := common.BytesToAddress(staker.Bytes()) -coins := sample.Coins() -err := sdkKeepers.BankKeeper.MintCoins(ctx, fungibletypes.ModuleName, coins) -require.NoError(t, err) -err = sdkKeepers.BankKeeper.SendCoinsFromModuleToAccount(ctx, fungibletypes.ModuleName, staker, coins) -require.NoError(t, err) -args := []interface{}{stakerEthAddr, sample.Validator(t, rand.New(rand.NewSource(42))).OperatorAddress, coins.AmountOf(config.BaseDenom).BigInt()} +ctx, contract, abi, sdkKeepers, mockEVM, mockVMContract, args := prepareStakingTest(t, methodID)This refactor not only reduces code duplication but also centralizes the setup process, making the tests easier to read and maintain.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files ignored due to path filters (1)
e2e/contracts/teststaking/TestStaking.binis excluded by!**/*.bin
Files selected for processing (15)
- changelog.md (1 hunks)
- cmd/zetae2e/local/local.go (1 hunks)
- contrib/localnet/orchestrator/start-zetae2e.sh (1 hunks)
- e2e/contracts/teststaking/TestStaking.abi (1 hunks)
- e2e/contracts/teststaking/TestStaking.go (1 hunks)
- e2e/contracts/teststaking/TestStaking.json (1 hunks)
- e2e/contracts/teststaking/TestStaking.sol (1 hunks)
- e2e/contracts/teststaking/bindings.go (1 hunks)
- e2e/e2etests/e2etests.go (2 hunks)
- e2e/e2etests/test_precompiles_staking.go (5 hunks)
- e2e/e2etests/test_precompiles_staking_through_contract.go (1 hunks)
- e2e/runner/runner.go (3 hunks)
- pkg/rpc/clients.go (3 hunks)
- precompiles/staking/staking.go (10 hunks)
- precompiles/staking/staking_test.go (44 hunks)
Additional context used
Path-based instructions (11)
e2e/contracts/teststaking/bindings.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.pkg/rpc/clients.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_precompiles_staking.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_precompiles_staking_through_contract.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.precompiles/staking/staking.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.contrib/localnet/orchestrator/start-zetae2e.sh (1)
Pattern
**/*.sh: Review the shell scripts, point out issues relative to security, performance, and maintainability.e2e/runner/runner.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.cmd/zetae2e/local/local.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/e2etests.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/contracts/teststaking/TestStaking.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.precompiles/staking/staking_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
Markdownlint
changelog.md
33-33: null
Bare URL used(MD034, no-bare-urls)
Additional comments not posted (26)
e2e/contracts/teststaking/bindings.go (2)
1-4: Review of Go generate commands for Solidity contract compilation and Go bindings generation.The Go generate commands are correctly set up to automate the compilation of the Solidity contract and the generation of Go bindings. This setup ensures that the ABI and binary of the Solidity contract are correctly extracted and used to generate Go bindings, which is essential for integrating Solidity contracts within Go codebases.
However, it's important to ensure that the paths and filenames used in these commands are maintained accurately in the project's documentation to avoid confusion during maintenance or further development.
6-6: Package declaration is standard and appropriate.The package name
teststakingis appropriately chosen to reflect the functionality of the file, which involves testing staking mechanisms. This is a good practice as it helps in organizing the codebase and makes it easier to locate related files and functionalities.e2e/contracts/teststaking/TestStaking.sol (2)
1-2: Header and Solidity VersionThe SPDX license identifier and Solidity version are correctly specified. This ensures compliance with licensing requirements and sets the appropriate compiler version for the contract.
5-10: Enum Definition: BondStatusThe
BondStatusenum is well-defined and covers all potential states of a bond in the staking process. This is crucial for maintaining clarity and consistency in the contract's state management.pkg/rpc/clients.go (2)
11-11: Addition of stakingtypes import.The import of
stakingtypesfrom the Cosmos SDK is necessary for the new staking functionality. This change is well-integrated and follows the existing pattern of importing specific modules as needed.
70-70: Addition of Staking client instantiation in newClients function.The instantiation of the
Stakingclient within thenewClientsfunction is correctly implemented and follows the pattern used for other clients. This ensures that all necessary clients are available for use, supporting the extended functionality of theClientsstruct.e2e/contracts/teststaking/TestStaking.abi (1)
1-191: Comprehensive Review of the TestStaking ABI FileThe ABI file is well-structured and follows the standard format for Ethereum smart contracts. Here are some specific observations and suggestions:
Constructor (lines 1-11): The constructor correctly takes an address type parameter for
_wzeta. This is standard practice for initializing contract state with external addresses.Fallback Function (lines 13-16): The fallback function is marked as
payable, which is appropriate if the contract is meant to receive Ether directly without a function call.depositWZETA Function (lines 17-23): This function is also marked as
payable, allowing it to accept Ether. Ensure that the contract logic properly handles or refunds excess Ether to avoid potential vulnerabilities.getAllValidators Function (lines 24-58): This function is marked as
view, indicating it does not modify the state, which is typical for a getter function. The output is a list of validators with detailed attributes. Ensure that all these fields are necessary for the callers since returning large data structures can be gas-intensive.getShares Function (lines 59-82): Another
viewfunction that returns the number of shares a staker has with a validator. The function signature is clear and follows best practices.moveStake Function (lines 83-116): This function allows moving stake from one validator to another. It is marked as
nonpayable, which is correct since it should not accept Ether. The output includes acompletionTime, which is useful for tracking the state change.stake Function (lines 117-145): Allows staking a specified amount with a validator. It is correctly marked as
nonpayable. The function does not specify a name for the boolean output, which could be named for clarity.unstake Function (lines 146-174): Similar to the
stakefunction but for unstaking. It also returns acompletionTimeto indicate when the unstaking completes.withdrawWZETA Function (lines 175-186): Allows withdrawing a token amount, marked as
nonpayable. It's crucial that the implementation ensures only the rightful owners can withdraw their tokens.Receive Function (lines 188-191): A simple
payablereceive function, which is essential for contracts that need to accept plain Ether transactions.Security Considerations:
- Ensure that all state-changing functions are protected against reentrancy attacks.
- Validate all inputs, especially addresses and uint256 types, to prevent underflows/overflows and other typical smart contract vulnerabilities.
Performance Considerations:
- Optimize gas usage by minimizing state changes and expensive operations, especially in view functions.
Overall, the ABI is well-formed, but the actual contract implementation must carefully handle security and efficiency, particularly in functions that interact with external contracts or handle significant value transfers.
e2e/e2etests/test_precompiles_staking.go (1)
69-75: Consistency in error handling and data retrieval.The pattern for checking delegation amounts is consistent and correctly implemented. However, ensure that the context used (
r.Ctx) is properly managed throughout the test to handle timeouts or cancellations effectively.precompiles/staking/staking.go (2)
174-174: Optimization inGetSharesmethod.The change to directly use
sdk.AccAddress(stakerAddress.Bytes())instead of convertingstakerAddressto a Bech32 string format first is a good optimization. It simplifies the code and potentially improves performance by reducing unnecessary conversions.
392-392: Review of method calls inRunfunction.The
Runfunction correctly routes the incoming requests to the appropriate staking methods based on the method ID. The changes ensure that thecontractparameter is consistently passed instead oforigin, aligning with the overall PR objectives to enhance authorization checks.Also applies to: 402-402, 412-412
contrib/localnet/orchestrator/start-zetae2e.sh (1)
Line range hint
49-63: LGTM!The
fund_eth_from_configfunction is well-structured and effectively uses helper functions to maintain clarity and separation of concerns.e2e/runner/runner.go (2)
90-90: Approved: Addition ofStakingClienttoE2ERunner.The addition of the
StakingClientfield is crucial for enabling staking-related queries, aligning with the PR's objectives to enhance staking precompile tests.
193-193: Approved: Initialization ofStakingClientinNewE2ERunner.The initialization of the
StakingClientfield in theNewE2ERunnerconstructor is correctly implemented, ensuring that theE2ERunneris equipped to handle staking-related queries.e2e/contracts/teststaking/TestStaking.json (7)
3-12: Constructor Definition is CorrectThe constructor is appropriately defined with the correct internal type and state mutability. It correctly initializes the contract with the necessary parameters.
14-17: Fallback Function is Appropriately DefinedThe fallback function is correctly marked as
payable, allowing the contract to receive Ether directly, which is a standard practice for contracts expected to handle transactions directly.
26-59: FunctiongetAllValidatorsis Well-DefinedThe function is correctly marked as
viewand returns a structured list of validators, which is consistent with its intended purpose of fetching data without modifying the state.
61-83: FunctiongetSharesis Correctly DefinedThe function takes appropriate inputs for a staker and a validator and returns the number of shares. It is correctly marked as
view, indicating it only reads from the state.
85-117: FunctionmoveStakeis Appropriately DefinedThe function allows moving stakes between validators and correctly returns the expected completion time. It is marked as
nonpayable, which is suitable since it does not involve direct Ether transactions.
119-146: Functionstakeis Correctly DefinedThe function allows for staking on a validator with a specified amount and returns a boolean status indicating success. It is appropriately marked as
nonpayable, aligning with its functionality.
148-175: Functionunstakeis Appropriately DefinedThe function allows for unstaking from a validator with a specified amount and returns the expected completion time. It is correctly marked as
nonpayable, which is suitable since it does not involve direct Ether transactions.e2e/e2etests/e2etests.go (2)
152-152: Addition ofTestPrecompilesStakingThroughContractNameconstant.The addition of the constant
TestPrecompilesStakingThroughContractNameis well-aligned with the existing naming conventions and enhances the clarity of test naming related to precompiled contracts. This change supports the PR's objective to extend end-to-end testing for staking functionality accessed through contracts.
830-835: Addition of a new test case for staking through contracts.The new test case using
TestPrecompilesStakingThroughContractNameis correctly added to theAllE2ETestsarray. This test is described as "test stateful precompiled contracts staking through contract" and is initialized with an empty argument definition, which is appropriate given the current test scope. This addition is crucial for enhancing the robustness of the testing suite by covering new scenarios involving staking precompiles accessed through contracts.e2e/contracts/teststaking/TestStaking.go (4)
1-3: Header Comments:The file header correctly indicates that this is a generated file and should not be manually edited. This is important for maintainability and to avoid accidental manual changes.
32-38: Struct Definition: ValidatorThe
Validatorstruct is well-defined with clear field names that match the expected types from the Solidity contract. This ensures that the Go code will correctly interact with the contract.
40-44: Metadata Definition:The
TestStakingMetaDatavariable is correctly set up with ABI and binary data for the contract. This centralizes the contract metadata, which is good practice for maintainability and reusability.
71-76: Type Definition: TestStakingThe
TestStakingstruct is well-organized, separating concerns into caller, transactor, and filterer components. This modularity enhances readability and maintainability.
Description
...
but will do that in follow up PR
closes: #2823
How Has This Been Tested?
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Chores