feat(oracle): support update for eth-native-restaking balance change #170
Conversation
WalkthroughThe changes introduce enhancements to the Exocore application, focusing on the integration of native token functionalities and oracle management. Key modifications include the addition of new parameters and methods for handling deposits, withdrawals, and delegations related to native tokens. New Protocol Buffers messages are defined for token pricing and staking, while existing functions are updated to accommodate these changes, improving the overall structure and capabilities of the application. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant OracleKeeper
participant BankKeeper
participant DelegationKeeper
User->>App: Initiate Deposit/Withdraw
App->>BankKeeper: PerformDepositOrWithdraw(assetID, amount)
BankKeeper->>OracleKeeper: UpdateNativeTokenByDepositOrWithdraw(assetID, stakerAddr, amount)
OracleKeeper-->>BankKeeper: Return updated balance
BankKeeper-->>App: Confirm Deposit/Withdraw
App-->>User: Notify Success
Poem
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 (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Outside diff range, codebase verification and nitpick comments (2)
x/delegation/keeper/delegation.go (1)
124-126: Add Test Cases for Native Token Handling inUndelegateFromThe current codebase lacks specific test cases to verify the handling of native tokens in the
UndelegateFromfunction. It is recommended to add test scenarios that cover this condition to ensure the new logic is correctly implemented and tested.
- Consider adding test cases in files where
UndelegateFromis already tested, such asdelegation_op_test.goor similar test files.- Ensure that the tests cover both positive and negative scenarios for native tokens.
Analysis chain
LGTM! But verify the function usage.
The function correctly includes new logic to handle native tokens. Ensure that all function calls to
UndelegateFromhandle this new condition correctly.The code changes are approved.
Run the following script to verify the function usage:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `UndelegateFrom` handle the new condition correctly. # Test: Search for the function usage. Expect: Only occurrences of the new condition. rg --type go -A 5 $'UndelegateFrom'Length of output: 10762
Script:
#!/bin/bash # Description: Search for test cases related to `UndelegateFrom` that handle native tokens. # Test: Search for occurrences of native token handling in test files. rg --type go 'IsNativeToken' -A 5Length of output: 3616
x/oracle/keeper/native_token.go (1)
199-241: Fix typo in comment.There is a typo in the comment on line 212. Replace "effect balance f stakerAddr[index] has changed" with "effective balance of stakerAddr[index] has changed".
Apply this diff to fix the typo:
- // effect balance f stakerAddr[index] has changed + // effective balance of stakerAddr[index] has changed
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
x/oracle/types/native_token.pb.gois excluded by!**/*.pb.go
Files selected for processing (18)
- app/app.go (1 hunks)
- proto/exocore/oracle/v1/native_token.proto (1 hunks)
- x/assets/keeper/bank.go (2 hunks)
- x/assets/types/expected_keepers.go (1 hunks)
- x/assets/types/general.go (2 hunks)
- x/delegation/keeper/abci.go (1 hunks)
- x/delegation/keeper/delegation.go (3 hunks)
- x/delegation/keeper/keeper.go (2 hunks)
- x/delegation/types/expected_keepers.go (2 hunks)
- x/delegation/types/hooks.go (1 hunks)
- x/operator/keeper/abci.go (2 hunks)
- x/operator/keeper/usd_value.go (2 hunks)
- x/oracle/keeper/native_token.go (1 hunks)
- x/oracle/keeper/prices.go (4 hunks)
- x/oracle/types/key_native_token.go (1 hunks)
- x/oracle/types/keys.go (1 hunks)
- x/oracle/types/native_token.go (1 hunks)
- x/oracle/types/params.go (1 hunks)
Files skipped from review due to trivial changes (2)
- x/delegation/types/hooks.go
- x/oracle/types/keys.go
Additional context used
Learnings (1)
x/operator/keeper/usd_value.go (1)
Learnt from: TimmyExogenous PR: ExocoreNetwork/exocore#129 File: x/operator/keeper/consensus_keys.go:394-395 Timestamp: 2024-07-25T06:38:34.825Z Learning: The decimals and prices retrieval in the `GetOrCalculateOperatorUSDValues` function cannot be combined because they call different modules.
GitHub Check: Run golangci-lint
x/oracle/types/key_native_token.go
[warning] 15-15:
unused-parameter: parameter 'stakerAddr' seems to be unused, consider removing or renaming it as _ (revive)x/assets/keeper/bank.go
[failure] 9-9:
ST1019: package "github.com/ExocoreNetwork/exocore/x/assets/types" is being imported more than once (stylecheck)
[failure] 10-10:
ST1019(related information): other import of "github.com/ExocoreNetwork/exocore/x/assets/types" (stylecheck)x/delegation/keeper/abci.go
[failure] 49-49:
File is notgofumpt-ed (gofumpt)x/oracle/keeper/prices.go
[failure] 210-210:
Error return value ofk.UpdateNativeTokenByBalanceChangeis not checked (errcheck)x/oracle/keeper/native_token.go
[warning] 134-134:
indent-error-flow: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary) (revive)
GitHub Check: CodeQL
x/oracle/keeper/native_token.go
[warning] 171-188: Iteration over map
Iteration over map may be a possible source of non-determinism
Additional comments not posted (33)
x/assets/types/expected_keepers.go (1)
12-12: LGTM! But verify the function implementation.The function signature looks correct. However, ensure that the function is implemented correctly in the corresponding struct that implements this interface.
The code changes are approved.
Run the following script to verify the function implementation:
Verification successful
Verification successful: Function implementation found.
The function
UpdateNativeTokenByDepositOrWithdrawis correctly implemented in theKeeperstruct within thex/oracle/keeper/native_token.gofile, matching the interface addition.
- Implementation found in:
x/oracle/keeper/native_token.goScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `UpdateNativeTokenByDepositOrWithdraw`. # Test: Search for the function implementation. Expect: The function should be implemented in the corresponding struct. rg --type go 'func (.*) UpdateNativeTokenByDepositOrWithdraw'Length of output: 212
x/oracle/types/native_token.go (1)
24-37: LGTM!The function is correctly implemented.
The code changes are approved.
proto/exocore/oracle/v1/native_token.proto (6)
9-13: LGTM!The message is correctly defined.
The code changes are approved.
15-19: LGTM!The message is correctly defined.
The code changes are approved.
21-27: LGTM!The message is correctly defined.
The code changes are approved.
34-36: LGTM!The message is correctly defined.
The code changes are approved.
38-41: LGTM!The message is correctly defined.
The code changes are approved.
42-44: LGTM!The message is correctly defined.
The code changes are approved.
x/oracle/types/key_native_token.go (4)
5-13: LGTM!The constants are well-defined and follow a consistent naming convention.
The code changes are approved.
19-21: LGTM!The function is correctly implemented.
The code changes are approved.
23-26: LGTM!The function is correctly implemented.
The code changes are approved.
28-30: LGTM!The function is correctly implemented.
The code changes are approved.
x/delegation/keeper/keeper.go (2)
20-20: LGTM!The addition of the
oracleKeeperfield is consistent with the PR objectives and enhances the functionality of theKeeper.The code changes are approved.
30-38: LGTM!The update to the constructor ensures that the
oracleKeeperdependency is properly initialized.The code changes are approved.
x/delegation/types/expected_keepers.go (2)
4-4: LGTM!The import statement is necessary for the new method's return type.
The code changes are approved.
59-61: LGTM!The method
UpdateNativeTokenByDelegationis consistent with the PR objectives and enhances the functionality related to token management.The code changes are approved.
x/assets/keeper/bank.go (1)
49-52: LGTM! But clarify or address the comment about skipping the check for withdrawable amounts.The new logic for handling native tokens is correctly implemented. However, the comment about skipping the check for withdrawable amounts should be clarified or addressed to ensure robustness.
The code changes are approved.
Ensure that the skipping of the check for withdrawable amounts is intentional and will not cause any issues in the future.
x/operator/keeper/abci.go (1)
67-70: LGTM! But verify the error handling inGetSpecifiedAssetsPrice.The new logic for updating prices of native token asset IDs is correctly implemented. However, ensure that the
GetSpecifiedAssetsPricemethod handles errors gracefully and does not cause any issues.The code changes are approved.
Ensure that the
GetSpecifiedAssetsPricemethod handles errors gracefully and does not cause any issues.x/assets/types/general.go (3)
174-177: LGTM!The function correctly checks if an asset ID corresponds to a native token. The TODO comment indicates future extensibility.
The code changes are approved.
179-182: LGTM!The function correctly returns a list of native token asset IDs. The TODO comment indicates future extensibility.
The code changes are approved.
184-186: LGTM!The function correctly retrieves the base token associated with a given asset ID.
The code changes are approved.
x/oracle/keeper/prices.go (2)
285-291: LGTM!The function correctly returns the
nextRoundIDvalue.The code changes are approved.
58-61: LGTM! But verify the function usage.The function correctly includes a new conditional check to handle cases where the
assetIDcorresponds to a native token. Ensure that all function calls toGetSpecifiedAssetsPricehandle this new condition correctly.The code changes are approved.
Run the following script to verify the function usage:
x/delegation/keeper/delegation.go (1)
52-54: LGTM! But verify the function usage.The function correctly includes new logic to handle native tokens. Ensure that all function calls to
delegateTohandle this new condition correctly.The code changes are approved.
Run the following script to verify the function usage:
Verification successful
Verification Successful: Native Token Handling in
delegateToFunctionThe
delegateTofunction correctly implements the logic for handling native tokens. The usage in bothgenesis.goanddelegation.goaligns with the intended functionality, ensuring that the new condition is applied appropriately.
- File:
x/delegation/keeper/delegation.go- Function:
delegateToScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `delegateTo` handle the new condition correctly. # Test: Search for the function usage. Expect: Only occurrences of the new condition. rg --type go -A 5 $'delegateTo'Length of output: 6396
Script:
#!/bin/bash # Extract the full implementation of the `delegateTo` function and its usage in relevant files. # Extract the implementation of `delegateTo` function. ast-grep --lang go --pattern 'func (k *Keeper) delegateTo($_, $_, $_) error { $$$ }' # Extract the usage of `delegateTo` in `genesis.go`. rg --type go -A 10 -B 2 'delegateTo' x/delegation/keeper/genesis.go # Extract the usage of `delegateTo` in `delegation.go`. rg --type go -A 10 -B 2 'delegateTo' x/delegation/keeper/delegation.goLength of output: 7513
x/oracle/keeper/native_token.go (6)
150-159: LGTM!The function is correctly implemented.
The code changes are approved.
192-197: LGTM!The function is correctly implemented.
The code changes are approved.
244-249: LGTM!The function is correctly implemented.
The code changes are approved.
251-256: LGTM!The function is correctly implemented.
The code changes are approved.
258-280: LGTM!The functions are correctly implemented.
The code changes are approved.
161-188: Reminder: Complete the TODO items and review iteration over map.The function has a TODO comment indicating incomplete implementation. Additionally, review the iteration over the map for potential non-determinism.
Do you want me to generate the implementation for the TODO items or open a GitHub issue to track this task?
Run the following script to verify the iteration over the map:
Tools
GitHub Check: CodeQL
[warning] 171-188: Iteration over map
Iteration over map may be a possible source of non-determinismx/oracle/types/params.go (1)
496-501: LGTM!The function is correctly implemented.
The code changes are approved.
x/operator/keeper/usd_value.go (1)
303-305: LGTM!The function is correctly implemented. The new conditional check for native tokens enhances the logic.
The code changes are approved.
app/app.go (1)
554-554: Verify the usage of the new parameter&app.OracleKeeper.Ensure that all components or functions receiving
&app.OracleKeeperare correctly handling it. This change might impact how the application interacts with the oracle service.Run the following script to verify the function usage:
| @@ -0,0 +1,37 @@ | |||
| package types | |||
|
|
|||
| // TODO: vlaidatorIndex need bridge data | |||
There was a problem hiding this comment.
Reminder: Address the TODO comments.
The TODO comments indicate that some data is missing.
Do you want me to help address the TODO comments or open a GitHub issue to track this task?
| message NativeTokenPrice { | ||
| string token = 1; | ||
| repeated PriceInfo prece_list = 2; | ||
| } |
There was a problem hiding this comment.
Fix the typo in the field name.
There is a typo in the field name prece_list. It should be price_list.
Apply this diff to fix the typo:
message NativeTokenPrice {
string token = 1;
- repeated PriceInfo prece_list = 2;
+ repeated PriceInfo price_list = 2;
}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.
| message NativeTokenPrice { | |
| string token = 1; | |
| repeated PriceInfo prece_list = 2; | |
| } | |
| message NativeTokenPrice { | |
| string token = 1; | |
| repeated PriceInfo price_list = 2; | |
| } |
x/oracle/types/key_native_token.go
Outdated
| func NativeTokenStakerDelegationKey(assetID, stakerAddr, operatorAddr string) []byte { | ||
| return append([]byte(NativeTokenStakerDelegationKeyPrefix), []byte(assetID)...) | ||
| } |
There was a problem hiding this comment.
Remove or use the unused parameter stakerAddr.
The parameter stakerAddr is not used in the function, which may lead to confusion or potential bugs. Consider removing it or using it appropriately.
Apply this diff to remove the unused parameter:
-func NativeTokenStakerDelegationKey(assetID, stakerAddr, operatorAddr string) []byte {
+func NativeTokenStakerDelegationKey(assetID, operatorAddr string) []byte {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.
| func NativeTokenStakerDelegationKey(assetID, stakerAddr, operatorAddr string) []byte { | |
| return append([]byte(NativeTokenStakerDelegationKeyPrefix), []byte(assetID)...) | |
| } | |
| func NativeTokenStakerDelegationKey(assetID, operatorAddr string) []byte { | |
| return append([]byte(NativeTokenStakerDelegationKeyPrefix), []byte(assetID)...) | |
| } |
Tools
GitHub Check: Run golangci-lint
[warning] 15-15:
unused-parameter: parameter 'stakerAddr' seems to be unused, consider removing or renaming it as _ (revive)
x/delegation/keeper/abci.go
Outdated
| @@ -46,6 +46,7 @@ | |||
| } | |||
| // and the other is the fact that it matures at the next block | |||
| err = k.StorePendingUndelegationRecord(ctx, recordKey, record) | |||
| //TODO: remove previous index with currentHeight for pendingUndelegationRecord | |||
There was a problem hiding this comment.
Acknowledged TODO comment.
The TODO comment about removing the previous index with currentHeight for pendingUndelegationRecord is noted. If you need assistance with this task, please let me know.
Do you want me to generate the code for this task or open a GitHub issue to track it?
Tools
GitHub Check: Run golangci-lint
[failure] 49-49:
File is notgofumpt-ed (gofumpt)
x/oracle/keeper/prices.go
Outdated
| // update for native tokens | ||
| // TODO: set hooks as a genral approach | ||
| var p types.Params | ||
| // get params from cache if exists | ||
| if agc != nil { | ||
| p = agc.GetParams() | ||
| } else { | ||
| p = k.GetParams(ctx) | ||
| } | ||
| assetIDs := p.GetAssetIDsFromTokenID(tokenID) | ||
| for _, assetID := range assetIDs { | ||
| if assetstypes.IsNativeToken(assetID) { | ||
| k.UpdateNativeTokenByBalanceChange(ctx, assetID, []byte(priceTR.Price), roundID) | ||
| } |
There was a problem hiding this comment.
Check the error return value.
The error return value of k.UpdateNativeTokenByBalanceChange is not checked. Ensure that the error is handled appropriately.
Apply this diff to check the error return value:
- k.UpdateNativeTokenByBalanceChange(ctx, assetID, []byte(priceTR.Price), roundID)
+ if err := k.UpdateNativeTokenByBalanceChange(ctx, assetID, []byte(priceTR.Price), roundID); err != nil {
+ return false
+ }Committable suggestion was skipped due to low confidence.
Tools
GitHub Check: Run golangci-lint
[failure] 210-210:
Error return value ofk.UpdateNativeTokenByBalanceChangeis not checked (errcheck)
x/oracle/keeper/native_token.go
Outdated
| func (k Keeper) UpdateNativeTokenByDepositOrWithdraw(ctx sdk.Context, assetID, stakerAddr, amount string) *big.Int { | ||
| // TODO: just convert the number for assets module, and don't store state in oracleModule, can use cache only here | ||
| // TODO: we havn't included validatorIndex here, need the bridge info | ||
| store := ctx.KVStore(k.storeKey) | ||
| key := types.NativeTokenStakerKey(assetID, stakerAddr) | ||
| stakerInfo := &types.StakerInfo{} | ||
| if value := store.Get(key); value == nil { | ||
| // create a new item for this staker | ||
| stakerInfo = types.NewStakerInfo(stakerAddr) | ||
| } else { | ||
| k.cdc.MustUnmarshal(value, stakerInfo) | ||
| } | ||
| latestIndex := len(stakerInfo.PriceList) - 1 | ||
| // calculate amount of virtual LST from nativetoken with price | ||
| // amountVirtualLST := convertNativeAmountStringToVirtualLSTAmount(amount, stakerInfo.PriceList[latestIndex].Price) | ||
| amountFloat := convertAmountOriginalStringToAmountFloat(amount, stakerInfo.PriceList[latestIndex].Price) | ||
| amountInt, _ := amountFloat.Int(nil) | ||
| prevTotalDeposit, _ := new(big.Int).SetString(stakerInfo.TotalDeposit, 10) | ||
| // update totalDeposit of staker, and price won't change on either deposit or withdraw | ||
| stakerInfo.TotalDeposit = prevTotalDeposit.Add(prevTotalDeposit, amountInt).String() | ||
| bz := k.cdc.MustMarshal(stakerInfo) | ||
| store.Set(key, bz) | ||
| return amountInt |
There was a problem hiding this comment.
Reminder: Complete the TODO items.
The function has several TODO comments indicating incomplete implementation. Ensure to address these before finalizing the PR.
Do you want me to generate the implementation for the TODO items or open a GitHub issue to track this task?
x/oracle/keeper/native_token.go
Outdated
| func (k Keeper) UpdateNativeTokenByDelegation(ctx sdk.Context, assetID, operatorAddr, stakerAddr, amountOriginal string) *big.Int { | ||
| store := ctx.KVStore(k.storeKey) | ||
| keyOperator := types.NativeTokenOperatorKey(assetID, operatorAddr) | ||
| operatorInfo := &types.OperatorInfo{} | ||
| value := store.Get(keyOperator) | ||
| if value == nil { | ||
| operatorInfo = types.NewOperatorInfo(operatorAddr) | ||
| } else { | ||
| k.cdc.MustUnmarshal(value, operatorInfo) | ||
| } | ||
| stakerInfo := &types.StakerInfo{} | ||
| keyStaker := types.NativeTokenStakerKey(assetID, stakerAddr) | ||
| if value = store.Get(keyStaker); value == nil { | ||
| panic("staker must exist before delegation") | ||
| } | ||
| k.cdc.MustUnmarshal(value, stakerInfo) | ||
|
|
||
| operatorAmountFloat, operatorAmountOriginalFloat := getOperatorAmountFloat(operatorInfo) | ||
| amountFloat, amountOriginalFloat := parseStakerAmountOriginal(amountOriginal, stakerInfo) | ||
|
|
||
| operatorAmountOriginalFloat = operatorAmountOriginalFloat.Add(operatorAmountOriginalFloat, amountOriginalFloat) | ||
| operatorAmountFloat = operatorAmountFloat.Add(operatorAmountFloat, amountFloat) | ||
|
|
||
| // update operator's price for native token base on new delegation | ||
| operatorInfo.PriceList = append(operatorInfo.PriceList, &types.PriceInfo{ | ||
| Price: operatorAmountOriginalFloat.Quo(operatorAmountOriginalFloat, operatorAmountFloat).String(), | ||
| Block: uint64(ctx.BlockHeight()), | ||
| }) | ||
| tmpAmountInt, _ := operatorAmountFloat.Int(nil) | ||
| // update operator's total amount for native token, for this 'amount' we don't disginguish different tokens from different stakers. That difference reflects in 'operator price' | ||
| operatorInfo.TotalAmount = tmpAmountInt.String() | ||
| bz := k.cdc.MustMarshal(operatorInfo) | ||
| store.Set(keyOperator, bz) | ||
| amountInt, _ := amountFloat.Int(nil) | ||
| keyDelegation := types.NativeTokenStakerDelegationKey(assetID, stakerAddr, operatorAddr) | ||
| stakerDelegation := &types.StakerDelegationInfo{} | ||
| if value = store.Get(keyDelegation); value == nil { | ||
| stakerDelegation.Delegations = []*types.DelegationInfo{ | ||
| { | ||
| OperatorAddr: operatorAddr, | ||
| Amount: amountInt.String(), | ||
| }, | ||
| } | ||
| } else { | ||
| k.cdc.MustUnmarshal(value, stakerDelegation) | ||
| for idx, delegationInfo := range stakerDelegation.Delegations { | ||
| if delegationInfo.OperatorAddr == operatorAddr { | ||
| prevAmount, _ := new(big.Int).SetString(delegationInfo.Amount, 10) | ||
| if prevAmount = prevAmount.Add(prevAmount, amountInt); prevAmount.Cmp(big.NewInt(0)) <= 0 { | ||
| stakerDelegation.Delegations = append(stakerDelegation.Delegations[0:idx], stakerDelegation.Delegations[idx:]...) | ||
| } else { | ||
| delegationInfo.Amount = prevAmount.Add(prevAmount, amountInt).String() | ||
| } | ||
| value = k.cdc.MustMarshal(stakerDelegation) | ||
| store.Set(keyDelegation, value) | ||
| return amountInt | ||
| } | ||
| } | ||
| stakerDelegation.Delegations = append(stakerDelegation.Delegations, &types.DelegationInfo{ | ||
| OperatorAddr: operatorAddr, | ||
| Amount: amountInt.String(), | ||
| }) | ||
| } | ||
| // update staker delegation infos for related operators | ||
| value = k.cdc.MustMarshal(stakerDelegation) | ||
| store.Set(keyDelegation, value) | ||
|
|
||
| return amountInt |
There was a problem hiding this comment.
Replace panic statement with proper error handling.
The function contains a panic statement if the staker does not exist. Replace it with proper error handling to avoid crashing the application.
Apply this diff to replace the panic statement with proper error handling:
- panic("staker must exist before delegation")
+ return errors.New("staker must exist before delegation")Committable suggestion was skipped due to low confidence.
x/oracle/keeper/native_token.go
Outdated
| func (k Keeper) GetNativeTokenPriceUSDForOperator(ctx sdk.Context, assetID string) (types.Price, error) { | ||
| parsedAssetID := strings.Split(assetID, "_") | ||
| if len(parsedAssetID) != 3 { | ||
| return types.Price{}, types.ErrGetPriceAssetNotFound | ||
| } | ||
| assetID = strings.Join([]string{parsedAssetID[0], parsedAssetID[1]}, "_") | ||
| operatorAddr := parsedAssetID[2] | ||
|
|
||
| store := ctx.KVStore(k.storeKey) | ||
| key := types.NativeTokenOperatorKey(assetID, operatorAddr) | ||
| if value := store.Get(key); value == nil { | ||
| return types.Price{}, types.ErrGetPriceAssetNotFound | ||
| } else { | ||
| operatorInfo := &types.OperatorInfo{} | ||
| k.cdc.MustUnmarshal(value, operatorInfo) | ||
| baseTokenUSDPrice, err := k.GetSpecifiedAssetsPrice(ctx, assetstypes.GetBaseTokenForNativeToken(assetID)) | ||
| if err != nil { | ||
| return types.Price{}, types.ErrGetPriceAssetNotFound | ||
| } | ||
| operatorPriceFloat := getLatestOperatorPriceFloat(operatorInfo) | ||
| priceValueFloat := new(big.Float).SetInt(baseTokenUSDPrice.Value.BigInt()) | ||
| priceValueFloat = priceValueFloat.Mul(priceValueFloat, operatorPriceFloat) | ||
| tmpValue, _ := priceValueFloat.Int(nil) | ||
| baseTokenUSDPrice.Value = sdkmath.NewIntFromBigInt(tmpValue) | ||
| return baseTokenUSDPrice, nil | ||
| } |
There was a problem hiding this comment.
Remove else block for better readability.
The function contains an else block that can be removed for better readability.
Apply this diff to remove the else block:
- if value := store.Get(key); value == nil {
- return types.Price{}, types.ErrGetPriceAssetNotFound
- } else {
- operatorInfo := &types.OperatorInfo{}
- k.cdc.MustUnmarshal(value, operatorInfo)
- baseTokenUSDPrice, err := k.GetSpecifiedAssetsPrice(ctx, assetstypes.GetBaseTokenForNativeToken(assetID))
- if err != nil {
- return types.Price{}, types.ErrGetPriceAssetNotFound
- }
- operatorPriceFloat := getLatestOperatorPriceFloat(operatorInfo)
- priceValueFloat := new(big.Float).SetInt(baseTokenUSDPrice.Value.BigInt())
- priceValueFloat = priceValueFloat.Mul(priceValueFloat, operatorPriceFloat)
- tmpValue, _ := priceValueFloat.Int(nil)
- baseTokenUSDPrice.Value = sdkmath.NewIntFromBigInt(tmpValue)
- return baseTokenUSDPrice, nil
- }
+ value := store.Get(key)
+ if value == nil {
+ return types.Price{}, types.ErrGetPriceAssetNotFound
+ }
+ operatorInfo := &types.OperatorInfo{}
+ k.cdc.MustUnmarshal(value, operatorInfo)
+ baseTokenUSDPrice, err := k.GetSpecifiedAssetsPrice(ctx, assetstypes.GetBaseTokenForNativeToken(assetID))
+ if err != nil {
+ return types.Price{}, types.ErrGetPriceAssetNotFound
+ }
+ operatorPriceFloat := getLatestOperatorPriceFloat(operatorInfo)
+ priceValueFloat := new(big.Float).SetInt(baseTokenUSDPrice.Value.BigInt())
+ priceValueFloat = priceValueFloat.Mul(priceValueFloat, operatorPriceFloat)
+ tmpValue, _ := priceValueFloat.Int(nil)
+ baseTokenUSDPrice.Value = sdkmath.NewIntFromBigInt(tmpValue)
+ return baseTokenUSDPrice, nilCommittable 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.
| func (k Keeper) GetNativeTokenPriceUSDForOperator(ctx sdk.Context, assetID string) (types.Price, error) { | |
| parsedAssetID := strings.Split(assetID, "_") | |
| if len(parsedAssetID) != 3 { | |
| return types.Price{}, types.ErrGetPriceAssetNotFound | |
| } | |
| assetID = strings.Join([]string{parsedAssetID[0], parsedAssetID[1]}, "_") | |
| operatorAddr := parsedAssetID[2] | |
| store := ctx.KVStore(k.storeKey) | |
| key := types.NativeTokenOperatorKey(assetID, operatorAddr) | |
| if value := store.Get(key); value == nil { | |
| return types.Price{}, types.ErrGetPriceAssetNotFound | |
| } else { | |
| operatorInfo := &types.OperatorInfo{} | |
| k.cdc.MustUnmarshal(value, operatorInfo) | |
| baseTokenUSDPrice, err := k.GetSpecifiedAssetsPrice(ctx, assetstypes.GetBaseTokenForNativeToken(assetID)) | |
| if err != nil { | |
| return types.Price{}, types.ErrGetPriceAssetNotFound | |
| } | |
| operatorPriceFloat := getLatestOperatorPriceFloat(operatorInfo) | |
| priceValueFloat := new(big.Float).SetInt(baseTokenUSDPrice.Value.BigInt()) | |
| priceValueFloat = priceValueFloat.Mul(priceValueFloat, operatorPriceFloat) | |
| tmpValue, _ := priceValueFloat.Int(nil) | |
| baseTokenUSDPrice.Value = sdkmath.NewIntFromBigInt(tmpValue) | |
| return baseTokenUSDPrice, nil | |
| } | |
| func (k Keeper) GetNativeTokenPriceUSDForOperator(ctx sdk.Context, assetID string) (types.Price, error) { | |
| parsedAssetID := strings.Split(assetID, "_") | |
| if len(parsedAssetID) != 3 { | |
| return types.Price{}, types.ErrGetPriceAssetNotFound | |
| } | |
| assetID = strings.Join([]string{parsedAssetID[0], parsedAssetID[1]}, "_") | |
| operatorAddr := parsedAssetID[2] | |
| store := ctx.KVStore(k.storeKey) | |
| key := types.NativeTokenOperatorKey(assetID, operatorAddr) | |
| value := store.Get(key) | |
| if value == nil { | |
| return types.Price{}, types.ErrGetPriceAssetNotFound | |
| } | |
| operatorInfo := &types.OperatorInfo{} | |
| k.cdc.MustUnmarshal(value, operatorInfo) | |
| baseTokenUSDPrice, err := k.GetSpecifiedAssetsPrice(ctx, assetstypes.GetBaseTokenForNativeToken(assetID)) | |
| if err != nil { | |
| return types.Price{}, types.ErrGetPriceAssetNotFound | |
| } | |
| operatorPriceFloat := getLatestOperatorPriceFloat(operatorInfo) | |
| priceValueFloat := new(big.Float).SetInt(baseTokenUSDPrice.Value.BigInt()) | |
| priceValueFloat = priceValueFloat.Mul(priceValueFloat, operatorPriceFloat) | |
| tmpValue, _ := priceValueFloat.Int(nil) | |
| baseTokenUSDPrice.Value = sdkmath.NewIntFromBigInt(tmpValue) | |
| return baseTokenUSDPrice, nil | |
| } |
Tools
GitHub Check: Run golangci-lint
[warning] 134-134:
indent-error-flow: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary) (revive)
There was a problem hiding this comment.
Actionable comments posted: 12
Outside diff range, codebase verification and nitpick comments (3)
x/assets/types/expected_keepers.go (1)
12-12: Inconsistent return types inOracleKeeperinterface.The
UpdateNativeTokenByDepositOrWithdrawmethod returns a*big.Int, which is inconsistent with the other methods in theOracleKeeperinterface that returnoracletypes.Priceanderror. Consider aligning the return types for consistency and clarity.
- File:
x/assets/types/expected_keepers.go- Interface:
OracleKeeperAnalysis chain
Review return type and consider simplifying the function name.
The function signature appears correct. However, ensure that returning a pointer to
big.Intis consistent with other methods in the interface. Additionally, consider simplifying the function name for readability.Verify the return type consistency with other methods in the interface:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify return type consistency with other methods in the interface. # Test: Search for other methods in the OracleKeeper interface. Expect: Consistent return types. rg --type go -A 5 $'type OracleKeeper interface {'Length of output: 173
Script:
#!/bin/bash # Description: Corrected search for the OracleKeeper interface and its methods. # Test: Locate the OracleKeeper interface and list its methods to verify return type consistency. rg --type go 'type OracleKeeper interface {' -A 10Length of output: 174
Script:
#!/bin/bash # Description: Attempt to locate the OracleKeeper interface and its methods using a simpler pattern. # Test: Search for the OracleKeeper interface and list its methods to verify return type consistency. rg 'OracleKeeper' -A 10 --type goLength of output: 33214
x/oracle/keeper/native_token.go (1)
199-241: Remove commented-out lines.The commented-out lines should be removed for better readability.
Apply this diff to remove the commented-out lines:
- // lenChanges := len(changes)app/app.go (1)
554-554: Update function calls to match the new signatureThe function
NewExocoreApphas been updated to include&app.OracleKeeperas a parameter. Ensure that all calls to this function are updated accordingly to prevent potential issues.
- Files to update:
testutil/network/network.gocmd/exocored/root.goapp/test_helpers.goapp/ethtest_helper.goAnalysis chain
Verify the function usage in the codebase.
The function signature has been updated to include
&app.OracleKeeper. Ensure that all function calls toNewExocoreAppmatch the new signature.Run the following script to verify the function usage:
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `NewExocoreApp` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type go -A 5 $'NewExocoreApp'Length of output: 2548
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
x/oracle/types/native_token.pb.gois excluded by!**/*.pb.go
Files selected for processing (18)
- app/app.go (1 hunks)
- proto/exocore/oracle/v1/native_token.proto (1 hunks)
- x/assets/keeper/bank.go (2 hunks)
- x/assets/types/expected_keepers.go (1 hunks)
- x/assets/types/general.go (2 hunks)
- x/delegation/keeper/abci.go (1 hunks)
- x/delegation/keeper/delegation.go (3 hunks)
- x/delegation/keeper/keeper.go (2 hunks)
- x/delegation/types/expected_keepers.go (2 hunks)
- x/delegation/types/hooks.go (1 hunks)
- x/operator/keeper/abci.go (2 hunks)
- x/operator/keeper/usd_value.go (2 hunks)
- x/oracle/keeper/native_token.go (1 hunks)
- x/oracle/keeper/prices.go (4 hunks)
- x/oracle/types/key_native_token.go (1 hunks)
- x/oracle/types/keys.go (1 hunks)
- x/oracle/types/native_token.go (1 hunks)
- x/oracle/types/params.go (1 hunks)
Files skipped from review due to trivial changes (2)
- x/delegation/types/hooks.go
- x/oracle/types/keys.go
Additional context used
Learnings (1)
x/operator/keeper/usd_value.go (1)
Learnt from: TimmyExogenous PR: ExocoreNetwork/exocore#129 File: x/operator/keeper/consensus_keys.go:394-395 Timestamp: 2024-07-25T06:38:34.825Z Learning: The decimals and prices retrieval in the `GetOrCalculateOperatorUSDValues` function cannot be combined because they call different modules.
golangci-lint
x/oracle/types/key_native_token.go
[warning] 15-15: unused-parameter: parameter 'stakerAddr' seems to be unused, consider removing or renaming it as _
(revive)
x/assets/keeper/bank.go
9-9: ST1019: package "github.com/ExocoreNetwork/exocore/x/assets/types" is being imported more than once
(stylecheck)
10-10: ST1019(related information): other import of "github.com/ExocoreNetwork/exocore/x/assets/types"
(stylecheck)
x/delegation/keeper/abci.go
49-49: File is not
gofumpt-ed(gofumpt)
x/oracle/keeper/prices.go
210-210: Error return value of
k.UpdateNativeTokenByBalanceChangeis not checked(errcheck)
x/oracle/keeper/native_token.go
[warning] 134-134: indent-error-flow: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)
(revive)
GitHub Check: Run golangci-lint
x/oracle/types/key_native_token.go
[warning] 15-15:
unused-parameter: parameter 'stakerAddr' seems to be unused, consider removing or renaming it as _ (revive)x/assets/keeper/bank.go
[failure] 9-9:
ST1019: package "github.com/ExocoreNetwork/exocore/x/assets/types" is being imported more than once (stylecheck)
[failure] 10-10:
ST1019(related information): other import of "github.com/ExocoreNetwork/exocore/x/assets/types" (stylecheck)x/delegation/keeper/abci.go
[failure] 49-49:
File is notgofumpt-ed (gofumpt)x/oracle/keeper/prices.go
[failure] 210-210:
Error return value ofk.UpdateNativeTokenByBalanceChangeis not checked (errcheck)x/oracle/keeper/native_token.go
[warning] 134-134:
indent-error-flow: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary) (revive)
GitHub Check: CodeQL
x/oracle/keeper/native_token.go
[warning] 171-188: Iteration over map
Iteration over map may be a possible source of non-determinism
Additional comments not posted (31)
x/oracle/types/native_token.go (1)
24-36: LGTM!The function is correctly implemented.
The code changes are approved.
proto/exocore/oracle/v1/native_token.proto (6)
9-13: LGTM!The message is correctly defined.
The code changes are approved.
15-19: LGTM!The message is correctly defined.
The code changes are approved.
21-27: LGTM!The message is correctly defined.
The code changes are approved.
34-36: LGTM!The message is correctly defined.
The code changes are approved.
38-41: LGTM!The message is correctly defined.
The code changes are approved.
42-44: LGTM!The message is correctly defined.
The code changes are approved.
x/oracle/types/key_native_token.go (3)
19-21: LGTM!The function is correctly implemented.
The code changes are approved.
23-26: LGTM!The function is correctly implemented.
The code changes are approved.
28-30: LGTM!The function is correctly implemented.
The code changes are approved.
x/delegation/keeper/keeper.go (2)
20-20: LGTM!The addition of the
oracleKeeperfield to theKeeperstruct is correctly implemented.The code changes are approved.
30-38: LGTM!The update to the
NewKeeperconstructor is correctly implemented.The code changes are approved.
x/delegation/types/expected_keepers.go (1)
59-61: LGTM!The addition of the
OracleKeeperinterface is correctly implemented.The code changes are approved.
x/assets/keeper/bank.go (2)
49-51: Address the TODO comment.The TODO comment indicates that the check for withdrawal amounts greater than the withdrawable amount is skipped. Ensure this edge case is handled to prevent potential issues.
Do you want me to help implement the check for this edge case?
13-13: Verify the function usage in the codebase.Ensure that all function calls to
PerformDepositOrWithdrawmatch the new logic.Run the following script to verify the function usage:
x/operator/keeper/abci.go (1)
5-5: LGTM!The new import is necessary for the updated logic.
The code changes are approved.
x/assets/types/general.go (3)
174-177: LGTM!The function is correctly implemented. Remember to address the TODO comment in future updates.
The code changes are approved.
179-182: LGTM!The function is correctly implemented. Remember to address the TODO comment in future updates.
The code changes are approved.
184-186: LGTM!The function is correctly implemented.
The code changes are approved.
x/oracle/keeper/prices.go (2)
58-61: LGTM!The function is correctly implemented. Ensure that the
GetNativeTokenPriceUSDForOperatormethod is properly tested.The code changes are approved.
285-291: LGTM!The function is correctly implemented.
The code changes are approved.
x/delegation/keeper/delegation.go (2)
52-54: LGTM!The function is correctly implemented. Ensure that the
UpdateNativeTokenByDelegationmethod is properly tested.The code changes are approved.
124-126: LGTM!The function is correctly implemented. Ensure that the
UpdateNativeTokenByDelegationmethod is properly tested.The code changes are approved.
x/oracle/keeper/native_token.go (5)
150-159: LGTM!The function is correctly implemented and retrieves the staker list as expected.
The code changes are approved.
192-197: LGTM!The function is correctly implemented and retrieves the staker list as expected.
The code changes are approved.
244-249: LGTM!The function is correctly implemented and retrieves the latest operator price as expected.
The code changes are approved.
251-256: LGTM!The function is correctly implemented and converts the amount string to a float as expected.
The code changes are approved.
258-280: LGTM!The functions are correctly implemented and retrieve and parse amounts as expected.
The code changes are approved.
x/oracle/types/params.go (1)
496-501: LGTM!The function is correctly implemented and retrieves asset IDs as expected.
The code changes are approved.
x/operator/keeper/usd_value.go (1)
303-305: LGTM!The function is correctly calculating the USD value for an operator. The new conditional check for native tokens is appropriately handling the asset ID modification.
The code changes are approved.
app/app.go (1)
554-554: LGTM!The addition of
&app.OracleKeeperto the function signature is correctly implemented.The code changes are approved.
x/oracle/types/native_token.go
Outdated
| // TODO: vlaidatorIndex need bridge data | ||
| // func NewStakerInfo(stakerAddr string, validatorIndex uint64) *StakerInfo { | ||
| func NewStakerInfo(stakerAddr string) *StakerInfo { | ||
| return &StakerInfo{ | ||
| StakerAddr: stakerAddr, | ||
| StakerIndex: 0, | ||
| // TODO: need bridge information | ||
| // ValidatorIndexs: []uint64{validatorIndex}, | ||
| ValidatorIndexs: make([]uint64, 0, 1), | ||
| TotalDeposit: "0", | ||
| PriceList: []*PriceInfo{ | ||
| { | ||
| // default price should be 1 | ||
| Price: "1", | ||
| Block: 0, | ||
| RoundID: 0, | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
Reminder: Address TODO comments and confirm initialization.
The function initializes the StakerInfo struct correctly. However, the TODO comments indicate incomplete implementation. Confirm if initializing ValidatorIndexs with an empty slice is intentional.
Do you want me to generate the missing bridge information or open a GitHub issue to track this task?
| message NativeTokenPrice { | ||
| string token = 1; | ||
| repeated PriceInfo prece_list = 2; | ||
| } |
There was a problem hiding this comment.
Fix the typo in the field name.
There is a typo in the field name prece_list. It should be price_list.
Apply this diff to fix the typo:
message NativeTokenPrice {
string token = 1;
- repeated PriceInfo prece_list = 2;
+ repeated PriceInfo price_list = 2;
}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.
| message NativeTokenPrice { | |
| string token = 1; | |
| repeated PriceInfo prece_list = 2; | |
| } | |
| message NativeTokenPrice { | |
| string token = 1; | |
| repeated PriceInfo price_list = 2; | |
| } |
x/oracle/types/key_native_token.go
Outdated
| func NativeTokenStakerDelegationKey(assetID, stakerAddr, operatorAddr string) []byte { | ||
| return append([]byte(NativeTokenStakerDelegationKeyPrefix), []byte(assetID)...) | ||
| } |
There was a problem hiding this comment.
Remove unused parameter.
The stakerAddr parameter is not used in the function and should be removed to avoid confusion.
Apply this diff to remove the unused parameter:
-func NativeTokenStakerDelegationKey(assetID, stakerAddr, operatorAddr string) []byte {
+func NativeTokenStakerDelegationKey(assetID, operatorAddr string) []byte {
return append([]byte(NativeTokenStakerDelegationKeyPrefix), []byte(assetID)...)
}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.
| func NativeTokenStakerDelegationKey(assetID, stakerAddr, operatorAddr string) []byte { | |
| return append([]byte(NativeTokenStakerDelegationKeyPrefix), []byte(assetID)...) | |
| } | |
| func NativeTokenStakerDelegationKey(assetID, operatorAddr string) []byte { | |
| return append([]byte(NativeTokenStakerDelegationKeyPrefix), []byte(assetID)...) | |
| } |
Tools
golangci-lint
[warning] 15-15: unused-parameter: parameter 'stakerAddr' seems to be unused, consider removing or renaming it as _
(revive)
GitHub Check: Run golangci-lint
[warning] 15-15:
unused-parameter: parameter 'stakerAddr' seems to be unused, consider removing or renaming it as _ (revive)
x/assets/keeper/bank.go
Outdated
| @@ -43,6 +46,11 @@ | |||
| return errorsmod.Wrapf(assetstypes.ErrInvalidOperationType, "the operation type is: %v", params.Action) | |||
| } | |||
|
|
|||
| if types.IsNativeToken(assetID) { | |||
| // TODO: we skip check for case like withdraw amount>withdrawable is fine since it will fail for later check and the state will be rollback | |||
| actualOpAmount = sdkmath.NewIntFromBigInt(k.UpdateNativeTokenByDepositOrWithdraw(ctx, assetID, hexutil.Encode(params.StakerAddress), params.OpAmount.String())) | |||
There was a problem hiding this comment.
Reminder: Add tests for the new function.
Ensure that the UpdateNativeTokenByDepositOrWithdraw function is thoroughly tested to verify its correctness.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
x/delegation/keeper/abci.go
Outdated
| @@ -46,6 +46,7 @@ | |||
| } | |||
| // and the other is the fact that it matures at the next block | |||
| err = k.StorePendingUndelegationRecord(ctx, recordKey, record) | |||
| //TODO: remove previous index with currentHeight for pendingUndelegationRecord | |||
There was a problem hiding this comment.
Reminder: Address the TODO comment.
The TODO comment indicates a planned enhancement to remove the previous index with the current height for pendingUndelegationRecord. Ensure this enhancement is tracked and implemented.
Do you want me to open a GitHub issue to track this task?
Tools
golangci-lint
49-49: File is not
gofumpt-ed(gofumpt)
GitHub Check: Run golangci-lint
[failure] 49-49:
File is notgofumpt-ed (gofumpt)
x/oracle/keeper/prices.go
Outdated
| // update for native tokens | ||
| // TODO: set hooks as a genral approach | ||
| var p types.Params | ||
| // get params from cache if exists | ||
| if agc != nil { | ||
| p = agc.GetParams() | ||
| } else { | ||
| p = k.GetParams(ctx) | ||
| } | ||
| assetIDs := p.GetAssetIDsFromTokenID(tokenID) | ||
| for _, assetID := range assetIDs { | ||
| if assetstypes.IsNativeToken(assetID) { | ||
| k.UpdateNativeTokenByBalanceChange(ctx, assetID, []byte(priceTR.Price), roundID) | ||
| } |
There was a problem hiding this comment.
Check the error return value.
The error return value of k.UpdateNativeTokenByBalanceChange is not checked. Ensure to handle the error properly.
Apply this diff to check the error return value:
for _, assetID := range assetIDs {
if assetstypes.IsNativeToken(assetID) {
- k.UpdateNativeTokenByBalanceChange(ctx, assetID, []byte(priceTR.Price), roundID)
+ if err := k.UpdateNativeTokenByBalanceChange(ctx, assetID, []byte(priceTR.Price), roundID); err != nil {
+ return false
+ }
}
}Committable suggestion was skipped due to low confidence.
Tools
golangci-lint
210-210: Error return value of
k.UpdateNativeTokenByBalanceChangeis not checked(errcheck)
GitHub Check: Run golangci-lint
[failure] 210-210:
Error return value ofk.UpdateNativeTokenByBalanceChangeis not checked (errcheck)
x/oracle/keeper/native_token.go
Outdated
| func (k Keeper) UpdateNativeTokenByDepositOrWithdraw(ctx sdk.Context, assetID, stakerAddr, amount string) *big.Int { | ||
| // TODO: just convert the number for assets module, and don't store state in oracleModule, can use cache only here | ||
| // TODO: we havn't included validatorIndex here, need the bridge info | ||
| store := ctx.KVStore(k.storeKey) | ||
| key := types.NativeTokenStakerKey(assetID, stakerAddr) | ||
| stakerInfo := &types.StakerInfo{} | ||
| if value := store.Get(key); value == nil { | ||
| // create a new item for this staker | ||
| stakerInfo = types.NewStakerInfo(stakerAddr) | ||
| } else { | ||
| k.cdc.MustUnmarshal(value, stakerInfo) | ||
| } | ||
| latestIndex := len(stakerInfo.PriceList) - 1 | ||
| // calculate amount of virtual LST from nativetoken with price | ||
| // amountVirtualLST := convertNativeAmountStringToVirtualLSTAmount(amount, stakerInfo.PriceList[latestIndex].Price) | ||
| amountFloat := convertAmountOriginalStringToAmountFloat(amount, stakerInfo.PriceList[latestIndex].Price) | ||
| amountInt, _ := amountFloat.Int(nil) | ||
| prevTotalDeposit, _ := new(big.Int).SetString(stakerInfo.TotalDeposit, 10) | ||
| // update totalDeposit of staker, and price won't change on either deposit or withdraw | ||
| stakerInfo.TotalDeposit = prevTotalDeposit.Add(prevTotalDeposit, amountInt).String() | ||
| bz := k.cdc.MustMarshal(stakerInfo) | ||
| store.Set(key, bz) | ||
| return amountInt |
There was a problem hiding this comment.
Reminder: Address TODO items.
The function is correctly updating the staker's total deposit. However, the TODO comments indicate that the implementation is not complete, and there are potential issues with state management and validator index inclusion.
Do you want me to help address the TODO items or open a GitHub issue to track this task?
x/oracle/keeper/native_token.go
Outdated
| func (k Keeper) UpdateNativeTokenByDelegation(ctx sdk.Context, assetID, operatorAddr, stakerAddr, amountOriginal string) *big.Int { | ||
| store := ctx.KVStore(k.storeKey) | ||
| keyOperator := types.NativeTokenOperatorKey(assetID, operatorAddr) | ||
| operatorInfo := &types.OperatorInfo{} | ||
| value := store.Get(keyOperator) | ||
| if value == nil { | ||
| operatorInfo = types.NewOperatorInfo(operatorAddr) | ||
| } else { | ||
| k.cdc.MustUnmarshal(value, operatorInfo) | ||
| } | ||
| stakerInfo := &types.StakerInfo{} | ||
| keyStaker := types.NativeTokenStakerKey(assetID, stakerAddr) | ||
| if value = store.Get(keyStaker); value == nil { | ||
| panic("staker must exist before delegation") | ||
| } | ||
| k.cdc.MustUnmarshal(value, stakerInfo) | ||
|
|
||
| operatorAmountFloat, operatorAmountOriginalFloat := getOperatorAmountFloat(operatorInfo) | ||
| amountFloat, amountOriginalFloat := parseStakerAmountOriginal(amountOriginal, stakerInfo) | ||
|
|
||
| operatorAmountOriginalFloat = operatorAmountOriginalFloat.Add(operatorAmountOriginalFloat, amountOriginalFloat) | ||
| operatorAmountFloat = operatorAmountFloat.Add(operatorAmountFloat, amountFloat) | ||
|
|
||
| // update operator's price for native token base on new delegation | ||
| operatorInfo.PriceList = append(operatorInfo.PriceList, &types.PriceInfo{ | ||
| Price: operatorAmountOriginalFloat.Quo(operatorAmountOriginalFloat, operatorAmountFloat).String(), | ||
| Block: uint64(ctx.BlockHeight()), | ||
| }) | ||
| tmpAmountInt, _ := operatorAmountFloat.Int(nil) | ||
| // update operator's total amount for native token, for this 'amount' we don't disginguish different tokens from different stakers. That difference reflects in 'operator price' | ||
| operatorInfo.TotalAmount = tmpAmountInt.String() | ||
| bz := k.cdc.MustMarshal(operatorInfo) | ||
| store.Set(keyOperator, bz) | ||
| amountInt, _ := amountFloat.Int(nil) | ||
| keyDelegation := types.NativeTokenStakerDelegationKey(assetID, stakerAddr, operatorAddr) | ||
| stakerDelegation := &types.StakerDelegationInfo{} | ||
| if value = store.Get(keyDelegation); value == nil { | ||
| stakerDelegation.Delegations = []*types.DelegationInfo{ | ||
| { | ||
| OperatorAddr: operatorAddr, | ||
| Amount: amountInt.String(), | ||
| }, | ||
| } | ||
| } else { | ||
| k.cdc.MustUnmarshal(value, stakerDelegation) | ||
| for idx, delegationInfo := range stakerDelegation.Delegations { | ||
| if delegationInfo.OperatorAddr == operatorAddr { | ||
| prevAmount, _ := new(big.Int).SetString(delegationInfo.Amount, 10) | ||
| if prevAmount = prevAmount.Add(prevAmount, amountInt); prevAmount.Cmp(big.NewInt(0)) <= 0 { | ||
| stakerDelegation.Delegations = append(stakerDelegation.Delegations[0:idx], stakerDelegation.Delegations[idx:]...) | ||
| } else { | ||
| delegationInfo.Amount = prevAmount.Add(prevAmount, amountInt).String() | ||
| } | ||
| value = k.cdc.MustMarshal(stakerDelegation) | ||
| store.Set(keyDelegation, value) | ||
| return amountInt | ||
| } | ||
| } | ||
| stakerDelegation.Delegations = append(stakerDelegation.Delegations, &types.DelegationInfo{ | ||
| OperatorAddr: operatorAddr, | ||
| Amount: amountInt.String(), | ||
| }) | ||
| } | ||
| // update staker delegation infos for related operators | ||
| value = k.cdc.MustMarshal(stakerDelegation) | ||
| store.Set(keyDelegation, value) | ||
|
|
||
| return amountInt |
There was a problem hiding this comment.
Replace panic with proper error handling.
The panic statement should be replaced with proper error handling to avoid crashing the application if the staker does not exist.
Apply this diff to replace the panic statement with proper error handling:
- if value = store.Get(keyStaker); value == nil {
- panic("staker must exist before delegation")
- }
+ if value = store.Get(keyStaker); value == nil {
+ return nil, errors.New("staker must exist before delegation")
+ }Committable suggestion was skipped due to low confidence.
x/oracle/keeper/native_token.go
Outdated
| func (k Keeper) GetNativeTokenPriceUSDForOperator(ctx sdk.Context, assetID string) (types.Price, error) { | ||
| parsedAssetID := strings.Split(assetID, "_") | ||
| if len(parsedAssetID) != 3 { | ||
| return types.Price{}, types.ErrGetPriceAssetNotFound | ||
| } | ||
| assetID = strings.Join([]string{parsedAssetID[0], parsedAssetID[1]}, "_") | ||
| operatorAddr := parsedAssetID[2] | ||
|
|
||
| store := ctx.KVStore(k.storeKey) | ||
| key := types.NativeTokenOperatorKey(assetID, operatorAddr) | ||
| if value := store.Get(key); value == nil { | ||
| return types.Price{}, types.ErrGetPriceAssetNotFound | ||
| } else { | ||
| operatorInfo := &types.OperatorInfo{} | ||
| k.cdc.MustUnmarshal(value, operatorInfo) | ||
| baseTokenUSDPrice, err := k.GetSpecifiedAssetsPrice(ctx, assetstypes.GetBaseTokenForNativeToken(assetID)) | ||
| if err != nil { | ||
| return types.Price{}, types.ErrGetPriceAssetNotFound | ||
| } | ||
| operatorPriceFloat := getLatestOperatorPriceFloat(operatorInfo) | ||
| priceValueFloat := new(big.Float).SetInt(baseTokenUSDPrice.Value.BigInt()) | ||
| priceValueFloat = priceValueFloat.Mul(priceValueFloat, operatorPriceFloat) | ||
| tmpValue, _ := priceValueFloat.Int(nil) | ||
| baseTokenUSDPrice.Value = sdkmath.NewIntFromBigInt(tmpValue) | ||
| return baseTokenUSDPrice, nil | ||
| } |
There was a problem hiding this comment.
Simplify code by removing else block.
The code can be simplified by removing the else block since the if block ends with a return statement.
Apply this diff to simplify the code:
if value := store.Get(key); value == nil {
return types.Price{}, types.ErrGetPriceAssetNotFound
}
- else {
- operatorInfo := &types.OperatorInfo{}
- k.cdc.MustUnmarshal(value, operatorInfo)
- baseTokenUSDPrice, err := k.GetSpecifiedAssetsPrice(ctx, assetstypes.GetBaseTokenForNativeToken(assetID))
- if err != nil {
- return types.Price{}, types.ErrGetPriceAssetNotFound
- }
- operatorPriceFloat := getLatestOperatorPriceFloat(operatorInfo)
- priceValueFloat := new(big.Float).SetInt(baseTokenUSDPrice.Value.BigInt())
- priceValueFloat = priceValueFloat.Mul(priceValueFloat, operatorPriceFloat)
- tmpValue, _ := priceValueFloat.Int(nil)
- baseTokenUSDPrice.Value = sdkmath.NewIntFromBigInt(tmpValue)
- return baseTokenUSDPrice, nil
- }
+ operatorInfo := &types.OperatorInfo{}
+ k.cdc.MustUnmarshal(value, operatorInfo)
+ baseTokenUSDPrice, err := k.GetSpecifiedAssetsPrice(ctx, assetstypes.GetBaseTokenForNativeToken(assetID))
+ if err != nil {
+ return types.Price{}, types.ErrGetPriceAssetNotFound
+ }
+ operatorPriceFloat := getLatestOperatorPriceFloat(operatorInfo)
+ priceValueFloat := new(big.Float).SetInt(baseTokenUSDPrice.Value.BigInt())
+ priceValueFloat = priceValueFloat.Mul(priceValueFloat, operatorPriceFloat)
+ tmpValue, _ := priceValueFloat.Int(nil)
+ baseTokenUSDPrice.Value = sdkmath.NewIntFromBigInt(tmpValue)
+ return baseTokenUSDPrice, nilCommittable 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.
| func (k Keeper) GetNativeTokenPriceUSDForOperator(ctx sdk.Context, assetID string) (types.Price, error) { | |
| parsedAssetID := strings.Split(assetID, "_") | |
| if len(parsedAssetID) != 3 { | |
| return types.Price{}, types.ErrGetPriceAssetNotFound | |
| } | |
| assetID = strings.Join([]string{parsedAssetID[0], parsedAssetID[1]}, "_") | |
| operatorAddr := parsedAssetID[2] | |
| store := ctx.KVStore(k.storeKey) | |
| key := types.NativeTokenOperatorKey(assetID, operatorAddr) | |
| if value := store.Get(key); value == nil { | |
| return types.Price{}, types.ErrGetPriceAssetNotFound | |
| } else { | |
| operatorInfo := &types.OperatorInfo{} | |
| k.cdc.MustUnmarshal(value, operatorInfo) | |
| baseTokenUSDPrice, err := k.GetSpecifiedAssetsPrice(ctx, assetstypes.GetBaseTokenForNativeToken(assetID)) | |
| if err != nil { | |
| return types.Price{}, types.ErrGetPriceAssetNotFound | |
| } | |
| operatorPriceFloat := getLatestOperatorPriceFloat(operatorInfo) | |
| priceValueFloat := new(big.Float).SetInt(baseTokenUSDPrice.Value.BigInt()) | |
| priceValueFloat = priceValueFloat.Mul(priceValueFloat, operatorPriceFloat) | |
| tmpValue, _ := priceValueFloat.Int(nil) | |
| baseTokenUSDPrice.Value = sdkmath.NewIntFromBigInt(tmpValue) | |
| return baseTokenUSDPrice, nil | |
| } | |
| func (k Keeper) GetNativeTokenPriceUSDForOperator(ctx sdk.Context, assetID string) (types.Price, error) { | |
| parsedAssetID := strings.Split(assetID, "_") | |
| if len(parsedAssetID) != 3 { | |
| return types.Price{}, types.ErrGetPriceAssetNotFound | |
| } | |
| assetID = strings.Join([]string{parsedAssetID[0], parsedAssetID[1]}, "_") | |
| operatorAddr := parsedAssetID[2] | |
| store := ctx.KVStore(k.storeKey) | |
| key := types.NativeTokenOperatorKey(assetID, operatorAddr) | |
| if value := store.Get(key); value == nil { | |
| return types.Price{}, types.ErrGetPriceAssetNotFound | |
| } | |
| operatorInfo := &types.OperatorInfo{} | |
| k.cdc.MustUnmarshal(value, operatorInfo) | |
| baseTokenUSDPrice, err := k.GetSpecifiedAssetsPrice(ctx, assetstypes.GetBaseTokenForNativeToken(assetID)) | |
| if err != nil { | |
| return types.Price{}, types.ErrGetPriceAssetNotFound | |
| } | |
| operatorPriceFloat := getLatestOperatorPriceFloat(operatorInfo) | |
| priceValueFloat := new(big.Float).SetInt(baseTokenUSDPrice.Value.BigInt()) | |
| priceValueFloat = priceValueFloat.Mul(priceValueFloat, operatorPriceFloat) | |
| tmpValue, _ := priceValueFloat.Int(nil) | |
| baseTokenUSDPrice.Value = sdkmath.NewIntFromBigInt(tmpValue) | |
| return baseTokenUSDPrice, nil | |
| } |
Tools
golangci-lint
[warning] 134-134: indent-error-flow: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)
(revive)
GitHub Check: Run golangci-lint
[warning] 134-134:
indent-error-flow: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary) (revive)
x/oracle/keeper/native_token.go
Outdated
| func (k Keeper) UpdateNativeTokenByBalanceChange(ctx sdk.Context, assetID string, rawData []byte, roundID uint64) error { | ||
| sl := k.getStakerList(ctx, assetID) | ||
| if len(sl.StakerAddrs) == 0 { | ||
| return errors.New("staker list is empty") | ||
| } | ||
| stakerChanges, err := parseBalanceChange(rawData, sl) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| store := ctx.KVStore(k.storeKey) | ||
| for stakerAddr, change := range stakerChanges { | ||
| value := store.Get(types.NativeTokenStakerKey(assetID, stakerAddr)) | ||
| if value == nil { | ||
| return errors.New("stakerInfo does not exist") | ||
| } | ||
| stakerInfo := &types.StakerInfo{} | ||
| k.cdc.MustUnmarshal(value, stakerInfo) | ||
| changeOriginalFloat := big.NewFloat(float64(change)) | ||
| totalAmountFloat, totalAmountOriginalFloat := parseStakerAmount(stakerInfo.TotalDeposit, stakerInfo) | ||
| totalAmountOriginalFloat = totalAmountOriginalFloat.Add(totalAmountOriginalFloat, changeOriginalFloat) | ||
| // update staker price based on beacon chain effective balance change | ||
| stakerInfo.PriceList = append(stakerInfo.PriceList, &types.PriceInfo{ | ||
| Price: totalAmountOriginalFloat.Quo(totalAmountOriginalFloat, totalAmountFloat).String(), | ||
| Block: uint64(ctx.BlockHeight()), | ||
| RoundID: roundID, | ||
| }) | ||
| // update related operator's price | ||
| } | ||
| return nil |
There was a problem hiding this comment.
Address non-determinism in map iteration.
Iteration over a map may introduce non-determinism. Consider sorting the map keys before iteration to ensure deterministic behavior.
Apply this diff to address the non-determinism issue:
+ keys := make([]string, 0, len(stakerChanges))
+ for k := range stakerChanges {
+ keys = append(keys, k)
+ }
+ sort.Strings(keys)
+
for _, stakerAddr := range keys {
change := stakerChanges[stakerAddr]
value := store.Get(types.NativeTokenStakerKey(assetID, stakerAddr))
if value == nil {
return errors.New("stakerInfo does not exist")
}
stakerInfo := &types.StakerInfo{}
k.cdc.MustUnmarshal(value, stakerInfo)
changeOriginalFloat := big.NewFloat(float64(change))
totalAmountFloat, totalAmountOriginalFloat := parseStakerAmount(stakerInfo.TotalDeposit, stakerInfo)
totalAmountOriginalFloat = totalAmountOriginalFloat.Add(totalAmountOriginalFloat, changeOriginalFloat)
// update staker price based on beacon chain effective balance change
stakerInfo.PriceList = append(stakerInfo.PriceList, &types.PriceInfo{
Price: totalAmountOriginalFloat.Quo(totalAmountOriginalFloat, totalAmountFloat).String(),
Block: uint64(ctx.BlockHeight()),
RoundID: roundID,
})
// update related operator's price
}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.
| func (k Keeper) UpdateNativeTokenByBalanceChange(ctx sdk.Context, assetID string, rawData []byte, roundID uint64) error { | |
| sl := k.getStakerList(ctx, assetID) | |
| if len(sl.StakerAddrs) == 0 { | |
| return errors.New("staker list is empty") | |
| } | |
| stakerChanges, err := parseBalanceChange(rawData, sl) | |
| if err != nil { | |
| return err | |
| } | |
| store := ctx.KVStore(k.storeKey) | |
| for stakerAddr, change := range stakerChanges { | |
| value := store.Get(types.NativeTokenStakerKey(assetID, stakerAddr)) | |
| if value == nil { | |
| return errors.New("stakerInfo does not exist") | |
| } | |
| stakerInfo := &types.StakerInfo{} | |
| k.cdc.MustUnmarshal(value, stakerInfo) | |
| changeOriginalFloat := big.NewFloat(float64(change)) | |
| totalAmountFloat, totalAmountOriginalFloat := parseStakerAmount(stakerInfo.TotalDeposit, stakerInfo) | |
| totalAmountOriginalFloat = totalAmountOriginalFloat.Add(totalAmountOriginalFloat, changeOriginalFloat) | |
| // update staker price based on beacon chain effective balance change | |
| stakerInfo.PriceList = append(stakerInfo.PriceList, &types.PriceInfo{ | |
| Price: totalAmountOriginalFloat.Quo(totalAmountOriginalFloat, totalAmountFloat).String(), | |
| Block: uint64(ctx.BlockHeight()), | |
| RoundID: roundID, | |
| }) | |
| // update related operator's price | |
| } | |
| return nil | |
| func (k Keeper) UpdateNativeTokenByBalanceChange(ctx sdk.Context, assetID string, rawData []byte, roundID uint64) error { | |
| sl := k.getStakerList(ctx, assetID) | |
| if len(sl.StakerAddrs) == 0 { | |
| return errors.New("staker list is empty") | |
| } | |
| stakerChanges, err := parseBalanceChange(rawData, sl) | |
| if err != nil { | |
| return err | |
| } | |
| store := ctx.KVStore(k.storeKey) | |
| keys := make([]string, 0, len(stakerChanges)) | |
| for k := range stakerChanges { | |
| keys = append(keys, k) | |
| } | |
| sort.Strings(keys) | |
| for _, stakerAddr := range keys { | |
| change := stakerChanges[stakerAddr] | |
| value := store.Get(types.NativeTokenStakerKey(assetID, stakerAddr)) | |
| if value == nil { | |
| return errors.New("stakerInfo does not exist") | |
| } | |
| stakerInfo := &types.StakerInfo{} | |
| k.cdc.MustUnmarshal(value, stakerInfo) | |
| changeOriginalFloat := big.NewFloat(float64(change)) | |
| totalAmountFloat, totalAmountOriginalFloat := parseStakerAmount(stakerInfo.TotalDeposit, stakerInfo) | |
| totalAmountOriginalFloat = totalAmountOriginalFloat.Add(totalAmountOriginalFloat, changeOriginalFloat) | |
| // update staker price based on beacon chain effective balance change | |
| stakerInfo.PriceList = append(stakerInfo.PriceList, &types.PriceInfo{ | |
| Price: totalAmountOriginalFloat.Quo(totalAmountOriginalFloat, totalAmountFloat).String(), | |
| Block: uint64(ctx.BlockHeight()), | |
| RoundID: roundID, | |
| }) | |
| // update related operator's price | |
| } | |
| return nil |
Tools
GitHub Check: CodeQL
[warning] 171-188: Iteration over map
Iteration over map may be a possible source of non-determinism
| for stakerAddr, change := range stakerChanges { | ||
| key := types.NativeTokenStakerKey(assetID, stakerAddr) | ||
| value := store.Get(key) | ||
| if value == nil { | ||
| return errors.New("stakerInfo does not exist") | ||
| } | ||
| stakerInfo := &types.StakerInfo{} | ||
| k.cdc.MustUnmarshal(value, stakerInfo) | ||
| changeOriginalFloat := sdkmath.LegacyNewDec(int64(change)) | ||
| totalAmountFloat, totalAmountOriginalFloat := parseStakerAmountInt(stakerInfo.TotalDeposit, stakerInfo) | ||
| totalAmountOriginalFloat = totalAmountOriginalFloat.Add(changeOriginalFloat) | ||
| prevStakerPrice := getLatestStakerPriceFloat(stakerInfo) | ||
| // update staker price based on beacon chain effective balance change | ||
| stakerPrice := totalAmountOriginalFloat.Quo(totalAmountFloat) | ||
| stakerInfo.PriceList = append(stakerInfo.PriceList, &types.PriceInfo{ | ||
| Price: stakerPrice, | ||
| Block: uint64(ctx.BlockHeight()), | ||
| RoundID: roundID, | ||
| }) | ||
| bz := k.cdc.MustMarshal(stakerInfo) | ||
| store.Set(key, bz) | ||
| // update related operator's price | ||
| keyStakerDelegations := types.NativeTokenStakerDelegationKey(assetID, stakerAddr) | ||
| value = store.Get(keyStakerDelegations) | ||
| if value != nil { | ||
| delegationInfo := &types.StakerDelegationInfo{} | ||
| k.cdc.MustUnmarshal(value, delegationInfo) | ||
| for _, delegation := range delegationInfo.Delegations { | ||
| keyOperator := types.NativeTokenOperatorKey(assetID, delegation.OperatorAddr) | ||
| value = store.Get(keyOperator) | ||
| if value == nil { | ||
| panic("staker delegation related to operator not exists") | ||
| } | ||
| operatorInfo := &types.OperatorInfo{} | ||
| k.cdc.MustUnmarshal(value, operatorInfo) | ||
| AmountFloat, prevAmountOriginalFloat := getOperatorAmountFloat(operatorInfo) | ||
| delta := delegation.Amount.ToLegacyDec().Mul(stakerPrice.Sub(prevStakerPrice)) | ||
| operatorInfo.PriceList = append(operatorInfo.PriceList, &types.PriceInfo{ | ||
| Price: prevAmountOriginalFloat.Add(delta).Quo(AmountFloat), | ||
| Block: uint64(ctx.BlockHeight()), | ||
| RoundID: roundID, | ||
| }) | ||
| bz := k.cdc.MustMarshal(operatorInfo) | ||
| store.Set(keyOperator, bz) | ||
| } | ||
|
|
||
| } | ||
|
|
||
| } |
Check warning
Code scanning / CodeQL
Iteration over map Warning
| keyOperator := types.NativeTokenOperatorKey(assetID, delegation.OperatorAddr) | ||
| value = store.Get(keyOperator) | ||
| if value == nil { | ||
| panic("staker delegation related to operator not exists") |
Check warning
Code scanning / CodeQL
Panic in BeginBock or EndBlock consensus methods Warning
There was a problem hiding this comment.
Actionable comments posted: 6
Outside diff range, codebase verification and nitpick comments (1)
x/oracle/keeper/native_token.go (1)
221-264: Fix the typo in the comment.There is a typo in the comment on line 234. The word "effect" should be "effective".
Apply this diff to fix the typo:
- // effect balance f stakerAddr[index] has changed + // effective balance of stakerAddr[index] has changed
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
x/oracle/types/native_token.pb.gois excluded by!**/*.pb.go
Files selected for processing (8)
- proto/exocore/oracle/v1/native_token.proto (1 hunks)
- x/assets/keeper/bank.go (2 hunks)
- x/assets/types/expected_keepers.go (1 hunks)
- x/delegation/keeper/delegation.go (3 hunks)
- x/delegation/types/expected_keepers.go (1 hunks)
- x/oracle/keeper/native_token.go (1 hunks)
- x/oracle/types/key_native_token.go (1 hunks)
- x/oracle/types/native_token.go (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- x/delegation/keeper/delegation.go
- x/delegation/types/expected_keepers.go
- x/oracle/types/key_native_token.go
- x/oracle/types/native_token.go
Additional context used
buf
proto/exocore/oracle/v1/native_token.proto
5-5: import "gogoproto/gogo.proto": file does not exist
(COMPILE)
GitHub Check: Run golangci-lint
x/assets/keeper/bank.go
[failure] 9-9:
ST1019: package "github.com/ExocoreNetwork/exocore/x/assets/types" is being imported more than once (stylecheck)
[failure] 10-10:
ST1019(related information): other import of "github.com/ExocoreNetwork/exocore/x/assets/types" (stylecheck)x/oracle/keeper/native_token.go
[warning] 128-128:
indent-error-flow: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary) (revive)
GitHub Check: CodeQL
x/oracle/keeper/native_token.go
[warning] 162-210: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 193-193: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
Additional comments not posted (14)
x/assets/types/expected_keepers.go (1)
11-11: LGTM!The method signature is correctly implemented and follows the expected pattern for handling large integers in a financial context.
The code changes are approved.
proto/exocore/oracle/v1/native_token.proto (7)
9-13: LGTM!The message structure is correctly implemented and follows the expected pattern for handling price information.
The code changes are approved.
15-19: LGTM!The message structure is correctly implemented and follows the expected pattern for handling operator information.
The code changes are approved.
21-27: LGTM!The message structure is correctly implemented and follows the expected pattern for handling staker information.
The code changes are approved.
34-36: LGTM!The message structure is correctly implemented and follows the expected pattern for handling staker lists.
The code changes are approved.
38-41: LGTM!The message structure is correctly implemented and follows the expected pattern for handling delegation information.
The code changes are approved.
42-44: LGTM!The message structure is correctly implemented and follows the expected pattern for handling staker delegation information.
The code changes are approved.
29-32: Fix the typo in the field name.There is a typo in the field name
prece_list. It should beprice_list.Apply this diff to fix the typo:
message NativeTokenPrice { string token = 1; - repeated PriceInfo prece_list = 2; + repeated PriceInfo price_list = 2; }Likely invalid or redundant comment.
x/oracle/keeper/native_token.go (6)
141-150: LGTM!The function is correctly implemented.
The code changes are approved.
214-219: LGTM!The function is correctly implemented.
The code changes are approved.
266-269: LGTM!The function is correctly implemented.
The code changes are approved.
271-274: LGTM!The function is correctly implemented.
The code changes are approved.
276-279: LGTM!The function is correctly implemented.
The code changes are approved.
281-303: LGTM!The functions are correctly implemented.
The code changes are approved.
x/assets/keeper/bank.go
Outdated
| if types.IsNativeToken(assetID) { | ||
| // TODO: we skip check for case like withdraw amount>withdrawable is fine since it will fail for later check and the state will be rollback | ||
| actualOpAmount = k.UpdateNativeTokenByDepositOrWithdraw(ctx, assetID, hexutil.Encode(params.StakerAddress), params.OpAmount) | ||
| } |
There was a problem hiding this comment.
Reminder: Add tests for the new logic.
Ensure that the UpdateNativeTokenByDepositOrWithdraw function is thoroughly tested to verify its correctness.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
x/assets/keeper/bank.go
Outdated
| "github.com/ExocoreNetwork/exocore/x/assets/types" | ||
| assetstypes "github.com/ExocoreNetwork/exocore/x/assets/types" |
There was a problem hiding this comment.
Remove duplicate import.
The package github.com/ExocoreNetwork/exocore/x/assets/types is being imported more than once. Remove the duplicate import to clean up the code.
Apply this diff to remove the duplicate import:
- "github.com/ExocoreNetwork/exocore/x/assets/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.
| "github.com/ExocoreNetwork/exocore/x/assets/types" | |
| assetstypes "github.com/ExocoreNetwork/exocore/x/assets/types" | |
| assetstypes "github.com/ExocoreNetwork/exocore/x/assets/types" |
Tools
GitHub Check: Run golangci-lint
[failure] 9-9:
ST1019: package "github.com/ExocoreNetwork/exocore/x/assets/types" is being imported more than once (stylecheck)
[failure] 10-10:
ST1019(related information): other import of "github.com/ExocoreNetwork/exocore/x/assets/types" (stylecheck)
x/oracle/keeper/native_token.go
Outdated
| func (k Keeper) UpdateNativeTokenByDepositOrWithdraw(ctx sdk.Context, assetID, stakerAddr string, amount sdkmath.Int) sdkmath.Int { | ||
| // TODO: just convert the number for assets module, and don't store state in oracleModule, can use cache only here | ||
| // TODO: we havn't included validatorIndex here, need the bridge info | ||
| store := ctx.KVStore(k.storeKey) | ||
| key := types.NativeTokenStakerKey(assetID, stakerAddr) | ||
| stakerInfo := &types.StakerInfo{} | ||
| if value := store.Get(key); value == nil { | ||
| // create a new item for this staker | ||
| stakerInfo = types.NewStakerInfo(stakerAddr) | ||
| } else { | ||
| k.cdc.MustUnmarshal(value, stakerInfo) | ||
| } | ||
| latestIndex := len(stakerInfo.PriceList) - 1 | ||
| // calculate amount of virtual LST from nativetoken with price | ||
| amountInt := convertAmountOriginalIntToAmountFloat(amount, stakerInfo.PriceList[latestIndex].Price).RoundInt() | ||
| stakerInfo.TotalDeposit = stakerInfo.TotalDeposit.Add(amountInt) | ||
| // update totalDeposit of staker, and price won't change on either deposit or withdraw | ||
| bz := k.cdc.MustMarshal(stakerInfo) | ||
| store.Set(key, bz) | ||
| return amountInt |
There was a problem hiding this comment.
Reminder: Complete the TODO items.
The function has several TODO comments indicating incomplete implementation. Ensure to address these before finalizing the PR.
Do you want me to generate the implementation for the TODO items or open a GitHub issue to track this task?
x/oracle/keeper/native_token.go
Outdated
| func (k Keeper) UpdateNativeTokenByBalanceChange(ctx sdk.Context, assetID string, rawData []byte, roundID uint64) error { | ||
| sl := k.getStakerList(ctx, assetID) | ||
| if len(sl.StakerAddrs) == 0 { | ||
| return errors.New("staker list is empty") | ||
| } | ||
| stakerChanges, err := parseBalanceChange(rawData, sl) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| store := ctx.KVStore(k.storeKey) | ||
| for stakerAddr, change := range stakerChanges { | ||
| key := types.NativeTokenStakerKey(assetID, stakerAddr) | ||
| value := store.Get(key) | ||
| if value == nil { | ||
| return errors.New("stakerInfo does not exist") | ||
| } | ||
| stakerInfo := &types.StakerInfo{} | ||
| k.cdc.MustUnmarshal(value, stakerInfo) | ||
| changeOriginalFloat := sdkmath.LegacyNewDec(int64(change)) | ||
| totalAmountFloat, totalAmountOriginalFloat := parseStakerAmountInt(stakerInfo.TotalDeposit, stakerInfo) | ||
| totalAmountOriginalFloat = totalAmountOriginalFloat.Add(changeOriginalFloat) | ||
| prevStakerPrice := getLatestStakerPriceFloat(stakerInfo) | ||
| // update staker price based on beacon chain effective balance change | ||
| stakerPrice := totalAmountOriginalFloat.Quo(totalAmountFloat) | ||
| stakerInfo.PriceList = append(stakerInfo.PriceList, &types.PriceInfo{ | ||
| Price: stakerPrice, | ||
| Block: uint64(ctx.BlockHeight()), | ||
| RoundID: roundID, | ||
| }) | ||
| bz := k.cdc.MustMarshal(stakerInfo) | ||
| store.Set(key, bz) | ||
| // update related operator's price | ||
| keyStakerDelegations := types.NativeTokenStakerDelegationKey(assetID, stakerAddr) | ||
| value = store.Get(keyStakerDelegations) | ||
| if value != nil { | ||
| delegationInfo := &types.StakerDelegationInfo{} | ||
| k.cdc.MustUnmarshal(value, delegationInfo) | ||
| for _, delegation := range delegationInfo.Delegations { | ||
| keyOperator := types.NativeTokenOperatorKey(assetID, delegation.OperatorAddr) | ||
| value = store.Get(keyOperator) | ||
| if value == nil { | ||
| panic("staker delegation related to operator not exists") | ||
| } | ||
| operatorInfo := &types.OperatorInfo{} | ||
| k.cdc.MustUnmarshal(value, operatorInfo) | ||
| AmountFloat, prevAmountOriginalFloat := getOperatorAmountFloat(operatorInfo) | ||
| delta := delegation.Amount.ToLegacyDec().Mul(stakerPrice.Sub(prevStakerPrice)) | ||
| operatorInfo.PriceList = append(operatorInfo.PriceList, &types.PriceInfo{ | ||
| Price: prevAmountOriginalFloat.Add(delta).Quo(AmountFloat), | ||
| Block: uint64(ctx.BlockHeight()), | ||
| RoundID: roundID, | ||
| }) | ||
| bz := k.cdc.MustMarshal(operatorInfo) | ||
| store.Set(keyOperator, bz) | ||
| } | ||
|
|
||
| } | ||
|
|
||
| } | ||
| return nil |
There was a problem hiding this comment.
Replace panic statement with proper error handling.
The function contains a panic statement if the staker delegation related to the operator does not exist. Replace it with proper error handling to avoid crashing the application.
Apply this diff to replace the panic statement with proper error handling:
- if value == nil {
- panic("staker delegation related to operator not exists")
- }
+ if value == nil {
+ return errors.New("staker delegation related to operator not exists")
+ }Address non-determinism in map iteration.
Iteration over a map may introduce non-determinism. Consider sorting the map keys before iteration to ensure deterministic behavior.
Apply this diff to address the non-determinism issue:
+ keys := make([]string, 0, len(stakerChanges))
+ for k := range stakerChanges {
+ keys = append(keys, k)
+ }
+ sort.Strings(keys)
+
for stakerAddr, change := range keys {
change := stakerChanges[stakerAddr]
value := store.Get(types.NativeTokenStakerKey(assetID, stakerAddr))
if value == nil {
return errors.New("stakerInfo does not exist")
}
stakerInfo := &types.StakerInfo{}
k.cdc.MustUnmarshal(value, stakerInfo)
changeOriginalFloat := sdkmath.LegacyNewDec(int64(change))
totalAmountFloat, totalAmountOriginalFloat := parseStakerAmountInt(stakerInfo.TotalDeposit, stakerInfo)
totalAmountOriginalFloat = totalAmountOriginalFloat.Add(changeOriginalFloat)
prevStakerPrice := getLatestStakerPriceFloat(stakerInfo)
// update staker price based on beacon chain effective balance change
stakerPrice := totalAmountOriginalFloat.Quo(totalAmountFloat)
stakerInfo.PriceList = append(stakerInfo.PriceList, &types.PriceInfo{
Price: stakerPrice,
Block: uint64(ctx.BlockHeight()),
RoundID: roundID,
})
bz := k.cdc.MustMarshal(stakerInfo)
store.Set(key, bz)
// update related operator's price
keyStakerDelegations := types.NativeTokenStakerDelegationKey(assetID, stakerAddr)
value = store.Get(keyStakerDelegations)
if value != nil {
delegationInfo := &types.StakerDelegationInfo{}
k.cdc.MustUnmarshal(value, delegationInfo)
for _, delegation := range delegationInfo.Delegations {
keyOperator := types.NativeTokenOperatorKey(assetID, delegation.OperatorAddr)
value = store.Get(keyOperator)
if value == nil {
return errors.New("staker delegation related to operator not exists")
}
operatorInfo := &types.OperatorInfo{}
k.cdc.MustUnmarshal(value, operatorInfo)
AmountFloat, prevAmountOriginalFloat := getOperatorAmountFloat(operatorInfo)
delta := delegation.Amount.ToLegacyDec().Mul(stakerPrice.Sub(prevStakerPrice))
operatorInfo.PriceList = append(operatorInfo.PriceList, &types.PriceInfo{
Price: prevAmountOriginalFloat.Add(delta).Quo(AmountFloat),
Block: uint64(ctx.BlockHeight()),
RoundID: roundID,
})
bz := k.cdc.MustMarshal(operatorInfo)
store.Set(keyOperator, bz)
}
}
}Tools
GitHub Check: CodeQL
[warning] 162-210: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 193-193: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
x/oracle/keeper/native_token.go
Outdated
| func (k Keeper) UpdateNativeTokenByDelegation(ctx sdk.Context, assetID, operatorAddr, stakerAddr string, amountOriginal sdkmath.Int) sdkmath.Int { | ||
| store := ctx.KVStore(k.storeKey) | ||
| keyOperator := types.NativeTokenOperatorKey(assetID, operatorAddr) | ||
| operatorInfo := &types.OperatorInfo{} | ||
| value := store.Get(keyOperator) | ||
| if value == nil { | ||
| operatorInfo = types.NewOperatorInfo(operatorAddr) | ||
| } else { | ||
| k.cdc.MustUnmarshal(value, operatorInfo) | ||
| } | ||
| stakerInfo := &types.StakerInfo{} | ||
| keyStaker := types.NativeTokenStakerKey(assetID, stakerAddr) | ||
| if value = store.Get(keyStaker); value == nil { | ||
| panic("staker must exist before delegation") | ||
| } | ||
| k.cdc.MustUnmarshal(value, stakerInfo) | ||
|
|
||
| operatorAmountFloat, operatorAmountOriginalFloat := getOperatorAmountFloat(operatorInfo) | ||
| amountFloat, amountOriginalFloat := parseStakerAmountOriginalInt(amountOriginal, stakerInfo) | ||
|
|
||
| operatorAmountOriginalFloat = operatorAmountOriginalFloat.Add(amountOriginalFloat) | ||
| operatorAmountFloat = operatorAmountFloat.Add(amountFloat) | ||
|
|
||
| // update operator's price for native token base on new delegation | ||
| operatorInfo.PriceList = append(operatorInfo.PriceList, &types.PriceInfo{ | ||
| Price: operatorAmountOriginalFloat.Quo(operatorAmountFloat), | ||
| Block: uint64(ctx.BlockHeight()), | ||
| }) | ||
|
|
||
| // update operator's total amount for native token, for this 'amount' we don't disginguish different tokens from different stakers. That difference reflects in 'operator price' | ||
| operatorInfo.TotalAmount = operatorAmountFloat.RoundInt() | ||
| bz := k.cdc.MustMarshal(operatorInfo) | ||
| store.Set(keyOperator, bz) | ||
| amountInt := amountFloat.RoundInt() | ||
| // update staker's related operator list | ||
| keyDelegation := types.NativeTokenStakerDelegationKey(assetID, stakerAddr) | ||
| stakerDelegation := &types.StakerDelegationInfo{} | ||
| if value = store.Get(keyDelegation); value == nil { | ||
| stakerDelegation.Delegations = []*types.DelegationInfo{ | ||
| { | ||
| OperatorAddr: operatorAddr, | ||
| Amount: amountInt, | ||
| }, | ||
| } | ||
| } else { | ||
| k.cdc.MustUnmarshal(value, stakerDelegation) | ||
| for idx, delegationInfo := range stakerDelegation.Delegations { | ||
| if delegationInfo.OperatorAddr == operatorAddr { | ||
| if delegationInfo.Amount = delegationInfo.Amount.Add(amountInt); !delegationInfo.Amount.IsPositive() { | ||
| stakerDelegation.Delegations = append(stakerDelegation.Delegations[0:idx], stakerDelegation.Delegations[idx:]...) | ||
| } | ||
| value = k.cdc.MustMarshal(stakerDelegation) | ||
| store.Set(keyDelegation, value) | ||
| return amountInt | ||
| } | ||
| } | ||
| stakerDelegation.Delegations = append(stakerDelegation.Delegations, &types.DelegationInfo{ | ||
| OperatorAddr: operatorAddr, | ||
| Amount: amountInt, | ||
| }) | ||
| } | ||
| // update staker delegation infos for related operators | ||
| value = k.cdc.MustMarshal(stakerDelegation) | ||
| store.Set(keyDelegation, value) | ||
|
|
||
| return amountInt |
There was a problem hiding this comment.
Replace panic statement with proper error handling.
The function contains a panic statement if the staker does not exist. Replace it with proper error handling to avoid crashing the application.
Apply this diff to replace the panic statement with proper error handling:
- if value = store.Get(keyStaker); value == nil {
- panic("staker must exist before delegation")
- }
+ if value = store.Get(keyStaker); value == nil {
+ return nil, errors.New("staker must exist before delegation")
+ }Committable suggestion was skipped due to low confidence.
There was a problem hiding this comment.
Actionable comments posted: 4
Outside diff range, codebase verification and nitpick comments (2)
x/oracle/keeper/native_token_test.go (1)
165-282: LGTM! Consider adding more comments for better readability.The implementation of the
convertBalanceChangeToBytesfunction is correct. Adding more comments to explain the bitwise operations would improve readability and maintainability.x/oracle/keeper/native_token.go (1)
307-366: LGTM! Consider adding more comments for better readability.The implementation of the
parseBalanceChangefunction is correct. Adding more comments to explain the bitwise operations would improve readability and maintainability.Tools
golangci-lint
343-343: commentFormatting: put a space between
//and comment text(gocritic)
GitHub Check: Run golangci-lint
[failure] 343-343:
commentFormatting: put a space between//and comment text (gocritic)
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
Files selected for processing (3)
- go.mod (1 hunks)
- x/oracle/keeper/native_token.go (1 hunks)
- x/oracle/keeper/native_token_test.go (1 hunks)
Additional context used
golangci-lint
x/oracle/keeper/native_token.go
343-343: commentFormatting: put a space between
//and comment text(gocritic)
[warning] 211-211: indent-error-flow: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)
(revive)
GitHub Check: CodeQL
x/oracle/keeper/native_token.go
[warning] 248-296: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 279-279: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
GitHub Check: Run golangci-lint
x/oracle/keeper/native_token.go
[failure] 343-343:
commentFormatting: put a space between//and comment text (gocritic)
[warning] 211-211:
indent-error-flow: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary) (revive)
Additional comments not posted (7)
go.mod (1)
158-158: Verify the compatibility and security of the added dependency.The new indirect dependency
github.com/imroc/biushould be checked for compatibility with the existing codebase and potential security vulnerabilities. Ensure that the dependency is well-maintained and supported.Run the following script to check for any known vulnerabilities in the added dependency:
x/oracle/keeper/native_token_test.go (1)
50-163: LGTM! Consider adding more edge cases.The test cases for the native token lifecycle are well-implemented. Consider adding more edge cases, such as invalid inputs or edge conditions, to ensure comprehensive coverage.
x/oracle/keeper/native_token.go (5)
21-30: LGTM!The implementation of the
GetStakerInfofunction is correct.
32-41: LGTM!The implementation of the
GetStakerDelegationsfunction is correct.
43-52: LGTM!The implementation of the
GetOperatorInfofunction is correct.
224-233: LGTM!The implementation of the
GetStakerListfunction is correct.
300-305: LGTM!The implementation of the
getStakerListfunction is correct.
x/oracle/keeper/native_token.go
Outdated
| func (k Keeper) GetNativeTokenPriceUSDForOperator(ctx sdk.Context, assetID string) (types.Price, error) { | ||
| parsedAssetID := strings.Split(assetID, "_") | ||
| if len(parsedAssetID) != 3 { | ||
| return types.Price{}, types.ErrGetPriceAssetNotFound | ||
| } | ||
| assetID = strings.Join([]string{parsedAssetID[0], parsedAssetID[1]}, "_") | ||
| operatorAddr := parsedAssetID[2] | ||
|
|
||
| store := ctx.KVStore(k.storeKey) | ||
| key := types.NativeTokenOperatorKey(assetID, operatorAddr) | ||
| if value := store.Get(key); value == nil { | ||
| return types.Price{}, types.ErrGetPriceAssetNotFound | ||
| } else { | ||
| operatorInfo := &types.OperatorInfo{} | ||
| k.cdc.MustUnmarshal(value, operatorInfo) | ||
| baseTokenUSDPrice, err := k.GetSpecifiedAssetsPrice(ctx, assetstypes.GetBaseTokenForNativeToken(assetID)) | ||
| if err != nil { | ||
| return types.Price{}, types.ErrGetPriceAssetNotFound | ||
| } | ||
| operatorPriceFloat := getLatestOperatorPriceFloat(operatorInfo) | ||
| baseTokenUSDPrice.Value = (baseTokenUSDPrice.Value.ToLegacyDec().Mul(operatorPriceFloat)).RoundInt() | ||
| return baseTokenUSDPrice, nil | ||
| } |
There was a problem hiding this comment.
Remove else block for better readability.
The function contains an else block that can be removed for better readability.
Apply this diff to remove the else block:
if value := store.Get(key); value == nil {
return types.Price{}, types.ErrGetPriceAssetNotFound
}
- else {
- operatorInfo := &types.OperatorInfo{}
- k.cdc.MustUnmarshal(value, operatorInfo)
- baseTokenUSDPrice, err := k.GetSpecifiedAssetsPrice(ctx, assetstypes.GetBaseTokenForNativeToken(assetID))
- if err != nil {
- return types.Price{}, types.ErrGetPriceAssetNotFound
- }
- operatorPriceFloat := getLatestOperatorPriceFloat(operatorInfo)
- baseTokenUSDPrice.Value = (baseTokenUSDPrice.Value.ToLegacyDec().Mul(operatorPriceFloat)).RoundInt()
- return baseTokenUSDPrice, nil
- }
+ operatorInfo := &types.OperatorInfo{}
+ k.cdc.MustUnmarshal(value, operatorInfo)
+ baseTokenUSDPrice, err := k.GetSpecifiedAssetsPrice(ctx, assetstypes.GetBaseTokenForNativeToken(assetID))
+ if err != nil {
+ return types.Price{}, types.ErrGetPriceAssetNotFound
+ }
+ operatorPriceFloat := getLatestOperatorPriceFloat(operatorInfo)
+ baseTokenUSDPrice.Value = (baseTokenUSDPrice.Value.ToLegacyDec().Mul(operatorPriceFloat)).RoundInt()
+ return baseTokenUSDPrice, nilCommittable 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.
| func (k Keeper) GetNativeTokenPriceUSDForOperator(ctx sdk.Context, assetID string) (types.Price, error) { | |
| parsedAssetID := strings.Split(assetID, "_") | |
| if len(parsedAssetID) != 3 { | |
| return types.Price{}, types.ErrGetPriceAssetNotFound | |
| } | |
| assetID = strings.Join([]string{parsedAssetID[0], parsedAssetID[1]}, "_") | |
| operatorAddr := parsedAssetID[2] | |
| store := ctx.KVStore(k.storeKey) | |
| key := types.NativeTokenOperatorKey(assetID, operatorAddr) | |
| if value := store.Get(key); value == nil { | |
| return types.Price{}, types.ErrGetPriceAssetNotFound | |
| } else { | |
| operatorInfo := &types.OperatorInfo{} | |
| k.cdc.MustUnmarshal(value, operatorInfo) | |
| baseTokenUSDPrice, err := k.GetSpecifiedAssetsPrice(ctx, assetstypes.GetBaseTokenForNativeToken(assetID)) | |
| if err != nil { | |
| return types.Price{}, types.ErrGetPriceAssetNotFound | |
| } | |
| operatorPriceFloat := getLatestOperatorPriceFloat(operatorInfo) | |
| baseTokenUSDPrice.Value = (baseTokenUSDPrice.Value.ToLegacyDec().Mul(operatorPriceFloat)).RoundInt() | |
| return baseTokenUSDPrice, nil | |
| } | |
| func (k Keeper) GetNativeTokenPriceUSDForOperator(ctx sdk.Context, assetID string) (types.Price, error) { | |
| parsedAssetID := strings.Split(assetID, "_") | |
| if len(parsedAssetID) != 3 { | |
| return types.Price{}, types.ErrGetPriceAssetNotFound | |
| } | |
| assetID = strings.Join([]string{parsedAssetID[0], parsedAssetID[1]}, "_") | |
| operatorAddr := parsedAssetID[2] | |
| store := ctx.KVStore(k.storeKey) | |
| key := types.NativeTokenOperatorKey(assetID, operatorAddr) | |
| if value := store.Get(key); value == nil { | |
| return types.Price{}, types.ErrGetPriceAssetNotFound | |
| } | |
| operatorInfo := &types.OperatorInfo{} | |
| k.cdc.MustUnmarshal(value, operatorInfo) | |
| baseTokenUSDPrice, err := k.GetSpecifiedAssetsPrice(ctx, assetstypes.GetBaseTokenForNativeToken(assetID)) | |
| if err != nil { | |
| return types.Price{}, types.ErrGetPriceAssetNotFound | |
| } | |
| operatorPriceFloat := getLatestOperatorPriceFloat(operatorInfo) | |
| baseTokenUSDPrice.Value = (baseTokenUSDPrice.Value.ToLegacyDec().Mul(operatorPriceFloat)).RoundInt() | |
| return baseTokenUSDPrice, nil | |
| } |
Tools
golangci-lint
[warning] 211-211: indent-error-flow: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary)
(revive)
GitHub Check: Run golangci-lint
[warning] 211-211:
indent-error-flow: if block ends with a return statement, so drop this else and outdent its block (move short variable declaration to its own line if necessary) (revive)
| func (k Keeper) UpdateNativeTokenByBalanceChange(ctx sdk.Context, assetID string, rawData []byte, roundID uint64) error { | ||
| if len(rawData) < 32 { | ||
| return errors.New("length of indicate maps for stakers shoule be exactly 32 bytes") | ||
| } | ||
| sl := k.getStakerList(ctx, assetID) | ||
| if len(sl.StakerAddrs) == 0 { | ||
| return errors.New("staker list is empty") | ||
| } | ||
| stakerChanges, err := parseBalanceChange(rawData, sl) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| store := ctx.KVStore(k.storeKey) | ||
| for stakerAddr, change := range stakerChanges { | ||
| key := types.NativeTokenStakerKey(assetID, stakerAddr) | ||
| value := store.Get(key) | ||
| if value == nil { | ||
| return errors.New("stakerInfo does not exist") | ||
| } | ||
| stakerInfo := &types.StakerInfo{} | ||
| k.cdc.MustUnmarshal(value, stakerInfo) | ||
| changeOriginalFloat := sdkmath.LegacyNewDec(int64(change)) | ||
| totalAmountFloat, totalAmountOriginalFloat := parseStakerAmountInt(stakerInfo.TotalDeposit, stakerInfo) | ||
| totalAmountOriginalFloat = totalAmountOriginalFloat.Add(changeOriginalFloat) | ||
| prevStakerPrice := getLatestStakerPriceFloat(stakerInfo) | ||
| // update staker price based on beacon chain effective balance change | ||
| stakerPrice := totalAmountOriginalFloat.Quo(totalAmountFloat) | ||
| stakerInfo.PriceList = append(stakerInfo.PriceList, &types.PriceInfo{ | ||
| Price: stakerPrice, | ||
| Block: uint64(ctx.BlockHeight()), | ||
| RoundID: roundID, | ||
| }) | ||
| bz := k.cdc.MustMarshal(stakerInfo) | ||
| store.Set(key, bz) | ||
| // update related operator's price | ||
| keyStakerDelegations := types.NativeTokenStakerDelegationKey(assetID, stakerAddr) | ||
| value = store.Get(keyStakerDelegations) | ||
| if value != nil { | ||
| delegationInfo := &types.StakerDelegationInfo{} | ||
| k.cdc.MustUnmarshal(value, delegationInfo) | ||
| for _, delegation := range delegationInfo.Delegations { | ||
| keyOperator := types.NativeTokenOperatorKey(assetID, delegation.OperatorAddr) | ||
| value = store.Get(keyOperator) | ||
| if value == nil { | ||
| panic("staker delegation related to operator not exists") | ||
| } | ||
| operatorInfo := &types.OperatorInfo{} | ||
| k.cdc.MustUnmarshal(value, operatorInfo) | ||
| AmountFloat, prevAmountOriginalFloat := getOperatorAmountFloat(operatorInfo) | ||
| delta := delegation.Amount.ToLegacyDec().Mul(stakerPrice.Sub(prevStakerPrice)) | ||
| operatorInfo.PriceList = append(operatorInfo.PriceList, &types.PriceInfo{ | ||
| Price: prevAmountOriginalFloat.Add(delta).Quo(AmountFloat), | ||
| Block: uint64(ctx.BlockHeight()), | ||
| RoundID: roundID, | ||
| }) | ||
| bz := k.cdc.MustMarshal(operatorInfo) | ||
| store.Set(keyOperator, bz) | ||
| } | ||
|
|
||
| } | ||
|
|
||
| } | ||
| return nil |
There was a problem hiding this comment.
Replace panic statement with proper error handling.
The function contains a panic statement if the staker delegation related to the operator does not exist. Replace it with proper error handling to avoid crashing the application.
Apply this diff to replace the panic statement with proper error handling:
- if value == nil {
- panic("staker delegation related to operator not exists")
- }
+ if value == nil {
+ return errors.New("staker delegation related to operator not exists")
+ }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.
| func (k Keeper) UpdateNativeTokenByBalanceChange(ctx sdk.Context, assetID string, rawData []byte, roundID uint64) error { | |
| if len(rawData) < 32 { | |
| return errors.New("length of indicate maps for stakers shoule be exactly 32 bytes") | |
| } | |
| sl := k.getStakerList(ctx, assetID) | |
| if len(sl.StakerAddrs) == 0 { | |
| return errors.New("staker list is empty") | |
| } | |
| stakerChanges, err := parseBalanceChange(rawData, sl) | |
| if err != nil { | |
| return err | |
| } | |
| store := ctx.KVStore(k.storeKey) | |
| for stakerAddr, change := range stakerChanges { | |
| key := types.NativeTokenStakerKey(assetID, stakerAddr) | |
| value := store.Get(key) | |
| if value == nil { | |
| return errors.New("stakerInfo does not exist") | |
| } | |
| stakerInfo := &types.StakerInfo{} | |
| k.cdc.MustUnmarshal(value, stakerInfo) | |
| changeOriginalFloat := sdkmath.LegacyNewDec(int64(change)) | |
| totalAmountFloat, totalAmountOriginalFloat := parseStakerAmountInt(stakerInfo.TotalDeposit, stakerInfo) | |
| totalAmountOriginalFloat = totalAmountOriginalFloat.Add(changeOriginalFloat) | |
| prevStakerPrice := getLatestStakerPriceFloat(stakerInfo) | |
| // update staker price based on beacon chain effective balance change | |
| stakerPrice := totalAmountOriginalFloat.Quo(totalAmountFloat) | |
| stakerInfo.PriceList = append(stakerInfo.PriceList, &types.PriceInfo{ | |
| Price: stakerPrice, | |
| Block: uint64(ctx.BlockHeight()), | |
| RoundID: roundID, | |
| }) | |
| bz := k.cdc.MustMarshal(stakerInfo) | |
| store.Set(key, bz) | |
| // update related operator's price | |
| keyStakerDelegations := types.NativeTokenStakerDelegationKey(assetID, stakerAddr) | |
| value = store.Get(keyStakerDelegations) | |
| if value != nil { | |
| delegationInfo := &types.StakerDelegationInfo{} | |
| k.cdc.MustUnmarshal(value, delegationInfo) | |
| for _, delegation := range delegationInfo.Delegations { | |
| keyOperator := types.NativeTokenOperatorKey(assetID, delegation.OperatorAddr) | |
| value = store.Get(keyOperator) | |
| if value == nil { | |
| panic("staker delegation related to operator not exists") | |
| } | |
| operatorInfo := &types.OperatorInfo{} | |
| k.cdc.MustUnmarshal(value, operatorInfo) | |
| AmountFloat, prevAmountOriginalFloat := getOperatorAmountFloat(operatorInfo) | |
| delta := delegation.Amount.ToLegacyDec().Mul(stakerPrice.Sub(prevStakerPrice)) | |
| operatorInfo.PriceList = append(operatorInfo.PriceList, &types.PriceInfo{ | |
| Price: prevAmountOriginalFloat.Add(delta).Quo(AmountFloat), | |
| Block: uint64(ctx.BlockHeight()), | |
| RoundID: roundID, | |
| }) | |
| bz := k.cdc.MustMarshal(operatorInfo) | |
| store.Set(keyOperator, bz) | |
| } | |
| } | |
| } | |
| return nil | |
| func (k Keeper) UpdateNativeTokenByBalanceChange(ctx sdk.Context, assetID string, rawData []byte, roundID uint64) error { | |
| if len(rawData) < 32 { | |
| return errors.New("length of indicate maps for stakers shoule be exactly 32 bytes") | |
| } | |
| sl := k.getStakerList(ctx, assetID) | |
| if len(sl.StakerAddrs) == 0 { | |
| return errors.New("staker list is empty") | |
| } | |
| stakerChanges, err := parseBalanceChange(rawData, sl) | |
| if err != nil { | |
| return err | |
| } | |
| store := ctx.KVStore(k.storeKey) | |
| for stakerAddr, change := range stakerChanges { | |
| key := types.NativeTokenStakerKey(assetID, stakerAddr) | |
| value := store.Get(key) | |
| if value == nil { | |
| return errors.New("stakerInfo does not exist") | |
| } | |
| stakerInfo := &types.StakerInfo{} | |
| k.cdc.MustUnmarshal(value, stakerInfo) | |
| changeOriginalFloat := sdkmath.LegacyNewDec(int64(change)) | |
| totalAmountFloat, totalAmountOriginalFloat := parseStakerAmountInt(stakerInfo.TotalDeposit, stakerInfo) | |
| totalAmountOriginalFloat = totalAmountOriginalFloat.Add(changeOriginalFloat) | |
| prevStakerPrice := getLatestStakerPriceFloat(stakerInfo) | |
| // update staker price based on beacon chain effective balance change | |
| stakerPrice := totalAmountOriginalFloat.Quo(totalAmountFloat) | |
| stakerInfo.PriceList = append(stakerInfo.PriceList, &types.PriceInfo{ | |
| Price: stakerPrice, | |
| Block: uint64(ctx.BlockHeight()), | |
| RoundID: roundID, | |
| }) | |
| bz := k.cdc.MustMarshal(stakerInfo) | |
| store.Set(key, bz) | |
| // update related operator's price | |
| keyStakerDelegations := types.NativeTokenStakerDelegationKey(assetID, stakerAddr) | |
| value = store.Get(keyStakerDelegations) | |
| if value != nil { | |
| delegationInfo := &types.StakerDelegationInfo{} | |
| k.cdc.MustUnmarshal(value, delegationInfo) | |
| for _, delegation := range delegationInfo.Delegations { | |
| keyOperator := types.NativeTokenOperatorKey(assetID, delegation.OperatorAddr) | |
| value = store.Get(keyOperator) | |
| if value == nil { | |
| return errors.New("staker delegation related to operator not exists") | |
| } | |
| operatorInfo := &types.OperatorInfo{} | |
| k.cdc.MustUnmarshal(value, operatorInfo) | |
| AmountFloat, prevAmountOriginalFloat := getOperatorAmountFloat(operatorInfo) | |
| delta := delegation.Amount.ToLegacyDec().Mul(stakerPrice.Sub(prevStakerPrice)) | |
| operatorInfo.PriceList = append(operatorInfo.PriceList, &types.PriceInfo{ | |
| Price: prevAmountOriginalFloat.Add(delta).Quo(AmountFloat), | |
| Block: uint64(ctx.BlockHeight()), | |
| RoundID: roundID, | |
| }) | |
| bz := k.cdc.MustMarshal(operatorInfo) | |
| store.Set(keyOperator, bz) | |
| } | |
| } | |
| } | |
| return nil |
Tools
GitHub Check: CodeQL
[warning] 248-296: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 279-279: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
| // UpdateNativeTokenByDelegation update operator's price, operator's totalAmount, operator's totalShare, staker's share bsed on either delegation or undelegation | ||
| // this amount passed in from delegation hooks represent originalToken(not virtualLST) | ||
| func (k Keeper) UpdateNativeTokenByDelegation(ctx sdk.Context, assetID, operatorAddr, stakerAddr string, amountOriginal sdkmath.Int) sdkmath.Int { | ||
| store := ctx.KVStore(k.storeKey) | ||
| keyOperator := types.NativeTokenOperatorKey(assetID, operatorAddr) | ||
| operatorInfo := &types.OperatorInfo{} | ||
| valueOperator := store.Get(keyOperator) | ||
| if valueOperator == nil { | ||
| operatorInfo = types.NewOperatorInfo(operatorAddr) | ||
| } else { | ||
| k.cdc.MustUnmarshal(valueOperator, operatorInfo) | ||
| } | ||
| stakerInfo := &types.StakerInfo{} | ||
| keyStaker := types.NativeTokenStakerKey(assetID, stakerAddr) | ||
| value := store.Get(keyStaker) | ||
| if value == nil { | ||
| panic("staker must exist before delegation") | ||
| } | ||
| k.cdc.MustUnmarshal(value, stakerInfo) | ||
|
|
||
| operatorAmountFloat, operatorAmountOriginalFloat := getOperatorAmountFloat(operatorInfo) | ||
| amountFloat, amountOriginalFloat := parseStakerAmountOriginalInt(amountOriginal, stakerInfo) | ||
|
|
||
| operatorAmountOriginalFloat = operatorAmountOriginalFloat.Add(amountOriginalFloat) | ||
| operatorAmountFloat = operatorAmountFloat.Add(amountFloat) | ||
|
|
||
| // update operator's price for native token base on new delegation | ||
| if valueOperator == nil { | ||
| // undelegation should not happen on nil operatorInfo, this is delegate case | ||
| operatorInfo.PriceList = []*types.PriceInfo{ | ||
| { | ||
| Price: operatorAmountOriginalFloat.Quo(operatorAmountFloat), | ||
| Block: uint64(ctx.BlockHeight()), | ||
| }, | ||
| } | ||
| } else if operatorAmountFloat.IsPositive() { | ||
| // if amount <=0 thies operatorInfo will be rmoved, no need to append any price | ||
| operatorInfo.PriceList = append(operatorInfo.PriceList, &types.PriceInfo{ | ||
| Price: operatorAmountOriginalFloat.Quo(operatorAmountFloat), | ||
| Block: uint64(ctx.BlockHeight()), | ||
| }) | ||
| } | ||
| // update operator's total amount for native token, for this 'amount' we don't disginguish different tokens from different stakers. That difference reflects in 'operator price' | ||
| operatorInfo.TotalAmount = operatorAmountFloat.RoundInt() | ||
| if operatorInfo.TotalAmount.IsPositive() { | ||
| bz := k.cdc.MustMarshal(operatorInfo) | ||
| store.Set(keyOperator, bz) | ||
| } else { | ||
| store.Delete(keyOperator) | ||
| } | ||
| amountInt := amountFloat.RoundInt() | ||
| // update staker's related operator list | ||
| keyDelegation := types.NativeTokenStakerDelegationKey(assetID, stakerAddr) | ||
| stakerDelegation := &types.StakerDelegationInfo{} | ||
| if value = store.Get(keyDelegation); value == nil { | ||
| stakerDelegation.Delegations = []*types.DelegationInfo{ | ||
| { | ||
| OperatorAddr: operatorAddr, | ||
| Amount: amountInt, | ||
| }, | ||
| } | ||
| } else { | ||
| k.cdc.MustUnmarshal(value, stakerDelegation) | ||
| for idx, delegationInfo := range stakerDelegation.Delegations { | ||
| if delegationInfo.OperatorAddr == operatorAddr { | ||
| if delegationInfo.Amount = delegationInfo.Amount.Add(amountInt); !delegationInfo.Amount.IsPositive() { | ||
| stakerDelegation.Delegations = append(stakerDelegation.Delegations[:idx], stakerDelegation.Delegations[idx+1:]...) | ||
| } | ||
| value = k.cdc.MustMarshal(stakerDelegation) | ||
| store.Set(keyDelegation, value) | ||
| return amountInt | ||
| } | ||
| } | ||
| stakerDelegation.Delegations = append(stakerDelegation.Delegations, &types.DelegationInfo{ | ||
| OperatorAddr: operatorAddr, | ||
| Amount: amountInt, | ||
| }) | ||
| } | ||
| // update staker delegation infos for related operators | ||
| value = k.cdc.MustMarshal(stakerDelegation) | ||
| store.Set(keyDelegation, value) | ||
|
|
||
| return amountInt | ||
| } |
There was a problem hiding this comment.
Replace panic statement with proper error handling.
The function contains a panic statement if the staker does not exist. Replace it with proper error handling to avoid crashing the application.
Apply this diff to replace the panic statement with proper error handling:
- if value == nil {
- panic("staker must exist before delegation")
- }
+ if value == nil {
+ return sdkmath.Int{}, errors.New("staker must exist before delegation")
+ }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.
| // UpdateNativeTokenByDelegation update operator's price, operator's totalAmount, operator's totalShare, staker's share bsed on either delegation or undelegation | |
| // this amount passed in from delegation hooks represent originalToken(not virtualLST) | |
| func (k Keeper) UpdateNativeTokenByDelegation(ctx sdk.Context, assetID, operatorAddr, stakerAddr string, amountOriginal sdkmath.Int) sdkmath.Int { | |
| store := ctx.KVStore(k.storeKey) | |
| keyOperator := types.NativeTokenOperatorKey(assetID, operatorAddr) | |
| operatorInfo := &types.OperatorInfo{} | |
| valueOperator := store.Get(keyOperator) | |
| if valueOperator == nil { | |
| operatorInfo = types.NewOperatorInfo(operatorAddr) | |
| } else { | |
| k.cdc.MustUnmarshal(valueOperator, operatorInfo) | |
| } | |
| stakerInfo := &types.StakerInfo{} | |
| keyStaker := types.NativeTokenStakerKey(assetID, stakerAddr) | |
| value := store.Get(keyStaker) | |
| if value == nil { | |
| panic("staker must exist before delegation") | |
| } | |
| k.cdc.MustUnmarshal(value, stakerInfo) | |
| operatorAmountFloat, operatorAmountOriginalFloat := getOperatorAmountFloat(operatorInfo) | |
| amountFloat, amountOriginalFloat := parseStakerAmountOriginalInt(amountOriginal, stakerInfo) | |
| operatorAmountOriginalFloat = operatorAmountOriginalFloat.Add(amountOriginalFloat) | |
| operatorAmountFloat = operatorAmountFloat.Add(amountFloat) | |
| // update operator's price for native token base on new delegation | |
| if valueOperator == nil { | |
| // undelegation should not happen on nil operatorInfo, this is delegate case | |
| operatorInfo.PriceList = []*types.PriceInfo{ | |
| { | |
| Price: operatorAmountOriginalFloat.Quo(operatorAmountFloat), | |
| Block: uint64(ctx.BlockHeight()), | |
| }, | |
| } | |
| } else if operatorAmountFloat.IsPositive() { | |
| // if amount <=0 thies operatorInfo will be rmoved, no need to append any price | |
| operatorInfo.PriceList = append(operatorInfo.PriceList, &types.PriceInfo{ | |
| Price: operatorAmountOriginalFloat.Quo(operatorAmountFloat), | |
| Block: uint64(ctx.BlockHeight()), | |
| }) | |
| } | |
| // update operator's total amount for native token, for this 'amount' we don't disginguish different tokens from different stakers. That difference reflects in 'operator price' | |
| operatorInfo.TotalAmount = operatorAmountFloat.RoundInt() | |
| if operatorInfo.TotalAmount.IsPositive() { | |
| bz := k.cdc.MustMarshal(operatorInfo) | |
| store.Set(keyOperator, bz) | |
| } else { | |
| store.Delete(keyOperator) | |
| } | |
| amountInt := amountFloat.RoundInt() | |
| // update staker's related operator list | |
| keyDelegation := types.NativeTokenStakerDelegationKey(assetID, stakerAddr) | |
| stakerDelegation := &types.StakerDelegationInfo{} | |
| if value = store.Get(keyDelegation); value == nil { | |
| stakerDelegation.Delegations = []*types.DelegationInfo{ | |
| { | |
| OperatorAddr: operatorAddr, | |
| Amount: amountInt, | |
| }, | |
| } | |
| } else { | |
| k.cdc.MustUnmarshal(value, stakerDelegation) | |
| for idx, delegationInfo := range stakerDelegation.Delegations { | |
| if delegationInfo.OperatorAddr == operatorAddr { | |
| if delegationInfo.Amount = delegationInfo.Amount.Add(amountInt); !delegationInfo.Amount.IsPositive() { | |
| stakerDelegation.Delegations = append(stakerDelegation.Delegations[:idx], stakerDelegation.Delegations[idx+1:]...) | |
| } | |
| value = k.cdc.MustMarshal(stakerDelegation) | |
| store.Set(keyDelegation, value) | |
| return amountInt | |
| } | |
| } | |
| stakerDelegation.Delegations = append(stakerDelegation.Delegations, &types.DelegationInfo{ | |
| OperatorAddr: operatorAddr, | |
| Amount: amountInt, | |
| }) | |
| } | |
| // update staker delegation infos for related operators | |
| value = k.cdc.MustMarshal(stakerDelegation) | |
| store.Set(keyDelegation, value) | |
| return amountInt | |
| } | |
| // UpdateNativeTokenByDelegation update operator's price, operator's totalAmount, operator's totalShare, staker's share bsed on either delegation or undelegation | |
| // this amount passed in from delegation hooks represent originalToken(not virtualLST) | |
| func (k Keeper) UpdateNativeTokenByDelegation(ctx sdk.Context, assetID, operatorAddr, stakerAddr string, amountOriginal sdkmath.Int) (sdkmath.Int, error) { | |
| store := ctx.KVStore(k.storeKey) | |
| keyOperator := types.NativeTokenOperatorKey(assetID, operatorAddr) | |
| operatorInfo := &types.OperatorInfo{} | |
| valueOperator := store.Get(keyOperator) | |
| if valueOperator == nil { | |
| operatorInfo = types.NewOperatorInfo(operatorAddr) | |
| } else { | |
| k.cdc.MustUnmarshal(valueOperator, operatorInfo) | |
| } | |
| stakerInfo := &types.StakerInfo{} | |
| keyStaker := types.NativeTokenStakerKey(assetID, stakerAddr) | |
| value := store.Get(keyStaker) | |
| if value == nil { | |
| return sdkmath.Int{}, errors.New("staker must exist before delegation") | |
| } | |
| k.cdc.MustUnmarshal(value, stakerInfo) | |
| operatorAmountFloat, operatorAmountOriginalFloat := getOperatorAmountFloat(operatorInfo) | |
| amountFloat, amountOriginalFloat := parseStakerAmountOriginalInt(amountOriginal, stakerInfo) | |
| operatorAmountOriginalFloat = operatorAmountOriginalFloat.Add(amountOriginalFloat) | |
| operatorAmountFloat = operatorAmountFloat.Add(amountFloat) | |
| // update operator's price for native token base on new delegation | |
| if valueOperator == nil { | |
| // undelegation should not happen on nil operatorInfo, this is delegate case | |
| operatorInfo.PriceList = []*types.PriceInfo{ | |
| { | |
| Price: operatorAmountOriginalFloat.Quo(operatorAmountFloat), | |
| Block: uint64(ctx.BlockHeight()), | |
| }, | |
| } | |
| } else if operatorAmountFloat.IsPositive() { | |
| // if amount <=0 thies operatorInfo will be rmoved, no need to append any price | |
| operatorInfo.PriceList = append(operatorInfo.PriceList, &types.PriceInfo{ | |
| Price: operatorAmountOriginalFloat.Quo(operatorAmountFloat), | |
| Block: uint64(ctx.BlockHeight()), | |
| }) | |
| } | |
| // update operator's total amount for native token, for this 'amount' we don't disginguish different tokens from different stakers. That difference reflects in 'operator price' | |
| operatorInfo.TotalAmount = operatorAmountFloat.RoundInt() | |
| if operatorInfo.TotalAmount.IsPositive() { | |
| bz := k.cdc.MustMarshal(operatorInfo) | |
| store.Set(keyOperator, bz) | |
| } else { | |
| store.Delete(keyOperator) | |
| } | |
| amountInt := amountFloat.RoundInt() | |
| // update staker's related operator list | |
| keyDelegation := types.NativeTokenStakerDelegationKey(assetID, stakerAddr) | |
| stakerDelegation := &types.StakerDelegationInfo{} | |
| if value = store.Get(keyDelegation); value == nil { | |
| stakerDelegation.Delegations = []*types.DelegationInfo{ | |
| { | |
| OperatorAddr: operatorAddr, | |
| Amount: amountInt, | |
| }, | |
| } | |
| } else { | |
| k.cdc.MustUnmarshal(value, stakerDelegation) | |
| for idx, delegationInfo := range stakerDelegation.Delegations { | |
| if delegationInfo.OperatorAddr == operatorAddr { | |
| if delegationInfo.Amount = delegationInfo.Amount.Add(amountInt); !delegationInfo.Amount.IsPositive() { | |
| stakerDelegation.Delegations = append(stakerDelegation.Delegations[:idx], stakerDelegation.Delegations[idx+1:]...) | |
| } | |
| value = k.cdc.MustMarshal(stakerDelegation) | |
| store.Set(keyDelegation, value) | |
| return amountInt, nil | |
| } | |
| } | |
| stakerDelegation.Delegations = append(stakerDelegation.Delegations, &types.DelegationInfo{ | |
| OperatorAddr: operatorAddr, | |
| Amount: amountInt, | |
| }) | |
| } | |
| // update staker delegation infos for related operators | |
| value = k.cdc.MustMarshal(stakerDelegation) | |
| store.Set(keyDelegation, value) | |
| return amountInt, nil | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 4
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- x/assets/keeper/bank.go (2 hunks)
- x/delegation/keeper/abci.go (1 hunks)
- x/oracle/keeper/native_token.go (1 hunks)
- x/oracle/keeper/prices.go (4 hunks)
- x/oracle/types/errors.go (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- x/delegation/keeper/abci.go
Additional context used
GitHub Check: CodeQL
x/oracle/keeper/native_token.go
[warning] 248-296: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 279-279: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
Additional comments not posted (26)
x/oracle/types/errors.go (2)
16-16: LGTM!The addition of the new error constant
updateNativeTokenVirtualPriceFailis straightforward and follows the existing pattern.
21-27: LGTM!The registration of the new error constant
ErrUpdateNativeTokenVirtualPriceFailis straightforward and follows the existing pattern.x/assets/keeper/bank.go (3)
11-12: LGTM!The import of the
hexutilpackage from the Ethereum library is necessary for the new logic added to thePerformDepositOrWithdrawfunction.
50-50: LGTM!The call to
UpdateNativeTokenByDepositOrWithdrawis necessary for the new logic to update the native token's state.
52-52: LGTM!The empty line improves readability by separating the new logic from the existing code.
x/oracle/keeper/prices.go (9)
5-5: LGTM!The import of the
stringspackage is necessary for the new logic added to theGetSpecifiedAssetsPricemethod.
58-61: LGTM!The new logic enhances the method to handle native tokens specifically, which is a necessary addition for the new feature.
198-215: LGTM!The new logic enhances the method to handle native tokens specifically, which is a necessary addition for the new feature. The error handling is also appropriately added.
287-293: LGTM!The signature change enhances the method's utility by allowing the caller to access the updated round ID directly after it is incremented.
196-197: LGTM!The call to
IncreaseNextRoundIDand the assignment of its return value toroundIDis necessary for the new logic to update the native token's state.
216-216: LGTM!The empty line improves readability by separating the new logic from the existing code.
208-215: LGTM!The new logic enhances the method to handle native tokens specifically, which is a necessary addition for the new feature.
210-213: LGTM!The call to
UpdateNativeTokenByBalanceChangeand the error handling is necessary for the new logic to update the native token's state.
216-216: LGTM!The empty line improves readability by separating the new logic from the existing code.
x/oracle/keeper/native_token.go (12)
21-30: LGTM!The code changes are approved.
32-41: LGTM!The code changes are approved.
43-52: LGTM!The code changes are approved.
114-197: Replace panic statement with proper error handling.The function contains a panic statement if the staker does not exist. Replace it with proper error handling to avoid crashing the application.
Apply this diff to replace the panic statement with proper error handling:
- if value == nil { - panic("staker must exist before delegation") - } + if value == nil { + return sdkmath.Int{}, errors.New("staker must exist before delegation") + }
199-222: Remove else block for better readability.The function contains an else block that can be removed for better readability.
Apply this diff to remove the else block:
if value := store.Get(key); value == nil { return types.Price{}, types.ErrGetPriceAssetNotFound } - else { - operatorInfo := &types.OperatorInfo{} - k.cdc.MustUnmarshal(value, operatorInfo) - baseTokenUSDPrice, err := k.GetSpecifiedAssetsPrice(ctx, assetstypes.GetBaseTokenForNativeToken(assetID)) - if err != nil { - return types.Price{}, types.ErrGetPriceAssetNotFound - } - operatorPriceFloat := getLatestOperatorPriceFloat(operatorInfo) - baseTokenUSDPrice.Value = (baseTokenUSDPrice.Value.ToLegacyDec().Mul(operatorPriceFloat)).RoundInt() - return baseTokenUSDPrice, nil - } + operatorInfo := &types.OperatorInfo{} + k.cdc.MustUnmarshal(value, operatorInfo) + baseTokenUSDPrice, err := k.GetSpecifiedAssetsPrice(ctx, assetstypes.GetBaseTokenForNativeToken(assetID)) + if err != nil { + return types.Price{}, types.ErrGetPriceAssetNotFound + } + operatorPriceFloat := getLatestOperatorPriceFloat(operatorInfo) + baseTokenUSDPrice.Value = (baseTokenUSDPrice.Value.ToLegacyDec().Mul(operatorPriceFloat)).RoundInt() + return baseTokenUSDPrice, nil
224-233: LGTM!The code changes are approved.
235-297: Replace panic statement with proper error handling.The function contains a panic statement if the staker delegation related to the operator does not exist. Replace it with proper error handling to avoid crashing the application.
Apply this diff to replace the panic statement with proper error handling:
- if value == nil { - panic("staker delegation related to operator not exists") - } + if value == nil { + return errors.New("staker delegation related to operator not exists") + }Address non-determinism in map iteration.
Iteration over a map may introduce non-determinism. Consider sorting the map keys before iteration to ensure deterministic behavior.
Apply this diff to address the non-determinism issue:
+ keys := make([]string, 0, len(stakerChanges)) + for k := range stakerChanges { + keys = append(keys, k) + } + sort.Strings(keys) + for _, stakerAddr := range keys { change := stakerChanges[stakerAddr] value := store.Get(types.NativeTokenStakerKey(assetID, stakerAddr)) if value == nil { return errors.New("stakerInfo does not exist") } stakerInfo := &types.StakerInfo{} k.cdc.MustUnmarshal(value, stakerInfo) changeOriginalFloat := sdkmath.LegacyNewDec(int64(change)) totalAmountFloat, totalAmountOriginalFloat := parseStakerAmountInt(stakerInfo.TotalDeposit, stakerInfo) totalAmountOriginalFloat = totalAmountOriginalFloat.Add(changeOriginalFloat) prevStakerPrice := getLatestStakerPriceFloat(stakerInfo) // update staker price based on beacon chain effective balance change stakerPrice := totalAmountOriginalFloat.Quo(totalAmountFloat) stakerInfo.PriceList = append(stakerInfo.PriceList, &types.PriceInfo{ Price: stakerPrice, Block: uint64(ctx.BlockHeight()), RoundID: roundID, }) bz := k.cdc.MustMarshal(stakerInfo) store.Set(key, bz) // update related operator's price keyStakerDelegations := types.NativeTokenStakerDelegationKey(assetID, stakerAddr) value = store.Get(keyStakerDelegations) if value != nil { delegationInfo := &types.StakerDelegationInfo{} k.cdc.MustUnmarshal(value, delegationInfo) for _, delegation := range delegationInfo.Delegations { keyOperator := types.NativeTokenOperatorKey(assetID, delegation.OperatorAddr) value = store.Get(keyOperator) if value == nil { return errors.New("staker delegation related to operator not exists") } operatorInfo := &types.OperatorInfo{} k.cdc.MustUnmarshal(value, operatorInfo) AmountFloat, prevAmountOriginalFloat := getOperatorAmountFloat(operatorInfo) delta := delegation.Amount.ToLegacyDec().Mul(stakerPrice.Sub(prevStakerPrice)) operatorInfo.PriceList = append(operatorInfo.PriceList, &types.PriceInfo{ Price: prevAmountOriginalFloat.Add(delta).Quo(AmountFloat), Block: uint64(ctx.BlockHeight()), RoundID: roundID, }) bz := k.cdc.MustMarshal(operatorInfo) store.Set(keyOperator, bz) } } }Tools
GitHub Check: CodeQL
[warning] 248-296: Iteration over map
Iteration over map may be a possible source of non-determinism
[warning] 279-279: Panic in BeginBock or EndBlock consensus methods
Possible panics in BeginBock- or EndBlock-related consensus methods could cause a chain halt
300-305: LGTM!The code changes are approved.
307-366: LGTM! Ensure the length of the change value is at least 1 bit.The function is correctly parsing the balance changes. However, the length of the change value must be at least 1 bit, and this should be enforced.
The code changes are approved.
413-416: LGTM!The code changes are approved.
418-421: LGTM!The code changes are approved.
423-450: LGTM!The code changes are approved.
| if assetstypes.IsNativeToken(assetID) { | ||
| // TODO: we skip check for case like withdraw amount>withdrawable is fine since it will fail for later check and the state will be rollback | ||
| actualOpAmount = k.UpdateNativeTokenByDepositOrWithdraw(ctx, assetID, hexutil.Encode(params.StakerAddress), params.OpAmount) | ||
| } |
There was a problem hiding this comment.
LGTM! But add tests for the new logic.
The new logic enhances the function to handle native tokens specifically, which is a necessary addition for the new feature. Ensure that the UpdateNativeTokenByDepositOrWithdraw function is thoroughly tested to verify its correctness.
Do you want me to generate the unit testing code or open a GitHub issue to track this task?
| @@ -43,6 +45,11 @@ | |||
| return errorsmod.Wrapf(assetstypes.ErrInvalidOperationType, "the operation type is: %v", params.Action) | |||
| } | |||
|
|
|||
| if assetstypes.IsNativeToken(assetID) { | |||
| // TODO: we skip check for case like withdraw amount>withdrawable is fine since it will fail for later check and the state will be rollback | |||
There was a problem hiding this comment.
Reminder: Address the TODO comment.
The TODO comment indicates a potential edge case regarding withdrawal amounts. Ensure that this edge case is handled appropriately in the future.
Do you want me to help address this TODO comment or open a GitHub issue to track this task?
| roundID := k.IncreaseNextRoundID(ctx, tokenID) | ||
|
|
||
| // update for native tokens | ||
| // TODO: set hooks as a genral approach |
There was a problem hiding this comment.
Reminder: Address the TODO comment.
The TODO comment indicates a potential enhancement to set hooks as a general approach. Ensure that this enhancement is handled appropriately in the future.
Do you want me to help address this TODO comment or open a GitHub issue to track this task?
| func (k Keeper) UpdateNativeTokenByDepositOrWithdraw(ctx sdk.Context, assetID, stakerAddr string, amount sdkmath.Int) sdkmath.Int { | ||
| // TODO: just convert the number for assets module, and don't store state in oracleModule, can use cache only here | ||
| // TODO: we havn't included validatorIndex here, need the bridge info | ||
| store := ctx.KVStore(k.storeKey) | ||
| key := types.NativeTokenStakerKey(assetID, stakerAddr) | ||
| stakerInfo := &types.StakerInfo{} | ||
| if value := store.Get(key); value == nil { | ||
| // create a new item for this staker | ||
| stakerInfo = types.NewStakerInfo(stakerAddr) | ||
| } else { | ||
| k.cdc.MustUnmarshal(value, stakerInfo) | ||
| } | ||
| latestIndex := len(stakerInfo.PriceList) - 1 | ||
| // calculate amount of virtual LST from nativetoken with price | ||
| amountInt := convertAmountOriginalIntToAmountFloat(amount, stakerInfo.PriceList[latestIndex].Price).RoundInt() | ||
| stakerInfo.TotalDeposit = stakerInfo.TotalDeposit.Add(amountInt) | ||
|
|
||
| keyStakerList := types.NativeTokenStakerListKey(assetID) | ||
| valueStakerList := store.Get(keyStakerList) | ||
| stakerList := &types.StakerList{} | ||
| if valueStakerList != nil { | ||
| k.cdc.MustUnmarshal(valueStakerList, stakerList) | ||
| } | ||
| exists := false | ||
| for idx, stakerExists := range stakerList.StakerAddrs { | ||
| if stakerExists == stakerAddr { | ||
| if !stakerInfo.TotalDeposit.IsPositive() { | ||
| stakerList.StakerAddrs = append(stakerList.StakerAddrs[:idx], stakerList.StakerAddrs[idx+1:]...) | ||
| valueStakerList = k.cdc.MustMarshal(stakerList) | ||
| store.Set(keyStakerList, valueStakerList) | ||
| } | ||
| exists = true | ||
| stakerInfo.StakerIndex = int64(idx) | ||
| break | ||
| } | ||
| } | ||
|
|
||
| // update totalDeposit of staker, and price won't change on either deposit or withdraw | ||
| if !stakerInfo.TotalDeposit.IsPositive() { | ||
| store.Delete(key) | ||
| } else { | ||
| bz := k.cdc.MustMarshal(stakerInfo) | ||
| store.Set(key, bz) | ||
| } | ||
|
|
||
| if !exists { | ||
| if !stakerInfo.TotalDeposit.IsPositive() { | ||
| // this should not happened, if a staker execute the 'withdraw' action, he must have already been in the stakerList | ||
| return amountInt | ||
| } | ||
| stakerList.StakerAddrs = append(stakerList.StakerAddrs, stakerAddr) | ||
| valueStakerList = k.cdc.MustMarshal(stakerList) | ||
| store.Set(keyStakerList, valueStakerList) | ||
| } | ||
| return amountInt | ||
| } |
There was a problem hiding this comment.
Reminder: Complete the TODO items.
The function is correctly updating the staker's total deposit. However, the TODO comments indicate that the implementation is not complete, and there are potential issues with state management and validator index inclusion.
Do you want me to help address the TODO items or open a GitHub issue to track this task?
leonz789
changed the title
cloud8little
requested review from
bwhour,
cloud8little,
MaxMustermann2,
trestinlsd,
mikebraver and
TimmyExogenous
There was a problem hiding this comment.
It seems that this PR treats the native token as an independent token. It might be possible to treat the same native token from different stakers as a base token for processing. The oracle would only need to update the changes in the native token amount for each staker. When the amount increases, it would be equivalent to a deposit operation by the staker. When the amount decreases, it would be treated as a slash operation on the staker, similar to an immediate undelegation where the undelegated share gets slashed. This way, there would be no need to modify the existing processes for deposit, withdraw, delegation, undelegation, and updating voting power.
| @@ -62,6 +64,11 @@ func (k *Keeper) UpdateVotingPower(ctx sdk.Context, avsAddr string) error { | |||
| SelfUSDValue: sdkmath.LegacyNewDec(0), | |||
| ActiveUSDValue: sdkmath.LegacyNewDec(0), | |||
| } | |||
| for _, assetID := range assetstypes.GetNativeTokenAssetIDs() { | |||
| if _, ok := prices[assetID]; ok { | |||
| prices[assetID], err = k.oracleKeeper.GetSpecifiedAssetsPrice(ctx, strings.Join([]string{assetID, operator}, "_")) | |||
There was a problem hiding this comment.
should the returned error be addressed?
Description
This PR add support for feed effective balance change of staked ETH on beaconChain onto exocoreChain.
When staked ETH on beaconChain deposit to exocoreChain, we treat it as a virtual LST and set it with a virtual price of its original amount of ETH on beaconChain.
eg.
validator_A has 32 staked ETH on beaconChain, and then deposit these 32 staked ETH to eoxcoreChain. He will get 32 NRETH(virtualLST) on exocoreChain, with price as 1ETH/NRETH. When validator_A got slashed on beaconChain which lead to his effective balance became to 16 ETH, then the price of his 32NRETH on exocoreChain will change to 16/32=0.5ETH/NRETH correspondingly.
the symbol NRETH is used for clarify
TODO:- [x] testCloses #XXX
Summary by CodeRabbit
New Features
Bug Fixes
Documentation