feat(08-wasm): expose migrate entry point for 08-wasm #6231
Conversation
kienn6034
requested review from
colin-axner,
AdityaSripal,
damiannolan,
charleenfei,
chatton,
DimitrisJim,
srdtrk and
crodriguezvega
as code owners
WalkthroughWalkthroughThe recent update enhances the WASM client module by introducing a new command for migrating smart contracts to a newer code version. This command, integrated into the transaction functionality, enables users to manage contract migrations effectively by specifying client ID, checksum, and new code message parameters. The update also includes additional flags and validations to ensure secure and governance-compliant transaction processing. Changes
Recent Review DetailsConfiguration used: .coderabbit.yml Files selected for processing (1)
Additional Context UsedLanguageTool (9)
Path-based Instructions (1)
Additional comments not posted (1)
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
| err := cmd.MarkFlagRequired(govcli.FlagTitle) | ||
| if err != nil { | ||
| panic(err) | ||
| } |
There was a problem hiding this comment.
Consider replacing panic with a more graceful error handling method in the flag setup. Panicking in a CLI application can lead to abrupt termination which might not be user-friendly.
- panic(err)
+ if err != nil {
+ fmt.Fprintf(os.Stderr, "Error setting up required flags: %v\n", err)
+ os.Exit(1)
+ }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.
| err := cmd.MarkFlagRequired(govcli.FlagTitle) | |
| if err != nil { | |
| panic(err) | |
| } | |
| err := cmd.MarkFlagRequired(govcli.FlagTitle) | |
| if err != nil { | |
| fmt.Fprintf(os.Stderr, "Error setting up required flags: %v\n", err) | |
| os.Exit(1) | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
Out of diff range and nitpick comments (1)
docs/docs/03-light-clients/04-wasm/08-client.md (1)
40-40: Consider enhancing the description of the migrate message.It would be beneficial to specify what the migrate message should contain or provide a link to further documentation. This would help users understand how to properly format their migration messages.
| @@ -51,6 +51,7 @@ Ref: https://keepachangelog.com/en/1.0.0/ | |||
| ### Features | |||
|
|
|||
| * [#\5821](https://github.com/cosmos/ibc-go/pull/5821) feat: add `VerifyMembershipProof` RPC query (querier approach for conditional clients). | |||
| * [#\6231](https://github.com/cosmos/ibc-go/pull/6231) feat: add CLI to broadcast transaction with `MsgMigrateContract`. | |||
There was a problem hiding this comment.
Ensure consistency in the use of backticks for command names.
- * [#\6231](https://github.com/cosmos/ibc-go/pull/6231) feat: add CLI to broadcast transaction with `MsgMigrateContract`.
+ * [#\6231](https://github.com/cosmos/ibc-go/pull/6231) feat: add CLI to broadcast transaction with `MsgMigrateContract`.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.
| * [#\6231](https://github.com/cosmos/ibc-go/pull/6231) feat: add CLI to broadcast transaction with `MsgMigrateContract`. | |
| * [#\6231](https://github.com/cosmos/ibc-go/pull/6231) feat: add CLI to broadcast transaction with `MsgMigrateContract`. |
There was a problem hiding this comment.
Thank you, @kienn6034! I made a few changes, but this looks good to me now. It's a nice addition!
Did you actually test the CLI to check that it works as expected?
| @@ -27,7 +27,7 @@ func newSubmitStoreCodeProposalCmd() *cobra.Command { | |||
| Use: "store-code [path/to/wasm-file]", | |||
| Short: "Reads wasm code from the file and creates a proposal to store the wasm code", | |||
| Long: "Reads wasm code from the file and creates a proposal to store the wasm code", | |||
| Example: fmt.Sprintf("%s tx %s wasm [path/to/wasm_file]", version.AppName, ibcexported.ModuleName), | |||
| Example: fmt.Sprintf("%s tx %s-wasm store-code [path/to/wasm_file]", version.AppName, ibcexported.ModuleName), | |||
There was a problem hiding this comment.
I noticed this was wrong, so I took the liberty to fix it here.
|
|
||
| func newMigrateContractCmd() *cobra.Command { | ||
| cmd := &cobra.Command{ | ||
| Use: "migrate-contract [client-id] [checksum] [migrate-msg]", |
There was a problem hiding this comment.
I renamed new-code-msg to migrate-msg, since this is a JSON-encoded string that is passed to the contract that is being migrated.
|
|
||
| func newMigrateContractCmd() *cobra.Command { | ||
| cmd := &cobra.Command{ | ||
| Use: "migrate-contract [client-id] [checksum] [migrate-msg]", |
There was a problem hiding this comment.
Does this cmd also support passing the json msg as a path to a file? We do that on many other cmds that accept some kind of sub msg.
Do you think we should support this? cc. @crodriguezvega @srdtrk
There was a problem hiding this comment.
I guess most contracts for now will not do anything with this message, so most of the times it will be just {}? I am fine supporting it already now or adding support for it later.
Co-authored-by: Damian Nolan <damiannolan@gmail.com>
* feat: expose migrate entry point for 08-wasm * add CLI documentation * add changelog * improve CLI inline docs * small fix * rename variable * remove gov flags * use double quotes * fix lint warning * Apply suggestions from code review Co-authored-by: Damian Nolan <damiannolan@gmail.com> --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: Damian Nolan <damiannolan@gmail.com> (cherry picked from commit 34164ef)
* feat: expose migrate entry point for 08-wasm * add CLI documentation * add changelog * improve CLI inline docs * small fix * rename variable * remove gov flags * use double quotes * fix lint warning * Apply suggestions from code review Co-authored-by: Damian Nolan <damiannolan@gmail.com> --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: Damian Nolan <damiannolan@gmail.com> (cherry picked from commit 34164ef) Co-authored-by: Kien <kien@notional.ventures>
* feat: expose migrate entry point for 08-wasm * add CLI documentation * add changelog * improve CLI inline docs * small fix * rename variable * remove gov flags * use double quotes * fix lint warning * Apply suggestions from code review Co-authored-by: Damian Nolan <damiannolan@gmail.com> --------- Co-authored-by: Carlos Rodriguez <carlos@interchain.io> Co-authored-by: Damian Nolan <damiannolan@gmail.com>
Description
I think we should expose the
migrateContractentry point into the clicloses: #XXXX
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/).godoccomments.Files changedin the GitHub PR explorer.SonarCloud Reportin the comment section below once CI passes.Summary by CodeRabbit
migrate-contractin the08-client.mdfile to facilitate contract migration for light clients.