-
Notifications
You must be signed in to change notification settings - Fork 580
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
Adding docs for gov v1 migration #4628
Changes from 7 commits
cd37676
f897bfc
a8d1d71
a0c8b8d
4cfca8b
1f3de91
43d9ae6
3fde33e
89bdefb
1408613
62d9e58
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -88,6 +88,39 @@ Each module has a corresponding `MsgUpdateParams` message with a `Params` which | |||||
|
||||||
Legacy params subspaces must still be initialised in app.go in order to successfully migrate from x/params to the new self-contained approach. See reference: https://github.com/cosmos/ibc-go/blob/main/testing/simapp/app.go#L1001-L1006 | ||||||
|
||||||
### Governance V1 migration | ||||||
|
||||||
Proposals have been migrated to [gov v1 messages](https://docs.cosmos.network/v0.50/modules/gov#messages) ref: [#4620](https://github.com/cosmos/ibc-go/pull/4620). | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
Ensure that the correct authority field is provided to the ibc keeper. The default authority will be `gov` if not specified. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I might be wrong, but I think we don't default to the gov module address in our keepers? We just accept whatever is passed as argument, which now makes me think if we should validate the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes good call, we don't actually default to the gov, address. (maybe we should 🤔 ) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, also valid solution. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We do explicit checking of However you are correct that we don't default to gov! I can remove that section. |
||||||
|
||||||
Remove legacy proposal registration from app.go ref: [#4602](https://github.com/cosmos/ibc-go/pull/4602). | ||||||
|
||||||
Remove the ibcclient ProposalHandler from the govRouter. | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
```diff | ||||||
govRouter := govv1beta1.NewRouter() | ||||||
govRouter.AddRoute(govtypes.RouterKey, govv1beta1.ProposalHandler). | ||||||
AddRoute(paramproposal.RouterKey, params.NewParamChangeProposalHandler(app.ParamsKeeper)). | ||||||
- AddRoute(ibcclienttypes.RouterKey, ibcclient.NewClientProposalHandler(app.IBCKeeper.ClientKeeper)) | ||||||
``` | ||||||
|
||||||
Remove the UpgradeProposalHandler from the BasicModuleManager | ||||||
|
||||||
```diff | ||||||
app.BasicModuleManager = module.NewBasicManagerFromManager( | ||||||
app.ModuleManager, | ||||||
map[string]module.AppModuleBasic{ | ||||||
genutiltypes.ModuleName: genutil.NewAppModuleBasic(genutiltypes.DefaultMessageValidator), | ||||||
govtypes.ModuleName: gov.NewAppModuleBasic( | ||||||
[]govclient.ProposalHandler{ | ||||||
paramsclient.ProposalHandler, | ||||||
- ibcclientclient.UpgradeProposalHandler, | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also the line with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe there was one handler that handled both cases |
||||||
}, | ||||||
), | ||||||
}) | ||||||
``` | ||||||
|
||||||
### Transfer migration | ||||||
|
||||||
An automatic migration handler (TODO: add link after https://github.com/cosmos/ibc-go/pull/3104 is merged) is configured in the transfer module to set the [denomination metadata](https://github.com/cosmos/cosmos-sdk/blob/95178ce036741ae6aa7af131fa9fccf3e13fff7a/proto/cosmos/bank/v1beta1/bank.proto#L96-L125) for the IBC denominations of all vouchers minted by the transfer module. | ||||||
|
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.
should we say something about making sure the correct authority is set in the ibc module keeper on app.go, and that it will default to gov if not?
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.
good call