Check for invalid addresses in SetAffiliateWhitelist
#2594
Conversation
WalkthroughThe changes in this pull request focus on enhancing the error handling and validation logic within the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
protocol/x/affiliates/keeper/keeper.go (2)
Line range hint
313-324: Consider extracting address validation into a helper function.The same Bech32 address validation logic appears in both
SetAffiliateWhitelistandRegisterAffiliate. Consider extracting this into a helper function to promote code reuse and maintainability.Example implementation:
+// validateBech32Address validates if the given string is a valid Bech32 address +func (k Keeper) validateBech32Address(address string, context string) error { + if _, err := sdk.AccAddressFromBech32(address); err != nil { + return errorsmod.Wrapf(types.ErrInvalidAddress, "%s: %s", context, address) + } + return nil +} func (k Keeper) SetAffiliateWhitelist(ctx sdk.Context, whitelist types.AffiliateWhitelist) error { store := ctx.KVStore(k.storeKey) addressSet := make(map[string]bool) for _, tier := range whitelist.Tiers { if tier.TakerFeeSharePpm > types.AffiliatesRevSharePpmCap { return errorsmod.Wrapf(types.ErrRevShareSafetyViolation, "taker fee share ppm %d is greater than the cap %d", tier.TakerFeeSharePpm, types.AffiliatesRevSharePpmCap) } for _, address := range tier.Addresses { - if _, err := sdk.AccAddressFromBech32(address); err != nil { - return errorsmod.Wrapf(types.ErrInvalidAddress, "address to whitelist: %s", address) - } + if err := k.validateBech32Address(address, "address to whitelist"); err != nil { + return err + }
Line range hint
313-324: Add docstring to explain validation rules.Consider adding a docstring to document the method's purpose and validation rules.
Example:
+// SetAffiliateWhitelist updates the affiliate whitelist with the provided configuration. +// It performs the following validations: +// - Ensures all addresses are valid Bech32 addresses +// - Verifies taker fee share PPM does not exceed the cap +// - Checks for duplicate addresses across all tiers func (k Keeper) SetAffiliateWhitelist(ctx sdk.Context, whitelist types.AffiliateWhitelist) error {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
protocol/x/affiliates/keeper/keeper.go(1 hunks)protocol/x/affiliates/keeper/keeper_test.go(1 hunks)
🔇 Additional comments (3)
protocol/x/affiliates/keeper/keeper.go (2)
314-317: LGTM! Clean implementation of address validation.
The implementation correctly validates Bech32 addresses using the standard Cosmos SDK method and follows the codebase's error handling patterns.
314-317: Verify error type definition and test coverage.
Let's verify the error type definition and ensure test coverage exists for invalid addresses.
✅ Verification successful
Error type and test coverage verified successfully
The error type ErrInvalidAddress is properly defined in types/errors.go and comprehensive test coverage exists for invalid address scenarios:
- Test cases in
keeper_test.goverify invalid addresses for both referee and affiliate - Test cases in
msg_server_test.gocover invalid address validation in message handling - Error is properly registered with a unique code (5) and clear error message
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify error type definition and test coverage
# Check if ErrInvalidAddress is properly defined
rg "ErrInvalidAddress.*=.*errors\.Register" -A 2
# Check test coverage for invalid address cases
rg -A 5 "TestSetAffiliateWhitelist.*invalid.*address" --type go
Length of output: 116
Script:
#!/bin/bash
# Let's try different approaches to locate error definitions and tests
# Find error definitions in types package
rg "ErrInvalidAddress" protocol/x/affiliates/types/
# Look for test files related to affiliate whitelist
fd "whitelist.*test\.go" protocol/x/affiliates/
# Search for any test cases containing "invalid address"
rg "invalid.*address" --type go protocol/x/affiliates/
Length of output: 998
Script:
#!/bin/bash
# Let's examine the test implementations to verify coverage
# Check the test implementation in keeper_test.go
rg -B 5 -A 10 "invalid_address" protocol/x/affiliates/keeper/keeper_test.go
# Check the test implementation in msg_server_test.go
rg -B 5 -A 10 "invalid_address" protocol/x/affiliates/keeper/msg_server_test.go
# Look at the error definition and its usage
cat protocol/x/affiliates/types/errors.go
Length of output: 2792
protocol/x/affiliates/keeper/keeper_test.go (1)
487-516: LGTM! Good test coverage for address validation.
The new test cases effectively verify the address validation logic in SetAffiliateWhitelist:
- Rejection of invalid bech32 addresses
- Rejection of validator operator addresses
This ensures robust input validation for the affiliate whitelist.
Changelist
Check input contains only valid
bech32address inSetAffiliateWhitelistTest Plan
Unit test
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
New Features
Bug Fixes
Tests