chore(precompiles): disable stake, unstake and moveStake #3010
Conversation
fbac
requested review from
brewmaster012,
kingpinXD,
lumtis,
ws4charlie,
skosito and
swift1337
as code owners
📝 WalkthroughWalkthroughThe changes in this pull request primarily involve commenting out functionalities and test cases related to staking within the codebase. Specifically, the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (3)
🧰 Additional context used📓 Path-based instructions (3)
🔇 Additional comments (2)
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
|
fbac
added
no-changelog
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #3010 +/- ##
===========================================
- Coverage 66.29% 65.64% -0.66%
===========================================
Files 406 406
Lines 22772 22780 +8
===========================================
- Hits 15097 14953 -144
- Misses 6901 7052 +151
- Partials 774 775 +1
|
fbac
changed the title
fbac
force-pushed
the
GH3005/disable-stake-unstake-movestake
branch
from
4831330 to
01934cc
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
precompiles/staking/staking.go (1)
417-464: Approve changes with a minor suggestion for improved clarity.The modifications effectively disable the
stake,unstake, andmoveStakemethods as intended, aligning with the PR objectives. The implementation is correct and does not introduce any syntax errors or logical issues.To enhance code clarity and maintainability, consider the following suggestion:
- // case StakeMethodName: - // if readOnly { - // return nil, ptypes.ErrWriteMethod{ - // Method: method.Name, - // } - // } - - // var res []byte - // execErr := stateDB.ExecuteNativeAction(contract.Address(), nil, func(ctx sdk.Context) error { - // res, err = c.Stake(ctx, evm, contract, method, args) - // return err - // }) - // if execErr != nil { - // return nil, err - // } - // return res, nil - // case UnstakeMethodName: - // if readOnly { - // return nil, ptypes.ErrWriteMethod{ - // Method: method.Name, - // } - // } - - // var res []byte - // execErr := stateDB.ExecuteNativeAction(contract.Address(), nil, func(ctx sdk.Context) error { - // res, err = c.Unstake(ctx, evm, contract, method, args) - // return err - // }) - // if execErr != nil { - // return nil, err - // } - // return res, nil - // case MoveStakeMethodName: - // if readOnly { - // return nil, ptypes.ErrWriteMethod{ - // Method: method.Name, - // } - // } - - // var res []byte - // execErr := stateDB.ExecuteNativeAction(contract.Address(), nil, func(ctx sdk.Context) error { - // res, err = c.MoveStake(ctx, evm, contract, method, args) - // return err - // }) - // if execErr != nil { - // return nil, err - // } - // return res, nil + // Staking methods temporarily disabled + // case StakeMethodName, UnstakeMethodName, MoveStakeMethodName: + // return nil, ptypes.ErrInvalidMethod{ + // Method: method.Name, + // }This refactoring consolidates the disabled methods into a single case statement and returns an
ErrInvalidMethoderror, clearly indicating that these methods are intentionally disabled. This approach improves code readability and makes it easier to re-enable these methods in the future when needed.precompiles/staking/staking_test.go (3)
240-1088: Consider adding a TODO comment for future re-enablement of tests.The test functions for
Test_Stake,Test_Unstake, andTest_MoveStakehave been commented out, which aligns with the PR objectives. However, to ensure these tests are not forgotten and can be easily re-enabled in the future, consider adding a TODO comment at the beginning of this block.Add the following comment before line 240:
// TODO: Re-enable these tests when stake, unstake, and moveStake functionality is implemented
Line range hint
1126-1258: Consider enhancing error message assertions in Test_GetShares.While the test cases in
Test_GetSharesare comprehensive, the error assertions in the failure scenarios (lines 1186-1258) could be more specific. Instead of just checking for the presence of an error, consider asserting the specific error messages to ensure the correct error is being thrown.Replace generic error checks with specific error message assertions. For example:
- require.Error(t, err) + require.ErrorContains(t, err, "expected error message")This change will make the tests more robust and easier to debug if they fail in the future.
Line range hint
1260-1288: Consider refactoring the invalid method test for clarity.The
Test_RunInvalidMethodfunction correctly tests the behavior of calling an invalid method, but it does so by using a method from a different contract (prototype). This approach might be confusing and could potentially break if the prototype contract changes.Consider refactoring the test to use a non-existent method name instead of borrowing from another contract. This would make the test more clear and less dependent on other contracts. For example:
methodID := abi.Method{Name: "nonExistentMethod", ID: []byte{0x12, 0x34, 0x56, 0x78}} args := []interface{}{} mockVMContract.Input = append(methodID.ID, packInputArgs(t, methodID, args...)...)This change will make the test more robust and easier to understand.
🛑 Comments failed to post (1)
cmd/zetae2e/local/local.go (1)
332-333: 💡 Codebase verification
Additional References to Disabled Staking Tests Detected
The verification revealed active references to
TestPrecompilesStakingandTestPrecompilesStakingThroughContractin the following files:
e2e/e2etests/test_precompiles_staking_through_contract.goe2e/e2etests/test_precompiles_staking.goe2e/e2etests/e2etests.goTo fully align with the PR objective to disable staking methods:
Comment Out or Remove References:
- Ensure that all instances of
TestPrecompilesStakingandTestPrecompilesStakingThroughContractare commented out or removed to prevent their execution.Verify Invocation Points:
- Check if these tests are invoked elsewhere in the testing framework and disable those invocation points accordingly.
Ensure Comprehensive Test Coverage:
- After disabling, confirm that there are no unintended side effects and that the overall test coverage remains robust.
🔗 Analysis chain
Approval with considerations for test coverage.
The removal of staking-related precompile tests aligns with the PR objective to disable stake, unstake, and moveStake methods for the initial launch. However, to maintain comprehensive test coverage:
- Consider adding placeholder tests that verify these methods are indeed disabled.
- Implement a mechanism to easily re-enable these tests for future releases.
- Ensure any dependent test cases or setup procedures are adjusted accordingly.
To confirm no unintended side effects, please run the following verification:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check for any remaining references to the disabled staking tests rg -i "TestPrecompilesStaking(ThroughContract)?" --type goLength of output: 928
Description
Closes #3005.
Opened #3009.
How Has This Been Tested?
Summary by CodeRabbit
Bug Fixes
Tests