convert UpdateBalance to use relative amount instead of absolute amount change #219
Conversation
WalkthroughThe pull request introduces significant modifications to the Changes
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (5)
x/oracle/keeper/native_token_test.go (1)
⚠️ Remaining Absolute Balance Changes DetectedThe verification script identified absolute balance changes in
x/oracle/keeper/native_token_test.go:
amount32 := sdkmath.NewIntWithDecimal(32, 18)amount22 := sdkmath.NewIntWithDecimal(22, 18)amount54 := sdkmath.NewIntWithDecimal(54, 18)amount49 := sdkmath.NewIntWithDecimal(49, 18)Action Required:
- Refactor these instances to use relative amount changes instead of absolute values to ensure consistency with the PR objectives.
🔗 Analysis chain
Line range hint
1-300: Overall assessment: Changes align well with PR objectivesThe modifications to the
TestNSTLifeCycleOneStakerfunction successfully implement the PR objective of converting UpdateBalance to use relative amount instead of absolute amount change. The consistent updates to expected balances across various operations (deposits, balance changes, withdrawals) demonstrate a thorough implementation of the new approach.These changes enhance the test coverage for the new balance calculation logic while maintaining the overall structure and flow of the test function. This ensures that the test continues to validate the lifecycle of a non-staking token (NST) for a single staker accurately under the new relative balance change system.
The implementation appears to address the PR's goal of enhancing compatibility across various assets by eliminating hard-coded asset IDs. However, as mentioned in the PR description, this approach may introduce additional complexity, particularly concerning synchronization for the price feeder.
To ensure that these changes are consistently applied throughout the codebase, please run the following verification script:
This script will help identify any remaining instances of absolute balance changes in test files that might need to be updated to use relative amounts.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining instances of absolute balance changes in test files # Search for patterns that might indicate absolute balance changes in test files rg --type go -g '*_test.go' '(?i)(balance.*?=.*?[0-9]+e18|sdkmath\.NewIntWithDecimal\([0-9]+, 18\))' x/Length of output: 441
x/oracle/keeper/msg_server_create_price.go (1)
79-80: Usefmt.Sprintffor string concatenation to enhance readability.Concatenating strings using the
+operator can be less readable and more error-prone. Utilizingfmt.Sprintfimproves clarity and maintainability of the code.Apply the following changes:
// [tokenIDStr]_[roundIDStr]_[price]_[decimal]_price/hash -sdk.NewAttribute(types.AttributeKeyFinalPrice, tokenIDStr+"_"+roundIDStr+"_"+newItem.PriceTR.Price+"_"+decimalStr+"_"+priceFormat), +sdk.NewAttribute( + types.AttributeKeyFinalPrice, + fmt.Sprintf("%s_%s_%s_%s_%s", tokenIDStr, roundIDStr, newItem.PriceTR.Price, decimalStr, priceFormat), +),Ensure the
fmtpackage is imported if not already done (as shown in the previous comment).x/oracle/keeper/native_token.go (3)
Line range hint
157-171: Implement check for complete validator withdrawal before removalThe TODO comment indicates that additional checks are needed to verify if the validator has withdrawn all assets before removing it from
ValidatorPubkeyList. Implementing this check will ensure correctness.Would you like assistance in implementing this functionality?
Line range hint
197-202: Correct error message for clarityThe error message "remove unexist validator" is grammatically incorrect.
Proposed code change:
if amountInt64 <= 0 { - return errors.New("remove unexist validator") + return errors.New("attempt to remove a non-existent validator") }
229-230: Consider upgrading to support other chainsThe TODO comments indicate that the current limitation may not be suitable for other chains like Solana. Consider upgrading the implementation to be compatible with additional chains.
Would you like assistance in designing an upgrade to support other chains?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
- x/oracle/keeper/msg_server_create_price.go (2 hunks)
- x/oracle/keeper/native_token.go (9 hunks)
- x/oracle/keeper/native_token_test.go (2 hunks)
🧰 Additional context used
📓 Learnings (1)
x/oracle/keeper/native_token.go (2)
Learnt from: leonz789 PR: ExocoreNetwork/exocore#189 File: x/oracle/keeper/native_token.go:0-0 Timestamp: 2024-09-20T00:02:14.496Z Learning: The function `UpdateNativeTokenByDepositOrWithdraw` is invoked through the assets module, and data validation is properly handled there.Learnt from: leonz789 PR: ExocoreNetwork/exocore#189 File: x/oracle/keeper/native_token.go:0-0 Timestamp: 2024-10-09T01:26:20.424Z Learning: The function `UpdateNativeTokenByDepositOrWithdraw` is invoked through the assets module, and data validation is properly handled there.
🔇 Additional comments (8)
x/oracle/keeper/native_token_test.go (3)
141-143: LGTM: Balance calculation correctionThe change from 59 to 49 in the expected balance aligns with the PR objective of using relative amount changes instead of absolute amounts. This correction ensures that the test accurately reflects the new balance calculation logic after the
UpdateNSTByBalanceChangeoperation.
149-152: LGTM: Consistent balance update in assets moduleThe change from 59e18 to 49e18 in the expected total deposit and withdrawable amounts is consistent with the previous balance change in the oracle module. This modification ensures that the assets module's state remains in sync with the oracle module's calculations, maintaining consistency across the system.
162-162: LGTM: Consistent balance update after withdrawalThe change from 29 to 19 in the expected balance after withdrawal is consistent with the previous balance changes and reflects the cumulative effect of all operations (deposits, balance changes, and withdrawals) on the staker's balance. This modification aligns with the PR objective of using relative amount changes and ensures the test accurately represents the new balance calculation logic.
x/oracle/keeper/msg_server_create_price.go (2)
9-9: Importing "crypto/sha256" is appropriate.The addition of the
crypto/sha256package is necessary for hashing the price when it exceeds the maximum allowed size.
16-18: Constants are defined and named appropriately.The new constant
maxEventPriceSizeand existing constants follow consistent naming conventions and improve code readability.x/oracle/keeper/native_token.go (3)
172-173: Clarify need for extra checks on amount unitsThe TODO comment suggests uncertainty about whether the caller needs to perform extra checks to ensure the amount is in integer units. Consider clarifying whether validation should occur here or at the caller level.
Do you need assistance in determining where this validation should be implemented?
Line range hint
246-252: Logic correctly updates staker balancesThe code correctly iterates over
stakerChangesand updates the staker balances accordingly.
269-277: Balance updates and delegationKeeper call appear correctThe balance adjustments and the call to
delegationKeeper.UpdateNSTBalanceare implemented correctly.
| priceFormat := "price" | ||
| if len(newItem.PriceTR.Price) > maxEventPriceSize { | ||
| // This is mainly used for NST since they might have a string with big size to indicate the changes for stakers | ||
| hashPrice := sha256.Sum256([]byte(newItem.PriceTR.Price)) | ||
| newItem.PriceTR.Price = string(hashPrice[:]) | ||
| priceFormat = "hash" | ||
| } |
There was a problem hiding this comment.
Convert hash to a hexadecimal string for proper encoding.
Using string(hashPrice[:]) to convert the hash result into a string will produce unreadable characters because it interprets the byte array as a string of bytes. To correctly represent the hash as a readable string, it should be encoded in hexadecimal format.
Apply the following changes to fix the issue:
priceFormat := "price"
if len(newItem.PriceTR.Price) > maxEventPriceSize {
// This is mainly used for NST since they might have a string with big size to indicate the changes for stakers
hashPrice := sha256.Sum256([]byte(newItem.PriceTR.Price))
- newItem.PriceTR.Price = string(hashPrice[:])
+ newItem.PriceTR.Price = fmt.Sprintf("%x", hashPrice)
priceFormat = "hash"
}Additionally, ensure the fmt package is imported at the top of the file:
import (
"context"
"errors"
"strconv"
"time"
+ "fmt"
"crypto/sha256"
"github.com/ExocoreNetwork/exocore/x/oracle/types"
sdk "github.com/cosmos/cosmos-sdk/types"
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| priceFormat := "price" | |
| if len(newItem.PriceTR.Price) > maxEventPriceSize { | |
| // This is mainly used for NST since they might have a string with big size to indicate the changes for stakers | |
| hashPrice := sha256.Sum256([]byte(newItem.PriceTR.Price)) | |
| newItem.PriceTR.Price = string(hashPrice[:]) | |
| priceFormat = "hash" | |
| } | |
| priceFormat := "price" | |
| if len(newItem.PriceTR.Price) > maxEventPriceSize { | |
| // This is mainly used for NST since they might have a string with big size to indicate the changes for stakers | |
| hashPrice := sha256.Sum256([]byte(newItem.PriceTR.Price)) | |
| newItem.PriceTR.Price = fmt.Sprintf("%x", hashPrice) | |
| priceFormat = "hash" | |
| } |
| // TODO: set a persistent state to track this number | ||
| // GetNSTTotalIndex returns the count of how many time the NST balance of assetID has been changed including sources of deposit/withdraw, balanceChange | ||
| func (k Keeper) GetNSTTotalIndex(ctx sdk.Context, assetID string) int64 { | ||
| stakerInfos := k.GetStakerInfos(ctx, assetID) | ||
| totalIndex := int64(0) | ||
| for _, stakerInfo := range stakerInfos { | ||
| totalIndex += int64(len(stakerInfo.BalanceList)) | ||
| } | ||
| return totalIndex | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement persistent state to track NST total index
The TODO comment suggests that storing the total index in a persistent state may improve performance by avoiding iteration over all stakers.
Would you like assistance in implementing a persistent state to track this number?
| length := len(stakerInfo.BalanceList) | ||
| // length should always be greater than 0 since the staker must deposit first, then we can update balance change | ||
| if length <= 0 { | ||
| return errors.New("UpdateBalane should not be executed on an empty balanceList") | ||
| } | ||
| newBalance = *(stakerInfo.BalanceList[length-1]) |
There was a problem hiding this comment.
Fix typo in error message
Correct the typo in the error message: "UpdateBalane" should be "UpdateBalance".
Proposed code change:
if length <= 0 {
- return errors.New("UpdateBalane should not be executed on an empty balanceList")
+ return errors.New("UpdateBalance should not be executed on an empty balanceList")
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| length := len(stakerInfo.BalanceList) | |
| // length should always be greater than 0 since the staker must deposit first, then we can update balance change | |
| if length <= 0 { | |
| return errors.New("UpdateBalane should not be executed on an empty balanceList") | |
| } | |
| newBalance = *(stakerInfo.BalanceList[length-1]) | |
| length := len(stakerInfo.BalanceList) | |
| // length should always be greater than 0 since the staker must deposit first, then we can update balance change | |
| if length <= 0 { | |
| return errors.New("UpdateBalance should not be executed on an empty balanceList") | |
| } | |
| newBalance = *(stakerInfo.BalanceList[length-1]) |
| // UpdateValidatorListForStaker invoked when deposit/withdraw happedn for an NST asset | ||
| // deposit wiil increase the staker's balance with a new validatorPubkey added into that staker's validatorList | ||
| // withdraw will decrease the staker's balanec with a vadlidatorPubkey removed from that staker's validatorList |
There was a problem hiding this comment.
Correct typos in function comments
There are several typographical errors in the function comments. Correcting them will improve clarity.
Proposed changes:
-// UpdateValidatorListForStaker invoked when deposit/withdraw happedn for an NST asset
+// UpdateValidatorListForStaker invoked when deposit/withdraw happen for an NST asset
-// deposit wiil increase the staker's balance with a new validatorPubkey added into that staker's validatorList
+// Deposit will increase the staker's balance with a new validatorPubkey added into the staker's ValidatorPubkeyList
-// withdraw will decrease the staker's balanec with a vadlidatorPubkey removed from that staker's validatorList
+// Withdraw will decrease the staker's balance with a validatorPubkey removed from the staker's ValidatorPubkeyList📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // UpdateValidatorListForStaker invoked when deposit/withdraw happedn for an NST asset | |
| // deposit wiil increase the staker's balance with a new validatorPubkey added into that staker's validatorList | |
| // withdraw will decrease the staker's balanec with a vadlidatorPubkey removed from that staker's validatorList | |
| // UpdateValidatorListForStaker invoked when deposit/withdraw happen for an NST asset | |
| // Deposit will increase the staker's balance with a new validatorPubkey added into the staker's ValidatorPubkeyList | |
| // Withdraw will decrease the staker's balance with a validatorPubkey removed from the staker's ValidatorPubkeyList |
| if amountInt64 > 0 { | ||
| // deopsit add a new validator into staker's validatorList | ||
| stakerInfo.ValidatorPubkeyList = append(stakerInfo.ValidatorPubkeyList, validatorPubkey) | ||
| } |
There was a problem hiding this comment.
Fix typo and prevent adding duplicate validators
Correct the typo in the comment. Additionally, to prevent duplicate validator entries, add a check to ensure validatorPubkey is not already in ValidatorPubkeyList before appending.
Proposed changes:
-// deopsit add a new validator into staker's validatorList
+// Deposit adds a new validator into the staker's ValidatorPubkeyList
+// Check if validatorPubkey already exists
+exists := false
+for _, vPubkey := range stakerInfo.ValidatorPubkeyList {
+ if vPubkey == validatorPubkey {
+ // Validator already exists, do not add
+ exists = true
+ break
+ }
+}
+if !exists {
stakerInfo.ValidatorPubkeyList = append(stakerInfo.ValidatorPubkeyList, validatorPubkey)
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if amountInt64 > 0 { | |
| // deopsit add a new validator into staker's validatorList | |
| stakerInfo.ValidatorPubkeyList = append(stakerInfo.ValidatorPubkeyList, validatorPubkey) | |
| } | |
| if amountInt64 > 0 { | |
| // Deposit adds a new validator into the staker's ValidatorPubkeyList | |
| // Check if validatorPubkey already exists | |
| exists := false | |
| for _, vPubkey := range stakerInfo.ValidatorPubkeyList { | |
| if vPubkey == validatorPubkey { | |
| // Validator already exists, do not add | |
| exists = true | |
| break | |
| } | |
| } | |
| if !exists { | |
| stakerInfo.ValidatorPubkeyList = append(stakerInfo.ValidatorPubkeyList, validatorPubkey) | |
| } | |
| } |
| _, decimalInt, err := k.getDecimal(ctx, assetID) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| // transfer amount into integer, for restaking the effective balance should always be whole unit | ||
| amountInt64 := amount.Quo(decimalInt).Int64() |
There was a problem hiding this comment.
Ensure amount is a multiple of decimalInt to prevent rounding errors
When converting amount to whole units, any fractional part will be truncated. To avoid potential loss of value, consider adding a check to confirm that amount is exactly divisible by decimalInt.
Proposed code change:
+// Ensure that amount is a multiple of decimalInt
+if !amount.Mod(decimalInt).IsZero() {
+ return errors.New("amount must be a whole number of units")
+}
amountInt64 := amount.Quo(decimalInt).Int64()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _, decimalInt, err := k.getDecimal(ctx, assetID) | |
| if err != nil { | |
| return err | |
| } | |
| // transfer amount into integer, for restaking the effective balance should always be whole unit | |
| amountInt64 := amount.Quo(decimalInt).Int64() | |
| _, decimalInt, err := k.getDecimal(ctx, assetID) | |
| if err != nil { | |
| return err | |
| } | |
| // transfer amount into integer, for restaking the effective balance should always be whole unit | |
| // Ensure that amount is a multiple of decimalInt | |
| if !amount.Mod(decimalInt).IsZero() { | |
| return errors.New("amount must be a whole number of units") | |
| } | |
| amountInt64 := amount.Quo(decimalInt).Int64() |
|
This pull request has been automatically marked as stale because it has not had recent activity. It will be closed in 7 days-before-close if no further activity occurs. |
Description
Use relative balance change compared to previous amount, instead of absolute change compared to 32ETH as base.\
Pro: This will suites all assets, don't need to hard code NSTETH assetID
Cons: Introduce complexity of synchronization for price-feeder
Conclusion: If there's no too many chains for NST-restaking senario, the existing ETH-specified version is prefered.
Closes #XXX
Summary by CodeRabbit
New Features
Bug Fixes
Documentation