-
Notifications
You must be signed in to change notification settings - Fork 829
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add migration for new params #1840
Conversation
x/evm/module.go
Outdated
@@ -208,6 +208,10 @@ func (am AppModule) RegisterServices(cfg module.Configurator) { | |||
_ = cfg.RegisterMigration(types.ModuleName, 10, func(ctx sdk.Context) error { | |||
return migrations.MigrateCastAddressBalances(ctx, am.keeper) | |||
}) | |||
|
|||
_ = cfg.RegisterMigration(types.ModuleName, 11, func(ctx sdk.Context) error { | |||
return migrations.AddNewParamsAndSetAllToDefaults(ctx, am.keeper) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets not use this migration handler because it will reset params which we don't want to do, instead we should initialize the param value for the new param being introduced. And lets add tests for that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's a valid concern, let's only change for the new param
defaultParams := types.DefaultParams() | ||
defaultDeliverTxHookWasmGasLimit := defaultParams.DeliverTxHookWasmGasLimit | ||
defaultParams.DeliverTxHookWasmGasLimit = defaultDeliverTxHookWasmGasLimit | ||
k.SetParams(ctx, defaultParams) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will still override the other existing params, instead of starting from default params, we need to load the existing params, and set these two values in that struct
|
||
keeperParams := k.GetParams(ctx) | ||
|
||
require.Equal(t, keeperParams.GetDeliverTxHookWasmGasLimit(), types.DefaultParams().DeliverTxHookWasmGasLimit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For good measure we can start by setting some other value for a non new param and verify that it doesn't get overwritten
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah i relized that too. working on it
426c5b3
to
2650cd1
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1840 +/- ##
==========================================
+ Coverage 60.51% 60.69% +0.18%
==========================================
Files 258 259 +1
Lines 22734 22748 +14
==========================================
+ Hits 13758 13808 +50
+ Misses 7995 7956 -39
- Partials 981 984 +3
|
Describe your changes and provide context
Testing performed to validate your change