add validation for supported nstID #223
Conversation
WalkthroughThe pull request introduces significant modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 golangci-lintlevel=warning msg="The linter 'exportloopref' is deprecated (since v1.60.2) due to: Since Go1.22 (loopvar) this linter is no longer relevant. Replaced by copyloopvar." Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (6)
x/oracle/keeper/native_token.go (6)
24-26: Address TODOs and correct typos in commentsThe comments at lines 24-26 contain TODOs indicating areas that need attention. Consider implementing the validation before invoking oracle-related functions instead of checking hardcoded IDs here. Additionally, there's a typo in line 26: "ralted" should be "related".
Would you like assistance in addressing these TODOs or correcting the typos?
172-172: Fix typo in commentIn the comment at line 173, "deopsit" should be "deposit".
186-187: Address TODO and correct grammar in commentsThere's a TODO at line 186 suggesting to check if the validator has withdrawn all their assets before removing them from the staker's validator list. Implementing this check would enhance the accuracy of the validator list. Additionally, the comments contain grammatical errors:
- "has withdraw all its asset" should be "has withdrawn all its assets".
- "len(stkaerInfo.ValidatorPubkeyList)==0 shoule equal to newBalance.Balance<=0" should be "len(stakerInfo.ValidatorPubkeyList) == 0 should equal newBalance.Balance <= 0".
Would you like assistance in implementing this check and correcting the grammatical errors?
224-225: Correct error message grammarThe error message at line 225, "remove unexist validator", is grammatically incorrect. It should be "remove nonexistent validator" or "attempted to remove a validator that does not exist".
Apply this diff to correct the error message:
-return errors.New("remove unexist validator") +return errors.New("attempted to remove a validator that does not exist")
336-336: Address TODO and correct grammar in commentThe comment at line 336 contains a TODO indicating that the conversion method may need upgrading for future changes. Also, there are grammatical errors:
- "suites" should be "suits".
- "need to be upgrade" should be "need to be upgraded".
Would you like assistance in addressing this TODO and correcting the comment?
269-269: Ensure proper handling ofparseBalanceChangeCappederrorsAt line 269, after calling
parseBalanceChangeCapped, consider providing more context in the error handling to aid in debugging if an error occurs.Apply this diff to enhance error reporting:
if err != nil { - return err + return fmt.Errorf("failed to parse balance change: %w", err) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- x/oracle/keeper/native_token.go (10 hunks)
🧰 Additional context used
📓 Learnings (1)
x/oracle/keeper/native_token.go (2)
Learnt from: leonz789 PR: ExocoreNetwork/exocore#189 File: x/oracle/keeper/native_token.go:0-0 Timestamp: 2024-10-09T01:26:20.424Z Learning: The function `UpdateNativeTokenByDepositOrWithdraw` is invoked through the assets module, and data validation is properly handled there.Learnt from: leonz789 PR: ExocoreNetwork/exocore#189 File: x/oracle/keeper/native_token.go:0-0 Timestamp: 2024-09-20T00:02:14.496Z Learning: The function `UpdateNativeTokenByDepositOrWithdraw` is invoked through the assets module, and data validation is properly handled there.
🔇 Additional comments (1)
x/oracle/keeper/native_token.go (1)
154-157: Check for missingassetIDingetDecimalIn the call to
k.getDecimal(ctx, assetID)at line 154, ensure thatassetIDis valid. IfgetDecimalreturns an error due to a missing or invalidassetID, handle it appropriately.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
x/oracle/keeper/native_token.go (2)
Line range hint
173-195: Consider simplifying the validator list update logicThe logic for updating the validator list could be simplified and made more explicit. Currently, it's not clear why we're only adding validators on deposit and not removing them on withdrawal in this part of the code.
Consider refactoring this section to make the logic more explicit:
if amountInt64 > 0 { // Deposit: add a new validator to the staker's validatorList if not already present if !contains(stakerInfo.ValidatorPubkeyList, validatorPubkey) { stakerInfo.ValidatorPubkeyList = append(stakerInfo.ValidatorPubkeyList, validatorPubkey) } } else { // Withdrawal: remove the validator from the staker's validatorList stakerInfo.ValidatorPubkeyList = removeValidator(stakerInfo.ValidatorPubkeyList, validatorPubkey) } // Helper functions func contains(list []string, item string) bool { for _, v := range list { if v == item { return true } } return false } func removeValidator(list []string, item string) []string { for i, v := range list { if v == item { return append(list[:i], list[i+1:]...) } } return list }This refactoring makes the logic more explicit and handles both deposit and withdrawal cases.
Line range hint
337-411: Consider simplifying and optimizingparseBalanceChangeCappedThe
parseBalanceChangeCappedfunction is quite complex and may be difficult to maintain. Consider breaking it down into smaller, more manageable functions. Additionally, the bit manipulation could potentially be optimized.Consider refactoring this function into smaller, more focused functions:
- A function to parse the index bitmap
- A function to extract the length value
- A function to extract the change value
This would make the code more readable and easier to maintain. For example:
func parseBalanceChangeCapped(rawData []byte, sl types.StakerList) (map[string]int, error) { indexes := parseIndexBitmap(rawData[:32]) changes := rawData[32:] stakerChanges := make(map[string]int) for i, hasChange := range indexes { if hasChange { length, offset := extractLengthValue(changes, offset) change, offset := extractChangeValue(changes, offset, length) stakerChanges[sl.StakerAddrs[i]] = change } } return stakerChanges, nil } func parseIndexBitmap(data []byte) []bool { // Implementation } func extractLengthValue(data []byte, offset int) (int, int) { // Implementation } func extractChangeValue(data []byte, offset int, length int) (int, int) { // Implementation }This refactoring would make the function easier to understand and maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- x/oracle/keeper/native_token.go (10 hunks)
🧰 Additional context used
📓 Learnings (1)
x/oracle/keeper/native_token.go (2)
Learnt from: leonz789 PR: ExocoreNetwork/exocore#189 File: x/oracle/keeper/native_token.go:0-0 Timestamp: 2024-10-09T01:26:20.424Z Learning: The function `UpdateNativeTokenByDepositOrWithdraw` is invoked through the assets module, and data validation is properly handled there.Learnt from: leonz789 PR: ExocoreNetwork/exocore#189 File: x/oracle/keeper/native_token.go:0-0 Timestamp: 2024-09-20T00:02:14.496Z Learning: The function `UpdateNativeTokenByDepositOrWithdraw` is invoked through the assets module, and data validation is properly handled there.
🔇 Additional comments (2)
x/oracle/keeper/native_token.go (2)
Line range hint
1-421: Overall impression: Good changes with room for optimizationThe changes introduce support for multiple networks and implement balance limits for NSTETH assets, which is a good improvement. The new constants, variables, and functions are well-structured and implement the required functionality.
However, there are several areas where the code could be optimized and improved:
- Consider using more efficient data structures (e.g., maps instead of slices) for better performance.
- Unify the error handling approach throughout the file for better consistency.
- Simplify complex functions like
parseBalanceChangeCappedfor better maintainability.- Ensure consistent naming conventions (e.g., fix typos in error names).
These optimizations would enhance the overall quality and performance of the code.
152-158:⚠️ Potential issueCorrect typo in error name
There's a typo in the error name
ErrNSTAssetNotSurpported. It should beErrNSTAssetNotSupported.Apply this diff to correct the typo:
- return types.ErrNSTAssetNotSurpported + return types.ErrNSTAssetNotSupportedLikely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
x/oracle/keeper/native_token.go (4)
27-29: Consider addressing TODO comments regarding validationThe TODO comments at lines 27-29 suggest that hardcoding asset IDs is not ideal and that validation should occur before invoking oracle-related functions. Prioritizing these tasks will enhance code maintainability and flexibility.
179-179: Correct typos in comments for clarityIn the comment at line 179, there are typographical errors. It should read: "one validator can only deposit once before it completes withdrawal, which removes its pubkey from this list. So there's no need to check duplication."
342-343: Plan to upgradeparseBalanceChangeCappedfor future changesThe TODO comment at lines 342-343 indicates that
parseBalanceChangeCappedhas limitations for larger balance changes and may need upgrades if extended to other use cases. Consider creating a plan to address this for future scalability.Would you like assistance in outlining the upgrades or opening a GitHub issue to track this task?
418-418: Improve function comment for better understandingThe function comment at line 418 can be made clearer. Consider rephrasing it to:
// IsLimitedChangeNST checks if the given assetID corresponds to an asset with a capped balance change limitThis enhances readability and understanding of the function's purpose.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- x/oracle/keeper/native_token.go (11 hunks)
- x/oracle/types/errors.go (1 hunks)
✅ Files skipped from review due to trivial changes (1)
- x/oracle/types/errors.go
🧰 Additional context used
📓 Learnings (1)
x/oracle/keeper/native_token.go (3)
Learnt from: leonz789 PR: ExocoreNetwork/exocore#223 File: x/oracle/keeper/native_token.go:175-175 Timestamp: 2024-10-22T08:56:02.545Z Learning: In `x/oracle/keeper/native_token.go`, duplicate entries in `ValidatorPubkeyList` are not possible because a validator can only deposit once before withdrawal, and the public key is removed upon withdrawal.Learnt from: leonz789 PR: ExocoreNetwork/exocore#189 File: x/oracle/keeper/native_token.go:0-0 Timestamp: 2024-09-20T00:02:14.496Z Learning: The function `UpdateNativeTokenByDepositOrWithdraw` is invoked through the assets module, and data validation is properly handled there.Learnt from: leonz789 PR: ExocoreNetwork/exocore#189 File: x/oracle/keeper/native_token.go:0-0 Timestamp: 2024-10-09T01:26:20.424Z Learning: The function `UpdateNativeTokenByDepositOrWithdraw` is invoked through the assets module, and data validation is properly handled there.
| if amountInt64 <= 0 { | ||
| return errors.New("remove unexist validator") |
There was a problem hiding this comment.
Correct the error message for clarity
The error message at line 231 reads "remove unexist validator." Consider changing it to "remove nonexistent validator" for better readability.
Apply this diff to correct the error message:
-return errors.New("remove unexist validator")
+return errors.New("remove nonexistent validator")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if amountInt64 <= 0 { | |
| return errors.New("remove unexist validator") | |
| if amountInt64 <= 0 { | |
| return errors.New("remove nonexistent validator") |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
x/oracle/keeper/native_token.go (3)
179-179: Correct typo in commentIn the comment at line 179, there's a typo: "which remove its pubkey form this list" should be "which removes its pubkey from this list."
Apply this diff to correct the typo:
-// one validator can only deposit once before it completed withdraw which remove its pubkey form this list. So there's no need to check duplication +// One validator can only deposit once before it completes withdrawal, which removes its pubkey from this list. So there's no need to check duplication.
230-231: Ensure error message clarityThe error message at line 231 reads "remove unexist validator." For clarity and proper grammar, consider changing it to "remove nonexistent validator."
Apply this diff to correct the error message:
-return errors.New("remove unexist validator") +return errors.New("remove nonexistent validator")
342-343: Reminder: Address the TODO commentThe TODO comment at lines 342-343 indicates that
parseBalanceChangeCappedmay need upgrades to support larger value changes and other chains. Prioritizing this will ensure future compatibility and scalability.Would you like assistance in implementing these upgrades or opening a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
x/oracle/keeper/native_token.go(11 hunks)
🧰 Additional context used
📓 Learnings (1)
x/oracle/keeper/native_token.go (3)
Learnt from: leonz789
PR: ExocoreNetwork/exocore#223
File: x/oracle/keeper/native_token.go:175-175
Timestamp: 2024-10-22T08:56:02.545Z
Learning: In `x/oracle/keeper/native_token.go`, duplicate entries in `ValidatorPubkeyList` are not possible because a validator can only deposit once before withdrawal, and the public key is removed upon withdrawal.
Learnt from: leonz789
PR: ExocoreNetwork/exocore#189
File: x/oracle/keeper/native_token.go:0-0
Timestamp: 2024-09-20T00:02:14.496Z
Learning: The function `UpdateNativeTokenByDepositOrWithdraw` is invoked through the assets module, and data validation is properly handled there.
Learnt from: leonz789
PR: ExocoreNetwork/exocore#189
File: x/oracle/keeper/native_token.go:0-0
Timestamp: 2024-10-09T01:26:20.424Z
Learning: The function `UpdateNativeTokenByDepositOrWithdraw` is invoked through the assets module, and data validation is properly handled there.
Description
Restrict current implementation to support NSTETH only.
----Closes #XXX
Summary by CodeRabbit
New Features
Bug Fixes
Refactor