feat(avs):implement whitelist feature for AVS #262
Conversation
WalkthroughThe changes in this pull request primarily involve modifications to the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Outside diff range and nitpick comments (5)
x/avs/keeper/avs_test.go (2)
26-27: Consider using a more descriptive variable name.The variable name
operatorAddresscould be more descriptive, such aswhitelistedOperatorAddress, to better reflect its purpose in testing the whitelist feature.
43-43: Enhance test coverage for whitelist feature.Consider adding test cases for:
- Multiple whitelisted operators
- Empty whitelist
- Invalid operator addresses
proto/exocore/avs/v1/tx.proto (1)
57-58: Consider adding field options for better type safety.Add the cosmos_proto.scalar option to ensure proper type validation for bech32 addresses:
// whitelist_address are the bech32 addresses ,whitelist address of supported operators - repeated string whitelist_address = 19; + repeated string whitelist_address = 19 [(cosmos_proto.scalar) = "cosmos.AddressString"];precompiles/avs/IAVSManager.sol (1)
52-52: Enhance the whitelist parameter documentation.The documentation for
whitelistAddressparameter should be more descriptive and include:
- Purpose of the whitelist
- Format requirements for addresses
- Validation rules or constraints
- Maximum allowed addresses
precompiles/avs/types.go (1)
71-84: Reduce code duplication in whitelist address validation.The whitelist address validation logic is duplicated between
GetAVSParamsFromInputsandGetAVSParamsFromUpdateInputs. Consider extracting this into a helper function.Example refactor:
func validateWhitelistAddresses(addresses []string, argIndex int) ([]string, error) { if addresses == nil { return nil, fmt.Errorf(exocmn.ErrContractInputParaOrType, argIndex, "[]string", addresses) } exoWhiteAddresses := make([]string, len(addresses)) for i, addr := range addresses { accAddr, err := sdk.AccAddressFromBech32(addr) if err != nil { return nil, fmt.Errorf(exocmn.ErrContractInputParaOrType, argIndex, "[]string", addresses) } exoWhiteAddresses[i] = accAddr.String() } return exoWhiteAddresses, nil }Also applies to: 187-200
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
x/avs/types/tx.pb.gois excluded by!**/*.pb.go
📒 Files selected for processing (10)
precompiles/avs/IAVSManager.sol(4 hunks)precompiles/avs/abi.json(2 hunks)precompiles/avs/types.go(2 hunks)proto/exocore/avs/v1/tx.proto(1 hunks)x/avs/keeper/avs.go(2 hunks)x/avs/keeper/avs_test.go(2 hunks)x/avs/keeper/keeper.go(2 hunks)x/avs/types/types.go(1 hunks)x/operator/keeper/opt.go(1 hunks)x/operator/types/expected_keepers.go(1 hunks)
🧰 Additional context used
📓 Learnings (1)
x/operator/types/expected_keepers.go (2)
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore#149
File: x/operator/keeper/slash.go:227-227
Timestamp: 2024-11-12T10:03:15.304Z
Learning: The `GetAVSAddrByChainID` function in `x/operator/keeper/slash.go` does not require error handling since validation is performed prior to its call.
Learnt from: MaxMustermann2
PR: ExocoreNetwork/exocore#220
File: x/avs/types/expected_keepers.go:47-48
Timestamp: 2024-11-12T10:03:10.791Z
Learning: Adding new methods to the `OperatorKeeper` interface in `x/avs/types/expected_keepers.go` is acceptable, even if there are other unmodifiable `OperatorKeeper` interfaces elsewhere, because interfaces can be extended without causing conflicts.
🪛 golangci-lint (1.62.2)
x/avs/keeper/avs.go
4-4: File is not gofumpt-ed
(gofumpt)
🪛 GitHub Check: Run golangci-lint
x/avs/keeper/avs.go
[failure] 4-4:
File is not gofumpt-ed (gofumpt)
🔇 Additional comments (3)
x/operator/types/expected_keepers.go (1)
116-116: LGTM: Interface extension is well-designed
The addition of the IsWhitelisted method to the AVSKeeper interface is clean and follows the existing patterns. The method signature appropriately returns both the whitelist status and potential errors.
x/operator/keeper/opt.go (1)
32-35: LGTM: Validation order is correct
The placement of the whitelist check after the AVS registration check but before the opted-in check is logical and maintains proper validation order.
precompiles/avs/abi.json (1)
649-653: LGTM! ABI changes are consistent and well-structured.
The changes to the ABI:
- Correctly add the whitelistAddress parameter to both registerAVS and updateAVS functions
- Maintain consistent type definitions
- Include proper event definitions for tracking AVS operations
Also applies to: 786-790
| if params.WhitelistAddress != nil { | ||
| avs.WhitelistAddress = params.WhitelistAddress | ||
| } |
There was a problem hiding this comment.
Add validation for WhitelistAddress during AVS updates
Similar to the registration flow, the update flow also lacks address validation for WhitelistAddress.
Add validation before updating WhitelistAddress:
if params.WhitelistAddress != nil {
+ if err := k.validateAddresses(params.WhitelistAddress); err != nil {
+ return err
+ }
avs.WhitelistAddress = params.WhitelistAddress
}📝 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 params.WhitelistAddress != nil { | |
| avs.WhitelistAddress = params.WhitelistAddress | |
| } | |
| if params.WhitelistAddress != nil { | |
| if err := k.validateAddresses(params.WhitelistAddress); err != nil { | |
| return err | |
| } | |
| avs.WhitelistAddress = params.WhitelistAddress | |
| } |
There was a problem hiding this comment.
Please consider this comment from the AI.
There was a problem hiding this comment.
@MaxMustermann2, here's an example of how you could incorporate address validation for the WhitelistAddress before updating:
// helper function to validate a list of Bech32 addresses
func (k Keeper) validateAddresses(addresses []string) error {
for _, addr := range addresses {
if _, err := sdk.AccAddressFromBech32(addr); err != nil {
return errorsmod.Wrap(types.ErrInvalidAddr, fmt.Sprintf("Invalid whitelist address: %s", addr))
}
}
return nil
}
// snippet inside UpdateAVSInfo
if params.WhitelistAddress != nil {
if err := k.validateAddresses(params.WhitelistAddress); err != nil {
return err
}
avs.WhitelistAddress = params.WhitelistAddress
}This ensures each address is checked for validity prior to updating the whitelist.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
precompiles/avs/avs_test.go (3)
Line range hint
4-6: Consider using constants for magic numbers and improving variable names.The test setup uses several magic numbers (e.g.,
uint64(3)) and could benefit from more descriptive variable names. This would make the tests more maintainable and self-documenting.Consider defining constants and using more descriptive names:
+ const ( + DefaultUnbondingPeriod = uint64(3) + DefaultMinStakeAmount = uint64(3) + DefaultMinSelfDelegation = uint64(3) + DefaultMinOperators = uint64(2) + DefaultMinTotalStake = uint64(3) + DefaultRewardRate = uint64(4) + DefaultSlashRate = uint64(4) + ) - minStakeAmount, taskAddr := uint64(3), "0x3e..." - avsUnbondingPeriod, minSelfDelegation := uint64(3), uint64(3) - params := []uint64{2, 3, 4, 4} + minStakeAmount, taskAddr := DefaultMinStakeAmount, "0x3e..." + avsUnbondingPeriod, minSelfDelegation := DefaultUnbondingPeriod, DefaultMinSelfDelegation + params := []uint64{DefaultMinOperators, DefaultMinTotalStake, DefaultRewardRate, DefaultSlashRate}Also applies to: 12-24
Line range hint
1-1000: Enhance test coverage with error cases and edge scenarios.The current test suite primarily focuses on successful scenarios. Consider adding test cases for:
- Invalid inputs (e.g., empty addresses, zero values)
- Error conditions (e.g., duplicate registrations, invalid state transitions)
- Edge cases (e.g., maximum values, boundary conditions)
Example test cases to add:
// Test invalid AVS registration { name: "fail - empty AVS name", malleate: func() (common.Address, []byte) { input, err := suite.precompile.Pack( avs.MethodRegisterAVS, suite.Address, "", // empty name minStakeAmount, // ... rest of parameters ) return suite.Address, input }, expPass: false, errContains: "empty AVS name", }, { name: "fail - zero stake amount", malleate: func() (common.Address, []byte) { input, err := suite.precompile.Pack( avs.MethodRegisterAVS, suite.Address, avsName, uint64(0), // zero stake // ... rest of parameters ) return suite.Address, input }, expPass: false, errContains: "invalid stake amount", }
Line range hint
1-1000: Improve test organization and reduce duplication.The test methods are quite long and contain duplicated setup code. Consider refactoring to:
- Extract common test setup into reusable helper functions
- Split large test methods into smaller, focused test cases
- Use table-driven tests more consistently
Example refactoring:
// Common setup helper func (suite *AVSManagerPrecompileSuite) setupTestAVS(name string, stake uint64) *types.AVSInfo { slashAddress := "0xDF907c29719154eb9872f021d21CAE6E5025d7aB" rewardAddress := "0xDF907c29719154eb9872f021d21CAE6E5025d7aB" avsOwnerAddress := []string{ sdk.AccAddress(suite.Address.Bytes()).String(), sdk.AccAddress(utiltx.GenerateAddress().Bytes()).String(), sdk.AccAddress(utiltx.GenerateAddress().Bytes()).String(), } // ... rest of setup return avs } // Smaller, focused test method func (suite *AVSManagerPrecompileSuite) TestRegisterAVS_Validation() { testCases := []struct { name string avs *types.AVSInfo err string }{ { name: "valid AVS", avs: suite.setupTestAVS("test", 100), err: "", }, // ... more test cases } // ... test execution }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
precompiles/avs/avs_test.go(2 hunks)x/avs/keeper/avs.go(2 hunks)
🔇 Additional comments (2)
x/avs/keeper/avs.go (2)
6-6: LGTM!
The addition of the slices package is appropriate for the new whitelist functionality.
229-247:
Fix error handling and improve validation.
Several critical issues still need to be addressed:
- Missing validation for
avsAddrformat usingcommon.IsHexAddress - Line 244 incorrectly uses a nil
errin error wrapping - Missing function documentation
- The empty whitelist behavior should be documented
Please refer to the previous review comment for the detailed fix.
trestinlsd
requested review from
MaxMustermann2,
mikebraver and
TimmyExogenous
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
precompiles/avs/avs_test.go(4 hunks)precompiles/avs/types.go(2 hunks)x/avs/keeper/avs.go(2 hunks)x/operator/keeper/opt.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- x/operator/keeper/opt.go
🔇 Additional comments (2)
x/avs/keeper/avs.go (1)
229-251:
Fix error handling and improve validation.
Several issues need to be addressed:
- Line 248 incorrectly uses a nil
errin error wrapping - Missing validation for
avsAddrformat usingcommon.IsHexAddress - The empty whitelist behavior should be documented
Apply this diff to fix the issues:
// IsWhitelisted checks if an operator is in the AVS whitelist.
// If the whitelist is empty, any operator is considered whitelisted.
// Returns true if the operator is whitelisted, false otherwise.
// Returns an error if the AVS address is invalid, the operator address is invalid,
// or if the operator is not in the whitelist.
func (k *Keeper) IsWhitelisted(ctx sdk.Context, avsAddr, operatorAddr string) (bool, error) {
+ if !common.IsHexAddress(avsAddr) {
+ return false, errorsmod.Wrap(types.ErrInvalidAddr, fmt.Sprintf("IsWhitelisted: invalid AVS address %s", avsAddr))
+ }
avsInfo, err := k.GetAVSInfo(ctx, avsAddr)
if err != nil {
return false, errorsmod.Wrap(err, fmt.Sprintf("IsWhitelisted: key is %s", avsAddr))
}
_, err = sdk.AccAddressFromBech32(operatorAddr)
if err != nil {
return false, errorsmod.Wrap(err, "IsWhitelisted: error occurred when parse acc address from Bech32")
}
// Currently avs has no whitelist set and any operator can optin
if len(avsInfo.Info.WhitelistAddress) == 0 {
return true, nil
}
if !slices.Contains(avsInfo.Info.WhitelistAddress, operatorAddr) {
- return false, errorsmod.Wrap(err, fmt.Sprintf("operator %s not in whitelist", operatorAddr))
+ return false, errorsmod.Wrap(types.ErrNotWhitelisted, fmt.Sprintf("operator %s not in whitelist", operatorAddr))
}
return true, nil
}Likely invalid or redundant comment.
precompiles/avs/types.go (1)
198-198:
Fix critical bug: Using wrong slice variable for whitelist addresses.
The code is using exoAddresses instead of exoWhiteAddresses when storing validated whitelist addresses. This bug will cause the whitelist to be corrupted with owner addresses.
Apply this fix:
- exoAddresses[i] = accAddr.String()
+ exoWhiteAddresses[i] = accAddr.String()Likely invalid or redundant comment.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
precompiles/avs/avs_test.go (1)
Line range hint
1-1000: Add test cases for concurrent whitelist operations.The test suite should include scenarios testing concurrent operations with the whitelist:
- Multiple operators attempting to register simultaneously
- Whitelist updates while operator registration is in progress
- Race conditions during whitelist modifications
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
precompiles/avs/avs_test.go(4 hunks)precompiles/avs/types.go(2 hunks)x/avs/keeper/avs.go(2 hunks)x/operator/keeper/opt.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- x/operator/keeper/opt.go
- precompiles/avs/types.go
🔇 Additional comments (4)
x/avs/keeper/avs.go (2)
6-6: LGTM!
The addition of the slices package is appropriate for the new whitelist functionality.
229-251:
Fix error handling and enhance validation
In addition to the previously identified issues, consider:
- Adding example usage in the documentation
- Enhancing empty whitelist validation with size limits
Apply this diff to fix the issues:
// IsWhitelisted checks if an operator is in the AVS whitelist.
// If the whitelist is empty, any operator is considered whitelisted.
// Returns true if the operator is whitelisted, false otherwise.
// Returns an error if the AVS address is invalid, the operator address is invalid,
// or if the operator is not in the whitelist.
//
// Example:
//
// isWhitelisted, err := k.IsWhitelisted(ctx, "0x123...", "exo1...")
// if err != nil {
// return err
// }
func (k *Keeper) IsWhitelisted(ctx sdk.Context, avsAddr, operatorAddr string) (bool, error) {
+ if !common.IsHexAddress(avsAddr) {
+ return false, errorsmod.Wrap(types.ErrInvalidAddr, fmt.Sprintf("invalid AVS address format: %s", avsAddr))
+ }
avsInfo, err := k.GetAVSInfo(ctx, avsAddr)
if err != nil {
return false, errorsmod.Wrap(err, fmt.Sprintf("failed to get AVS info for address: %s", avsAddr))
}
_, err = sdk.AccAddressFromBech32(operatorAddr)
if err != nil {
return false, errorsmod.Wrap(err, fmt.Sprintf("invalid operator address format: %s", operatorAddr))
}
// Currently avs has no whitelist set and any operator can optin
+ // Ensure whitelist size doesn't exceed maximum allowed size
+ if len(avsInfo.Info.WhitelistAddress) > types.MaxWhitelistSize {
+ return false, errorsmod.Wrap(types.ErrInvalidWhitelist, "whitelist exceeds maximum size")
+ }
if len(avsInfo.Info.WhitelistAddress) == 0 {
return true, nil
}
if !slices.Contains(avsInfo.Info.WhitelistAddress, operatorAddr) {
- return false, errorsmod.Wrap(err, fmt.Sprintf("operator %s not in whitelist", operatorAddr))
+ return false, errorsmod.Wrap(types.ErrNotWhitelisted, fmt.Sprintf("operator %s not in whitelist", operatorAddr))
}
return true, nil
}Additionally, ensure these error types are defined in x/avs/types/errors.go:
// Error codes for AVS module
const (
ErrNotWhitelisted = "not_whitelisted"
ErrInvalidWhitelist = "invalid_whitelist"
MaxWhitelistSize = 1000 // Adjust this value based on your requirements
)precompiles/avs/avs_test.go (2)
96-99: Add comprehensive test cases for whitelist functionality.
The test suite needs additional test cases to verify the whitelist feature thoroughly:
- Empty whitelist behavior
- Invalid whitelist addresses
- Non-whitelisted operator registration attempts
- Whitelist updates during AVS updates
Also applies to: 357-360
115-115: Verify whitelist parameter consistency across test cases.
The whitelist parameter has been added to both register and update AVS methods, but the test cases don't verify:
- Whitelist validation during registration
- Whitelist modifications during updates
- Error cases for invalid whitelist entries
Also applies to: 376-376
✅ Verification successful
Whitelist parameter validation is properly implemented and tested
Based on the code analysis:
-
The whitelist parameter is properly validated in both registration and update methods:
- In
types.go, bothGetAVSParamsFromInputsandGetAVSParamsFromUpdateInputsvalidate the whitelist addresses by:- Checking for nil/empty arrays
- Converting and validating each address using
sdk.AccAddressFromBech32 - Returning appropriate error messages for invalid inputs
- In
-
Test cases in
avs_test.goverify whitelist functionality:- Tests include valid whitelist addresses using
exoWhiteAddresses - Addresses are properly generated and formatted using
sdk.AccAddress - Whitelist parameter is consistently used in both registration and update test cases
- Tests include valid whitelist addresses using
The implementation shows proper validation and consistent usage across the codebase, with no issues found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify whitelist parameter usage consistency
# Check for whitelist parameter validation
rg -A 5 "whitelist.*validation|validate.*whitelist" precompiles/
# Check for whitelist-related error handling
rg "whitelist.*error|error.*whitelist" precompiles/
# Check for whitelist update handling
ast-grep --pattern 'func $_(_, _) {
$$$
whitelist$_
$$$
}'
Length of output: 187
Script:
#!/bin/bash
# Let's check for whitelist-related test cases and implementation
# Search for whitelist-related test cases
rg -B 2 -A 2 "whitelist" precompiles/avs/avs_test.go
# Check the AVS contract implementation for whitelist handling
rg -B 2 -A 2 "whitelist" precompiles/avs/avs.go
# Look for any whitelist-related structs or types
ast-grep --pattern 'type $_ struct {
$$$
whitelist$_
$$$
}'
Length of output: 168
Script:
#!/bin/bash
# Let's try a different approach to find whitelist-related code
# Search for any files containing 'whitelist' in the precompiles directory
fd . precompiles -t f -x grep -l "whitelist" {} \;
# Look for test functions that might be testing whitelist functionality
ast-grep --pattern 'func Test$_(t *testing.T) {
$$$
}' precompiles/avs/avs_test.go
# Check for any registration or update method implementations
ast-grep --pattern 'func (c *Contract) Register($$$) error {
$$$
}'
ast-grep --pattern 'func (c *Contract) Update($$$) error {
$$$
}'
Length of output: 364
Script:
#!/bin/bash
# Let's examine the content of the identified files
# Check the interface definition
cat precompiles/avs/IAVSManager.sol
# Check the types definition
cat precompiles/avs/types.go
# Look for whitelist-related test cases with more context
rg -B 5 -A 5 "exoWhiteAddresses" precompiles/avs/avs_test.go
Length of output: 23316
| if params.WhitelistAddress != nil { | ||
| avs.WhitelistAddress = params.WhitelistAddress | ||
| } |
There was a problem hiding this comment.
Please consider this comment from the AI.
| @@ -96,6 +99,7 @@ interface IAVSManager { | |||
| address slashAddr, | |||
| address rewardAddr, | |||
| string[] memory avsOwnerAddress, | |||
| string[] memory whitelistAddress, | |||
There was a problem hiding this comment.
Why choose string type for these addresses and not address which is Solidity native and has great support? Each exo1... bech32-encoded address can be represented as a hex address and vice-versa.
| @@ -49,6 +49,7 @@ interface IAVSManager { | |||
| /// @param slashAddr The slash address of AVS. | |||
There was a problem hiding this comment.
Have you verified that this file compiles? Too many local variables (within a function) can cause Solidity to complain "stack too deep"
Description
implement whitelist feature for AVS
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests