feat: staking precompile #2784
feat: staking precompile #2784
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 Zeta Chain's staking functionality by adding new staking-related precompiled contracts and updating existing components to support these features. Key modifications include the integration of a staking keeper, updates to the changelog, and the introduction of end-to-end tests for staking operations. Additionally, new files defining the staking contract's ABI and Go bindings are created, facilitating interaction with staking functionalities within the blockchain environment. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant StakingContract
participant StakingKeeper
participant Validator
User->>StakingContract: stake(amount, validator)
StakingContract->>StakingKeeper: validateValidator(validator)
StakingKeeper-->>StakingContract: validationResult
alt validation successful
StakingContract->>StakingKeeper: updateStake(amount, validator)
StakingKeeper-->>StakingContract: updateSuccess
StakingContract-->>User: stakeSuccess
else validation failed
StakingContract-->>User: stakeFailed
end
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 ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2784 +/- ##
===========================================
+ Coverage 66.60% 66.68% +0.07%
===========================================
Files 369 370 +1
Lines 20727 20967 +240
===========================================
+ Hits 13806 13982 +176
- Misses 6283 6345 +62
- Partials 638 640 +2
|
skosito
requested review from
brewmaster012,
kingpinXD,
lumtis,
ws4charlie and
swift1337
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: 13
Outside diff range, codebase verification and nitpick comments (2)
precompiles/staking/bindings.go (1)
5-7: Clarify the purpose of the unused variable_ Contract.The declaration
var _ Contractis used to ensure that the package compiles even if the generated code is not yet utilized. This is a common practice in Go to avoid import errors during initial development stages. However, it's important to ensure that this line is removed or replaced with actual usage once the generated bindings are integrated into the project.precompiles/precompiles.go (1)
28-55: Approval of modifications toStatefulContractsfunction.The addition of the
stakingKeeperparameter and the integration of the staking contract into theStatefulContractsfunction are well-implemented. These changes enhance the contract management capabilities by allowing for the registration and handling of staking contracts.However, it is important to review the performance implications of these changes, especially in scenarios where the number of stateful contracts might be large.
Consider reviewing the performance implications of dynamically registering contracts, particularly in scenarios with a large number of contracts. Optimizations may be needed to ensure that the system remains performant.
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (20)
- app/app.go (1 hunks)
- changelog.md (1 hunks)
- cmd/zetae2e/local/local.go (1 hunks)
- codecov.yml (1 hunks)
- contrib/localnet/orchestrator/start-zetae2e.sh (1 hunks)
- contrib/localnet/scripts/start-zetacored.sh (1 hunks)
- e2e/e2etests/e2etests.go (2 hunks)
- e2e/e2etests/test_precompiles_prototype.go (1 hunks)
- e2e/e2etests/test_precompiles_staking.go (1 hunks)
- precompiles/precompiles.go (1 hunks)
- precompiles/precompiles_test.go (2 hunks)
- precompiles/prototype/prototype_test.go (1 hunks)
- precompiles/staking/IStaking.abi (1 hunks)
- precompiles/staking/IStaking.go (1 hunks)
- precompiles/staking/IStaking.json (1 hunks)
- precompiles/staking/IStaking.sol (1 hunks)
- precompiles/staking/bindings.go (1 hunks)
- precompiles/staking/staking.go (1 hunks)
- precompiles/staking/staking_test.go (1 hunks)
- scripts/bindings-stateful-precompiles.sh (1 hunks)
Files skipped from review due to trivial changes (3)
- changelog.md
- codecov.yml
- precompiles/prototype/prototype_test.go
Additional context used
Path-based instructions (14)
precompiles/staking/bindings.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.precompiles/precompiles_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.e2e/e2etests/test_precompiles_prototype.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.scripts/bindings-stateful-precompiles.sh (1)
Pattern
**/*.sh: Review the shell scripts, point out issues relative to security, performance, and maintainability.precompiles/precompiles.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.contrib/localnet/orchestrator/start-zetae2e.sh (1)
Pattern
**/*.sh: Review the shell scripts, point out issues relative to security, performance, and maintainability.precompiles/staking/staking.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.contrib/localnet/scripts/start-zetacored.sh (1)
Pattern
**/*.sh: Review the shell scripts, point out issues relative to security, performance, and maintainability.precompiles/staking/IStaking.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.precompiles/staking/staking_test.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.app/app.go (1)
Pattern
**/*.go: Review the Go code, point out issues relative to principles of clean code, expressiveness, and performance.
Shellcheck
scripts/bindings-stateful-precompiles.sh
[warning] 46-46: Use 'cd ... || exit' or 'cd ... || return' in case cd fails.
(SC2164)
GitHub Check: codecov/patch
precompiles/staking/staking.go
[warning] 66-67: precompiles/staking/staking.go#L66-L67
Added lines #L66 - L67 were not covered by tests
[warning] 162-163: precompiles/staking/staking.go#L162-L163
Added lines #L162 - L163 were not covered by tests
[warning] 169-169: precompiles/staking/staking.go#L169
Added line #L169 was not covered by tests
[warning] 358-361: precompiles/staking/staking.go#L358-L361
Added lines #L358 - L361 were not covered by tests
[warning] 364-366: precompiles/staking/staking.go#L364-L366
Added lines #L364 - L366 were not covered by tests
[warning] 369-369: precompiles/staking/staking.go#L369
Added line #L369 was not covered by tests
[warning] 371-379: precompiles/staking/staking.go#L371-L379
Added lines #L371 - L379 were not covered by tests
[warning] 381-389: precompiles/staking/staking.go#L381-L389
Added lines #L381 - L389 were not covered by tests
[warning] 391-399: precompiles/staking/staking.go#L391-L399
Added lines #L391 - L399 were not covered by tests
[warning] 401-409: precompiles/staking/staking.go#L401-L409
Added lines #L401 - L409 were not covered by tests
[warning] 411-419: precompiles/staking/staking.go#L411-L419
Added lines #L411 - L419 were not covered by tests
[warning] 421-421: precompiles/staking/staking.go#L421
Added line #L421 was not covered by tests
[warning] 423-425: precompiles/staking/staking.go#L423-L425
Added lines #L423 - L425 were not covered by tests
Additional comments not posted (21)
precompiles/staking/bindings.go (1)
1-3: Review Go generate commands for correctness and best practices.The Go generate commands are used to generate Go bindings from a Solidity contract ABI. These commands are correctly formatted and follow standard practices for such operations. Ensure that the paths and commands used in the
shcalls are secure and correctly scoped to avoid potential security risks or misconfigurations.precompiles/precompiles_test.go (2)
28-28: Validate the integration ofStakingKeeperwithinStatefulContracts.The integration of
StakingKeeperin theStatefulContractsfunction call is crucial for the correct functioning of staking-related tests. This change aligns with the PR's objective to enhance staking capabilities. Ensure that theStakingKeeperis correctly passed and utilized within the function to maintain the integrity of the test environment.
14-14: Review changes to theFungibleKeeperfunction call.The modification to include
sdkk, representing theStakingKeeper, in theFungibleKeeperfunction call is a significant change. This adjustment suggests a deeper integration of staking functionalities within the test setup. Ensure that thesdkkis properly initialized and used throughout the test to avoid null pointer exceptions or misconfigurations.Verification successful
Verify the initialization and usage of
sdkkinprecompiles/precompiles_test.go.The
sdkkvariable, representing theStakingKeeper, is initialized using thekeeper.FungibleKeeper(t)function. This initialization is consistent with the pattern observed across other test files, indicating thatsdkkis likely being set up correctly. The usage ofsdkkin this file involves interactions with theStakingKeeper, which suggests that it is being utilized as intended. No issues related to null pointer exceptions or misconfigurations were found in the context provided.
- File:
precompiles/precompiles_test.go- Line: 14
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the initialization and usage of `sdkk` in the test setup. # Test: Search for the initialization of `sdkk`. Expect: Proper initialization and usage. rg --type go -A 5 $'sdkk'Length of output: 200245
e2e/e2etests/test_precompiles_prototype.go (2)
11-11: Review the renaming of the test function to align with prototype functionality.The renaming of the test function to
TestPrecompilesPrototypeis appropriate and reflects the focus on testing the prototype functionality of the precompiles. This change helps in clearly distinguishing the test's purpose and scope, which is crucial for maintaining organized and understandable test suites.
11-11: Assess the implementation of the test function for prototype functionalities.The test function implementation is robust, covering various aspects of the prototype contract, such as address conversions and balance queries. This comprehensive testing ensures that the prototype functionalities are correctly implemented and behave as expected under different conditions.
scripts/bindings-stateful-precompiles.sh (1)
51-51: Approval of new binding generation command.The addition of
bindings ./precompiles/stakingis consistent with the existing structure of the script and correctly extends its functionality to include staking precompiles.precompiles/precompiles.go (3)
7-7: Approval of new import for staking functionality.The addition of the
stakingkeeperimport is necessary for the integration of staking functionality and is correctly placed within the import block.
14-14: Approval of new import for staking contract handling.The addition of the
stakingimport is necessary for handling staking contracts and is correctly placed within the import block.
23-23: Approval of update toEnabledStatefulContractsmap.The addition of
staking.ContractAddressto theEnabledStatefulContractsmap is necessary to enable the staking contract. However, it is crucial to verify that the address is correct and does not conflict with other addresses in the system.Please confirm the correctness of
staking.ContractAddressto ensure it does not conflict with other contract addresses in the system.precompiles/staking/IStaking.abi (1)
1-152: Approve ABI definition for staking operations.The ABI is well-defined and correctly specifies the inputs and outputs for the staking operations. The use of structured types like
enum BondStatusandstruct Validator[]enhances the clarity and usability of the ABI. The state mutability attributes are appropriately assigned, ensuring that the contract's behavior is predictable and secure.Overall, the ABI meets the requirements for the staking precompile and should facilitate effective interaction with the blockchain.
precompiles/staking/IStaking.json (1)
1-155: Approve JSON representation of the ABI.The JSON file accurately represents the ABI defined in the
.abifile. It maintains consistency in the function definitions, types, and state mutability attributes. This consistency is crucial for ensuring that the staking functionalities are correctly accessible and operable within the blockchain environment.The file is syntactically correct and well-structured, facilitating easy integration and interaction with the staking precompile.
precompiles/staking/staking.go (1)
103-121: Verify edge case handling inRequiredGasmethod.The
RequiredGasmethod calculates gas based on the input size and method type. Ensure that all edge cases, such as unexpected input sizes or invalid method IDs, are handled appropriately to prevent issues during execution.precompiles/staking/IStaking.go (5)
4-17: Best Practice: Package and Import OrganizationThe package declaration and imports are well-organized. However, consider grouping standard library imports together and third-party imports together for better readability. This is a common practice in Go projects.
32-38: Code Review: Struct DefinitionThe
Validatorstruct is clearly defined with appropriate field names and types. This struct is crucial for representing validators in the staking context. Ensure that the field types align with the expected formats and constraints from the Ethereum contract to avoid runtime issues.
41-43: Documentation and Metadata UsageThe
IStakingMetaDatavariable is well-documented and correctly uses the ABI for the staking contract. This metadata is essential for interfacing with the contract correctly. Ensure that the ABI string is kept up-to-date with any changes to the contract.
108-114: Error Handling: Contract BindingThe function
NewIStakingproperly handles errors which is crucial for debugging deployment issues. It returns detailed error information if the contract binding fails. This is a good practice in handling potential runtime errors effectively.
191-206: Function Implementation Review:GetAllValidatorsThe method
GetAllValidatorsis implemented to fetch validator details. It handles errors appropriately and converts the output to theValidatortype. Ensure that error handling and type conversions are thoroughly tested to prevent runtime panics.e2e/e2etests/e2etests.go (2)
151-151: Addition of New Constant for ClarityThe new constant
TestZetaPrecompilesStakingNameis added to clearly define the test name for staking precompiles. This addition improves the readability and maintainability of the test suite by categorizing the tests more distinctly.
824-827: New Test Case Addition: Staking PrecompilesThe addition of a new test case for staking precompiles under
TestZetaPrecompilesStakingNameis a positive change. It allows for focused testing on the newly introduced staking functionality. Ensure that the test implementation covers all critical paths and handles possible edge cases to maintain robustness.precompiles/staking/staking_test.go (1)
183-348: Issue: Potential logical errors in staking tests due to incorrect assumptions.Several tests in
Test_Stakeassume specific behaviors or states (like validator existence or correct argument formats) without explicitly verifying or setting these prerequisites within the test. This can lead to false positives or negatives in testing outcomes.Ensure each test independently sets up its required state and verifies all assumptions. For example, before testing staking operations, explicitly check or set the existence of validators and correct argument formats.
app/app.go (1)
570-575: Review Integration ofStakingKeeperinStatefulContractsFunction CallThe addition of
app.StakingKeeperto theprecompiles.StatefulContractsfunction call is a significant change aimed at enhancing the staking functionality of the node. It is crucial to ensure thatStakingKeeperis properly initialized before this point and that its integration does not adversely affect the system's performance or stability.Suggestions:
- Verify that
StakingKeeperis fully initialized and configured before its use inStatefulContracts.- Conduct performance benchmarks to assess any impact on transaction processing times or resource usage due to this change.
- Consider adding more detailed comments explaining the role of
StakingKeeperin this context to aid future maintainability.
There was a problem hiding this comment.
assumption is that origin of tx should be delegator, this can be changed with authorization but not sure about use cases
Is it a technical choice or a constraint? We should use msg.sender to allow smart contract to use the feature
we can also add precompile events in future PR
Let's create a PR if not done yet
Description
assumption is that origin of tx should be delegator, this can be changed with authorization but not sure about use cases
we can also add precompile events in future PR
How Has This Been Tested?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests