fix: calling precompile through sc with sc state update #2853
Conversation
skosito
requested review from
brewmaster012,
kingpinXD,
lumtis,
ws4charlie,
swift1337 and
fbac
as code owners
|
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 pull request introduces several enhancements to the Changes
Possibly related 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 #2853 +/- ##
========================================
Coverage 66.90% 66.91%
========================================
Files 370 370
Lines 20977 20980 +3
========================================
+ Hits 14035 14038 +3
Misses 6302 6302
Partials 640 640
|
![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
Outside diff range, codebase verification and nitpick comments (2)
precompiles/staking/staking.go (1)
Line range hint
185-241: LGTM! The changes handle state updates correctly whenStakeis called through a smart contract.The modifications to the
Stakefunction are well-structured and ensure that the state is updated consistently when the function is invoked through a smart contract. Manually reducing the balance of the staker in the state database is a good approach to maintain consistency between the EVM state and the bank module.To further improve the code, consider the following suggestions:
Add a comment explaining the purpose of the
if contract.CallerAddress != evm.Origincheck and the manual balance reduction. This will enhance code readability and maintainability.Consider extracting the manual balance reduction logic into a separate function to improve code modularity and reusability. This function can be reused in other precompile functions that require similar state updates.
Add error handling for the
stateDB.SubBalanceoperation to gracefully handle any potential errors that may occur during the balance reduction.These suggestions aim to enhance the overall quality and maintainability of the code.
e2e/contracts/teststaking/TestStaking.sol (1)
82-87: LGTM, but consider an optimization.The
stakeWithStateUpdatefunction is correctly implemented and can effectively track state changes when staking.However, consider optimizing the function by using the
++operator for incrementing thecounter. This will make the code more concise and gas-efficient.Apply this diff to optimize the function:
- counter = counter + 1; + counter++; bool success = staking.stake(staker, validator, amount); - counter = counter + 1; + counter++;
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 (7)
- e2e/contracts/teststaking/TestStaking.abi (2 hunks)
- e2e/contracts/teststaking/TestStaking.go (3 hunks)
- e2e/contracts/teststaking/TestStaking.json (3 hunks)
- e2e/contracts/teststaking/TestStaking.sol (2 hunks)
- e2e/e2etests/test_precompiles_staking_through_contract.go (2 hunks)
- precompiles/staking/staking.go (3 hunks)
- precompiles/staking/staking_test.go (16 hunks)
Additional context used
Path-based instructions (4)
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.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.
Additional comments not posted (21)
e2e/contracts/teststaking/TestStaking.json (3)
18-30: LGTM! Thecounterfunction is a valid addition to the contract.The
counterfunction is correctly defined as a view function, which aligns with its purpose of retrieving a value without modifying the contract state. The function signature and return type are properly specified, adhering to the Solidity syntax.The addition of this function enhances the contract's functionality by providing a convenient way to retrieve a counter value, which can be useful for tracking certain metrics or state variables.
160-188: LGTM! ThestakeAndRevertfunction is a valid addition to the contract.The
stakeAndRevertfunction is correctly defined as a non-payable function, which aligns with its purpose of performing a staking operation without accepting Ether. The function signature, input parameters, and return type are properly specified, adhering to the Solidity syntax.The purpose of the function is clear from its name, suggesting that it performs a staking operation and then reverts the transaction. This functionality can be useful for testing or simulating staking scenarios where a revert is expected, enhancing the contract's testability and robustness.
189-217: LGTM! ThestakeWithStateUpdatefunction is a valid addition to the contract.The
stakeWithStateUpdatefunction is correctly defined as a non-payable function, which aligns with its purpose of performing a staking operation without accepting Ether. The function signature, input parameters, and return type are properly specified, adhering to the Solidity syntax.The purpose of the function is clear from its name, suggesting that it performs a staking operation and updates the contract state accordingly. This functionality ensures that the staking process is properly reflected in the contract's state variables, maintaining consistency and accuracy.
The addition of this function enhances the contract's staking capabilities by providing a way to perform staking while ensuring that the contract state is properly updated.
e2e/contracts/teststaking/TestStaking.go (4)
213-228: LGTM! TheCounterfunction in theTestStakingCallerinterface is correctly implemented.The
Counterfunction is properly defined in theTestStakingCallerinterface, which is used for read-only operations. The function signature and return types are correctly specified, adhering to the Go syntax and the contract's ABI.The function uses the
Callmethod of the contract instance to invoke thecounterfunction, passing the necessary input parameters. The returned value is appropriately converted to*big.Intusingabi.ConvertType, ensuring type safety.The addition of this function in the
TestStakingCallerinterface allows for convenient retrieval of the counter value from the contract.
230-235: LGTM! TheCounterfunction in theTestStakingSessioninterface is correctly implemented.The
Counterfunction is properly defined in theTestStakingSessioninterface, which is used for read-only operations with a pre-configured session. The function signature and return types are correctly specified, adhering to the Go syntax and the contract's ABI.The function uses the
Counterfunction from theTestStakingCallerinterface to retrieve the counter value, leveraging the pre-configured session options.The addition of this function in the
TestStakingSessioninterface provides a convenient way to retrieve the counter value within a session context.
237-242: LGTM! TheCounterfunction in theTestStakingCallerSessioninterface is correctly implemented.The
Counterfunction is properly defined in theTestStakingCallerSessioninterface, which is used for read-only operations with a pre-configured caller session. The function signature and return types are correctly specified, adhering to the Go syntax and the contract's ABI.The function uses the
Counterfunction from theTestStakingCallerinterface to retrieve the counter value, leveraging the pre-configured caller session options.The addition of this function in the
TestStakingCallerSessioninterface provides a convenient way to retrieve the counter value within a caller session context.
369-374: LGTM! TheStakeAndRevertfunction in theTestStakingTransactorinterface is correctly implemented.The
StakeAndRevertfunction is properly defined in theTestStakingTransactorinterface, which is used for write operations. The function signature and input parameters are correctly specified, adhering to the Go syntax and the contract's ABI.The function uses the
Transactmethod of the contract instance to invoke thestakeAndRevertfunction, passing the necessary input parameters such asstaker,validator, andamount. The function returns a transaction object and an error, allowing for proper error handling.The addition of this function in the
TestStakingTransactorinterface enables the execution of thestakeAndRevertfunction on the contract.precompiles/staking/staking_test.go (3)
379-380: LGTM!The changes to the
setupfunction are approved. Returning themockEVMandmockVMContractinstances enhances the test setup by allowing for more controlled and isolated testing of the staking precompile interactions with the EVM.
398-398: LGTM!The changes to the
Test_Stakefunction are approved. Passing themockEVMinstance to theStakemethod calls improves the test coverage by simulating the EVM interactions during staking. The changes are consistent across all test cases.Also applies to: 424-424, 451-451, 478-478
821-821: LGTM!The changes to the
Test_MoveStakefunction are approved. Passing themockEVMinstance to theStakemethod calls improves the test coverage by simulating the EVM interactions during staking. The changes are consistent across all test cases.Also applies to: 847-847, 866-866, 892-892, 911-911, 937-937, 956-956, 982-982, 1001-1001, 1027-1027, 1155-1155
e2e/contracts/teststaking/TestStaking.sol (2)
55-56: LGTM!The
counterstate variable is correctly declared and initialized. It can be effectively used to track state changes within the contract.
89-94: LGTM!The
stakeAndRevertfunction is correctly implemented and serves as an effective test case to demonstrate state changes followed by a revert. It ensures that the contract's state remains unaffected when a staking operation is not finalized.e2e/contracts/teststaking/TestStaking.abi (3)
17-29: LGTM!The
counterfunction declaration is correct and aligns with the corresponding state variable in theTestStakingcontract. It allows retrieving the current value of thecounterwithout modifying the contract's state.
159-187: LGTM!The
stakeAndRevertfunction declaration is correct and aligns with the corresponding function in theTestStakingcontract. It allows for staking to a validator and subsequently reverting the transaction within the same function call.
188-216: LGTM!The
stakeWithStateUpdatefunction declaration is correct and aligns with the corresponding function in theTestStakingcontract. It allows for staking to a validator while updating the contract's state by incrementing thecountervariable.e2e/e2etests/test_precompiles_staking_through_contract.go (6)
63-64: LGTM!Introducing the
stakeAmountvariable enhances code readability and maintainability by avoiding the use of a hardcoded value directly in theWithdrawWZETAfunction call. This change makes the code more expressive and easier to understand.
68-75: LGTM!The added code block for querying the bank balance before staking operations is a valuable addition. It ensures that the initial state is validated against the expected
stakeAmount. The code is clean, expressive, and adheres to the best practices for interacting with the bank module using theBankClientandQueryBalanceRequest.
76-80: LGTM!The new test case for the
StakeAndRevertfunction is a valuable addition to the test suite. It verifies the behavior of the contract when a stake is made but not finalized, ensuring that the state remains unaffected. The code is clean and adheres to the best practices for interacting with the contract and waiting for the transaction receipt usingutils.MustWaitForTxReceipt.
81-98: LGTM!The added code block for checking the bank balance and contract state after the
StakeAndRevertfunction call is a crucial addition to the test suite. It thoroughly validates that the state remains unaffected by the reverted staking operation. The code is clean, expressive, and adheres to the best practices for querying balances using theBankClientand interacting with the contract functionsCounterandGetShares. The assertions provide a robust verification of the expected behavior.
99-111: LGTM!The new test case for the
Stakefunction without smart contract state update is a valuable addition to the test suite. It verifies that the staking operation is performed correctly and the bank balance is updated accordingly. The code is clean and adheres to the best practices for interacting with the contract, waiting for the transaction receipt, and querying the bank balance. The assertions provide a solid validation of the expected behavior.
112-134: LGTM!The new test case for the
StakeWithStateUpdatefunction is a critical addition to the test suite. It thoroughly verifies that the staking operation updates the contract state correctly, including the bank balance and thecountervariable. The code is clean, expressive, and adheres to the best practices for interacting with the contract, waiting for the transaction receipt, and querying the
Description
If stake precompile function is called through smart contract with some state changes on smart contract side, bank module state is not updated properly, it is overwritten, and bank balance is not reduced. In that case state on stateDB should manually be updated, so it is propagated to bank module update.
This PR adds counter in TestStaking.sol which is updated before and after stake call, and extends e2e test to verify that bank balance is reduced after stake method is executed. Also, adds one more function that stakes and reverts, to check that state on cosmos side is not updated.
How Has This Been Tested?
Summary by CodeRabbit
New Features
stakeAndRevertandstakeWithStateUpdate.counterto retrieve the current counter value.Bug Fixes
Documentation