Collin/v8 migration fixes #324
Conversation
…kResults and ScheduledCorks
WalkthroughThis pull request introduces significant updates to various modules within the application, focusing on governance, parameter management, and migration processes. Key changes include the integration of a new version of the governance module, enhancements to the upgrade handler for IBC migrations, and the introduction of new error handling and parameter validation mechanisms. Several files related to auction management have been modified or removed, streamlining the codebase. Additionally, new methods have been added to improve parameter retrieval and management across different modules. Changes
Possibly related PRs
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: 4
🧹 Outside diff range and nitpick comments (7)
x/auction/keeper/migrations.go (1)
21-25: Consider additional parameter migration safety measures.While the current implementation safely handles missing parameters, consider adding:
- Validation for existing parameter values
- Error handling for parameter setting operations
- Logging of the actual parameter values being set
Here's a suggested enhancement:
subspace := m.keeper.paramSpace if !subspace.Has(ctx, types.KeyAuctionBurnRate) { + ctx.Logger().Info("Setting default AuctionBurnRate parameter") subspace.Set(ctx, types.KeyAuctionBurnRate, types.DefaultParams().AuctionBurnRate) +} else { + // Validate existing parameter + var burnRate sdk.Dec + subspace.Get(ctx, types.KeyAuctionBurnRate, &burnRate) + if burnRate.IsNegative() || burnRate.GT(sdk.OneDec()) { + return fmt.Errorf("invalid existing AuctionBurnRate: %s", burnRate) + } + ctx.Logger().Info("Existing AuctionBurnRate parameter is valid", "value", burnRate) }x/cellarfees/types/v1/errors.go (1)
1-16: Consider adding error tests.To ensure the error definitions remain stable and well-documented, consider adding unit tests that verify:
- Error codes are unique
- Error messages match expectations
- Error wrapping and unwrapping behavior
Would you like me to help generate the test file with these test cases?
x/cellarfees/migrations/v1/store.go (1)
Line range hint
22-35: Consider implementing data backup before deletion.The
migrateCellarfeesFeeAccrualCountersfunction performs a destructive operation by deleting all FeeAccrualCounters without any backup mechanism. Consider:
- Adding a backup mechanism before deletion
- Implementing a rollback strategy
- Adding validation to ensure migration completion
Would you like assistance in implementing a safer migration strategy with proper backup and validation?
x/incentives/keeper/keeper.go (1)
58-62: Good architectural approach for migration safetyThe addition of
GetParamSetIfExistsalongside the existingGetParamSetprovides a clear separation between migration scenarios and regular operations. This is a good pattern for handling parameter migrations safely.Consider documenting this pattern in the module's migration guide to help other developers understand when to use each method.
x/cellarfees/keeper/keeper.go (1)
70-74: Document the method's behavior.Add documentation explaining:
- When to use this method vs
GetParams- The behavior when parameters don't exist
- The migration-specific use cases
+// GetParamSetIfExists retrieves parameters if they exist in the paramSpace. +// This method is primarily used during migrations where parameters might not be initialized. +// Unlike GetParams, this method doesn't panic when parameters are missing. func (k Keeper) GetParamSetIfExists(ctx sdk.Context) types.Params {x/axelarcork/keeper/genesis.go (1)
77-81: LGTM! Consider enhancing the comment for better clarity.Good addition of explicit initializations to prevent nil pointer issues. The comment explains the reason, but could be more specific.
Consider updating the comment to be more explicit:
- // These fields are nil by default, so we need to initialize them + // Initialize fields that would be nil by default to ensure a valid genesis exportapp/app.go (1)
1044-1044: Remove unnecessary variable reassignmentThe line
subspace := subspaceis redundant as there is no closure capturingsubspace. Removing this line will improve code clarity without affecting functionality.Apply this diff to remove the redundant line:
- subspace := subspace
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
x/auction/migrations/v1/types/auction.pb.gois excluded by!**/*.pb.go,!**/*.pb.gox/auction/migrations/v1/types/genesis.pb.gois excluded by!**/*.pb.go,!**/*.pb.go
📒 Files selected for processing (17)
app/app.go(5 hunks)app/upgrades/v8/upgrades.go(1 hunks)integration_tests/auction_test.go(2 hunks)x/auction/keeper/migrations.go(2 hunks)x/auction/migrations/v1/store.go(0 hunks)x/auction/migrations/v1/types/errors.go(0 hunks)x/auction/migrations/v1/types/keys.go(0 hunks)x/auction/migrations/v1/types/params.go(0 hunks)x/axelarcork/keeper/genesis.go(1 hunks)x/cellarfees/keeper/keeper.go(1 hunks)x/cellarfees/keeper/migrations.go(1 hunks)x/cellarfees/migrations/v1/store.go(1 hunks)x/cellarfees/module.go(4 hunks)x/cellarfees/types/v1/errors.go(1 hunks)x/cellarfees/types/v1/params.go(1 hunks)x/incentives/keeper/keeper.go(1 hunks)x/incentives/keeper/migrations.go(1 hunks)
💤 Files with no reviewable changes (4)
- x/auction/migrations/v1/store.go
- x/auction/migrations/v1/types/errors.go
- x/auction/migrations/v1/types/keys.go
- x/auction/migrations/v1/types/params.go
🔇 Additional comments (25)
x/auction/keeper/migrations.go (2)
5-5: LGTM: Import changes align with v8 migration.
The replacement of the v1 import with the types package from v8 is consistent with the migration objectives.
20-20: LGTM: Clear migration logging.
The addition of logging statements provides clear visibility into the migration progress, which is essential for monitoring and debugging.
Also applies to: 27-27
x/cellarfees/types/v1/errors.go (2)
1-5: LGTM! Clean package structure and imports.
The package declaration and imports are well-organized, using the recommended alias errorsmod for the errors package.
7-16: Verify error code sequence and consider documentation improvements.
The error definitions look good, but there are a few points to consider:
- Error code sequence starts at 2, we should verify if code 1 is intentionally skipped
- Consider adding godoc comments for better documentation
Let's verify the error code sequence in the module:
Consider adding godoc comments for better documentation:
// x/cellarfees module sentinel errors
var (
+ // ErrInvalidFeeAccrualAuctionThreshold is returned when the fee accrual auction threshold is invalid
ErrInvalidFeeAccrualAuctionThreshold = errorsmod.Register("cellarfees", 2, "invalid fee accrual auction threshold")
+ // ErrInvalidRewardEmissionPeriod is returned when the reward emission period is invalid
ErrInvalidRewardEmissionPeriod = errorsmod.Register("cellarfees", 3, "invalid reward emission period")
// ... (similar comments for other errors)
)✅ Verification successful
Based on the comprehensive search results, we can now provide a final response since we have sufficient information about the error codes in the module:
Error code sequence starts at 2 intentionally - no action needed
The error code sequence starting at 2 appears to be intentional, as there is no evidence of error code 1 being used in the current codebase or in the git history. This is a common practice in some codebases to reserve error code 1 for general or system-level errors.
The suggestion for adding godoc comments remains valid for better documentation:
// x/cellarfees module sentinel errors
var (
// ErrInvalidFeeAccrualAuctionThreshold is returned when the fee accrual auction threshold is invalid
ErrInvalidFeeAccrualAuctionThreshold = errorsmod.Register("cellarfees", 2, "invalid fee accrual auction threshold")
// ... (similar comments for other errors)
)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other error definitions in the cellarfees module
# to ensure there are no conflicts or gaps in error codes
# Search for other error registrations in the cellarfees module
rg --type go "errorsmod.Register.*cellarfees.*\d+"
# Search for any documentation about error code 1
rg --type go "error.*code.*1.*cellarfees"
Length of output: 1118
Script:
#!/bin/bash
# Let's check the entire cellarfees module for any other error definitions or historical changes
# that might explain the missing error code 1
# Search for any error-related code in the cellarfees module
rg --type go "error" x/cellarfees/
# Check git history for any removed error definitions
git log -p x/cellarfees/types/v1/errors.go
# Look for any documentation files in the cellarfees module
fd -e md -e txt x/cellarfees/
Length of output: 76250
Script:
#!/bin/bash
# Let's check for any error code 1 in the entire codebase and documentation
# Also check for any historical changes that might explain the gap
# Search for error code 1 in the entire codebase
rg "Register.*cellarfees.*1.*"
# Check for any documentation about error codes
fd -e md -e txt --full-path "x/cellarfees"
# Look at the git history of the module to see if error code 1 was previously defined
git log --full-history -p -- "x/cellarfees/types" | rg "Register.*cellarfees.*1.*"
Length of output: 10750
x/cellarfees/migrations/v1/store.go (3)
8-8: LGTM: Import statement is correctly updated.
The import statement correctly uses the standard paramtypes package from cosmos-sdk.
13-19: Verify parameter migration is handled elsewhere.
The parameter migration logic has been removed from this function. Please ensure that parameter migration is properly handled in another location to prevent data loss during the v8 upgrade.
Let's verify the parameter migration implementation:
#!/bin/bash
# Description: Look for parameter migration implementation
# Expect: Parameter migration should be implemented elsewhere
# Search for parameter migration patterns
rg -A 5 "param.*migrat" 12-12:
Consider maintaining error handling for migration robustness.
The removal of the error return type means migration failures won't be propagated up the call stack. This could make it harder to detect and handle migration failures properly.
Consider restoring error handling:
-func MigrateStore(ctx sdk.Context, storeKey storetypes.StoreKey, cdc codec.BinaryCodec, legacySubspace paramtypes.Subspace) {
+func MigrateStore(ctx sdk.Context, storeKey storetypes.StoreKey, cdc codec.BinaryCodec, legacySubspace paramtypes.Subspace) error {Let's check if other migration functions in the codebase maintain error handling:
x/incentives/keeper/migrations.go (2)
20-24: LGTM! Good logging practice.
The addition of logging statements helps track migration progress. The approach of getting the subspace once and reusing it is efficient.
20-42: Verify parameter dependencies and constraints.
The migration sets validator incentive parameters that might have dependencies or constraints. We should verify the relationships between these parameters in the codebase.
✅ Verification successful
Based on the search results, I can now provide a final response as the code shows clear parameter validations and relationships:
Parameter validations and constraints are properly implemented
The codebase has appropriate validation checks for the validator incentive parameters:
ValidatorIncentivesMaxFractionis validated to be non-negative and not greater than 1.0ValidatorIncentivesSetSizeLimitandValidatorIncentivesCutoffHeighthave type validation- Parameters are used together coherently in
keeper/abci.go:- Distribution is skipped if current height >= cutoff height
- Set size limit controls the number of qualifying validators
- Max fraction caps individual validator allocations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for parameter usage and validation logic
echo "Searching for parameter validation and dependency checks..."
rg -A 5 "ValidatorIncentives(CutoffHeight|MaxDistributionPerBlock|MaxFraction|SetSizeLimit)" --type go
# Search for any existing parameter validation
ast-grep --pattern 'func validateParameter($_, $_) {
$$$
}'
Length of output: 16639
x/incentives/keeper/keeper.go (2)
58-62: Implementation looks good!
The new method follows the existing pattern and provides a safer way to retrieve parameters during migration scenarios.
58-62: Verify paramSpace.GetParamSetIfExists usage
Let's confirm this method exists in the imported paramtypes package and verify its usage pattern.
x/cellarfees/keeper/keeper.go (2)
70-74: 🛠️ Refactor suggestion
Consider adding error handling for parameter retrieval.
The new GetParamSetIfExists method silently handles the case where parameters don't exist, which differs from GetParams. This could mask migration issues or parameter initialization problems.
Consider this implementation to make parameter existence explicit:
-func (k Keeper) GetParamSetIfExists(ctx sdk.Context) types.Params {
+func (k Keeper) GetParamSetIfExists(ctx sdk.Context) (types.Params, bool) {
var p types.Params
- k.paramSpace.GetParamSetIfExists(ctx, &p)
- return p
+ exists := k.paramSpace.GetParamSetIfExists(ctx, &p)
+ return p, exists
}Let's verify the usage of this method in migrations:
#!/bin/bash
# Search for GetParamSetIfExists usage to ensure proper error handling
rg -A 5 "GetParamSetIfExists" 70-74: Verify parameter validation during migrations.
Since this method is introduced as part of v8 migration fixes, ensure that parameter validation is properly handled during the migration process.
✅ Verification successful
Based on the search results, I can see that GetParamSetIfExists is used in the Migrate1to2 function in x/cellarfees/keeper/migrations.go, and there is proper parameter validation being performed. Let me generate the final response:
Parameter validation is properly implemented in the migration process
The migration handler in x/cellarfees/keeper/migrations.go correctly:
- Retrieves parameters using
GetParamSetIfExists - Creates new v2 params with default value for new field
AuctionThresholdUsdValue - Validates the parameters using
ValidateBasic()before proceeding with the migration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for migration handlers using this method
ast-grep --pattern 'func $_(ctx sdk.Context) error {
$$$
GetParamSetIfExists($$$)
$$$
}'
# Look for parameter validation in migrations
rg -A 10 "Migrate.*to.*"
Length of output: 8635
x/cellarfees/module.go (4)
15-15: LGTM: Import addition follows conventions
The new import for params types is properly placed and follows standard Cosmos SDK import conventions.
Line range hint 93-105: LGTM: Constructor properly updated for new parameter
The constructor changes correctly handle the new legacySubspace parameter while maintaining the existing initialization pattern.
#!/bin/bash
# Verify the updated constructor usage across the codebase
echo "Checking NewAppModule usage..."
rg "NewAppModule\(" app/131-134: Verify version consistency across the module
The migration setup looks good, but let's ensure version numbers are consistent:
- Migration: v1 -> v2
- Current ConsensusVersion: 2
#!/bin/bash
# Verify version consistency across the module
echo "Checking version references..."
rg -g "*.go" "version|Version" x/cellarfees/87-89: LGTM: Well-documented legacy subspace addition
The legacySubspace field is properly documented and follows the module's structural patterns. Since this is for v1 -> v2 migration, we should verify the migration handler implementation.
✅ Verification successful
Migration handler implementation verified and properly uses legacySubspace
The migration handler Migrate1to2 is correctly implemented and properly utilizes the legacySubspace field. The implementation:
- Migrates parameters from v1 to v2 format
- Sets default value for new parameter
AuctionThresholdUsdValue - Validates the migrated parameters
- Uses
legacySubspacein the store migration viav1.MigrateStore
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Verify the migration handler implementation
echo "Checking for Migrate1to2 implementation..."
rg -A 10 "Migrate1to2" x/cellarfees/keeper/migrations.go
Length of output: 2243
Script:
#!/bin/bash
# Let's check the content of the migrations file
echo "Checking migrations.go content..."
cat x/cellarfees/keeper/migrations.go
# Also check for any migration-related functions in the keeper
echo -e "\nChecking for other migration-related functions..."
rg -g 'keeper/*.go' -A 5 'func.*Migrate'
Length of output: 1744
integration_tests/auction_test.go (2)
318-318: LGTM! Good addition of burn amount validation.
This check ensures that the auction mechanism is working correctly by verifying that tokens are actually being burned (50% of total SOMM paid).
330-333: LGTM! Helpful debug information added.
Adding the auction module account address to the test logs will help with debugging auction-related issues.
x/cellarfees/keeper/migrations.go (3)
23-30: Confirm that AuctionThresholdUsdValue default value is valid and appropriate
The AuctionThresholdUsdValue is set using sdk.MustNewDecFromStr(v2.DefaultAuctionThresholdUsdValue). Please verify that v2.DefaultAuctionThresholdUsdValue contains a valid decimal string and that this default value aligns with the intended business logic for auction thresholds.
38-38: Ensure the v1.MigrateStore function call parameters are correct
The call to v1.MigrateStore includes ctx, m.keeper.storeKey, m.keeper.cdc, and m.legacySubspace. Verify that v1.MigrateStore accepts these parameters in this order and that they match the expected types in its function signature.
12-18: Verify that the addition of legacySubspace is reflected throughout the codebase
The addition of legacySubspace to the Migrator struct and the NewMigrator function changes their interfaces. Please ensure that all instantiations and usages of Migrator and NewMigrator have been updated to include the new legacySubspace parameter to prevent any compilation errors or unintended behavior.
Run the following script to find and review all usages of NewMigrator:
app/app.go (3)
69-69: Importing govtypesv1 is appropriate
The addition of govtypesv1 import is necessary for the updated governance parameter handling with govtypesv1.ParamKeyTable().
613-613: Update cellarfees.NewAppModule call to match updated signature
The inclusion of app.GetSubspace(cellarfeestypes.ModuleName) aligns with the new function signature for cellarfees.NewAppModule, ensuring correct module initialization.
754-754: Update cellarfees.NewAppModule call in simulation manager
Adding app.GetSubspace(cellarfeestypes.ModuleName) to cellarfees.NewAppModule in the simulation manager ensures consistency and correct initialization during simulations.
| if !subspace.Has(ctx, types.KeyValidatorIncentivesCutoffHeight) { | ||
| subspace.Set(ctx, types.KeyValidatorIncentivesCutoffHeight, types.DefaultParams().ValidatorIncentivesCutoffHeight) | ||
| } | ||
|
|
||
| if !subspace.Has(ctx, types.KeyValidatorMaxDistributionPerBlock) { | ||
| subspace.Set(ctx, types.KeyValidatorMaxDistributionPerBlock, types.DefaultParams().ValidatorMaxDistributionPerBlock) | ||
| } | ||
|
|
||
| if !subspace.Has(ctx, types.KeyValidatorIncentivesMaxFraction) { | ||
| subspace.Set(ctx, types.KeyValidatorIncentivesMaxFraction, types.DefaultParams().ValidatorIncentivesMaxFraction) | ||
| } | ||
|
|
||
| if !subspace.Has(ctx, types.KeyValidatorIncentivesSetSizeLimit) { | ||
| subspace.Set(ctx, types.KeyValidatorIncentivesSetSizeLimit, types.DefaultParams().ValidatorIncentivesSetSizeLimit) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider enhancing parameter migration safety and maintainability.
While the current implementation works, there are several areas for improvement:
- The code could benefit from transaction handling to ensure atomicity
- Consider validating existing values even if they exist
- The repetitive parameter initialization pattern could be refactored
Consider this more robust approach:
func (m Migrator) Migrate1to2(ctx sdk.Context) error {
ctx.Logger().Info("incentives v1 to v2: New params")
- // New params
subspace := m.keeper.paramSpace
+
+ // Define parameter mappings
+ defaultParams := types.DefaultParams()
+ paramMappings := map[[]byte]interface{}{
+ types.KeyValidatorIncentivesCutoffHeight: defaultParams.ValidatorIncentivesCutoffHeight,
+ types.KeyValidatorMaxDistributionPerBlock: defaultParams.ValidatorMaxDistributionPerBlock,
+ types.KeyValidatorIncentivesMaxFraction: defaultParams.ValidatorIncentivesMaxFraction,
+ types.KeyValidatorIncentivesSetSizeLimit: defaultParams.ValidatorIncentivesSetSizeLimit,
+ }
- if !subspace.Has(ctx, types.KeyValidatorIncentivesCutoffHeight) {
- subspace.Set(ctx, types.KeyValidatorIncentivesCutoffHeight, types.DefaultParams().ValidatorIncentivesCutoffHeight)
- }
- // ... similar blocks for other parameters
+ // Begin transaction
+ ctx = ctx.WithMultiStore(ctx.MultiStore().CacheWrap())
- ctx.Logger().Info("incentives v1 to v2: Params migration complete")
+ for key, defaultValue := range paramMappings {
+ if !subspace.Has(ctx, key) {
+ subspace.Set(ctx, key, defaultValue)
+ } else {
+ // Optionally validate existing values
+ currentValue := subspace.Get(ctx, key)
+ if err := validateParameter(key, currentValue); err != nil {
+ return err
+ }
+ }
+ }
+
+ // Commit changes if all operations succeed
+ ctx.MultiStore().(sdk.CacheMultiStore).Write()
+ ctx.Logger().Info("incentives v1 to v2: Params migration complete")
return nil
}Committable suggestion skipped: line range outside the PR's diff.
| func MigrateAuctionAccountPermissions(ctx sdk.Context, accountKeeper *authkeeper.AccountKeeper) error { | ||
| ctx.Logger().Info("Migrating auction account permissions") | ||
| oldAcctI := accountKeeper.GetModuleAccount(ctx, auctiontypes.ModuleName) | ||
|
|
||
| if oldAcctI == nil { | ||
| return fmt.Errorf("module account not found") | ||
| } | ||
|
|
||
| newAcct := authtypes.NewEmptyModuleAccount(auctiontypes.ModuleName, authtypes.Burner) | ||
| newAcct.AccountNumber = oldAcctI.GetAccountNumber() | ||
| newAcct.Address = oldAcctI.GetAddress().String() | ||
| newAcct.Sequence = oldAcctI.GetSequence() | ||
| newAcct.Name = oldAcctI.GetName() | ||
| newAcctI := (accountKeeper.NewAccount(ctx, newAcct)).(authtypes.ModuleAccountI) | ||
|
|
||
| accountKeeper.SetModuleAccount(ctx, newAcctI) | ||
|
|
||
| ctx.Logger().Info("Auction account permissions migrated") | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify auction account permissions migration by updating existing account
Instead of creating a new module account and manually copying fields from the old account, you can simplify the migration by directly modifying the permissions of the existing account. This approach reduces complexity and minimizes the risk of introducing errors.
Here's a suggested change:
func MigrateAuctionAccountPermissions(ctx sdk.Context, accountKeeper *authkeeper.AccountKeeper) error {
ctx.Logger().Info("Migrating auction account permissions")
oldAcctI := accountKeeper.GetModuleAccount(ctx, auctiontypes.ModuleName)
if oldAcctI == nil {
return fmt.Errorf("module account not found")
}
+ // Add the Burner permission to the existing account
+ oldPermissions := oldAcctI.GetPermissions()
+ newPermissions := append(oldPermissions, authtypes.Burner)
+ oldAcctI.SetPermissions(newPermissions)
+
+ // Update the module account with the new permissions
+ accountKeeper.SetModuleAccount(ctx, oldAcctI)
- newAcct := authtypes.NewEmptyModuleAccount(auctiontypes.ModuleName, authtypes.Burner)
- newAcct.AccountNumber = oldAcctI.GetAccountNumber()
- newAcct.Address = oldAcctI.GetAddress().String()
- newAcct.Sequence = oldAcctI.GetSequence()
- newAcct.Name = oldAcctI.GetName()
- newAcctI := (accountKeeper.NewAccount(ctx, newAcct)).(authtypes.ModuleAccountI)
-
- accountKeeper.SetModuleAccount(ctx, newAcctI)
ctx.Logger().Info("Auction account permissions migrated")
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.
| func MigrateAuctionAccountPermissions(ctx sdk.Context, accountKeeper *authkeeper.AccountKeeper) error { | |
| ctx.Logger().Info("Migrating auction account permissions") | |
| oldAcctI := accountKeeper.GetModuleAccount(ctx, auctiontypes.ModuleName) | |
| if oldAcctI == nil { | |
| return fmt.Errorf("module account not found") | |
| } | |
| newAcct := authtypes.NewEmptyModuleAccount(auctiontypes.ModuleName, authtypes.Burner) | |
| newAcct.AccountNumber = oldAcctI.GetAccountNumber() | |
| newAcct.Address = oldAcctI.GetAddress().String() | |
| newAcct.Sequence = oldAcctI.GetSequence() | |
| newAcct.Name = oldAcctI.GetName() | |
| newAcctI := (accountKeeper.NewAccount(ctx, newAcct)).(authtypes.ModuleAccountI) | |
| accountKeeper.SetModuleAccount(ctx, newAcctI) | |
| ctx.Logger().Info("Auction account permissions migrated") | |
| return nil | |
| } | |
| func MigrateAuctionAccountPermissions(ctx sdk.Context, accountKeeper *authkeeper.AccountKeeper) error { | |
| ctx.Logger().Info("Migrating auction account permissions") | |
| oldAcctI := accountKeeper.GetModuleAccount(ctx, auctiontypes.ModuleName) | |
| if oldAcctI == nil { | |
| return fmt.Errorf("module account not found") | |
| } | |
| // Add the Burner permission to the existing account | |
| oldPermissions := oldAcctI.GetPermissions() | |
| newPermissions := append(oldPermissions, authtypes.Burner) | |
| oldAcctI.SetPermissions(newPermissions) | |
| // Update the module account with the new permissions | |
| accountKeeper.SetModuleAccount(ctx, oldAcctI) | |
| ctx.Logger().Info("Auction account permissions migrated") | |
| return nil | |
| } |
| // ValidateBasic performs basic validation on cellarfees parameters. | ||
| func (p *Params) ValidateBasic() error { | ||
| if err := validateFeeAccrualAuctionThreshold(p.FeeAccrualAuctionThreshold); err != nil { | ||
| return err | ||
| } | ||
| if err := validateRewardEmissionPeriod(p.RewardEmissionPeriod); err != nil { | ||
| return err | ||
| } | ||
| if err := validateInitialPriceDecreaseRate(p.InitialPriceDecreaseRate); err != nil { | ||
| return err | ||
| } | ||
| if err := validatePriceDecreaseBlockInterval(p.PriceDecreaseBlockInterval); err != nil { | ||
| return err | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Missing validation for AuctionInterval in ValidateBasic
In the ValidateBasic function, you are validating all parameters except for AuctionInterval. To ensure all parameters are properly validated, please include a call to validateAuctionInterval(p.AuctionInterval).
Apply this diff to include the missing validation:
func (p *Params) ValidateBasic() error {
if err := validateFeeAccrualAuctionThreshold(p.FeeAccrualAuctionThreshold); err != nil {
return err
}
if err := validateRewardEmissionPeriod(p.RewardEmissionPeriod); err != nil {
return err
}
if err := validateInitialPriceDecreaseRate(p.InitialPriceDecreaseRate); err != nil {
return err
}
if err := validatePriceDecreaseBlockInterval(p.PriceDecreaseBlockInterval); err != nil {
return err
}
+ if err := validateAuctionInterval(p.AuctionInterval); err != nil {
+ return err
+ }
return nil
}Committable suggestion skipped: line range outside the PR's diff.
| func validateInitialPriceDecreaseRate(i interface{}) error { | ||
| rate, ok := i.(sdk.Dec) | ||
| if !ok { | ||
| return errorsmod.Wrapf(ErrInvalidInitialPriceDecreaseRate, "initial price decrease rate: %T", i) | ||
| } | ||
|
|
||
| if rate == sdk.ZeroDec() { | ||
| return errorsmod.Wrapf(ErrInvalidInitialPriceDecreaseRate, "initial price decrease rate cannot be zero, must be 0 < x < 1") | ||
| } | ||
|
|
||
| if rate == sdk.OneDec() { | ||
| return errorsmod.Wrapf(ErrInvalidInitialPriceDecreaseRate, "initial price decrease rate cannot be one, must be 0 < x < 1") | ||
| } | ||
|
|
||
| return nil | ||
| } |
There was a problem hiding this comment.
Ensure InitialPriceDecreaseRate is strictly between 0 and 1
The validateInitialPriceDecreaseRate function currently checks if the rate is exactly zero or one. However, it should also verify that the rate is greater than zero and less than one (0 < rate < 1). Values less than zero or greater than or equal to one would pass validation, which may not be intended.
Apply this diff to strengthen the validation logic:
func validateInitialPriceDecreaseRate(i interface{}) error {
rate, ok := i.(sdk.Dec)
if !ok {
return errorsmod.Wrapf(ErrInvalidInitialPriceDecreaseRate, "initial price decrease rate: %T", i)
}
- if rate == sdk.ZeroDec() {
+ if rate.LTE(sdk.ZeroDec()) {
return errorsmod.Wrapf(ErrInvalidInitialPriceDecreaseRate, "initial price decrease rate must be greater than zero, got %s", rate)
}
- if rate == sdk.OneDec() {
+ if rate.GTE(sdk.OneDec()) {
return errorsmod.Wrapf(ErrInvalidInitialPriceDecreaseRate, "initial price decrease rate must be less than one, got %s", rate)
}
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.
| func validateInitialPriceDecreaseRate(i interface{}) error { | |
| rate, ok := i.(sdk.Dec) | |
| if !ok { | |
| return errorsmod.Wrapf(ErrInvalidInitialPriceDecreaseRate, "initial price decrease rate: %T", i) | |
| } | |
| if rate == sdk.ZeroDec() { | |
| return errorsmod.Wrapf(ErrInvalidInitialPriceDecreaseRate, "initial price decrease rate cannot be zero, must be 0 < x < 1") | |
| } | |
| if rate == sdk.OneDec() { | |
| return errorsmod.Wrapf(ErrInvalidInitialPriceDecreaseRate, "initial price decrease rate cannot be one, must be 0 < x < 1") | |
| } | |
| return nil | |
| } | |
| func validateInitialPriceDecreaseRate(i interface{}) error { | |
| rate, ok := i.(sdk.Dec) | |
| if !ok { | |
| return errorsmod.Wrapf(ErrInvalidInitialPriceDecreaseRate, "initial price decrease rate: %T", i) | |
| } | |
| if rate.LTE(sdk.ZeroDec()) { | |
| return errorsmod.Wrapf(ErrInvalidInitialPriceDecreaseRate, "initial price decrease rate must be greater than zero, got %s", rate) | |
| } | |
| if rate.GTE(sdk.OneDec()) { | |
| return errorsmod.Wrapf(ErrInvalidInitialPriceDecreaseRate, "initial price decrease rate must be less than one, got %s", rate) | |
| } | |
| return nil | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
x/cellarfees/types/v2/params.go (1)
105-110: Fix typo in error message.The validation logic is correct, but there's a minor typo in the error message (missing space after comma).
Apply this diff to fix the error message:
- return errorsmod.Wrapf(types.ErrInvalidInitialPriceDecreaseRate, "initial price decrease rate cannot be zero or negative,must be 0 < x < 1") + return errorsmod.Wrapf(types.ErrInvalidInitialPriceDecreaseRate, "initial price decrease rate cannot be zero or negative, must be 0 < x < 1")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
x/cellarfees/types/v2/params.go(2 hunks)
🔇 Additional comments (2)
x/cellarfees/types/v2/params.go (2)
76-82: LGTM: Comprehensive parameter validation added.
The additional validation checks for AuctionInterval and AuctionThresholdUsdValue enhance the robustness of the parameter validation system. The implementation follows the existing pattern and maintains consistency in error handling.
Line range hint 1-150: Consider migration impact on existing configurations.
Since this is part of a v8 migration and involves changes to parameter validation, ensure that:
- Existing parameter values are properly migrated and validated during the upgrade
- Any configurations that might become invalid under the new validation rules are identified and handled appropriately
- The migration process includes a plan for communicating these changes to node operators
Let's check for any existing parameter values in the codebase:
✅ Verification successful
Let me check the migration code to see how these parameters are being handled during the v8 upgrade:
Let me check the parameter validation changes between v1 and v2:
Let me check the complete validation functions to see if there are any differences in the validation rules:
Parameter validation changes are safe and backward compatible
The validation changes in v2 are actually more comprehensive and maintain backward compatibility with v1:
- V1 only checked for exact equality with zero and one
- V2 properly handles all cases with
LTE(less than or equal) andGTE(greater than or equal) - The default value of
0.0000648remains valid under both versions - No existing valid configurations would become invalid under the new rules
The migration code correctly preserves the parameter values during the upgrade, and the validation changes strengthen the checks without breaking existing configurations.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for parameter values that might be affected by the new validation
rg -A 2 "InitialPriceDecreaseRate.*=.*" --type go
Length of output: 8097
Script:
#!/bin/bash
# Check for migration code
rg -A 10 "func Migrate" x/cellarfees/migrations/
Length of output: 840
Script:
#!/bin/bash
# Compare parameter validation between versions
echo "=== V1 Parameter Validation ==="
rg -A 5 "validateInitialPriceDecreaseRate" x/cellarfees/types/v1/
echo -e "\n=== V2 Parameter Validation ==="
rg -A 5 "validateInitialPriceDecreaseRate" x/cellarfees/types/v2/
Length of output: 3090
Script:
#!/bin/bash
# Get complete validation functions from both versions
echo "=== V1 Complete Validation Function ==="
rg -A 15 "^func validateInitialPriceDecreaseRate" x/cellarfees/types/v1/
echo -e "\n=== V2 Complete Validation Function ==="
rg -A 15 "^func validateInitialPriceDecreaseRate" x/cellarfees/types/v2/
Length of output: 2455
Closes: #XXX
Description
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Refactor
Documentation