feat(genesis export): genesis exporting for avs,distribution modules #209
Conversation
trestinlsd
force-pushed
the
genesis-export
branch
from
96407c6 to
4e24e16
Compare
trestinlsd
force-pushed
the
genesis-export
branch
from
80a0f26 to
b7f239b
Compare
trestinlsd
requested review from
mikebraver,
TimmyExogenous,
magj2006,
bwhour,
adu-web3,
cloud8little,
MaxMustermann2 and
leonz789
|
Here I can query the avs for a specific operator, but fail to get operator list of a specific avs. The reason here is this PR added check for whether the operator is opt into the avs, while it stores the avs address as checksum address( https://github.com/ExocoreNetwork/exocore/blob/develop/x/operator/client/cli/query.go#L386 |
|
Test export distribution failed, run local_node.sh and wait for several epoch(minute) to generate new blocks, distribution module should record the default staker reward. |
|
Warning Rate limit exceeded@trestinlsd has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 2 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThis pull request introduces significant changes to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 24
🧹 Outside diff range and nitpick comments (7)
proto/exocore/avs/v1/genesis.proto (1)
13-37: Consider adding validation rules for addressesThe message contains multiple address fields across different repeated messages. Consider adding gogoproto validation rules to ensure addresses follow the correct format.
Example for AVSInfo:
- repeated AVSInfo avs_infos = 1 [(gogoproto.nullable) = false]; + repeated AVSInfo avs_infos = 1 [ + (gogoproto.nullable) = false, + (gogoproto.jsontag) = "avs_infos", + (validate.rules).repeated.items.string = { + pattern: "^0x[0-9a-fA-F]{40}$" + } + ];x/avs/module.go (1)
132-132: Document the validator updates behavior.Add a comment explaining why validator updates are not needed for this module.
+ // AVS module doesn't modify the validator set during genesis initialization return []abci.ValidatorUpdate{}x/avs/keeper/avs.go (1)
191-208: Consider pagination for large datasets.The current implementation loads all chain ID information into memory at once. For large datasets, this could lead to performance issues or out-of-memory errors.
Consider implementing pagination using the Cosmos SDK's pagination utilities. Here's a suggested signature:
func (k *Keeper) GetAllChainIDInfos( ctx sdk.Context, pagination *query.PageRequest, ) ([]types.ChainIDInfo, *query.PageResponse, error)x/avs/keeper/keeper.go (1)
343-359: Consider adding pagination support.For large-scale deployments, retrieving all AVS information in a single call could lead to performance issues. Consider implementing pagination support using Cosmos SDK's pagination primitives to handle large datasets efficiently.
x/feedistribution/keeper/genesis.go (1)
56-56: Simplifypaniccall by removing unnecessary.Error().The
panicfunction can accept an error directly. Calling.Error()converts the error to a string, butpanicwill handle the error properly without it. Consider removing.Error()for cleaner code.Apply this diff to simplify the panic statement:
- panic(errorsmod.Wrap(err, "Error getting validator data").Error()) + panic(errorsmod.Wrap(err, "Error getting validator data"))x/avs/keeper/genesis.go (1)
12-53: Ensure proper error handling without excessive panickingThe
InitGenesisfunction panics on any error encountered during state initialization. While panicking might be acceptable during genesis initialization since it's a critical failure point, it's essential to consider whether there's a more graceful way to handle errors. Excessive panicking can make debugging difficult and may not provide the best experience for end-users or developers.Consider accumulating errors and returning them at the end of the function, or logging detailed error messages before panicking to provide more context. This approach can improve maintainability and ease of debugging.
x/feedistribution/keeper/keeper.go (1)
128-146: Handle unknown key prefixes explicitly to avoid unintended behavior.The
defaultcase in theswitchstatement silently continues when an unrecognized key prefix is encountered. Consider logging or handling this scenario to aid in debugging and ensure that all relevant data is processed.Add a default case handling:
default: + k.Logger().Warn("unrecognized key prefix", "key", key) continueThis will log a warning when an unknown key prefix is found, helping identify any unexpected keys in the store.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
x/avs/types/genesis.pb.gois excluded by!**/*.pb.gox/feedistribution/types/genesis.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (11)
proto/exocore/avs/v1/genesis.proto(1 hunks)proto/exocore/feedistribution/v1/genesis.proto(2 hunks)x/avs/keeper/avs.go(1 hunks)x/avs/keeper/genesis.go(1 hunks)x/avs/keeper/keeper.go(1 hunks)x/avs/keeper/task.go(6 hunks)x/avs/module.go(2 hunks)x/avs/types/genesis.go(1 hunks)x/feedistribution/keeper/genesis.go(2 hunks)x/feedistribution/keeper/keeper.go(2 hunks)x/operator/keeper/operator.go(2 hunks)
🧰 Additional context used
🪛 buf
proto/exocore/avs/v1/genesis.proto
4-4: import "exocore/avs/v1/tx.proto": file does not exist
(COMPILE)
🔇 Additional comments (16)
x/avs/types/genesis.go (1)
28-31: LGTM! Consider the previous suggestion.
The function is correctly implemented, but its behavior would be improved by the previous suggestion to initialize empty slices in NewGenesisState. This would ensure consistent JSON marshaling behavior.
proto/exocore/avs/v1/genesis.proto (2)
41-46: Investigate missing field number 2 in TaskID message
There's a gap in field numbering (1,3 but no 2). This suggests a field was removed. While this won't affect functionality, it's worth documenting if field 2 was reserved for backward compatibility.
Let's check the history of this file:
✅ Verification successful
Based on the git history, this is a new file that was recently added with only two commits. The first commit added the file, and the second commit only added documentation comments. There's no evidence of a previously removed field 2 in the TaskID message.
Field numbering gap appears intentional for future use
The gap in field numbering (1,3) in TaskID message appears to be intentional as this is a new file with no history of removed fields. This is a common practice in protocol buffers to reserve numbers for future use and maintain backward compatibility.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check git history for any previous versions of this file
git log -p -- proto/exocore/avs/v1/genesis.proto
Length of output: 4118
4-4:
Verify the import path for tx.proto
The import "exocore/avs/v1/tx.proto" cannot be resolved. This could cause compilation issues.
Let's verify if the file exists:
Please ensure that:
- The tx.proto file has been created
- The import path is correct
- Any dependent PRs containing tx.proto are merged first
✅ Verification successful
Let me search for the tx.proto file and its references to verify if it exists or if there are any related files that might help us understand the import situation.
Import path for tx.proto is valid and file exists
The import "exocore/avs/v1/tx.proto" is correctly specified as the file exists at proto/exocore/avs/v1/tx.proto. The import is properly resolved and should not cause any compilation issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if tx.proto exists in the proto directory
fd -t f "tx.proto" proto/
Length of output: 4118
Script:
#!/bin/bash
# Check if tx.proto exists in the avs directory
fd -t f "tx.proto" proto/exocore/avs/
# Check for any references to tx.proto in the codebase
rg -l "tx.proto" proto/
Length of output: 484
🧰 Tools
🪛 buf
4-4: import "exocore/avs/v1/tx.proto": file does not exist
(COMPILE)
x/avs/module.go (1)
Line range hint 120-132: Verify the removal of validator updates propagation.
The changes now return an empty validator update slice instead of propagating updates from the keeper. While this might be intentional if the AVS module doesn't need to modify the validator set, this is a significant behavioral change that warrants verification.
✅ Verification successful
Based on the accumulated evidence from the git history and code analysis:
The change to return empty validator updates is correct and intentional.
The AVS module does not need to modify the validator set during genesis initialization. This is supported by:
- The keeper's
InitGenesisimplementation only handles AVS info setup and has no validator-related functionality - No validator update logic exists in the AVS keeper
- The change was part of a larger genesis implementation PR that properly modularized the functionality
- The comment "// Initialize global index to index in genesis state" indicates this is just state initialization
- Other modules in the codebase (like evm, exomint, etc.) follow the same pattern of returning empty validator updates during genesis
The previous implementation that returned the keeper's result was incorrect as the AVS module's scope does not include validator set management.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if validator updates were previously used
# and if other modules depend on them
# Check for validator update usage in AVS keeper
rg -A 5 "ValidatorUpdate" "x/avs/keeper/"
# Check for AVS validator update dependencies
rg -A 5 "InitGenesis.*avs.*ValidatorUpdate"
Length of output: 85
Script:
#!/bin/bash
# Let's try a broader search to understand the context better
# Check git history for the specific file to see previous implementation
git log -p x/avs/module.go
# Look for any InitGenesis implementations in keeper
rg "func.*InitGenesis" "x/avs/keeper/"
# Check for validator update patterns in the entire codebase
rg "ValidatorUpdate" --type go
# Look for any tests related to AVS genesis
fd "genesis.*test.*go" --exec grep -l "avs"
Length of output: 57777
Script:
#!/bin/bash
# Let's check the AVS keeper's InitGenesis implementation and related code
# Check AVS keeper's InitGenesis implementation
rg -A 5 "func.*InitGenesis" "x/avs/keeper/genesis.go"
# Look for any validator-related functionality in AVS keeper
rg -A 5 "ValidatorUpdate" "x/avs/keeper/"
# Check AVS genesis tests
cat x/avs/types/genesis_test.go
Length of output: 1207
x/avs/keeper/avs.go (2)
193-196: LGTM: Proper store and iterator handling.
The implementation correctly:
- Uses prefix store for isolation
- Uses KVStorePrefixIterator for efficient iteration
- Properly defers iterator closure
191-208: Verify consistent address handling across the codebase.
Based on reported issues with AVS querying, we should verify that address handling is consistent across the codebase, particularly regarding case sensitivity.
✅ Verification successful
Let me analyze the findings from the shell scripts to generate a final response.
The code shows consistent handling of AVS addresses across the codebase:
- AVS addresses are consistently validated using
common.IsHexAddress()before processing - AVS addresses are stored in lowercase format through
strings.ToLower()inGenerateAVSAddr() - All address comparisons use the normalized hex format through
common.HexToAddress() - The
GetAllChainIDInfos()function correctly retrieves addresses from storage without any case sensitivity issues
No inconsistencies found in AVS address handling.
The reported querying issues were likely related to other factors, as the codebase demonstrates proper address normalization and validation throughout the AVS-related functionality.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for inconsistent address handling across the codebase
# Test 1: Find all places where AVS addresses are stored or retrieved
echo "Checking AVS address handling patterns..."
rg -A 2 "AvsAddress|avsAddr|AVSAddr"
# Test 2: Find all hex address conversions and validations
echo "Checking hex address handling..."
rg "common\..*Address|HexToAddress|IsHexAddress"
Length of output: 149775
x/operator/keeper/operator.go (2)
194-196: LGTM! Improved filtering of opted-in AVS addresses.
The addition of the IsOptedIn check ensures that only actively opted-in AVS addresses are returned.
240-242: Verify the address comparison behavior.
Let's verify if there are any other instances of direct address comparison that might need case-insensitive handling.
✅ Verification successful
Let me gather more specific information about address comparisons in the operator package.
Let me gather more specific information about the address comparison in the operator package, particularly focusing on the IsOptedIn function and related code.
Based on the gathered information, I can now provide a final response about the address comparison behavior.
Direct address comparison is safe in this context.
The address comparison avsAddr == keys[1] in the operator package is safe because:
- The AVS addresses are consistently handled as hex addresses and validated through
common.IsHexAddress()checks before storage - The operator addresses are properly validated and normalized through SDK's address handling functions
- The
IsOptedInfunction uses these validated addresses as keys in the store lookups
The direct string comparison is appropriate here as it's comparing already validated and normalized addresses from the store keys.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct string comparisons with addresses
rg --type go 'addr.*==' .
rg --type go '==.*addr' .
# Search for address-related functions that might need case normalization
ast-grep --pattern 'func $_(ctx $_, addr $_) $_'
Length of output: 1242
Script:
#!/bin/bash
# Look for address comparison patterns specifically in the operator package
rg --type go -C 3 'addr|address' ./x/operator/
# Check for any string/address normalization functions
rg --type go -C 2 'strings\.ToLower|normalize|[Cc]ase[Ii]nsensitive' .
# Look for address validation or processing functions
ast-grep --pattern 'func $_($addr string) error'
Length of output: 125075
Script:
#!/bin/bash
# Look for IsOptedIn implementation and usage
rg -C 3 'IsOptedIn' ./x/operator/
# Look for any address comparison or validation in the operator package
ast-grep --pattern 'if $addr == $_'
ast-grep --pattern 'if $_ == $addr'
Length of output: 3017
x/avs/keeper/keeper.go (1)
360-360: LGTM! Good optimization.
The change from value receiver to pointer receiver is a good optimization, especially since Keeper contains multiple fields.
x/avs/keeper/task.go (1)
377-393: Reuse existing IterateResultInfo method.
x/feedistribution/keeper/genesis.go (5)
4-4: LGTM!
Importing errorsmod is necessary for error wrapping later in the code.
20-22: Verify proper initialization of FeePool.
Ensure that genState.FeePool is correctly initialized and contains the expected data to prevent any runtime issues during genesis initialization.
24-26: LGTM!
Setting validator accumulated commissions correctly.
29-31: LGTM!
Validator current rewards are being set appropriately.
34-36: LGTM!
Validator outstanding rewards are initialized properly.
x/avs/keeper/genesis.go (1)
15-16: Clarify the use of //nolint:gosec directives
The //nolint:gosec directive is used to suppress security-related linter warnings from gosec. It's important to ensure that suppressing these warnings is necessary and justified. For example, on line 15:
err := k.SetAVSInfo(ctx, &avs) //nolint:gosecPlease verify whether these //nolint:gosec directives are required. If they are, consider adding comments to explain why it's safe to ignore the linter warnings. This practice helps maintain code readability and aids future developers in understanding the reasoning.
Also applies to: 22-23, 29-30, 40-41
|
export avs module, avs address show mal-formated string. task address is bench32 formatted. |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (13)
x/avs/keeper/msg_server.go (1)
Line range hint
28-46: Implement the TODO methods for AVS registration functionality.Several critical AVS-related methods are currently unimplemented:
- RegisterAVS
- DeRegisterAVS
- RegisterAVSTask
These methods are essential for the AVS module functionality and should be implemented before this feature can be considered complete.
Would you like me to help implement these methods or create GitHub issues to track their implementation? I can assist with:
- Implementing proper parameter validation
- Adding necessary state changes
- Including appropriate event emissions
- Adding error handling
x/avs/keeper/submit_task_test.go (3)
Line range hint
39-85: Consider extracting test configuration into constants or test fixtures.The test setup contains several hardcoded values that could make tests brittle and harder to maintain:
- Operator address "exo13h6xg79g82e2g2vhjwg7j4r2z2hlncelwutkjr"
- Client chain ID 101
- Asset decimals 6
Consider creating test constants or configuration structs:
const ( TestOperatorAddr = "exo13h6xg79g82e2g2vhjwg7j4r2z2hlncelwutkjr" TestClientChainLzID = 101 TestAssetDecimals = 6 )
Line range hint
87-147: Consider using test-specific addresses instead of mainnet USDT address.Using mainnet contract addresses in tests could be confusing and might accidentally interact with real contracts in some environments.
Consider using a clearly marked test address:
var TestUSDTAddress = common.HexToAddress("0x0000000000000000000000000000000000000001")
Line range hint
176-224: Consider adding more test cases for comprehensive coverage.The current tests only cover the happy path for both phases. Consider adding tests for:
- Invalid BLS signatures
- Incorrect task response format
- Missing or invalid task contract address
- Verification that task results are correctly stored
- Error cases when submitting results in wrong order (phase two before phase one)
Example additional test case:
func (suite *AVSTestSuite) TestSubmitTask_InvalidSignature() { suite.prepare() taskRes := avstypes.TaskResponse{TaskID: 1, NumberSum: big.NewInt(100)} jsonData, err := avstypes.MarshalTaskResponse(taskRes) suite.NoError(err) // Use different key for signing to create invalid signature wrongKey, _ := blst.RandKey() wrongSig := wrongKey.Sign(jsonData) info := &avstypes.TaskResultInfo{ // ... setup info BlsSignature: wrongSig.Marshal(), } err = suite.App.AVSManagerKeeper.SubmitTaskResult(suite.Ctx, suite.operatorAddr.String(), info) suite.Error(err) // Should fail with invalid signature error }x/avs/keeper/multi_operator_submit_task_test.go (3)
Line range hint
40-44: Fix AVS address generation in tests.The current AVS address generation using
BytesToAddress([]byte("avsTestAddr"))could lead to malformed addresses, which aligns with the reported issue of corrupted AVS addresses in the system. Consider using a proper hex address string or deriving it deterministically.-suite.avsAddr = common.BytesToAddress([]byte("avsTestAddr")).String() +suite.avsAddr = common.HexToAddress("0x1234567890123456789012345678901234567890").String()
Line range hint
156-170: Consider extracting hardcoded values as constants.The USD values (500, 100) are hardcoded in the setup. Consider extracting these as test constants for better maintainability and clarity.
+const ( + TestAVSUSDValue = 500 + TestOperatorUSDValue = 100 +) -suite.App.OperatorKeeper.SetAVSUSDValue(suite.Ctx, suite.avsAddr, sdkmath.LegacyNewDec(500)) +suite.App.OperatorKeeper.SetAVSUSDValue(suite.Ctx, suite.avsAddr, sdkmath.LegacyNewDec(TestAVSUSDValue)) -SelfUSDValue: sdkmath.LegacyNewDec(100), -TotalUSDValue: sdkmath.LegacyNewDec(100), -ActiveUSDValue: sdkmath.LegacyNewDec(100), +SelfUSDValue: sdkmath.LegacyNewDec(TestOperatorUSDValue), +TotalUSDValue: sdkmath.LegacyNewDec(TestOperatorUSDValue), +ActiveUSDValue: sdkmath.LegacyNewDec(TestOperatorUSDValue),
Line range hint
172-237: Add sub-tests for different scenarios.Consider breaking down the test cases into sub-tests using
t.Run()to cover various scenarios:
- Invalid BLS signatures
- Missing operator registrations
- Invalid task responses
- Out-of-order phase submissions
Example structure:
func (suite *AVSTestSuite) TestSubmitTask_Mul() { testCases := []struct { name string setup func() malformData func(*avstypes.TaskResultInfo) expectedErr error }{ { name: "valid phase one submission", setup: suite.prepareMul, malformData: nil, expectedErr: nil, }, { name: "invalid BLS signature", setup: suite.prepareMul, malformData: func(info *avstypes.TaskResultInfo) { info.BlsSignature = []byte("invalid") }, expectedErr: types.ErrInvalidBlsSignature, }, // Add more test cases } for _, tc := range testCases { suite.Run(tc.name, func() { tc.setup() // Test logic here }) } }x/avs/keeper/task.go (5)
46-62: Optimize slice allocation for better performance.Pre-allocate the slice to avoid unnecessary reallocations during append operations.
func (k Keeper) GetAllTaskInfos(ctx sdk.Context) ([]types.TaskInfo, error) { - var taskInfos []types.TaskInfo store := prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyPrefixAVSTaskInfo) + + // Count items first for optimal slice allocation + count := 0 + iterator := sdk.KVStorePrefixIterator(store, nil) + for ; iterator.Valid(); iterator.Next() { + count++ + } + iterator.Close() + iterator := sdk.KVStorePrefixIterator(store, nil) defer iterator.Close() + taskInfos := make([]types.TaskInfo, 0, count) for ; iterator.Valid(); iterator.Next() { var taskInfo types.TaskInfo err := k.cdc.Unmarshal(iterator.Value(), &taskInfo)
100-117: Optimize slice allocation for better performance.Pre-allocate the slice to avoid unnecessary reallocations during append operations.
func (k *Keeper) GetAllBlsPubKeys(ctx sdk.Context) ([]types.BlsPubKeyInfo, error) { - var pubKeys []types.BlsPubKeyInfo store := prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyPrefixOperatePub) + // Count items first for optimal slice allocation + count := 0 + iterator := sdk.KVStorePrefixIterator(store, nil) + for ; iterator.Valid(); iterator.Next() { + count++ + } + iterator.Close() + iterator := sdk.KVStorePrefixIterator(store, nil) defer iterator.Close() + pubKeys := make([]types.BlsPubKeyInfo, 0, count) for ; iterator.Valid(); iterator.Next() {
167-182: Optimize slice allocation for better performance.Pre-allocate the slice to avoid unnecessary reallocations during append operations.
func (k *Keeper) GetAllTaskNums(ctx sdk.Context) ([]types.TaskID, error) { store := prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyPrefixLatestTaskNum) + + // Count items first for optimal slice allocation + count := 0 + iterator := sdk.KVStorePrefixIterator(store, nil) + for ; iterator.Valid(); iterator.Next() { + count++ + } + iterator.Close() + iterator := sdk.KVStorePrefixIterator(store, nil) defer iterator.Close() - ret := make([]types.TaskID, 0) + ret := make([]types.TaskID, 0, count)
222-238: Optimize slice allocation for better performance.Pre-allocate the slice to avoid unnecessary reallocations during append operations.
func (k *Keeper) GetAllTaskResultInfos(ctx sdk.Context) ([]types.TaskResultInfo, error) { store := prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyPrefixTaskResult) + + // Count items first for optimal slice allocation + count := 0 + iterator := sdk.KVStorePrefixIterator(store, nil) + for ; iterator.Valid(); iterator.Next() { + count++ + } + iterator.Close() + iterator := sdk.KVStorePrefixIterator(store, nil) defer iterator.Close() - ret := make([]types.TaskResultInfo, 0) + ret := make([]types.TaskResultInfo, 0, count)
Line range hint
46-350: Consider implementing a generic iterator pattern.Multiple functions in this file follow the same pattern of iterating over store entries and unmarshaling data. Consider implementing a generic iterator pattern to reduce code duplication and improve maintainability.
Example implementation:
// Generic iterator function func (k Keeper) iterateStore[T any](prefix []byte, unmarshal func([]byte) (T, error)) ([]T, error) { store := prefix.NewStore(ctx.KVStore(k.storeKey), prefix) // Count items first count := 0 iterator := sdk.KVStorePrefixIterator(store, nil) for ; iterator.Valid(); iterator.Next() { count++ } iterator.Close() // Iterate and unmarshal iterator = sdk.KVStorePrefixIterator(store, nil) defer iterator.Close() items := make([]T, 0, count) for ; iterator.Valid(); iterator.Next() { item, err := unmarshal(iterator.Value()) if err != nil { return nil, err } items = append(items, item) } return items, nil }x/avs/keeper/keeper.go (1)
601-607: Enhance BLS signature verification security.The BLS signature verification could be strengthened by:
- Adding a domain separator to prevent cross-protocol attacks
- Including the task contract address in the signed message
- flag, err := blst.VerifySignature(info.BlsSignature, taskResponseDigest, pubKey) + // Add domain separator and task address to prevent cross-protocol attacks + domainSeparator := crypto.Keccak256([]byte("TASK_RESPONSE"), []byte(info.TaskContractAddress)) + messageToVerify := crypto.Keccak256(domainSeparator, taskResponseDigest.Bytes()) + flag, err := blst.VerifySignature(info.BlsSignature, messageToVerify, pubKey)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
app/app.go(1 hunks)x/avs/keeper/genesis.go(1 hunks)x/avs/keeper/keeper.go(3 hunks)x/avs/keeper/msg_server.go(1 hunks)x/avs/keeper/multi_operator_submit_task_test.go(2 hunks)x/avs/keeper/submit_task_test.go(2 hunks)x/avs/keeper/task.go(7 hunks)
✅ Files skipped from review due to trivial changes (1)
- app/app.go
🧰 Additional context used
📓 Learnings (1)
x/avs/keeper/genesis.go (2)
Learnt from: TimmyExogenous
PR: ExocoreNetwork/exocore#108
File: x/delegation/keeper/genesis.go:52-73
Timestamp: 2024-10-09T01:26:20.424Z
Learning: In the `ExportGenesis` function, panicking on errors without additional logging is acceptable as panic provides detailed context for debugging.
Learnt from: TimmyExogenous
PR: ExocoreNetwork/exocore#108
File: x/delegation/keeper/genesis.go:52-73
Timestamp: 2024-09-17T14:45:22.028Z
Learning: In the `ExportGenesis` function, panicking on errors without additional logging is acceptable as panic provides detailed context for debugging.
🪛 GitHub Check: Run golangci-lint
x/avs/keeper/keeper.go
[failure] 7-7:
File is not gofumpt-ed (gofumpt)
🔇 Additional comments (9)
x/avs/keeper/msg_server.go (2)
23-26: LGTM! Error handling follows Go conventions.
The change from SetTaskResultInfo to SubmitTaskResult is implemented correctly with proper error propagation.
Line range hint 1-46: Verify AVS address handling in the keeper methods.
Based on the PR comments, there seems to be an issue with AVS address handling where addresses are stored as checksum addresses but queried in lowercase. Let's verify the address handling in the keeper methods.
x/avs/keeper/genesis.go (3)
4-7: LGTM: Import changes align with new error handling approach.
The addition of errorsmod and removal of unused ABCI types import are appropriate for the updated implementation.
53-90: Verify completeness of exported genesis state.
The implementation looks correct, but let's verify that all required state components are being exported and that there are no missing fields in the genesis state.
Let's check for any potential missing state components:
#!/bin/bash
# Check genesis.proto for all defined fields
rg -A 10 "message GenesisState"
# Check for any keeper methods that might indicate additional state
ast-grep --pattern 'func \(k Keeper\) Set.*\('12-50: Verify address validation in HexToAddress calls.
The direct use of common.HexToAddress without validation could potentially lead to issues if malformed addresses are provided in the genesis state.
Let's verify the address format validation in the codebase:
Consider documenting the security implications of nolint annotations.
The //nolint:gosec annotations are used without explanation. Please document why these security checks are being disabled.
✅ Verification successful
Address validation is properly implemented throughout the codebase.
The codebase shows comprehensive address validation:
-
The
common.IsHexAddress()validation is used consistently beforeHexToAddress()calls in various modules:- x/avs/keeper/task.go validates task contract addresses
- x/avs/keeper/avs.go validates AVS addresses
- x/operator/types/msg.go validates AVS addresses in messages
- x/assets/types validates addresses in genesis and params
-
The
//nolint:gosecannotations are used in genesis initialization where the data has already been validated:- Genesis state validation occurs in
GenesisState.Validate()method - Individual components like AVSInfo, TaskInfo etc. have their own validation
- Genesis state validation occurs in
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if there's any address validation before using HexToAddress
rg -A 3 "HexToAddress.*TaskAddr|HexToAddress.*AvsAddress"
# Check if there are any existing address validation utilities
rg -A 3 "func.*[Vv]alidate.*[Aa]ddress"
Length of output: 16160
Script:
#!/bin/bash
# Check for address validation in AVS types and validation functions
rg -A 5 "func.*[Vv]alidate.*AVSInfo|func.*[Vv]alidate.*TaskInfo|func.*[Vv]alidate.*ChainId" x/avs/types/
# Check for address validation in common utilities
rg -A 5 "func.*[Vv]alidate.*[Aa]ddress" x/common/
# Check for HexToAddress usage and any surrounding validation
ast-grep --pattern 'common.HexToAddress($addr)'
Length of output: 258
Script:
#!/bin/bash
# Check for validation in AVS types
rg -A 5 "func.*[Vv]alidate" x/avs/types/
# Check for HexToAddress usage in the entire codebase
rg -A 3 "HexToAddress"
# Look for any address validation patterns in the codebase
rg -A 3 "IsHexAddress|IsValidAddress|ValidateAddress"
Length of output: 31554
x/avs/keeper/submit_task_test.go (1)
193-193: LGTM! Method rename improves clarity.
The rename from SetTaskResultInfo to SubmitTaskResult better reflects the action being performed, as it's an active operation rather than just setting data.
Also applies to: 222-222
x/avs/keeper/task.go (1)
333-350:
Add error handling and optimize performance.
- Add error handling for AccAddress conversion
- Pre-allocate the slice for better performance
func (k *Keeper) GetAllChallengeInfos(ctx sdk.Context) ([]types.ChallengeInfo, error) {
store := prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyPrefixTaskChallengeResult)
+
+ // Count items first for optimal slice allocation
+ count := 0
+ iterator := sdk.KVStorePrefixIterator(store, nil)
+ for ; iterator.Valid(); iterator.Next() {
+ count++
+ }
+ iterator.Close()
+
iterator := sdk.KVStorePrefixIterator(store, nil)
defer iterator.Close()
- ret := make([]types.ChallengeInfo, 0)
+ ret := make([]types.ChallengeInfo, 0, count)
for ; iterator.Valid(); iterator.Next() {
key := string(iterator.Key())
challengeAddr := sdk.AccAddress(iterator.Value())
+ if len(challengeAddr) == 0 {
+ return nil, errorsmod.Wrap(types.ErrInvalidAddr, fmt.Sprintf("invalid challenge address for key: %s", key))
+ }
ret = append(ret, types.ChallengeInfo{
Key: key,Likely invalid or redundant comment.
x/avs/keeper/keeper.go (2)
345-361: 🛠️ Refactor suggestion
Refactor to use existing IterateAVSInfo method.
The current implementation duplicates the iteration logic that already exists in IterateAVSInfo. Consider refactoring to improve maintainability.
func (k *Keeper) GetAllAVSInfos(ctx sdk.Context) ([]types.AVSInfo, error) {
- store := prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyPrefixAVSInfo)
- iterator := sdk.KVStorePrefixIterator(store, nil)
- defer iterator.Close()
-
var ret []types.AVSInfo
- for ; iterator.Valid(); iterator.Next() {
- var avsInfo types.AVSInfo
- err := k.cdc.Unmarshal(iterator.Value(), &avsInfo)
- if err != nil {
- return nil, fmt.Errorf("unmarshaling AVSInfo: %w", err)
- }
- ret = append(ret, avsInfo)
- }
+ k.IterateAVSInfo(ctx, func(_ int64, avsInfo types.AVSInfo) bool {
+ ret = append(ret, avsInfo)
+ return false
+ })
return ret, nil
}520-615: Consider adding transaction isolation for two-phase commit.
The two-phase commit implementation could be susceptible to race conditions if multiple operators submit results simultaneously. Consider adding transaction isolation or mutex locking mechanisms to ensure atomicity.
Let's verify if there are any concurrent submission handlers:
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (3)
x/feedistribution/keeper/genesis.go (2)
20-21: Add validation for FeePool.Consider validating the FeePool state before setting it to prevent potential issues with invalid states.
// Set fee pool +if genState.FeePool.CommunityPool.IsValid() { k.SetFeePool(ctx, &genState.FeePool) +} else { + panic("invalid fee pool in genesis state") +}
50-53: Consider returning errors instead of panicking.The function currently panics on error, which is not ideal for a module that might be used as a dependency. Consider modifying the function signature to return an error.
-func (k Keeper) ExportGenesis(ctx sdk.Context) *types.GenesisState { +func (k Keeper) ExportGenesis(ctx sdk.Context) (*types.GenesisState, error) { genesis := types.DefaultGenesis() genesis.Params = k.GetParams(ctx) validatorData, err := k.GetAllValidatorData(ctx) if err != nil { - panic(errorsmod.Wrap(err, "Error getting validator data").Error()) + return nil, errorsmod.Wrap(err, "error getting validator data") }x/feedistribution/keeper/keeper.go (1)
112-157: Consider implementing pagination for better scalability.The current implementation loads all validator data into memory at once, which could cause performance issues with large datasets. Consider implementing pagination to handle large amounts of data more efficiently.
Consider these improvements:
-func (k Keeper) GetAllValidatorData(ctx sdk.Context) (map[string]interface{}, error) { +func (k Keeper) GetAllValidatorData(ctx sdk.Context, pageReq *query.PageRequest) (*types.ValidatorDataResponse, error) { store := ctx.KVStore(k.storeKey) - iterator := store.Iterator(nil, nil) + + // Use pagination + start, end := client.Paginate( + store, pageReq, func(key []byte, value []byte) error { + // Process each item + return nil + }, + ) + + iterator := store.Iterator(start, end) defer iterator.Close() // ... rest of the implementation
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (7)
x/avs/keeper/avs.go(2 hunks)x/avs/keeper/genesis.go(1 hunks)x/avs/keeper/keeper.go(3 hunks)x/avs/keeper/task.go(8 hunks)x/avs/types/genesis.go(1 hunks)x/feedistribution/keeper/genesis.go(2 hunks)x/feedistribution/keeper/keeper.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/avs/keeper/avs.go
🧰 Additional context used
📓 Learnings (1)
x/avs/keeper/genesis.go (2)
Learnt from: TimmyExogenous
PR: ExocoreNetwork/exocore#108
File: x/delegation/keeper/genesis.go:52-73
Timestamp: 2024-10-09T01:26:20.424Z
Learning: In the `ExportGenesis` function, panicking on errors without additional logging is acceptable as panic provides detailed context for debugging.
Learnt from: TimmyExogenous
PR: ExocoreNetwork/exocore#108
File: x/delegation/keeper/genesis.go:52-73
Timestamp: 2024-09-17T14:45:22.028Z
Learning: In the `ExportGenesis` function, panicking on errors without additional logging is acceptable as panic provides detailed context for debugging.
🔇 Additional comments (11)
x/avs/types/genesis.go (1)
50-53: LGTM: Clean and correct implementation.
The function correctly leverages NewGenesisState's nil handling to create a default state with empty collections.
x/feedistribution/keeper/genesis.go (3)
4-4: LGTM: Appropriate error handling import.
The addition of errorsmod is appropriate for standardized error handling in Cosmos SDK modules.
39-41:
Address type inconsistency in SetStakerRewards.
The address type usage is inconsistent with other sections where validator addresses are converted using sdk.ValAddress.
Apply this diff to maintain consistency:
for _, elem := range genState.StakerOutstandingRewardsList {
- k.SetStakerRewards(ctx, elem.ValAddr, *elem.StakerOutstandingRewards)
+ k.SetStakerRewards(ctx, sdk.ValAddress(elem.ValAddr), *elem.StakerOutstandingRewards)
}48-49:
Fix typo in variable name.
The variable name "feelPool" should be "feePool".
-feelPool := k.GetFeePool(ctx)
-genesis.FeePool = *feelPool
+feePool := k.GetFeePool(ctx)
+genesis.FeePool = *feePoolx/avs/keeper/task.go (3)
162-165:
Add validation for task address.
The method should validate that the task address is not zero.
func (k Keeper) SetTaskID(ctx sdk.Context, taskAddr common.Address, id uint64) {
+ if taskAddr == (common.Address{}) {
+ panic("invalid task address: zero address not allowed")
+ }
store := prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyPrefixLatestTaskNum)
store.Set(taskAddr.Bytes(), sdk.Uint64ToBigEndian(id))
}Likely invalid or redundant comment.
303-314:
Improve transaction safety in SetAllTaskChallengedInfo.
Validate all addresses before making any changes to ensure transaction atomicity.
func (k *Keeper) SetAllTaskChallengedInfo(ctx sdk.Context, states []types.ChallengeInfo) error {
+ // Validate all addresses first to ensure atomicity
+ for _, state := range states {
+ if _, err := sdk.AccAddressFromBech32(state.ChallengeAddr); err != nil {
+ return errorsmod.Wrap(err, fmt.Sprintf("invalid challenge address: %s", state.ChallengeAddr))
+ }
+ }
+
store := prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyPrefixTaskChallengeResult)
for i := range states {
state := states[i]
bz, err := sdk.AccAddressFromBech32(state.ChallengeAddr)
- if err != nil {
- return err
- }
+ // We've already validated the address, so this shouldn't error
+ if err != nil {
+ panic(fmt.Sprintf("unexpected error: %v", err))
+ }
store.Set([]byte(state.Key), bz)
}
return nil
}Likely invalid or redundant comment.
339-356:
Add validation and optimize GetAllChallengeInfos.
The method should validate challenge addresses and pre-allocate the slice.
func (k *Keeper) GetAllChallengeInfos(ctx sdk.Context) ([]types.ChallengeInfo, error) {
store := prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyPrefixTaskChallengeResult)
iterator := sdk.KVStorePrefixIterator(store, nil)
defer iterator.Close()
- ret := make([]types.ChallengeInfo, 0)
+ // Count items first for optimal slice allocation
+ count := 0
+ for ; iterator.Valid(); iterator.Next() {
+ count++
+ }
+ iterator.Close()
+
+ // Reset iterator
+ iterator = sdk.KVStorePrefixIterator(store, nil)
+ defer iterator.Close()
+
+ ret := make([]types.ChallengeInfo, 0, count)
for ; iterator.Valid(); iterator.Next() {
key := string(iterator.Key())
challengeAddr := sdk.AccAddress(iterator.Value())
+ if len(challengeAddr) == 0 {
+ return nil, errorsmod.Wrap(types.ErrInvalidAddr, "invalid challenge address")
+ }
ret = append(ret, types.ChallengeInfo{
Key: key,
ChallengeAddr: challengeAddr.String(),
})
}
return ret, nil
}Likely invalid or redundant comment.
x/avs/keeper/keeper.go (2)
346-362: 🛠️ Refactor suggestion
Refactor to use existing IterateAVSInfo method.
The implementation duplicates logic from the existing IterateAVSInfo method. Consider refactoring to reuse the existing iterator.
func (k *Keeper) GetAllAVSInfos(ctx sdk.Context) ([]types.AVSInfo, error) {
- store := prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyPrefixAVSInfo)
- iterator := sdk.KVStorePrefixIterator(store, nil)
- defer iterator.Close()
-
var ret []types.AVSInfo
- for ; iterator.Valid(); iterator.Next() {
- var avsInfo types.AVSInfo
- err := k.cdc.Unmarshal(iterator.Value(), &avsInfo)
- if err != nil {
- return nil, fmt.Errorf("unmarshaling AVSInfo: %w", err)
- }
- ret = append(ret, avsInfo)
- }
+ k.IterateAVSInfo(ctx, func(_ int64, avsInfo types.AVSInfo) bool {
+ ret = append(ret, avsInfo)
+ return false
+ })
return ret, nil
}553-553:
Add error handling for SetTaskResultInfo calls.
The SetTaskResultInfo calls don't check for potential errors. Consider handling potential storage errors.
- k.SetTaskResultInfo(ctx, info)
- return nil
+ if err := k.SetTaskResultInfo(ctx, info); err != nil {
+ return fmt.Errorf("failed to set task result info: %w", err)
+ }
+ return nilAlso applies to: 609-609
x/avs/keeper/genesis.go (2)
12-53: InitGenesis Function is Well-Structured and Correct
The InitGenesis function properly initializes the module's state from the provided GenesisState. All state slices are iterated over correctly, and errors are consistently handled with informative panic messages using errorsmod.Wrap. The implementation aligns with best practices for initializing module state in Cosmos SDK modules.
56-93: ExportGenesis Function Correctly Exports Module State
The ExportGenesis function effectively retrieves all necessary state components using the appropriate getter methods. Errors are handled consistently by panicking with wrapped error messages, providing clear context for debugging. This implementation ensures that the module's state can be accurately exported and is consistent with the initialization logic.
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (6)
proto/exocore/avs/v1/genesis.proto (2)
13-37: Consider adding address validation and formatting rules.Based on the PR comments about malformed AVS addresses and querying issues, consider:
- Adding field options for address validation
- Documenting the expected address format (checksum vs lowercase)
- Adding examples in comments for proper address formatting
This will help prevent issues with address storage and querying.
Example field annotation:
// avs_address should be stored in checksum format // e.g., "0x1234...5678" instead of "0x1234...5678".toLowerCase() string avs_address = 1 [(gogoproto.moretags) = "validate:\"hexaddress\""];
55-61: Enhance documentation for ChainIDInfo message.The current comments could be more descriptive to prevent misuse:
- Clarify the relationship between AVS and chain_id
- Document any constraints or format requirements
- Add examples of valid values
Apply this change:
// ChainIDInfo is helper structure to store the dogfood ChainID information for the genesis state. message ChainIDInfo { - // avs_address is the address of avs as a hex string. + // avs_address is the checksum-formatted address of the AVS (e.g., "0x1234...5678") string avs_address = 1; - // chain_id is a dogfood parameter + // chain_id uniquely identifies the blockchain network that this AVS operates on + // Format: <network>-<version> (e.g., "exocore-1") string chain_id = 2; }x/avs/types/genesis_test.go (3)
22-29: LGTM! Consider adding test case descriptions.The test cases for default and valid genesis states are well-structured. The AVS addresses follow the correct Ethereum format.
Consider adding comments to document the purpose of each test case:
{ + // Test case: Validates that multiple AVS infos can be included in a valid genesis state desc: "valid genesis state", genState: &types.GenesisState{
30-46: Fix typo and enhance AVS validation test coverage.The AVS validation tests cover important cases, but there are some improvements needed:
- Fix the typo in the test description:
- desc: "invalid genesis state due hex address", + desc: "invalid genesis state due to empty hex address",
- Consider adding test cases for:
- Malformed hex addresses (non-hex characters)
- Addresses with incorrect length
- Addresses without "0x" prefix
47-64: Improve task validation test consistency and coverage.The task validation tests need some improvements:
- Make test descriptions consistent with the code:
- desc: "duplicated task num", + desc: "duplicated task ID",
- Consider adding test cases for:
- Invalid task contract addresses
- Task IDs with zero or negative values
- Task IDs with maximum uint64 values
x/avs/types/genesis.go (1)
17-47: Consider optimizing slice initialization and adding basic validation.While the nil checks are good, consider these improvements:
- Pre-allocate slices with capacity to avoid reallocations
- Add basic validation for critical fields before constructing the state
func NewGenesisState( // ... parameters ... ) *GenesisState { // Pre-allocate with capacity if avsInfos == nil { - avsInfos = []AVSInfo{} + avsInfos = make([]AVSInfo, 0, 10) // adjust capacity based on expected size } + // Basic validation for critical fields + for _, info := range avsInfos { + if !common.IsHexAddress(info.AvsAddress) { + panic(fmt.Sprintf("invalid AVS address in genesis: %s", info.AvsAddress)) + } + } // Similar changes for other slices...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
proto/exocore/avs/v1/genesis.proto(1 hunks)x/avs/types/genesis.go(1 hunks)x/avs/types/genesis_test.go(1 hunks)
🧰 Additional context used
🪛 buf
proto/exocore/avs/v1/genesis.proto
4-4: import "exocore/avs/v1/tx.proto": file does not exist
(COMPILE)
🪛 GitHub Check: Run golangci-lint
x/avs/types/genesis.go
[failure] 4-4:
File is not gofumpt-ed (gofumpt)
[failure] 8-8:
File is not gofumpt-ed (gofumpt)
🔇 Additional comments (1)
proto/exocore/avs/v1/genesis.proto (1)
48-54: Previous review comment about ChallengeInfo structure remains unaddressed.
The current composite key approach in ChallengeInfo could be contributing to the querying issues mentioned in the PR comments. As suggested in the previous review, breaking down the composite key into structured fields would:
- Make querying more reliable
- Prevent inconsistencies in key generation
- Improve code maintainability
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
x/avs/types/genesis.go (2)
28-48: Consider pre-allocating slices with capacity.Pre-allocating slices with an expected capacity can improve performance by reducing reallocations.
if avsInfos == nil { - avsInfos = []AVSInfo{} + avsInfos = make([]AVSInfo, 0, 10) // adjust capacity based on expected size } // Apply similar changes to other slice initializations
92-104: Improve task ID validation.The task ID validation should include additional checks for the task ID value.
// Check for duplicated taskID taskNumMap := make(map[string]bool) for _, taskNum := range gs.TaskNums { + if taskNum.TaskId == 0 { + return fmt.Errorf("invalid task ID: must be greater than 0") + } if !common.IsHexAddress(taskNum.TaskAddr) { return fmt.Errorf("invalid hex address: %s", taskNum.TaskAddr) } taskNumKey := assetstype.GetJoinedStoreKey(strings.ToLower(taskNum.TaskAddr), strconv.FormatUint(taskNum.TaskId, 10)) if taskNumMap[string(taskNumKey)] { return fmt.Errorf("duplicate task ID %v ", taskNum) } taskNumMap[string(taskNumKey)] = true }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
x/avs/types/genesis.pb.gois excluded by!**/*.pb.gox/feedistribution/types/genesis.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (2)
x/avs/types/genesis.go(1 hunks)x/avs/types/genesis_test.go(1 hunks)
🔇 Additional comments (5)
x/avs/types/genesis_test.go (3)
22-29: LGTM! Valid genesis state test cases are well-structured.
The test cases appropriately validate both the default genesis state and a state with multiple AVS infos. The test data uses properly formatted Ethereum addresses.
65-90: LGTM! Task result validation test cases are comprehensive.
The test cases thoroughly validate task result scenarios including operator address validation, duplicate detection, and task ID consistency checks. The test descriptions accurately reflect their purposes.
91-98: 🛠️ Refactor suggestion
Enhance BLS public key validation test coverage.
The BLS public key validation test coverage is minimal. As mentioned in the past review, please add essential test cases:
- Valid BLS public key
- Invalid BLS public key format
- Missing operator address
- Duplicate BLS public keys for the same operator
Add these test cases:
{
desc: "invalid BLS public key info with invalid operator address",
genState: &types.GenesisState{
BlsPubKeys: []types.BlsPubKeyInfo{
{Operator: "0x9876543210fedcba9876543210fedcba98765432", PubKey: nil},
}},
valid: false,
},
+ {
+ desc: "valid BLS public key",
+ genState: &types.GenesisState{
+ BlsPubKeys: []types.BlsPubKeyInfo{
+ {
+ Operator: "exo1qypqxpq9qcrsszg2pvxq6rs0zqg3yyc5z5tpwx",
+ PubKey: []byte("valid-bls-public-key"),
+ },
+ }},
+ valid: true,
+ },
+ {
+ desc: "duplicate BLS public keys for same operator",
+ genState: &types.GenesisState{
+ BlsPubKeys: []types.BlsPubKeyInfo{
+ {
+ Operator: "exo1qypqxpq9qcrsszg2pvxq6rs0zqg3yyc5z5tpwx",
+ PubKey: []byte("bls-key-1"),
+ },
+ {
+ Operator: "exo1qypqxpq9qcrsszg2pvxq6rs0zqg3yyc5z5tpwx",
+ PubKey: []byte("bls-key-2"),
+ },
+ }},
+ valid: false,
+ },x/avs/types/genesis.go (2)
3-11: LGTM: Import statements are well-organized.
The imports are properly organized following Go conventions and all imported packages are utilized in the code.
18-63: LGTM: Constructor and DefaultGenesis implementations are sound.
The implementation properly handles nil cases and provides a clean interface for genesis state creation.
trestinlsd
force-pushed
the
genesis-export
branch
from
ea79f7e to
e4552d9
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (12)
x/feedistribution/keeper/genesis.go (1)
20-21: Add validation for FeePool state.Consider adding validation for the FeePool state before setting it to prevent potential issues with invalid states during genesis initialization.
// Set fee pool +if err := genState.FeePool.Validate(); err != nil { + panic(fmt.Sprintf("invalid fee pool in genesis state: %s", err)) +} k.SetFeePool(ctx, &genState.FeePool)proto/exocore/feedistribution/v1/genesis.proto (1)
40-75: Consider adding field validation rules.The address fields in all helper messages could benefit from additional validation rules using the
validateprotobuf extension. This would ensure that addresses follow the correct format at the protocol level.Example addition for ValidatorAccumulatedCommissions (apply similar pattern to other messages):
message ValidatorAccumulatedCommissions { // val_addr is the address of validator - string val_addr = 1; + string val_addr = 1 [(validate.rules).string = { + pattern: "^0x[0-9a-fA-F]{40}$", + max_len: 42 + }]; ValidatorAccumulatedCommission commission = 2; }x/avs/types/genesis.go (2)
18-57: Consider optimizing slice initialization and adding input validation.The constructor could be improved in two ways:
- Pre-allocate slices with capacity to reduce memory reallocations
- Add basic input validation for critical fields
func NewGenesisState( avsInfos []AVSInfo, taskInfos []TaskInfo, blsPubKeys []BlsPubKeyInfo, taskResultInfos []TaskResultInfo, challengeInfos []ChallengeInfo, taskNums []TaskID, chainIDInfos []ChainIDInfo, ) *GenesisState { // Pre-allocate with capacity if avsInfos == nil { - avsInfos = []AVSInfo{} + avsInfos = make([]AVSInfo, 0, 10) // adjust capacity based on expected size } + // Basic validation for critical fields + for _, info := range avsInfos { + if info.AvsAddress == "" { + panic("AVS address cannot be empty") + } + } // Similar changes for other slices...
67-162: Consider refactoring validation blocks for better maintainability.The validation logic could be split into separate methods for each type of validation to improve readability and maintainability.
Example refactor:
func (gs GenesisState) validateAVSInfos() error { addresses := make(map[string]bool) for _, info := range gs.AvsInfos { if err := validateAndNormalizeAddress(&info.AvsAddress); err != nil { return err } if addresses[info.AvsAddress] { return fmt.Errorf("duplicate AVS address: %s", info.AvsAddress) } addresses[info.AvsAddress] = true } return nil } func (gs GenesisState) Validate() error { if err := gs.validateAVSInfos(); err != nil { return err } // Call other validation methods... return nil }precompiles/avs/tx.go (3)
Line range hint
389-401: Enhance input validation for task submission.Consider adding these validations:
- Validate
taskIDis not zero- Validate
taskAddris not zero address after the type assertion- Add length validation for
blsSignaturetaskID, ok := args[1].(uint64) if !ok { return nil, fmt.Errorf(exocmn.ErrContractInputParaOrType, 1, "uint64", args[1]) } + if taskID == 0 { + return nil, fmt.Errorf("taskID cannot be zero") + } resultParams.TaskID = taskID taskResponse, ok := args[2].([]byte) if !ok { return nil, fmt.Errorf(exocmn.ErrContractInputParaOrType, 2, "[]byte", taskResponse) } resultParams.TaskResponse = taskResponse if len(taskResponse) == 0 { resultParams.TaskResponse = nil } blsSignature, ok := args[3].([]byte) if !ok { return nil, fmt.Errorf(exocmn.ErrContractInputParaOrType, 3, "[]byte", blsSignature) } + if len(blsSignature) == 0 { + return nil, fmt.Errorf("blsSignature cannot be empty") + } resultParams.BlsSignature = blsSignature taskAddr, ok := args[4].(common.Address) if !ok || (taskAddr == common.Address{}) { return nil, fmt.Errorf(exocmn.ErrContractInputParaOrType, 4, "common.Address", taskAddr) }
Line range hint
42-67: Strengthen authorization checks in RegisterAVS.The current authorization check using
slices.Containson string addresses might be susceptible to case sensitivity issues mentioned in the PR comments. Consider:
- Normalizing addresses before comparison
- Adding a helper function for consistent address comparison
- if !slices.Contains(avsParams.AvsOwnerAddress, avsParams.CallerAddress.String()) { + // Add a helper function for consistent address comparison + func normalizeAndCompareAddresses(addresses []string, target string) bool { + normalizedTarget := strings.ToLower(target) + for _, addr := range addresses { + if strings.ToLower(addr) == normalizedTarget { + return true + } + } + return false + } + + if !normalizeAndCompareAddresses(avsParams.AvsOwnerAddress, avsParams.CallerAddress.String()) { return nil, errorsmod.Wrap(err, "not qualified to registerOrDeregister") }
Line range hint
385-386: Standardize address handling to fix querying issues.Based on the PR comments about AVS querying issues, ensure consistent address handling:
- Store addresses in a normalized format (either all lowercase or checksum)
- Apply the same normalization when querying
Consider adding an address handling utility:
// Add to a common utility package func NormalizeAddress(address string) string { // Convert to checksum address if it's an ethereum address if common.IsHexAddress(address) { return common.HexToAddress(address).Hex() } // For cosmos addresses, ensure consistent casing return strings.ToLower(address) }x/avs/keeper/task.go (3)
46-53: Optimize slice allocation in GetAllTaskInfos.Pre-allocate the slice for better performance by counting items first.
func (k Keeper) GetAllTaskInfos(ctx sdk.Context) ([]types.TaskInfo, error) { - var taskInfos []types.TaskInfo + // Count items first + count := 0 + k.IterateTaskAVSInfo(ctx, func(_ int64, _ types.TaskInfo) bool { + count++ + return false + }) + + taskInfos := make([]types.TaskInfo, 0, count) k.IterateTaskAVSInfo(ctx, func(_ int64, taskInfo types.TaskInfo) bool { taskInfos = append(taskInfos, taskInfo) return false }) return taskInfos, nil }
170-185: Optimize performance and improve documentation.
- Pre-allocate the slice for better performance.
- Add documentation about address case handling.
// GetAllTaskNums returns a map containing all task addresses and their corresponding task IDs. +// Note: Task addresses are stored and returned in lowercase format for consistent comparison. func (k *Keeper) GetAllTaskNums(ctx sdk.Context) ([]types.TaskID, error) { store := prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyPrefixLatestTaskNum) iterator := sdk.KVStorePrefixIterator(store, nil) defer iterator.Close() - ret := make([]types.TaskID, 0) + + // Count items first for optimal slice allocation + count := 0 + for ; iterator.Valid(); iterator.Next() { + count++ + } + iterator.Close() + + // Reset iterator + iterator = sdk.KVStorePrefixIterator(store, nil) + defer iterator.Close() + + ret := make([]types.TaskID, 0, count) for ; iterator.Valid(); iterator.Next() {
311-327: Optimize slice allocation.Pre-allocate the slice for better performance.
func (k *Keeper) GetAllTaskResultInfos(ctx sdk.Context) ([]types.TaskResultInfo, error) { store := prefix.NewStore(ctx.KVStore(k.storeKey), types.KeyPrefixTaskResult) iterator := sdk.KVStorePrefixIterator(store, nil) defer iterator.Close() - ret := make([]types.TaskResultInfo, 0) + // Count items first for optimal slice allocation + count := 0 + for ; iterator.Valid(); iterator.Next() { + count++ + } + iterator.Close() + + // Reset iterator + iterator = sdk.KVStorePrefixIterator(store, nil) + defer iterator.Close() + + ret := make([]types.TaskResultInfo, 0, count)x/avs/keeper/keeper.go (1)
456-486: Consider extracting BLS key validation to a helper method.The BLS public key validation logic could be moved to a separate helper method to improve readability and reusability.
+func (k Keeper) validateOperatorBLSKey(ctx sdk.Context, operatorAddr string) (*bls.PublicKey, error) { + keyInfo, err := k.GetOperatorPubKey(ctx, operatorAddr) + if err != nil || keyInfo.PubKey == nil { + return nil, errorsmod.Wrap( + types.ErrPubKeyIsNotExists, + fmt.Sprintf("operator address: %s", operatorAddr), + ) + } + pubKey, err := blst.PublicKeyFromBytes(keyInfo.PubKey) + if err != nil || pubKey == nil { + return nil, errorsmod.Wrap( + types.ErrParsePubKey, + fmt.Sprintf("operator address: %s", operatorAddr), + ) + } + return pubKey, nil +}x/avs/keeper/genesis.go (1)
18-18: Review usage of//nolint:goseccommentsThe
//nolint:goseccomments suppress security linter warnings when setting AVS info, task info, and operator public keys. Consider addressing any security concerns raised by the linter to ensure the code is secure, rather than suppressing them.Also applies to: 25-25, 32-32
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
x/avs/types/genesis.pb.gois excluded by!**/*.pb.gox/feedistribution/types/genesis.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (16)
precompiles/avs/tx.go(1 hunks)proto/exocore/avs/v1/genesis.proto(1 hunks)proto/exocore/feedistribution/v1/genesis.proto(2 hunks)x/avs/keeper/avs.go(1 hunks)x/avs/keeper/genesis.go(1 hunks)x/avs/keeper/keeper.go(2 hunks)x/avs/keeper/msg_server.go(1 hunks)x/avs/keeper/multi_operator_submit_task_test.go(2 hunks)x/avs/keeper/submit_task_test.go(2 hunks)x/avs/keeper/task.go(5 hunks)x/avs/module.go(2 hunks)x/avs/types/genesis.go(1 hunks)x/avs/types/genesis_test.go(1 hunks)x/feedistribution/keeper/genesis.go(2 hunks)x/feedistribution/keeper/keeper.go(2 hunks)x/operator/keeper/operator.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- x/avs/keeper/avs.go
- x/avs/keeper/msg_server.go
- x/avs/keeper/multi_operator_submit_task_test.go
- x/avs/keeper/submit_task_test.go
- x/avs/module.go
- x/avs/types/genesis_test.go
- x/feedistribution/keeper/keeper.go
- x/operator/keeper/operator.go
🧰 Additional context used
🪛 buf
proto/exocore/avs/v1/genesis.proto
4-4: import "exocore/avs/v1/tx.proto": file does not exist
(COMPILE)
🔇 Additional comments (25)
proto/exocore/avs/v1/genesis.proto (4)
4-4: Fix missing import file.
The import "exocore/avs/v1/tx.proto" is missing, which will cause compilation failures.
🧰 Tools
🪛 buf
4-4: import "exocore/avs/v1/tx.proto": file does not exist
(COMPILE)
12-38: LGTM! Well-structured genesis state definition.
The GenesisState message is well-organized with clear field descriptions and consistent use of gogoproto options.
25-30: Consider using a composite key message for indexed fields.
Multiple fields (task_result_infos, challenge_infos) are indexed by the same combination of fields (operator_address, task_address, task_id). Consider creating a dedicated key message type.
40-46: LGTM! Well-defined helper messages.
The TaskID and ChainIDInfo messages are well-structured with clear field documentation.
Also applies to: 56-61
x/feedistribution/keeper/genesis.go (4)
39-41: Previous review comment is still valid: Inconsistent address type usage.
The address type inconsistency noted in the previous review still needs to be addressed.
55-77: Previous review comment is still valid: Improve type assertion error handling.
The suggestion to improve type assertion error handling from the previous review is still applicable.
50-53: 🛠️ Refactor suggestion
Consider returning error instead of panicking.
The current implementation panics on error. Consider modifying the function signature to return an error instead.
-func (k Keeper) ExportGenesis(ctx sdk.Context) *types.GenesisState {
+func (k Keeper) ExportGenesis(ctx sdk.Context) (*types.GenesisState, error) {
genesis := types.DefaultGenesis()
genesis.Params = k.GetParams(ctx)
validatorData, err := k.GetAllValidatorData(ctx)
if err != nil {
- panic(errorsmod.Wrap(err, "Error getting validator data").Error())
+ return nil, errorsmod.Wrap(err, "error getting validator data")
}⛔ Skipped due to learnings
Learnt from: TimmyExogenous
PR: ExocoreNetwork/exocore#108
File: x/delegation/keeper/genesis.go:52-73
Timestamp: 2024-10-09T01:26:20.424Z
Learning: In the `ExportGenesis` function, panicking on errors without additional logging is acceptable as panic provides detailed context for debugging.
Learnt from: TimmyExogenous
PR: ExocoreNetwork/exocore#108
File: x/delegation/keeper/genesis.go:52-73
Timestamp: 2024-09-17T14:45:22.028Z
Learning: In the `ExportGenesis` function, panicking on errors without additional logging is acceptable as panic provides detailed context for debugging.
Learnt from: TimmyExogenous
PR: ExocoreNetwork/exocore#108
File: x/operator/keeper/genesis.go:63-96
Timestamp: 2024-09-17T14:46:25.700Z
Learning: In the `ExportGenesis` function, panicking on errors is acceptable because the node is offline during genesis export.
Learnt from: TimmyExogenous
PR: ExocoreNetwork/exocore#108
File: x/operator/keeper/genesis.go:63-96
Timestamp: 2024-10-09T01:26:23.510Z
Learning: In the `ExportGenesis` function, panicking on errors is acceptable because the node is offline during genesis export.
19-77: Verify reward recording functionality.
Based on the PR objectives mentioning issues with rewards not being recorded correctly, we should verify the implementation.
✅ Verification successful
Let me gather more specific information about the reward recording functionality by examining the implementation and tests.
Let me try a different approach to examine the reward recording functionality, particularly focusing on the genesis implementation and its tests.
Based on the examination of the implementation, I can now provide a final response regarding the reward recording functionality.
Reward recording implementation is properly structured and functional
The reward recording implementation in the feedistribution module shows a well-structured system with proper state management:
- All reward types (validator accumulated commission, current rewards, outstanding rewards, and staker rewards) have dedicated setter functions that correctly marshal and store the data
- The genesis import/export functions correctly handle all reward types
- The reward allocation logic includes proper event emission and logging
- State modifications are atomic and consistent across different reward types
Key implementation points:
SetValidatorCurrentRewards: Properly stores validator rewards stateSetValidatorOutstandingRewards: Maintains outstanding rewards with proper state updatesSetStakerRewards: Correctly handles staker reward recordingAllocateTokensToStakers: Includes proper error handling and logging
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any reward-related error logs or panics in the codebase
# that might indicate issues with reward recording.
echo "Checking for reward-related error patterns..."
# Look for potential error patterns in reward handling
rg -n "panic|error|fail" --type go | grep -i "reward"
# Look for reward-related test files and their assertions
echo "\nChecking reward-related test coverage..."
fd -e go | grep -i "test" | grep -i "reward"
Length of output: 27590
Script:
#!/bin/bash
# Check the implementation of reward recording functions and their tests
# Look for reward recording related functions and their tests
echo "Checking reward recording implementation..."
ast-grep --pattern 'func $FUNC($$$) {
$$$
SetValidatorCurrentRewards($$$)
$$$
}'
echo "\nChecking reward recording tests..."
rg -A 10 "TestSetValidatorCurrentRewards|TestGetValidatorCurrentRewards" --type go
# Check for any reward-related state modifications
echo "\nChecking reward state modifications..."
ast-grep --pattern 'func ($KEEPER Keeper) Set$_Rewards($$$) {
$$$
}'
Length of output: 517
Script:
#!/bin/bash
# Check the implementation details of reward recording in genesis and keeper
# Check the keeper implementation for reward recording
echo "Checking keeper implementation for rewards..."
rg -A 10 "SetValidatorCurrentRewards|SetValidatorAccumulatedCommission|SetValidatorOutstandingRewards|SetStakerRewards" x/feedistribution/keeper/
# Check test coverage for reward recording
echo "\nChecking test coverage for reward recording..."
rg -A 10 "TestInitGenesis|TestExportGenesis" x/feedistribution/keeper/
# Check for any reward-related state validation
echo "\nChecking reward state validation..."
rg -A 10 "ValidateGenesis|Validate" x/feedistribution/types/
Length of output: 11156
proto/exocore/feedistribution/v1/genesis.proto (3)
5-5: LGTM: Import statement is correctly added.
The new import for distribution.proto is necessary for the FeePool and other distribution-related message types.
18-34: LGTM: GenesisState structure is well-defined.
The new fields are properly documented and correctly use protobuf annotations. The structure effectively captures all necessary state for fee distribution.
40-75: Address case sensitivity handling needs attention.
Based on the PR comments about AVS address querying issues, consider standardizing address case handling. You might want to:
- Add a comment documenting the expected address format (checksum vs lowercase)
- Consider adding a custom validator that normalizes addresses to checksum format
This will help prevent the reported issues where queries fail due to case mismatches.
x/avs/types/genesis.go (2)
60-63: LGTM!
The DefaultGenesis implementation correctly uses the NewGenesisState constructor with nil parameters, following standard Cosmos SDK patterns.
67-162: 🛠️ Refactor suggestion
Add cross-field validation checks.
The current validation lacks checks for relationships between different fields. Add validation for:
- AVS addresses in ChainIdInfos must exist in AvsInfos
- Task addresses in TaskNums must exist in TaskInfos
- Task addresses in TaskResultInfos must exist in TaskInfos
func (gs GenesisState) Validate() error {
// ... existing validation code ...
+ // Cross-field validation
+ for _, chainInfo := range gs.ChainIdInfos {
+ if !avsAddresses[chainInfo.AvsAddress] {
+ return fmt.Errorf("AVS address %s in ChainIdInfos not found in AvsInfos", chainInfo.AvsAddress)
+ }
+ }
+
+ for _, taskNum := range gs.TaskNums {
+ if !taskInfoMap[taskNum.TaskAddr] {
+ return fmt.Errorf("task address %s in TaskNums not found in TaskInfos", taskNum.TaskAddr)
+ }
+ }
+
+ for _, result := range gs.TaskResultInfos {
+ if !taskInfoMap[result.TaskContractAddress] {
+ return fmt.Errorf("task address %s in TaskResultInfos not found in TaskInfos", result.TaskContractAddress)
+ }
+ }
return nil
}Likely invalid or redundant comment.
precompiles/avs/tx.go (1)
402-402: Verify the impact of keeper method change.
The change from SetTaskResultInfo to SubmitTaskResult might affect existing callers. Please ensure all dependent code is updated accordingly.
✅ Verification successful
Based on the search results, I can see that:
- The old method
SetTaskResultInfois still present in the codebase but is only used internally within the keeper implementation. - The new method
SubmitTaskResultis properly integrated across the codebase:- It's defined in the keeper interface
- It's used in tests
- It has corresponding protobuf messages and gRPC endpoints
- It's properly wired up in CLI commands
- The precompile is correctly using the new method
The change is properly implemented and all callers are updated.
The change from SetTaskResultInfo to SubmitTaskResult appears to be a refactoring where the internal keeper method is still used by the new public-facing SubmitTaskResult method, maintaining backward compatibility while providing a cleaner API. This is a good practice as it encapsulates the internal implementation details.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining calls to the old method
rg "SetTaskResultInfo"
# Search for all calls to the new method to verify consistent usage
rg "SubmitTaskResult"
Length of output: 23590
x/avs/keeper/task.go (4)
91-117: LGTM! Well-implemented function with optimized slice allocation.
The implementation includes proper error handling and performance optimization through slice pre-allocation.
147-153: LGTM! Proper validation of task address.
The implementation includes appropriate validation before setting the task ID.
187-201: LGTM! Proper validation of addresses.
The implementation includes appropriate validation for both operator and task contract addresses.
342-372: LGTM! Well-implemented function with proper optimizations.
The implementation includes:
- Optimized slice pre-allocation
- Proper validation of challenge addresses
- Clear error handling
x/avs/keeper/keeper.go (4)
447-454: LGTM! Clean implementation using existing iterator.
The implementation correctly reuses the IterateAVSInfo method to collect all AVS information.
543-543: Add error handling for SetTaskResultInfo calls.
The SetTaskResultInfo calls don't check for potential errors.
Also applies to: 600-600
584-590: Verify task response format consistency.
Let's ensure the task response format is consistent across the codebase.
✅ Verification successful
Based on the results, I can now generate the final response:
Task response validation is correctly implemented
The verification shows consistent implementation across the codebase:
UnmarshalTaskResponseis used consistently in both occurrences (keeper.go)- Single, well-defined
TaskResponsestruct intypes.gowith clear fields (TaskIDandNumberSum) - The validation in the reviewed code correctly checks both unmarshaling errors and TaskID consistency
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent task response unmarshaling across the codebase
# Search for UnmarshalTaskResponse usage
echo "Checking UnmarshalTaskResponse usage patterns:"
rg -A 3 "UnmarshalTaskResponse"
# Search for TaskResponse struct usage
echo "Checking TaskResponse struct definitions:"
ast-grep --pattern 'type TaskResponse struct {
$$$
}'
Length of output: 1391
567-578:
Add overflow protection for epoch calculations.
The epoch calculations involve adding multiple int64 values which could potentially overflow. Add checks to prevent this.
+ // Check for potential overflow
+ totalPeriods := task.TaskResponsePeriod + task.TaskStatisticalPeriod
+ if totalPeriods < task.TaskResponsePeriod {
+ return errorsmod.Wrap(
+ types.ErrParamError,
+ "epoch calculation overflow",
+ )
+ }
+ startEpoch := int64(task.StartingEpoch)
+ if startEpoch > math.MaxInt64 - int64(totalPeriods) {
+ return errorsmod.Wrap(
+ types.ErrParamError,
+ "epoch calculation would overflow",
+ )
+ }
if epoch.CurrentEpoch > int64(task.StartingEpoch)+int64(task.TaskResponsePeriod)+int64(task.TaskStatisticalPeriod) {Likely invalid or redundant comment.
x/avs/keeper/genesis.go (3)
4-7: Imports added correctly for new functionalities
The added imports for errorsmod and common are appropriate and necessary for error wrapping and Ethereum address handling.
12-12: Updated InitGenesis function signature improves clarity
The InitGenesis function now accepts state types.GenesisState directly, removing the unused parameter and return value. This change enhances code clarity and aligns with the module's initialization needs.
56-93: ExportGenesis function correctly exports module state
The ExportGenesis function has been updated to accurately collect and return all necessary module state information. Error handling is appropriately implemented, and the function aligns with the expected behavior of the module.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (1)
x/feedistribution/keeper/genesis.go (1)
19-77: Consider implementing genesis validation.The current implementation would benefit from a separate validation function to ensure the genesis state is valid before initialization.
Consider adding:
- A
ValidateGenesisfunction to check the validity of all fields- Validation of relationships between different components (e.g., ensuring reward amounts are consistent)
- Sanity checks for economic parameters
Would you like me to provide an example implementation of the validation function?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
x/avs/client/cli/query.go(1 hunks)x/feedistribution/keeper/genesis.go(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- x/avs/client/cli/query.go
🔇 Additional comments (3)
x/feedistribution/keeper/genesis.go (3)
4-4: LGTM!
The addition of errorsmod import follows Cosmos SDK best practices for error handling.
39-41:
Inconsistent address type usage in SetStakerRewards.
For consistency with other sections where validator addresses are converted using sdk.ValAddress, apply the same conversion here.
Apply this diff:
for _, elem := range genState.StakerOutstandingRewardsList {
- k.SetStakerRewards(ctx, elem.ValAddr, *elem.StakerOutstandingRewards)
+ k.SetStakerRewards(ctx, sdk.ValAddress(elem.ValAddr), *elem.StakerOutstandingRewards)
}Likely invalid or redundant comment.
55-77: 🛠️ Refactor suggestion
Refactor type assertions for better error handling.
The current implementation uses panic for type assertion failures. Consider implementing a helper function for safer type assertions.
Apply this diff to improve error handling:
+func assertValidatorData[T any](data interface{}, key string) (T, error) {
+ if val, ok := data.(T); ok {
+ return val, nil
+ }
+ return *new(T), fmt.Errorf("failed to assert %s type", key)
+}
-if accumulatedCommissions, ok := validatorData["ValidatorAccumulatedCommissions"].([]types.ValidatorAccumulatedCommissions); ok {
- genesis.ValidatorAccumulatedCommissions = accumulatedCommissions
-} else {
- panic("Failed to assert ValidatorAccumulatedCommissions type")
-}
+accumulatedCommissions, err := assertValidatorData[[]types.ValidatorAccumulatedCommissions](
+ validatorData["ValidatorAccumulatedCommissions"],
+ "ValidatorAccumulatedCommissions",
+)
+if err != nil {
+ return nil, errorsmod.Wrap(err, "failed to export genesis state")
+}
+genesis.ValidatorAccumulatedCommissions = accumulatedCommissions
// Apply similar changes for other type assertions...This refactor:
- Introduces a type-safe helper function
- Returns errors instead of panicking
- Provides consistent error handling
- Improves code maintainability
Likely invalid or redundant comment.
|
Test passed.
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
x/feedistribution/keeper/genesis.go (1)
48-56: Improve error handling in ExportGenesis.The current implementation uses panic for error handling. While panics are acceptable in genesis functions, the error messages could be more descriptive.
Apply this improvement:
feePool := k.GetFeePool(ctx) if feePool == nil { - panic("fee pool cannot be nil in genesis export") + panic("fee pool not found: ensure fee pool is properly initialized during genesis import") } genesis.FeePool = *feePool validatorData, err := k.GetAllValidatorData(ctx) if err != nil { - panic(errorsmod.Wrap(err, "Error getting validator data").Error()) + panic(errorsmod.Wrapf(err, "failed to export genesis state: error retrieving validator data").Error()) }proto/exocore/feedistribution/v1/genesis.proto (1)
38-75: Consider consistent naming convention for messagesWhile the message structures are well-defined, consider using singular form for message names to follow protobuf conventions:
-message ValidatorAccumulatedCommissions +message ValidatorAccumulatedCommissionEntry -message ValidatorCurrentRewardsList +message ValidatorCurrentRewardsEntry -message ValidatorOutstandingRewardsList +message ValidatorOutstandingRewardsEntry -message StakerOutstandingRewardsList +message StakerOutstandingRewardsEntryThis would better reflect that each message represents a single entry in the repeated fields.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
x/avs/types/genesis.pb.gois excluded by!**/*.pb.gox/feedistribution/types/genesis.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (4)
proto/exocore/avs/v1/genesis.proto(1 hunks)proto/exocore/feedistribution/v1/genesis.proto(2 hunks)x/feedistribution/keeper/genesis.go(2 hunks)x/feedistribution/keeper/keeper.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/feedistribution/keeper/keeper.go
🧰 Additional context used
🪛 buf
proto/exocore/avs/v1/genesis.proto
4-4: import "exocore/avs/v1/tx.proto": file does not exist
(COMPILE)
🔇 Additional comments (6)
x/feedistribution/keeper/genesis.go (2)
20-21:
Add validation for FeePool before dereferencing.
To prevent potential panics, add a nil check before dereferencing genState.FeePool.
Apply this diff:
// Set fee pool
+if &genState.FeePool == nil {
+ panic("fee pool cannot be nil in genesis")
+}
k.SetFeePool(ctx, &genState.FeePool)Likely invalid or redundant comment.
23-41:
Add validation and improve error handling for rewards initialization.
Several potential issues in the rewards initialization:
- No validation of Commission, CurrentRewards, OutstandingRewards, and StakerOutstandingRewards pointers before dereferencing
- Inconsistent address type handling between validator and staker addresses
- Based on the PR objectives, there are issues with rewards not being recorded correctly
Apply these improvements:
// Set all the validatorAccumulatedCommission
for _, elem := range genState.ValidatorAccumulatedCommissions {
+ if elem.Commission == nil {
+ panic("validator commission cannot be nil in genesis")
+ }
k.SetValidatorAccumulatedCommission(ctx, sdk.ValAddress(elem.ValAddr), *elem.Commission)
}
// Set all the validatorCurrentRewards
for _, elem := range genState.ValidatorCurrentRewardsList {
+ if elem.CurrentRewards == nil {
+ panic("validator current rewards cannot be nil in genesis")
+ }
k.SetValidatorCurrentRewards(ctx, sdk.ValAddress(elem.ValAddr), *elem.CurrentRewards)
}
// Set all the validatorOutstandingRewards
for _, elem := range genState.ValidatorOutstandingRewardsList {
+ if elem.OutstandingRewards == nil {
+ panic("validator outstanding rewards cannot be nil in genesis")
+ }
k.SetValidatorOutstandingRewards(ctx, sdk.ValAddress(elem.ValAddr), *elem.OutstandingRewards)
}
// Set all the stakerRewards
for _, elem := range genState.StakerOutstandingRewardsList {
+ if elem.StakerOutstandingRewards == nil {
+ panic("staker outstanding rewards cannot be nil in genesis")
+ }
- k.SetStakerRewards(ctx, elem.StakerAddr, *elem.StakerOutstandingRewards)
+ stakerAddr, err := sdk.AccAddressFromBech32(elem.StakerAddr)
+ if err != nil {
+ panic(fmt.Sprintf("invalid staker address: %s", err))
+ }
+ k.SetStakerRewards(ctx, stakerAddr, *elem.StakerOutstandingRewards)
}Let's verify the reward recording issue mentioned in the PR objectives:
proto/exocore/feedistribution/v1/genesis.proto (4)
5-5: LGTM: Import addition is appropriate
The new import for distribution.proto is necessary to support the new message types and follows the correct package structure.
58-58:
Fix copy-paste error in comment
The comment incorrectly refers to "TaskID" instead of "ValidatorOutstandingRewardsList".
-// TaskID is helper structure to store the validator outstanding rewards for the genesis state.
+// ValidatorOutstandingRewardsList is helper structure to store the validator outstanding rewards for the genesis state.Likely invalid or redundant comment.
Line range hint 1-75: Verify reward calculation flow
Given the reported issues with rewards not being recorded correctly, please verify:
- The proto definitions support all required reward calculation scenarios
- The reward types can handle the precision needed for calculations
- The state transitions are properly captured in these structures
✅ Verification successful
Based on the search results, I can now verify the reward calculation flow. Let me generate the final response:
Proto definitions properly support reward calculations and state management
The reward calculation flow is well-supported by the proto definitions with:
-
Proper decimal precision handling through
DecCoinstypes used consistently across:ValidatorHistoricalRewards.cumulative_reward_ratioValidatorCurrentRewards.rewardsValidatorAccumulatedCommission.commissionValidatorOutstandingRewards.rewardsStakerOutstandingRewards.rewards
-
Complete state tracking through:
- Fee pool for global distribution state
- Validator accumulated commissions
- Current rewards with period tracking
- Outstanding rewards for both validators and stakers
- Historical rewards with cumulative ratios
-
Proper commission and reward separation:
- Validator commissions tracked separately from rewards
- Staker rewards isolated from validator rewards
- Community pool properly segregated
The proto definitions provide all necessary structures with appropriate precision and state management capabilities for accurate reward calculations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for reward calculation implementation
rg -A 10 "CalculateRewards|UpdateRewards|SetRewards"
# Search for precision/decimal handling
rg -A 5 "DecCoin|sdk.Dec|decimal"
Length of output: 88550
18-34: Verify field types and enhance documentation
The field structure looks good, but given the reported issues with distribution rewards not being recorded correctly:
- Please verify that
FeePooland reward types from distribution.proto correctly handle decimal values - Consider enhancing the documentation to:
- Specify the expected format of decimal values
- Document any minimum/maximum constraints
- Add examples of valid values
✅ Verification successful
Field types correctly handle decimal values
The verification shows that all reward-related fields are properly using DecCoin from the Cosmos SDK with appropriate protobuf configuration:
FeePool.community_pool: UsesDecCoinwithDecCoinscastingValidatorAccumulatedCommission.commission: UsesDecCoinwithDecCoinscastingValidatorCurrentRewards.rewards: UsesDecCoinwithDecCoinscastingValidatorOutstandingRewards.rewards: UsesDecCoinwithDecCoinscastingStakerOutstandingRewards.rewards: UsesDecCoinwithDecCoinscasting
All fields are properly configured with:
- Non-nullable values
- Correct repeated field handling
- Proper casting to SDK's decimal coin types
The field types are correctly implemented to handle decimal values for rewards and fee distribution.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the field types in distribution.proto
rg -A 2 "message (FeePool|ValidatorAccumulatedCommission|ValidatorCurrentRewards|ValidatorOutstandingRewards|StakerOutstandingRewards)"
Length of output: 88550
Script:
#!/bin/bash
# Search for FeePool and reward types in distribution.proto
rg -A 5 "message (FeePool|ValidatorAccumulatedCommission|ValidatorCurrentRewards|ValidatorOutstandingRewards|StakerOutstandingRewards)" proto/exocore/feedistribution/v1/distribution.proto
Length of output: 1589
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (2)
x/feedistribution/keeper/genesis_test.go (2)
18-20: Improve timing constants and documentation.
- The epsilon of 1 nanosecond might be too small for reliable testing. Consider using a larger buffer.
- The timing calculation is complex and needs documentation explaining its significance.
- epsilon := time.Nanosecond // negligible amount of buffer duration + // Buffer duration to ensure we're safely past the epoch boundary + epsilon := time.Second suite.Commit() - suite.CommitAfter(time.Hour*24 + epsilon - time.Minute) + // Commit just before the next epoch to test reward accumulation + epochDuration := time.Hour * 24 + suite.CommitAfter(epochDuration - time.Minute + epsilon)
11-41: Enhance test coverage with additional test cases.Consider adding the following test scenarios to improve coverage:
- Test with empty validator set
- Test with invalid/malformed validator public keys
- Test with zero rewards
- Test genesis export/import roundtrip
- Add sub-tests using
t.Run()for better organization and parallel execution capabilityWould you like me to provide example implementations for these test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
x/feedistribution/keeper/genesis.go(2 hunks)x/feedistribution/keeper/genesis_test.go(1 hunks)
🧰 Additional context used
📓 Learnings (1)
x/feedistribution/keeper/genesis.go (2)
Learnt from: TimmyExogenous
PR: ExocoreNetwork/exocore#108
File: x/delegation/keeper/genesis.go:52-73
Timestamp: 2024-10-09T01:26:20.424Z
Learning: In the `ExportGenesis` function, panicking on errors without additional logging is acceptable as panic provides detailed context for debugging.
Learnt from: TimmyExogenous
PR: ExocoreNetwork/exocore#108
File: x/delegation/keeper/genesis.go:52-73
Timestamp: 2024-09-17T14:45:22.028Z
Learning: In the `ExportGenesis` function, panicking on errors without additional logging is acceptable as panic provides detailed context for debugging.
🔇 Additional comments (2)
x/feedistribution/keeper/genesis.go (2)
20-21:
Add nil check for FeePool.
Add a nil check before dereferencing genState.FeePool to prevent potential panics.
Apply this diff:
// Set fee pool
+if &genState.FeePool == nil {
+ panic("fee pool cannot be nil in genesis")
+}
k.SetFeePool(ctx, &genState.FeePool)Likely invalid or redundant comment.
53-56: Verify GetAllValidatorData implementation.
Based on the PR objectives mentioning issues with distribution module not recording rewards, we should verify the implementation of GetAllValidatorData.
✅ Verification successful
Let me gather more information about the implementation and tests.
Let me gather more information about the processing methods and related types.
Let me check the remaining processing methods and key prefixes.
Based on the gathered information, I can now provide the final response:
GetAllValidatorData implementation is correctly handling all validator rewards data
The implementation properly:
- Iterates through all KV store entries
- Processes all validator-related data types:
- Accumulated commissions
- Current rewards
- Outstanding rewards
- Staker rewards
- Uses correct key prefixes for data retrieval
- Includes proper error handling and validation
- Returns structured data used by genesis export
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify GetAllValidatorData implementation and usage
# Search for GetAllValidatorData implementation
echo "Searching for GetAllValidatorData implementation..."
ast-grep --pattern 'func ($k Keeper) GetAllValidatorData($ctx sdk.Context) ($_ map[string]interface{}, $_ error) {
$$$
}'
# Search for tests
echo "Searching for related tests..."
rg -l "TestGetAllValidatorData|TestExportGenesis" --type go
Length of output: 412
Script:
#!/bin/bash
# Search for GetAllValidatorData implementation with more context
echo "Searching for GetAllValidatorData implementation with context..."
rg -A 20 "func.*GetAllValidatorData" --type go
# Check the test implementation
echo "Checking test implementation..."
cat x/feedistribution/keeper/genesis_test.go
# Search for any other usages of GetAllValidatorData
echo "Searching for other usages..."
rg "GetAllValidatorData" --type go
Length of output: 3876
Script:
#!/bin/bash
# Search for processing methods implementation
echo "Searching for processing methods..."
rg -A 10 "func.*process(ValidatorAccumulatedCommission|ValidatorCurrentRewards)" --type go
# Search for related types
echo "Searching for related types..."
rg -A 5 "type.*(ValidatorAccumulatedCommissions|ValidatorCurrentRewardsList|ValidatorOutstandingRewardsList|StakerOutstandingRewardsList)" --type go
# Check if there are any other files using these types
echo "Checking usage of reward types..."
rg "ValidatorAccumulatedCommissions|ValidatorCurrentRewardsList|ValidatorOutstandingRewardsList|StakerOutstandingRewardsList" --type go
Length of output: 32513
Script:
#!/bin/bash
# Search for the remaining processing method and its usage
echo "Searching for ValidatorOutstandingRewards processing..."
rg -A 10 "processValidatorOutstandingRewards" --type go
# Search for key prefix definitions
echo "Searching for key prefix definitions..."
rg "GetValidator.*Key" --type go
# Check if there are any other related methods in the keeper
echo "Checking related keeper methods..."
rg "func.*Keeper.*Validator.*Rewards" --type go
Length of output: 5201
| if accumulatedCommissions, ok := validatorData["ValidatorAccumulatedCommissions"].([]types.ValidatorAccumulatedCommissions); ok { | ||
| genesis.ValidatorAccumulatedCommissions = accumulatedCommissions | ||
| } else { | ||
| panic("Failed to assert ValidatorAccumulatedCommissions type") | ||
| } | ||
|
|
||
| if currentRewardsList, ok := validatorData["ValidatorCurrentRewardsList"].([]types.ValidatorCurrentRewardsList); ok { | ||
| genesis.ValidatorCurrentRewardsList = currentRewardsList | ||
| } else { | ||
| panic("Failed to assert ValidatorCurrentRewardsList type") | ||
| } | ||
|
|
||
| if outstandingRewardsList, ok := validatorData["ValidatorOutstandingRewardsList"].([]types.ValidatorOutstandingRewardsList); ok { | ||
| genesis.ValidatorOutstandingRewardsList = outstandingRewardsList | ||
| } else { | ||
| panic("Failed to assert ValidatorOutstandingRewardsList type") | ||
| } | ||
|
|
||
| if stakerRewardsList, ok := validatorData["StakerOutstandingRewardsList"].([]types.StakerOutstandingRewardsList); ok { | ||
| genesis.StakerOutstandingRewardsList = stakerRewardsList | ||
| } else { | ||
| panic("Failed to assert StakerOutstandingRewardsList type") | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor type assertions and add data validation.
The current implementation has repetitive type assertion code and lacks validation of the exported data.
Apply this refactor:
+func assertAndValidateData[T any](data map[string]interface{}, key string) []T {
+ val, ok := data[key].([]T)
+ if !ok {
+ panic(fmt.Sprintf("failed to assert %s type", key))
+ }
+ // Validate that we have data
+ if len(val) == 0 {
+ panic(fmt.Sprintf("no %s data found in genesis export", key))
+ }
+ return val
+}
-if accumulatedCommissions, ok := validatorData["ValidatorAccumulatedCommissions"].([]types.ValidatorAccumulatedCommissions); ok {
- genesis.ValidatorAccumulatedCommissions = accumulatedCommissions
-} else {
- panic("Failed to assert ValidatorAccumulatedCommissions type")
-}
+genesis.ValidatorAccumulatedCommissions = assertAndValidateData[types.ValidatorAccumulatedCommissions](
+ validatorData, "ValidatorAccumulatedCommissions")
// Apply similar changes for other type assertions...Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
x/feedistribution/keeper/genesis.go (1)
56-59: Improve error handling for validator data retrieval.The error handling could be more descriptive to aid in debugging genesis export issues.
Enhance the error handling:
validatorData, err := k.GetAllValidatorData(ctx) if err != nil { - panic(errorsmod.Wrap(err, "Error getting validator data").Error()) + panic(errorsmod.Wrapf(err, + "failed to export genesis state: error retrieving validator data").Error()) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
x/feedistribution/keeper/genesis.go(2 hunks)
🧰 Additional context used
📓 Learnings (1)
x/feedistribution/keeper/genesis.go (2)
Learnt from: TimmyExogenous
PR: ExocoreNetwork/exocore#108
File: x/delegation/keeper/genesis.go:52-73
Timestamp: 2024-10-09T01:26:20.424Z
Learning: In the `ExportGenesis` function, panicking on errors without additional logging is acceptable as panic provides detailed context for debugging.
Learnt from: TimmyExogenous
PR: ExocoreNetwork/exocore#108
File: x/delegation/keeper/genesis.go:52-73
Timestamp: 2024-09-17T14:45:22.028Z
Learning: In the `ExportGenesis` function, panicking on errors without additional logging is acceptable as panic provides detailed context for debugging.
🔇 Additional comments (3)
x/feedistribution/keeper/genesis.go (3)
4-4: LGTM: Import addition is appropriate.
The errorsmod import is correctly added and is used for proper error wrapping in the ExportGenesis function.
61-83: 🛠️ Refactor suggestion
Refactor repetitive type assertions and add data validation.
The current implementation has repetitive type assertion code and lacks validation of the exported data. This could be refactored to be more maintainable and robust.
Create a helper function to handle type assertions and validation:
+func assertAndValidateData[T any](data map[string]interface{}, key string) []T {
+ val, ok := data[key].([]T)
+ if !ok {
+ panic(fmt.Sprintf("failed to assert %s type", key))
+ }
+ return val
+}
-if accumulatedCommissions, ok := validatorData["ValidatorAccumulatedCommissions"].([]types.ValidatorAccumulatedCommissions); ok {
- genesis.ValidatorAccumulatedCommissions = accumulatedCommissions
-} else {
- panic("Failed to assert ValidatorAccumulatedCommissions type")
-}
+genesis.ValidatorAccumulatedCommissions = assertAndValidateData[types.ValidatorAccumulatedCommissions](
+ validatorData, "ValidatorAccumulatedCommissions")
// Apply similar changes for other assertionsLikely invalid or redundant comment.
49-84: Verify genesis export functionality.
Given the reported issues with distribution module not recording rewards correctly, we should verify the genesis export functionality.
✅ Verification successful
Let me analyze the implementation of genesis export functionality based on the provided results.
The verification shows:
-
The
GetAllValidatorDataimplementation correctly iterates through all KV store entries and processes different types of rewards (validator accumulated commissions, current rewards, outstanding rewards, and staker rewards). -
The genesis export and import functions are symmetrical:
- Export reads all data using
GetAllValidatorData - Import uses corresponding Set methods (
SetValidatorAccumulatedCommission,SetValidatorCurrentRewards,SetValidatorOutstandingRewards,SetStakerRewards)
- Export reads all data using
-
The reward setting functions are properly implemented in the keeper and are used consistently across the codebase.
Let me verify one more aspect to ensure complete validation:
Let me try a different pattern to check the process functions:
Genesis export functionality is correctly implemented
The verification shows that the genesis export functionality is properly implemented with:
- Complete data collection through
GetAllValidatorDatathat correctly iterates all KV store entries - Proper processing of all reward types through well-implemented helper functions that handle:
- Validator accumulated commissions
- Validator current rewards
- Validator outstanding rewards
- Staker outstanding rewards
- Correct unmarshaling of stored data and address handling
- Symmetrical import/export operations with corresponding getter/setter methods
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the completeness of genesis export data
# Test 1: Check if there are any validator rewards in the system
echo "Checking validator rewards..."
rg -A 5 "SetValidatorCurrentRewards|SetValidatorAccumulatedCommission|SetValidatorOutstandingRewards"
# Test 2: Check if there are any staker rewards in the system
echo "Checking staker rewards..."
rg -A 5 "SetStakerRewards"
# Test 3: Check the implementation of GetAllValidatorData
echo "Checking GetAllValidatorData implementation..."
ast-grep --pattern 'func (k Keeper) GetAllValidatorData(ctx $_) ($_, $_) {
$$$
}'
Length of output: 9086
Script:
#!/bin/bash
# Check the process* helper functions implementation
ast-grep --pattern 'func (k Keeper) process$_(key []byte, value []byte, $list *[]$_) error {
$$$
}'
Length of output: 105
Script:
#!/bin/bash
# Check the process helper functions implementation
rg -A 10 "func \(k Keeper\) process.*Rewards|func \(k Keeper\) process.*Commission" x/feedistribution/keeper/
Length of output: 3855
| validatorAddr, _ := sdk.AccAddressFromBech32(elem.ValAddr) | ||
| k.SetValidatorAccumulatedCommission(ctx, sdk.ValAddress(validatorAddr), *elem.Commission) |
There was a problem hiding this comment.
Add error handling for address conversions.
The Bech32 address conversions ignore potential errors which could lead to invalid addresses being processed. This is particularly risky in genesis initialization.
Apply this pattern to all address conversions:
-validatorAddr, _ := sdk.AccAddressFromBech32(elem.ValAddr)
+validatorAddr, err := sdk.AccAddressFromBech32(elem.ValAddr)
+if err != nil {
+ panic(fmt.Sprintf("invalid validator address in genesis: %s", err))
+}Also applies to: 31-32, 37-38
| for _, elem := range genState.ValidatorAccumulatedCommissions { | ||
| validatorAddr, _ := sdk.AccAddressFromBech32(elem.ValAddr) | ||
| k.SetValidatorAccumulatedCommission(ctx, sdk.ValAddress(validatorAddr), *elem.Commission) | ||
| } |
There was a problem hiding this comment.
Add nil checks for reward and commission pointers.
Multiple places dereference pointers without checking for nil values, which could cause panics during genesis initialization.
Add nil checks before dereferencing pointers:
for _, elem := range genState.ValidatorAccumulatedCommissions {
validatorAddr, err := sdk.AccAddressFromBech32(elem.ValAddr)
if err != nil {
panic(fmt.Sprintf("invalid validator address in genesis: %s", err))
}
+ if elem.Commission == nil {
+ panic("nil commission in genesis for validator " + elem.ValAddr)
+ }
k.SetValidatorAccumulatedCommission(ctx, sdk.ValAddress(validatorAddr), *elem.Commission)
}Also applies to: 29-33, 35-39, 41-44
x/feedistribution/keeper/genesis.go
Outdated
| // Set all the stakerRewards | ||
| for _, elem := range genState.StakerOutstandingRewardsList { | ||
| k.SetStakerRewards(ctx, elem.StakerAddr, *elem.StakerOutstandingRewards) | ||
| } |
There was a problem hiding this comment.
Inconsistent address handling in SetStakerRewards.
While validator addresses are converted using sdk.ValAddress, staker addresses are used directly. This inconsistency could lead to issues.
Apply this fix:
for _, elem := range genState.StakerOutstandingRewardsList {
- k.SetStakerRewards(ctx, elem.StakerAddr, *elem.StakerOutstandingRewards)
+ stakerAddr, err := sdk.AccAddressFromBech32(elem.StakerAddr)
+ if err != nil {
+ panic(fmt.Sprintf("invalid staker address in genesis: %s", err))
+ }
+ if elem.StakerOutstandingRewards == nil {
+ panic("nil outstanding rewards in genesis for staker " + elem.StakerAddr)
+ }
+ k.SetStakerRewards(ctx, sdk.AccAddress(stakerAddr), *elem.StakerOutstandingRewards)
}📝 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.
| // Set all the stakerRewards | |
| for _, elem := range genState.StakerOutstandingRewardsList { | |
| k.SetStakerRewards(ctx, elem.StakerAddr, *elem.StakerOutstandingRewards) | |
| } | |
| // Set all the stakerRewards | |
| for _, elem := range genState.StakerOutstandingRewardsList { | |
| stakerAddr, err := sdk.AccAddressFromBech32(elem.StakerAddr) | |
| if err != nil { | |
| panic(fmt.Sprintf("invalid staker address in genesis: %s", err)) | |
| } | |
| if elem.StakerOutstandingRewards == nil { | |
| panic("nil outstanding rewards in genesis for staker " + elem.StakerAddr) | |
| } | |
| k.SetStakerRewards(ctx, sdk.AccAddress(stakerAddr), *elem.StakerOutstandingRewards) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (5)
x/avs/keeper/genesis.go (1)
66-103: Add validation and logging for exported genesis state.The function should validate the exported state and add logging for large state exports.
Consider adding validation and logging:
func (k Keeper) ExportGenesis(ctx sdk.Context) *types.GenesisState { res := types.GenesisState{} var err error + logger := k.Logger(ctx) res.AvsInfos, err = k.GetAllAVSInfos(ctx) if err != nil { panic(errorsmod.Wrap(err, "failed to get all avs infos")) } + if len(res.AvsInfos) > 1000 { + logger.Info("Exporting large number of AVS infos", "count", len(res.AvsInfos)) + } // ... similar changes for other components ... + // Validate exported state + if err := res.Validate(); err != nil { + panic(errorsmod.Wrap(err, "invalid exported genesis state")) + } return &res }proto/exocore/feedistribution/v1/genesis.proto (1)
23-34: Consider pagination strategy for large genesis states.The repeated fields for validator and staker rewards could potentially grow very large in a production network with many participants. Consider implementing a pagination strategy or documenting size limitations for genesis export/import operations.
x/avs/types/genesis.go (2)
18-58: Consider optimizing the constructor with pre-allocation and validation.The current implementation could be enhanced for better performance and reliability:
- Pre-allocate slices with expected capacity to reduce memory reallocations
- Add basic input validation for critical fields
func NewGenesisState( avsInfos []AVSInfo, taskInfos []TaskInfo, blsPubKeys []BlsPubKeyInfo, taskResultInfos []TaskResultInfo, challengeInfos []ChallengeInfo, taskNums []TaskID, chainIDInfos []ChainIDInfo, ) *GenesisState { // Pre-allocate with capacity if avsInfos == nil { - avsInfos = []AVSInfo{} + avsInfos = make([]AVSInfo, 0, 10) // adjust capacity based on expected size } if taskInfos == nil { - taskInfos = []TaskInfo{} + taskInfos = make([]TaskInfo, 0, 20) } // ... similar changes for other slices ... + // Basic validation + for _, info := range avsInfos { + if info.AvsAddress == "" { + panic("AVS address cannot be empty") + } + } return &GenesisState{ AvsInfos: avsInfos, TaskInfos: taskInfos, BlsPubKeys: blsPubKeys, TaskResultInfos: taskResultInfos, ChallengeInfos: challengeInfos, TaskNums: taskNums, ChainIdInfos: chainIDInfos, } }
99-100: Consider using a helper function for key generation.Multiple places use the same key generation pattern. Consider extracting it into a helper function to improve maintainability and reduce duplication.
+func generateTaskKey(taskAddr string, taskId uint64) string { + return string(assetstype.GetJoinedStoreKey( + strings.ToLower(taskAddr), + strconv.FormatUint(taskId, 10), + )) +} // In Validate function -taskNumKey := assetstype.GetJoinedStoreKey(strings.ToLower(taskNum.TaskAddr), - strconv.FormatUint(taskNum.TaskId, 10)) +taskNumKey := generateTaskKey(taskNum.TaskAddr, taskNum.TaskId)Also applies to: 134-135
x/feedistribution/types/genesis.go (1)
31-67: Refactor validation loops to reduce code duplicationThe validation logic within the loops for
ValidatorAccumulatedCommissions,ValidatorCurrentRewardsList, andValidatorOutstandingRewardsListis similar. Consider abstracting the common validation steps into a helper function to enhance maintainability and reduce repetition.For example, create a helper function:
func validateValidatorRewards(valAddr string, rewards sdk.DecCoins, addrType string) error { var err error if addrType == "validator" { _, err = sdk.ValAddressFromBech32(valAddr) } else { _, err = sdk.AccAddressFromBech32(valAddr) } if err != nil { return fmt.Errorf("invalid %s address: %s", addrType, err) } err = rewards.Validate() if err != nil { return fmt.Errorf("invalid rewards: %s", err) } return nil }Then use it in your loops:
for _, e := range gs.ValidatorCurrentRewardsList { err := validateValidatorRewards(e.ValAddr, e.CurrentRewards.Rewards, "validator") if err != nil { return err } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
proto/exocore/feedistribution/v1/genesis.proto(2 hunks)x/avs/keeper/genesis.go(1 hunks)x/avs/types/genesis.go(1 hunks)x/feedistribution/keeper/genesis_test.go(1 hunks)x/feedistribution/types/genesis.go(2 hunks)x/feedistribution/types/params.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/feedistribution/keeper/genesis_test.go
🔇 Additional comments (9)
x/feedistribution/types/params.go (2)
4-5: LGTM: Import addition is appropriate.
The fmt import is correctly added to support error message formatting in the Validate method.
45-52: Verify impact on distribution functionality.
Given the reported issues with distribution functionality in the PR comments, we should verify if the epoch validation affects reward distribution timing.
✅ Verification successful
Based on the search results, I can now generate the final response since I have enough information about how the EpochIdentifier is used across the codebase.
Epoch validation is correctly implemented and critical for distribution timing
The epoch validation in params.go is essential and correctly implemented as it ensures:
- The EpochIdentifier is used by the fee distribution hooks to trigger reward distribution at epoch boundaries
- It's synchronized with the reward module's epoch-based calculations
- It maintains consistency with the epoch types used across other modules (week, day, hour, minute)
The validation helps prevent distribution timing issues by ensuring only valid epoch identifiers are used, which is critical for:
- Reward distribution timing in x/reward/keeper/hooks.go
- Fee distribution timing in x/feedistribution/keeper/hooks.go
- AVS task scheduling and reward calculations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for dependencies on EpochIdentifier in distribution logic
echo "Checking distribution reward calculation dependencies on epoch..."
rg -l "EpochIdentifier" | grep -v "params.go" | while read -r file; do
echo "=== $file ==="
rg -A 5 "EpochIdentifier" "$file"
done
Length of output: 44513
x/avs/keeper/genesis.go (1)
47-47: Verify address handling consistency across the codebase.
Based on reported issues with AVS address querying and malformed addresses, we should verify the address handling approach across the codebase.
Also applies to: 62-62
✅ Verification successful
Based on the shell script results, I can now generate the final response:
Address handling is consistent and properly validated across the codebase
The codebase demonstrates consistent address handling patterns:
- All addresses are validated using
common.IsHexAddress()before processing - Addresses are consistently converted to lowercase using
strings.ToLower()before storage or comparison - The
SetTaskIDusage in the review is part of this consistent pattern, as the address is properly validated in the genesis validation (x/avs/types/genesis.go)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for inconsistent address handling across the codebase
echo "Checking for direct HexToAddress usage without validation..."
rg "HexToAddress\(" --type go
echo "Checking for address case conversion patterns..."
rg "\.ToLower\(" --type go -A 2 -B 2
echo "Checking for address validation patterns..."
rg "IsHexAddress\(" --type go
Length of output: 38471
proto/exocore/feedistribution/v1/genesis.proto (4)
5-5: LGTM: Import statement is correctly added.
The new import for distribution.proto is necessary for the FeePool and other distribution-related message types.
38-47: Consider renaming message types for clarity and consistency.
The message types are still using plural form which was previously flagged. Consider renaming them to singular form for better consistency:
ValidatorAccumulatedCommissions→ValidatorAccumulatedCommission
68-76: LGTM: StakerOutstandingRewardsList is properly defined.
The message definition has been corrected to properly reference staker address instead of validator address, addressing the previous feedback.
18-34: LGTM: GenesisState fields are well-structured and documented.
The new fields are properly defined with appropriate annotations and clear documentation. The structure allows for comprehensive tracking of fees, commissions, and rewards at both validator and staker levels.
Let's verify the consistency of these proto definitions with their Go implementations:
✅ Verification successful
Proto definitions are correctly implemented and used in the codebase
The verification confirms that:
- The GenesisState struct in the generated Go code matches the proto definition with correct field numbers (1-6)
- All fields have proper protobuf tags and JSON annotations
- The types are being actively used in the codebase, particularly in genesis.go for validation
- Keys are properly defined for state management in keys.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that the Go structs match these proto definitions
# and that the field tags are properly generated.
# Check the generated Go file and its field definitions
rg -A 2 "type (GenesisState|FeePool|ValidatorAccumulatedCommission|ValidatorCurrentRewards|ValidatorOutstandingRewards|StakerOutstandingRewards) struct" x/feedistribution/types/
Length of output: 2402
Script:
#!/bin/bash
# Let's verify the field numbers and types in the generated Go code for GenesisState
rg -A 10 "^type GenesisState struct" x/feedistribution/types/genesis.pb.go
# Also check for any potential usage of these fields to ensure they're being used correctly
rg "FeePool|ValidatorAccumulatedCommissions|ValidatorCurrentRewardsList|ValidatorOutstandingRewardsList|StakerOutstandingRewardsList" x/feedistribution/types/ --type go -g '!*.pb.go'
Length of output: 1857
x/avs/types/genesis.go (2)
60-63: LGTM!
The DefaultGenesis implementation correctly utilizes the NewGenesisState constructor with nil parameters, which will be initialized as empty slices.
67-163: 🛠️ Refactor suggestion
Add cross-field validation checks.
The current validation focuses on individual fields but lacks validation of relationships between different types of data.
Add these validations:
- Verify AVS addresses in ChainIdInfos exist in AvsInfos
- Verify task addresses in TaskNums exist in TaskInfos
- Verify task addresses in TaskResultInfos exist in TaskInfos
func (gs GenesisState) Validate() error {
// ... existing validation code ...
+ // Cross-field validation
+ for _, chainInfo := range gs.ChainIdInfos {
+ if !avsAddresses[chainInfo.AvsAddress] {
+ return fmt.Errorf("AVS address %s in ChainIdInfos not found in AvsInfos", chainInfo.AvsAddress)
+ }
+ }
+
+ for _, taskNum := range gs.TaskNums {
+ if !taskInfoMap[taskNum.TaskAddr] {
+ return fmt.Errorf("task address %s in TaskNums not found in TaskInfos", taskNum.TaskAddr)
+ }
+ }
+
+ for _, result := range gs.TaskResultInfos {
+ if !taskInfoMap[result.TaskContractAddress] {
+ return fmt.Errorf("task address %s in TaskResultInfos not found in TaskInfos", result.TaskContractAddress)
+ }
+ }
return nil
}Likely invalid or redundant comment.
x/feedistribution/types/params.go
Outdated
| if p.EpochIdentifier == "" { | ||
| p.EpochIdentifier = DefaultEpochIdentifier | ||
| } | ||
| if p.EpochIdentifier != epochstypes.WeekEpochID && p.EpochIdentifier != epochstypes.DayEpochID && | ||
| p.EpochIdentifier != epochstypes.HourEpochID && p.EpochIdentifier != epochstypes.MinuteEpochID { | ||
| return fmt.Errorf("EpochIdentifier does not exist") | ||
| } | ||
|
|
There was a problem hiding this comment.
Several critical issues in the Validate method need attention.
- The assignment to
p.EpochIdentifierwon't work as the receiver is passed by value. The method should use a pointer receiver. - The error message should list the valid epoch identifiers.
- Missing validation for
CommunityTax.
Here's the suggested fix:
- func (p Params) Validate() error {
+ func (p *Params) Validate() error {
if p.EpochIdentifier == "" {
p.EpochIdentifier = DefaultEpochIdentifier
}
if p.EpochIdentifier != epochstypes.WeekEpochID && p.EpochIdentifier != epochstypes.DayEpochID &&
p.EpochIdentifier != epochstypes.HourEpochID && p.EpochIdentifier != epochstypes.MinuteEpochID {
- return fmt.Errorf("EpochIdentifier does not exist")
+ return fmt.Errorf("invalid epoch identifier %s: must be one of %s, %s, %s, or %s",
+ p.EpochIdentifier,
+ epochstypes.WeekEpochID,
+ epochstypes.DayEpochID,
+ epochstypes.HourEpochID,
+ epochstypes.MinuteEpochID)
}
+
+ if p.CommunityTax.IsNegative() || p.CommunityTax.GT(sdk.OneDec()) {
+ return fmt.Errorf("community tax must be between 0 and 1: %s", p.CommunityTax)
+ }
return nil
}Additionally, consider defining constants for the valid epoch identifiers to improve maintainability:
var ValidEpochIdentifiers = []string{
epochstypes.WeekEpochID,
epochstypes.DayEpochID,
epochstypes.HourEpochID,
epochstypes.MinuteEpochID,
}| ctx sdk.Context, | ||
| _ types.GenesisState, | ||
| ) []abci.ValidatorUpdate { | ||
| func (k Keeper) InitGenesis(ctx sdk.Context, state types.GenesisState) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add nil state validation.
The function should validate that the input state is not nil before processing.
Add validation at the start of the function:
func (k Keeper) InitGenesis(ctx sdk.Context, state types.GenesisState) {
+ if state.AvsInfos == nil || state.TaskInfos == nil || state.BlsPubKeys == nil ||
+ state.TaskNums == nil || state.TaskResultInfos == nil ||
+ state.ChallengeInfos == nil || state.ChainIdInfos == nil {
+ panic(errorsmod.Wrap(types.ErrInvalidGenesis, "nil state components"))
+ }📝 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) InitGenesis(ctx sdk.Context, state types.GenesisState) { | |
| func (k Keeper) InitGenesis(ctx sdk.Context, state types.GenesisState) { | |
| if state.AvsInfos == nil || state.TaskInfos == nil || state.BlsPubKeys == nil || | |
| state.TaskNums == nil || state.TaskResultInfos == nil || | |
| state.ChallengeInfos == nil || state.ChainIdInfos == nil { | |
| panic(errorsmod.Wrap(types.ErrInvalidGenesis, "nil state components")) | |
| } |
| for _, avs := range state.AvsInfos { | ||
| avs.AvsAddress = strings.ToLower(avs.AvsAddress) | ||
| avs.TaskAddr = strings.ToLower(avs.TaskAddr) | ||
| avs.RewardAddr = strings.ToLower(avs.RewardAddr) | ||
| avs.SlashAddr = strings.ToLower(avs.SlashAddr) | ||
| err := k.SetAVSInfo(ctx, &avs) //nolint:gosec | ||
| if err != nil { | ||
| panic(errorsmod.Wrap(err, "failed to set all avs info")) | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance address validation for AVS info.
While converting addresses to lowercase helps with case sensitivity, additional validation should be performed to ensure addresses are valid Ethereum addresses.
Consider adding address validation:
for _, avs := range state.AvsInfos {
+ if !common.IsHexAddress(avs.AvsAddress) || !common.IsHexAddress(avs.TaskAddr) ||
+ !common.IsHexAddress(avs.RewardAddr) || !common.IsHexAddress(avs.SlashAddr) {
+ panic(errorsmod.Wrap(types.ErrInvalidAddress, "invalid address in AVS info"))
+ }
avs.AvsAddress = strings.ToLower(avs.AvsAddress)
avs.TaskAddr = strings.ToLower(avs.TaskAddr)
avs.RewardAddr = strings.ToLower(avs.RewardAddr)
avs.SlashAddr = strings.ToLower(avs.SlashAddr)Committable suggestion skipped: line range outside the PR's diff.
| if !common.IsHexAddress(info.AvsAddress) { | ||
| return fmt.Errorf("invalid AVS address: %s", info.AvsAddress) | ||
| } |
There was a problem hiding this comment.
Standardize address validation to handle case sensitivity.
To address the reported case sensitivity issues, convert and store addresses in checksum format consistently:
-if !common.IsHexAddress(info.AvsAddress) {
+address := common.HexToAddress(info.AvsAddress)
+if address == common.HexToAddress("") {
return fmt.Errorf("invalid AVS address: %s", info.AvsAddress)
}
+// Store the checksummed address
+info.AvsAddress = address.Hex()Apply similar changes to all address validations in the file.
Also applies to: 84-85, 96-97, 127-128, 155-156
| _, err := sdk.AccAddressFromBech32(e.ValAddr) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid addr: %s", err) | ||
| } |
There was a problem hiding this comment.
Use sdk.ValAddressFromBech32 for validator addresses
In lines 32, 42, and 52, the code uses sdk.AccAddressFromBech32 to parse validator addresses (ValAddr). Validator addresses should be parsed using sdk.ValAddressFromBech32 because they are of type sdk.ValAddress, not sdk.AccAddress. Using the incorrect address type may lead to validation failures or incorrect behavior.
Apply this diff to correct the address parsing:
- _, err := sdk.AccAddressFromBech32(e.ValAddr)
+ _, err := sdk.ValAddressFromBech32(e.ValAddr)This change should be applied to each instance where ValAddr is parsed.
Also applies to: 42-45, 52-55
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (3)
x/feedistribution/types/genesis.go (2)
26-28: Enhance FeePool validation error messageConsider making the error message more specific by including the actual validation error details.
- return fmt.Errorf("invalid FeePool: %s", err) + return fmt.Errorf("invalid FeePool CommunityPool: %s", err)
26-66: Consider enhancing the validation frameworkGiven the reported issues with rewards distribution and empty rewards, consider these architectural improvements:
- Add validation for reward amounts (e.g., non-negative values)
- Add validation for duplicate entries in the lists
- Consider adding invariant checks (e.g., total rewards distribution matches expected values)
- Add detailed logging for validation failures to aid debugging
This would help catch potential issues early in the genesis validation phase.
x/feedistribution/keeper/genesis.go (1)
51-85: Refactor type assertions to reduce code duplication.The repeated type assertion pattern can be simplified using a generic helper function.
Apply this diff:
+func assertValidatorData[T any](data map[string]interface{}, key string) []T { + val, ok := data[key].([]T) + if !ok { + panic(fmt.Sprintf("failed to assert %s type", key)) + } + return val +} func (k Keeper) ExportGenesis(ctx sdk.Context) *types.GenesisState { genesis := types.GenesisState{} genesis.Params = k.GetParams(ctx) feePool := k.GetFeePool(ctx) if feePool == nil { panic("fee pool cannot be nil in genesis export") } genesis.FeePool = *feePool validatorData, err := k.GetAllValidatorData(ctx) if err != nil { panic(errorsmod.Wrap(err, "Error getting validator data").Error()) } - if accumulatedCommissions, ok := validatorData["ValidatorAccumulatedCommissions"].([]types.ValidatorAccumulatedCommissions); ok { - genesis.ValidatorAccumulatedCommissions = accumulatedCommissions - } else { - panic("Failed to assert ValidatorAccumulatedCommissions type") - } + genesis.ValidatorAccumulatedCommissions = assertValidatorData[types.ValidatorAccumulatedCommissions]( + validatorData, "ValidatorAccumulatedCommissions") - if currentRewardsList, ok := validatorData["ValidatorCurrentRewardsList"].([]types.ValidatorCurrentRewardsList); ok { - genesis.ValidatorCurrentRewardsList = currentRewardsList - } else { - panic("Failed to assert ValidatorCurrentRewardsList type") - } + genesis.ValidatorCurrentRewardsList = assertValidatorData[types.ValidatorCurrentRewardsList]( + validatorData, "ValidatorCurrentRewardsList") - if outstandingRewardsList, ok := validatorData["ValidatorOutstandingRewardsList"].([]types.ValidatorOutstandingRewardsList); ok { - genesis.ValidatorOutstandingRewardsList = outstandingRewardsList - } else { - panic("Failed to assert ValidatorOutstandingRewardsList type") - } + genesis.ValidatorOutstandingRewardsList = assertValidatorData[types.ValidatorOutstandingRewardsList]( + validatorData, "ValidatorOutstandingRewardsList") - if stakerRewardsList, ok := validatorData["StakerOutstandingRewardsList"].([]types.StakerOutstandingRewardsList); ok { - genesis.StakerOutstandingRewardsList = stakerRewardsList - } else { - panic("Failed to assert StakerOutstandingRewardsList type") - } + genesis.StakerOutstandingRewardsList = assertValidatorData[types.StakerOutstandingRewardsList]( + validatorData, "StakerOutstandingRewardsList") return &genesis }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
x/feedistribution/keeper/genesis.go(2 hunks)x/feedistribution/types/genesis.go(2 hunks)x/feedistribution/types/params.go(2 hunks)
🧰 Additional context used
📓 Learnings (1)
x/feedistribution/keeper/genesis.go (4)
Learnt from: TimmyExogenous
PR: ExocoreNetwork/exocore#108
File: x/delegation/keeper/genesis.go:52-73
Timestamp: 2024-10-09T01:26:20.424Z
Learning: In the `ExportGenesis` function, panicking on errors without additional logging is acceptable as panic provides detailed context for debugging.
Learnt from: TimmyExogenous
PR: ExocoreNetwork/exocore#108
File: x/delegation/keeper/genesis.go:52-73
Timestamp: 2024-09-17T14:45:22.028Z
Learning: In the `ExportGenesis` function, panicking on errors without additional logging is acceptable as panic provides detailed context for debugging.
Learnt from: TimmyExogenous
PR: ExocoreNetwork/exocore#108
File: x/operator/keeper/genesis.go:63-96
Timestamp: 2024-09-17T14:46:25.700Z
Learning: In the `ExportGenesis` function, panicking on errors is acceptable because the node is offline during genesis export.
Learnt from: TimmyExogenous
PR: ExocoreNetwork/exocore#108
File: x/operator/keeper/genesis.go:63-96
Timestamp: 2024-10-09T01:26:23.510Z
Learning: In the `ExportGenesis` function, panicking on errors is acceptable because the node is offline during genesis export.
🔇 Additional comments (7)
x/feedistribution/types/params.go (1)
4-5: LGTM!
The fmt import is correctly added to support error formatting.
x/feedistribution/types/genesis.go (3)
3-7: LGTM: Required imports added correctly
30-39: Previous review comment about validator address validation is still applicable
The code still uses sdk.AccAddressFromBech32 for validator addresses instead of sdk.ValAddressFromBech32.
60-66: Verify rewards validation implementation
Given the reported issues with empty staker rewards in the PR objectives, let's verify the rewards validation implementation:
#!/bin/bash
# Search for StakerOutstandingRewards usage and implementation
echo "Searching for StakerOutstandingRewards usage..."
rg -A 5 "StakerOutstandingRewards"
# Search for reward calculation logic
echo "Searching for reward calculation implementation..."
rg -A 10 "calculateRewards|UpdateRewards|AllocateTokens"The previous review comment about validating staker addresses is still applicable.
x/feedistribution/keeper/genesis.go (3)
4-7: LGTM!
The new imports are necessary and used appropriately for error handling and string formatting.
20-21: LGTM!
The improved panic message provides better context by including the epoch ID. Panicking is appropriate here as it occurs during genesis initialization.
22-23:
Add nil check for FeePool.
Add a nil check before dereferencing genState.FeePool to prevent potential panics.
| if err := epochstypes.ValidateEpochIdentifierInterface(p.EpochIdentifier); err != nil { | ||
| return fmt.Errorf("epoch identifier: %w", err) | ||
| } |
There was a problem hiding this comment.
Critical issues in the Validate method need attention.
While the epoch identifier validation has improved, there are still several critical issues that need to be addressed:
- The method should use a pointer receiver to allow modifications
- Missing validation for
CommunityTaxwhich should be between 0 and 1
Apply this diff to fix the issues:
- func (p Params) Validate() error {
+ func (p *Params) Validate() error {
if err := epochstypes.ValidateEpochIdentifierInterface(p.EpochIdentifier); err != nil {
return fmt.Errorf("epoch identifier: %w", err)
}
+
+ if p.CommunityTax.IsNegative() || p.CommunityTax.GT(sdk.OneDec()) {
+ return fmt.Errorf("community tax must be between 0 and 1: %s", p.CommunityTax)
+ }
+
return nil
}📝 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 err := epochstypes.ValidateEpochIdentifierInterface(p.EpochIdentifier); err != nil { | |
| return fmt.Errorf("epoch identifier: %w", err) | |
| } | |
| func (p *Params) Validate() error { | |
| if err := epochstypes.ValidateEpochIdentifierInterface(p.EpochIdentifier); err != nil { | |
| return fmt.Errorf("epoch identifier: %w", err) | |
| } | |
| if p.CommunityTax.IsNegative() || p.CommunityTax.GT(sdk.OneDec()) { | |
| return fmt.Errorf("community tax must be between 0 and 1: %s", p.CommunityTax) | |
| } | |
| return nil | |
| } |
| // Set all the validatorOutstandingRewards | ||
| for _, elem := range genState.ValidatorOutstandingRewardsList { | ||
| validatorAddr, _ := sdk.AccAddressFromBech32(elem.ValAddr) | ||
| k.SetValidatorOutstandingRewards(ctx, sdk.ValAddress(validatorAddr), *elem.OutstandingRewards) | ||
| } |
There was a problem hiding this comment.
Add error handling and nil checks.
The code has the same potential issues as the previous sections.
Apply this diff:
for _, elem := range genState.ValidatorOutstandingRewardsList {
- validatorAddr, _ := sdk.AccAddressFromBech32(elem.ValAddr)
+ validatorAddr, err := sdk.AccAddressFromBech32(elem.ValAddr)
+ if err != nil {
+ panic(fmt.Sprintf("invalid validator address in genesis: %s", err))
+ }
+ if elem.OutstandingRewards == nil {
+ panic("nil outstanding rewards in genesis for validator " + elem.ValAddr)
+ }
k.SetValidatorOutstandingRewards(ctx, sdk.ValAddress(validatorAddr), *elem.OutstandingRewards)
}📝 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.
| // Set all the validatorOutstandingRewards | |
| for _, elem := range genState.ValidatorOutstandingRewardsList { | |
| validatorAddr, _ := sdk.AccAddressFromBech32(elem.ValAddr) | |
| k.SetValidatorOutstandingRewards(ctx, sdk.ValAddress(validatorAddr), *elem.OutstandingRewards) | |
| } | |
| // Set all the validatorOutstandingRewards | |
| for _, elem := range genState.ValidatorOutstandingRewardsList { | |
| validatorAddr, err := sdk.AccAddressFromBech32(elem.ValAddr) | |
| if err != nil { | |
| panic(fmt.Sprintf("invalid validator address in genesis: %s", err)) | |
| } | |
| if elem.OutstandingRewards == nil { | |
| panic("nil outstanding rewards in genesis for validator " + elem.ValAddr) | |
| } | |
| k.SetValidatorOutstandingRewards(ctx, sdk.ValAddress(validatorAddr), *elem.OutstandingRewards) | |
| } |
| // Set all the stakerRewards | ||
| for _, elem := range genState.StakerOutstandingRewardsList { | ||
| k.SetStakerRewards(ctx, elem.StakerAddr, *elem.StakerOutstandingRewards) |
There was a problem hiding this comment.
Add address validation and nil check.
The staker rewards initialization is missing important validations.
Apply this diff:
for _, elem := range genState.StakerOutstandingRewardsList {
+ if _, err := sdk.AccAddressFromBech32(elem.StakerAddr); err != nil {
+ panic(fmt.Sprintf("invalid staker address in genesis: %s", err))
+ }
+ if elem.StakerOutstandingRewards == nil {
+ panic("nil outstanding rewards in genesis for staker " + elem.StakerAddr)
+ }
k.SetStakerRewards(ctx, elem.StakerAddr, *elem.StakerOutstandingRewards)
}📝 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.
| // Set all the stakerRewards | |
| for _, elem := range genState.StakerOutstandingRewardsList { | |
| k.SetStakerRewards(ctx, elem.StakerAddr, *elem.StakerOutstandingRewards) | |
| // Set all the stakerRewards | |
| for _, elem := range genState.StakerOutstandingRewardsList { | |
| if _, err := sdk.AccAddressFromBech32(elem.StakerAddr); err != nil { | |
| panic(fmt.Sprintf("invalid staker address in genesis: %s", err)) | |
| } | |
| if elem.StakerOutstandingRewards == nil { | |
| panic("nil outstanding rewards in genesis for staker " + elem.StakerAddr) | |
| } | |
| k.SetStakerRewards(ctx, elem.StakerAddr, *elem.StakerOutstandingRewards) |
| // Set all the validatorCurrentRewards | ||
| for _, elem := range genState.ValidatorCurrentRewardsList { | ||
| validatorAddr, _ := sdk.AccAddressFromBech32(elem.ValAddr) | ||
| k.SetValidatorCurrentRewards(ctx, sdk.ValAddress(validatorAddr), *elem.CurrentRewards) | ||
| } |
There was a problem hiding this comment.
Add error handling and nil checks.
The code has the same potential issues as the previous section.
Apply this diff:
for _, elem := range genState.ValidatorCurrentRewardsList {
- validatorAddr, _ := sdk.AccAddressFromBech32(elem.ValAddr)
+ validatorAddr, err := sdk.AccAddressFromBech32(elem.ValAddr)
+ if err != nil {
+ panic(fmt.Sprintf("invalid validator address in genesis: %s", err))
+ }
+ if elem.CurrentRewards == nil {
+ panic("nil current rewards in genesis for validator " + elem.ValAddr)
+ }
k.SetValidatorCurrentRewards(ctx, sdk.ValAddress(validatorAddr), *elem.CurrentRewards)
}📝 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.
| // Set all the validatorCurrentRewards | |
| for _, elem := range genState.ValidatorCurrentRewardsList { | |
| validatorAddr, _ := sdk.AccAddressFromBech32(elem.ValAddr) | |
| k.SetValidatorCurrentRewards(ctx, sdk.ValAddress(validatorAddr), *elem.CurrentRewards) | |
| } | |
| // Set all the validatorCurrentRewards | |
| for _, elem := range genState.ValidatorCurrentRewardsList { | |
| validatorAddr, err := sdk.AccAddressFromBech32(elem.ValAddr) | |
| if err != nil { | |
| panic(fmt.Sprintf("invalid validator address in genesis: %s", err)) | |
| } | |
| if elem.CurrentRewards == nil { | |
| panic("nil current rewards in genesis for validator " + elem.ValAddr) | |
| } | |
| k.SetValidatorCurrentRewards(ctx, sdk.ValAddress(validatorAddr), *elem.CurrentRewards) | |
| } |
| // Set all the validatorAccumulatedCommission | ||
| for _, elem := range genState.ValidatorAccumulatedCommissions { | ||
| validatorAddr, _ := sdk.AccAddressFromBech32(elem.ValAddr) | ||
| k.SetValidatorAccumulatedCommission(ctx, sdk.ValAddress(validatorAddr), *elem.Commission) | ||
| } |
There was a problem hiding this comment.
Add error handling and nil checks.
The code has two potential issues:
- Address conversion errors are ignored
- Missing nil check for Commission pointer
Apply this diff:
for _, elem := range genState.ValidatorAccumulatedCommissions {
- validatorAddr, _ := sdk.AccAddressFromBech32(elem.ValAddr)
+ validatorAddr, err := sdk.AccAddressFromBech32(elem.ValAddr)
+ if err != nil {
+ panic(fmt.Sprintf("invalid validator address in genesis: %s", err))
+ }
+ if elem.Commission == nil {
+ panic("nil commission in genesis for validator " + elem.ValAddr)
+ }
k.SetValidatorAccumulatedCommission(ctx, sdk.ValAddress(validatorAddr), *elem.Commission)
}📝 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.
| // Set all the validatorAccumulatedCommission | |
| for _, elem := range genState.ValidatorAccumulatedCommissions { | |
| validatorAddr, _ := sdk.AccAddressFromBech32(elem.ValAddr) | |
| k.SetValidatorAccumulatedCommission(ctx, sdk.ValAddress(validatorAddr), *elem.Commission) | |
| } | |
| // Set all the validatorAccumulatedCommission | |
| for _, elem := range genState.ValidatorAccumulatedCommissions { | |
| validatorAddr, err := sdk.AccAddressFromBech32(elem.ValAddr) | |
| if err != nil { | |
| panic(fmt.Sprintf("invalid validator address in genesis: %s", err)) | |
| } | |
| if elem.Commission == nil { | |
| panic("nil commission in genesis for validator " + elem.ValAddr) | |
| } | |
| k.SetValidatorAccumulatedCommission(ctx, sdk.ValAddress(validatorAddr), *elem.Commission) | |
| } |
Description
genesis exporting for avs,distribution modules
Summary by CodeRabbit
Release Notes
New Features
GenesisState,TaskID, and reward tracking messages.GenesisStatewith various scenarios.Bug Fixes
Documentation
GenesisStateand task result submissions.Chores