fix: genesis data validation #259
Conversation
WalkthroughThe pull request introduces modifications to the initialization logic of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
app/app.go(1 hunks)x/assets/keeper/genesis.go(3 hunks)x/assets/keeper/keeper.go(3 hunks)x/assets/types/expected_keepers.go(2 hunks)x/assets/types/genesis.go(1 hunks)x/operator/types/genesis.go(1 hunks)
🔇 Additional comments (12)
x/assets/keeper/genesis.go (2)
5-7: Imports added correctly
The added imports for utils and delegationtypes are necessary and appropriately included.
Line range hint 51-63: Correct accumulation of exoDelegation for Exocore assets
The logic to accumulate exoDelegation when assetInfo.AssetID == types.ExocoreAssetID is correctly implemented.
x/assets/types/expected_keepers.go (1)
19-22: Addition of AccountKeeper interface is appropriate
The AccountKeeper interface with the GetModuleAccount method is correctly defined to support interactions with module accounts.
x/assets/keeper/keeper.go (3)
18-18: Included ak field in Keeper struct
Adding the ak field of type assetstype.AccountKeeper enhances the keeper's ability to access module account information.
28-28: Updated NewKeeper constructor to accept AccountKeeper
Including ak assetstype.AccountKeeper as a parameter in the NewKeeper function is necessary for initializing the ak field.
41-41: Initialized ak field in Keeper
The ak field is properly initialized within the Keeper struct.
x/assets/types/genesis.go (2)
248-251: Adjusted validation to exclude native asset from total staking check
The validation now skips the total staking amount check for the native asset (ExocoreAssetID). This change acknowledges that native assets are handled by x/bank and should not be subjected to the same constraints.
Line range hint 412-417: Allowed zero values in slashing amounts from the assets pool
The validation logic now permits zero values for slashFromAssetsPool.Amount, ensuring consistency when exporting data that may include zero values.
x/operator/types/genesis.go (1)
412-417: Permitted zero values in slashing amount validation
The updated validation in ValidateSlashStates allows zero values for slashFromAssetsPool.Amount, which aligns with the requirement to handle zero slashing exports consistently.
app/app.go (3)
Line range hint 554-562: LGTM! Dependencies properly ordered.
The addition of AccountKeeper and BankKeeper to DelegationKeeper is consistent with the changes to AssetsKeeper. The dependency order is correct, with AssetsKeeper being initialized first.
Line range hint 1012-1024: LGTM! Module initialization order is correct.
The module initialization order properly handles dependencies:
- Assets module is initialized first
- Delegation module follows
- Other dependent modules are initialized afterward
This order ensures proper genesis data validation, which aligns with the PR objectives.
550-553: LGTM! Verify keeper interface usage.
The addition of AccountKeeper and BankKeeper as dependencies to AssetsKeeper is appropriate for handling non-Ethereum assets. This change aligns with the PR objectives.
Let's verify the keeper interface usage:
✅ Verification successful
Let me verify the keeper interface usage with one more script to check the actual implementation in the assets keeper:
Keeper interfaces are properly defined and used
The verification confirms that:
- The
AccountKeeperandBankKeeperinterfaces are properly defined inx/assets/types/expected_keepers.go - The
AssetsKeepercorrectly includes both keepers as dependencies in its struct - The interfaces define appropriate methods needed for asset management:
AccountKeeper: Access to module accountsBankKeeper: Balance queries and coin operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the AccountKeeper and BankKeeper interfaces are properly defined and used
# Check for AccountKeeper interface definition
ast-grep --pattern 'type AccountKeeper interface {
$$$
}'
# Check for BankKeeper interface definition
ast-grep --pattern 'type BankKeeper interface {
$$$
}'
# Check for usage of these interfaces in AssetsKeeper
ast-grep --pattern 'type Keeper struct {
$$$
ak AccountKeeper
bk BankKeeper
$$$
}'
Length of output: 8216
Script:
#!/bin/bash
# Check the AssetsKeeper struct and its initialization
rg -A 10 "type Keeper struct" x/assets/keeper/
Length of output: 572
|
Testing. |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
x/delegation/types/query.pb.gois excluded by!**/*.pb.gox/delegation/types/query.pb.gw.gois excluded by!**/*.pb.gw.go
📒 Files selected for processing (5)
proto/exocore/delegation/v1/query.proto(2 hunks)x/delegation/client/cli/query.go(2 hunks)x/delegation/keeper/delegation_state.go(1 hunks)x/delegation/keeper/grpc_query.go(1 hunks)x/dogfood/types/params.go(1 hunks)
🔇 Additional comments (7)
x/delegation/keeper/grpc_query.go (1)
61-70: LGTM! Clean implementation following established patterns.
The implementation is concise, handles errors appropriately, and follows the same patterns as other query methods in the file.
proto/exocore/delegation/v1/query.proto (2)
117-127: LGTM! Well-structured message definitions.
The message types are well-defined with clear field names and appropriate documentation.
170-175: LGTM! RPC method follows best practices.
The RPC method is properly configured with:
- Module query safety option
- RESTful HTTP endpoint
- Consistent path parameter mapping
x/delegation/client/cli/query.go (2)
35-35: LGTM! Command properly registered.
The new command is correctly added to the parent command's list.
249-279: LGTM! Well-structured command implementation.
The implementation follows best practices:
- Clear usage description
- Proper argument validation
- Consistent error handling
- Follows the established pattern of other query commands
x/dogfood/types/params.go (2)
123-132: LGTM! Efficient duplicate checking implementation.
The implementation uses a map for O(1) lookup of duplicates, which is the optimal approach. The error message is clear and helpful for debugging.
125-128: Verify ParseID compatibility with non-Ethereum assets.
Since this PR adds support for non-Ethereum assets, we should verify that assetstypes.ParseID can handle all supported asset ID formats.
✅ Verification successful
Based on the gathered information, I can now provide a final response:
ParseID implementation supports non-Ethereum assets
The ParseID function and asset ID validation system is designed to handle both Ethereum and non-Ethereum assets:
- The asset ID format is flexible, using a simple
address_chainIDstructure where:- The address part can be any hex string (not strictly Ethereum-specific)
- The chainID part is a hex-encoded number identifying the client chain
- The system includes specific support for:
- Native assets (ExocoreAssetID)
- Native staking tokens (NST with prefix "NST")
- General staking assets (validated via IsStakingAsset)
The implementation is chain-agnostic and the validation focuses on format correctness rather than enforcing Ethereum-specific rules.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check ParseID implementation for non-Ethereum asset support
# Search for ParseID implementation and tests
ast-grep --pattern 'func ParseID($$$) {
$$$
}'
# Look for test cases covering different asset formats
rg -A 5 "TestParseID"
Length of output: 72
Script:
#!/bin/bash
# Let's try a different approach to locate ParseID implementation and its tests
# Search for ParseID function across the codebase
rg -g '*.go' -B 2 -A 5 'func ParseID'
# Search for any test files containing ParseID
fd -e go -x grep -l "ParseID" {}
# Look for asset ID related constants or documentation
rg -g '*.go' -g '*.md' "asset.*ID"
Length of output: 69736
Script:
#!/bin/bash
# Let's examine the ParseID implementation and its usage pattern
# Look for ParseID implementation
rg -g '*.go' -B 2 -A 10 'func ParseID.*string.*error'
# Look for ValidateID implementation which might be used for validation
rg -g '*.go' -B 2 -A 10 'func ValidateID'
# Search for any tests or documentation about supported asset ID formats
rg -g '*.go' -g '*.md' -i 'asset.*id.*format'
Length of output: 3105
Script:
#!/bin/bash
# Let's check for asset ID validation and native asset support
# Look for native/non-native asset constants and definitions
rg -g '*.go' "NativeAssetID|ExocoreAssetID|NSTIDPrefix"
# Check for any asset ID validation in the assets module
rg -g '*.go' -B 2 -A 5 "IsStakingAsset|IsNST"
# Look for asset ID format examples in tests
rg -g '*_test.go' "assetID.*:.*\".*\""
Length of output: 9902
|
Query test using Command exocored query delegation QueryAssociatedStakersByOperator \
exo18cggcpvwspnd5c6ny8wrqxpffj5zmhklprtnph --output json | jqResponse {
"stakers": [
"0x3e108c058e8066da635321dc3018294ca82ddedf_0x65"
]
} |
total_staking_amountfor theexotoken is not recorded, and hence, the validation fails. Instead, this should be statefully checked.Summary by CodeRabbit
Release Notes
New Features
AssetsKeeper,DelegationKeeper, andStakingKeeper.AccountKeeperinterface for better account management.Bug Fixes
Documentation