feat(native restaking): add a function to update the states of native token restaking #183
Conversation
|
Important Review skippedAuto 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 WalkthroughThe changes encompass modifications to several functions within the Changes
Sequence Diagram(s)sequenceDiagram
participant Keeper
participant Staker
participant Operator
Staker->>Keeper: AssociateOperatorWithStaker(stakerID, operatorAddress)
Keeper-->>Staker: (true, nil) // Success
alt Error Occurs
Keeper-->>Staker: (true, error) // Operation attempted but failed
end
Staker->>Keeper: DissociateOperatorFromStaker(stakerID, associatedOperator)
Keeper-->>Staker: (false, nil) // Success
alt Error Occurs
Keeper-->>Staker: (true, error) // Operation attempted but failed
end
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- x/delegation/keeper/delegation.go (2 hunks)
- x/delegation/keeper/delegation_state.go (6 hunks)
- x/delegation/keeper/share.go (1 hunks)
- x/delegation/keeper/un_delegation_state.go (2 hunks)
- x/delegation/keeper/update_native_restaking_balance.go (1 hunks)
- x/operator/keeper/opt.go (2 hunks)
- x/operator/keeper/usd_value.go (1 hunks)
Additional comments not posted (10)
x/operator/keeper/opt.go (2)
4-4: Approved import statement.The addition of
errorsmod "cosmossdk.io/errors"is necessary for the enhanced error handling in theOptInmethod.
23-23: Enhanced error handling approved.The use of
errorsmod.Wrapfto include the operator address in the error message enhances clarity and debugging capabilities.Run the following script to verify the integration of the error handling change:
Verification successful
Verification successful: Enhanced error handling is consistently integrated.
The use of
errorsmod.Wrapfto include additional context in error messages is consistent across the codebase, enhancing clarity and debugging capabilities. The presence of tests further supports the integration of this change.
- Instances of
delegationtypes.ErrOperatorNotExistare consistently wrapped with additional context in relevant files.- Tests in
x/avs/keeper/avs_test.goindicate that error handling is being verified.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of the error handling change across the system. # Test: Search for the usage of `delegationtypes.ErrOperatorNotExist` to ensure it's handled consistently. rg --type go -A 5 $'delegationtypes.ErrOperatorNotExist'Length of output: 2929
x/delegation/keeper/update_native_restaking_balance.go (1)
10-112: Review ofUpdateNativeRestakingBalancefunction.The function handles updates to the native restaking balance effectively, with separate logic for positive and negative changes. The integration with
assetsKeeperfor updating the staker's asset state is correctly implemented.Suggestions:
- Consider adding more detailed comments to explain the logic, especially for the handling of negative amounts.
- Verify the integration of this function with other parts of the system to ensure consistency in error handling and state updates.
Run the following script to verify the integration of this function with other parts of the system:
x/delegation/keeper/share.go (1)
184-184: Approved change inRemoveShareFromOperator.The logic change to decrease the operator's share under certain conditions is correctly implemented and aligns with the system's requirements.
Suggestions:
- Verify the integration of this change with other parts of the system to ensure consistency in share management.
Run the following script to verify the integration of the change across the system:
Verification successful
Integration of
RemoveShareFromOperatoris consistent and well-tested.The function is used internally within the same file, and there are comprehensive test cases covering its behavior, ensuring its correct integration and functionality.
- The function is defined and used in
x/delegation/keeper/share.go.- Test cases are present in
x/delegation/keeper/share_test.go.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the integration of the change in `RemoveShareFromOperator` across the system. # Test: Search for the usage of `RemoveShareFromOperator` to ensure it's handled consistently. rg --type go -A 5 $'RemoveShareFromOperator'Length of output: 2712
x/delegation/keeper/delegation.go (2)
186-196: Confirm the logic change inAssociateOperatorWithStaker.The function now uses an inner function
opFuncthat returns a tuple(bool, error). This change is intended to provide more explicit control over error handling and operation success indication. Ensure that the calling function properly handles the boolean return value to manage the flow based on operation success or failure.Run the following script to verify the handling of the boolean return value:
233-243: Confirm the logic change inDissociateOperatorFromStaker.The function now uses an inner function
opFuncthat returns a tuple(bool, error). This change is intended to provide more explicit control over error handling and operation success indication. Ensure that the calling function properly handles the boolean return value to manage the flow based on operation success or failure.Run the following script to verify the handling of the boolean return value:
x/operator/keeper/usd_value.go (1)
420-443: Refactor suggested for clarity and error handling inopFunc.The nested function
opFuncwithinCalculateUSDValueForStakerhas been modified to return(bool, error)which enhances the error handling by allowing the function to explicitly indicate success or failure. However, the use ofboolto indicate success might be misleading in some contexts, especially when the function returnsfalsebut no error, which could be interpreted as a failure when it's actually a successful non-operation.Consider refactoring the return values to use more descriptive types or enums that can clearly indicate the reason for returning
false, such asNoMatchingConditionor similar, to avoid ambiguity in the function's consumer code.Additionally, ensure that all error paths are correctly handled and that the function's documentation is updated to reflect these changes.
x/delegation/keeper/un_delegation_state.go (2)
80-80: Improved Error Handling inGetUndelegationRecordsThe update to the error handling in
GetUndelegationRecordsenhances clarity by specifying the missing key, which aids in debugging and is a good practice.
153-182: New Function:IterateUndelegationsByStakerAndAssetThis new function adds significant functionality by allowing iteration over undelegation records based on
stakerIDandassetID. The error handling is robust, appropriately usingerrorsmod.Wrapto enhance error messages, which is crucial for debugging.Consider verifying the performance of this function, especially in scenarios with large data sets, to ensure that the iteration does not introduce performance bottlenecks.
x/delegation/keeper/delegation_state.go (1)
15-15: Enhanced Control Flow with UpdatedDelegationOpFuncSignatureThe modification of the
DelegationOpFunctype to return(bool, error)is a significant improvement, allowing for more granular control over iterations. This change is well-implemented across various functions, enabling them to break out of loops based on runtime conditions, which can improve efficiency and error handling.All related changes in the functions
IterateDelegationsForStakerAndAsset,IterateDelegationsForStaker,StakerDelegatedTotalAmount,AllDelegatedAmountForStakerAsset, andGetDelegationInfocorrectly implement the new signature and control flow logic.Also applies to: 31-37, 54-60, 69-84, 97-109, 186-188
25f8111 to
133e7d5
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range and nitpick comments (4)
x/delegation/keeper/update_native_restaking_balance_test.go (1)
16-19: Use constants for test amounts to improve readabilityThe test uses hardcoded numeric values for amounts (e.g.,
100,20,10). Defining these values as constants with descriptive names can improve readability and make the test easier to maintain.Apply this diff to define constants:
+const ( + depositAmountValue = 100 + delegateAmountToDefaultOperatorValue = 20 + undelegateAmountFromDefaultOperatorValue = 10 + delegateAmountToAnotherOperatorValue = 20 +) -depositAmount := sdkmath.NewInt(100) +depositAmount := sdkmath.NewInt(depositAmountValue) -delegateAmountToDefaultOperator := sdkmath.NewInt(20) +delegateAmountToDefaultOperator := sdkmath.NewInt(delegateAmountToDefaultOperatorValue) -undelegateAmountFromDefaultOperator := sdkmath.NewInt(10) +undelegateAmountFromDefaultOperator := sdkmath.NewInt(undelegateAmountFromDefaultOperatorValue) -delegateAmountToAnotherOperator := sdkmath.NewInt(20) +delegateAmountToAnotherOperator := sdkmath.NewInt(delegateAmountToAnotherOperatorValue)x/delegation/keeper/update_native_restaking_balance.go (3)
13-13: Correct the typo in the TODO commentIn line 13, the comment has a typo: "retaking" should be "restaking".
73-73: Fix typo in comment: 'ture' should be 'true'In the comment at line 73, "ture" should be corrected to "true".
125-125: Correct grammar in log messageIn the log message at line 125, the phrase "all staking funds has been slashed" should be "all staking funds have been slashed" to be grammatically correct.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (12)
- testutil/utils.go (1 hunks)
- x/delegation/keeper/delegation.go (2 hunks)
- x/delegation/keeper/delegation_op_test.go (7 hunks)
- x/delegation/keeper/delegation_state.go (4 hunks)
- x/delegation/keeper/share.go (1 hunks)
- x/delegation/keeper/share_test.go (4 hunks)
- x/delegation/keeper/un_delegation_state.go (2 hunks)
- x/delegation/keeper/update_native_restaking_balance.go (1 hunks)
- x/delegation/keeper/update_native_restaking_balance_test.go (1 hunks)
- x/delegation/types/genesis.go (1 hunks)
- x/delegation/types/keys.go (1 hunks)
- x/operator/keeper/usd_value.go (1 hunks)
Files skipped from review due to trivial changes (1)
- x/delegation/types/genesis.go
Files skipped from review as they are similar to previous changes (2)
- x/delegation/keeper/delegation_op_test.go
- x/delegation/types/keys.go
Additional comments not posted (18)
x/delegation/keeper/share.go (1)
184-184: LGTM!The updated comment accurately reflects the behavior of the code, which decreases the operator's share if the staker belongs to the delegated operator. This change provides clarity and aligns the comment with the actual logic implemented in the subsequent code.
x/delegation/keeper/delegation.go (2)
186-196: LGTM!The changes to the
opFuncsignature and return statements improve error handling and control flow. The function correctly updates the operator's share and the associated operator for the staker.
233-243: LGTM!Similar to the
AssociateOperatorWithStakerfunction, the changes to theopFuncsignature and return statements inDissociateOperatorFromStakerimprove error handling and control flow. The function correctly decreases the operator's share and deletes the associated operator for the staker.x/delegation/keeper/un_delegation_state.go (2)
97-97: LGTM!The updated error message provides more context about the missing key, which improves the clarity of the error and helps with debugging.
170-199: New function to iterate over undelegation records for a staker and asset.The new
IterateUndelegationsByStakerAndAssetfunction is a useful addition that allows iterating over undelegation records for a specificstakerIDandassetID. It provides the flexibility to perform custom operations on each record through theopFunccallback function.Some key points about the function:
- It uses the
KeyPrefixStakerUndelegationInfoprefix to efficiently iterate over the relevant records.- The
isUpdateflag allows updating the records during the iteration if needed.- It handles errors and supports breaking the iteration if the callback function returns
isBreakas true.This function will be helpful in scenarios where you need to process or update undelegation records for a specific staker and asset.
x/delegation/keeper/share_test.go (4)
227-229: LGTM!The changes to the test initialization process look good. The new
basicPreparemethod followed by parameterized calls toprepareDepositandprepareDelegationimproves the readability and maintainability of the test setup.
248-250: LGTM!The changes to the test initialization process look good. The new
basicPreparemethod followed by parameterized calls toprepareDepositandprepareDelegationimproves the readability and maintainability of the test setup.
269-271: LGTM!The changes to the test initialization process look good. The new
basicPreparemethod followed by parameterized calls toprepareDepositandprepareDelegationimproves the readability and maintainability of the test setup.
308-310: LGTM!The changes to the test initialization process look good. The new
basicPreparemethod followed by parameterized calls toprepareDepositandprepareDelegationimproves the readability and maintainability of the test setup.x/delegation/keeper/delegation_state.go (6)
15-15: LGTM!The change to
DelegationOpFuncto return a tuple of(bool, error)is a good enhancement for better error handling and control flow.
Line range hint
44-64: Refactoring looks good!The refactoring of
IterateDelegationsto accept aniteratorPrefixparameter simplifies the iteration logic and improves flexibility. The changes are consistent with the updatedDelegationOpFuncsignature and enhance readability.
67-71: LGTM!The changes to
IterateDelegationsForStakerAndAssetare consistent with the refactoring ofIterateDelegations. The function now serves as a wrapper to callIterateDelegationswith the appropriateiteratorPrefixfor the staker and asset context.
77-99: Renaming and error handling improvements look good!The renaming of
StakerDelegatedTotalAmounttoTotalDelegatedAmountForStakerAssetimproves clarity and aligns with the purpose of the function. Returning early with an error improves error handling and avoids unnecessary iterations. The changes toopFuncare consistent with the updatedDelegationOpFuncsignature.
Line range hint
102-124: Renaming, return value initialization, and error handling improvements look good!The renaming of
AllDelegatedAmountForStakerAssettoAllDelegatedInfoForStakerAssetimproves clarity and aligns with the purpose of the function. The initialization of the return value usingmake(map[string]sdkmath.Int)is more appropriate for the returned data structure. The changes toopFuncare consistent with the updatedDelegationOpFuncsignature, and returning early with an error improves error handling and avoids unnecessary iterations.
Line range hint
194-203: LGTM!The initialization of
ret.DelegationInfosusingmake(map[string]*delegationtype.DelegationAmounts)is appropriate for the returned data structure. The changes toopFuncare consistent with the updatedDelegationOpFuncsignature.testutil/utils.go (1)
80-80: Please clarify the purpose and impact of this change.The boolean argument passed to
SetupTestingAppis changed fromtruetofalse. It would be helpful to understand the implications of this change on the test suite and ensure that the desired behavior is achieved.Could you please provide more context on the purpose of this argument and how changing it to
falseaffects the application setup process for testing?x/delegation/keeper/update_native_restaking_balance_test.go (1)
35-36: Clarify the precision loss in slash amount calculationThe comment indicates that the actual slash amount is 79 instead of 80 due to precision loss. To improve clarity, consider adding a detailed explanation for this difference to ensure future maintainability and understanding of the test.
If this precision loss is expected due to the underlying calculation mechanisms, it's important to document it clearly. You might want to verify the cause of this precision loss and ensure that it aligns with the system's expected behavior.
x/operator/keeper/usd_value.go (1)
495-495: Verify the consistency of return values inopFunc.At line 495~,
opFuncreturnsfalse, nil, which suggests that the iteration should continue. In contrast, on encountering an error, it returnstrue, err, indicating the iteration should stop.Please ensure that this behavior aligns with the expected control flow of
IterateDelegationsForStaker, where the boolean return value dictates whether to continue or halt the iteration.
133e7d5 to
67b0bbe
Compare
67b0bbe to
be1ff45
Compare
adjust the implementation of UpdateNativeRestakingBalance and add the unit test for it
… pendingSlashAmount
be1ff45 to
f975790
Compare
Description
This PR adds a function to update the state when Oracle updates the balance of the native restaking token.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation