Add cli to submit gov proposal to upgrade multiple channels #5548

chatton · 2024-01-08T16:00:05Z

Description

closes: #5540

This PR adds a CLI to which queries all channels, and allows for specifying a pattern for the portID, and an optional comma separated list for channelIDs which will either generate a proposal.json file which contains MsgChanUpgradeInits, or actually broadcast a MsgSubmitProposal directly.

--dry-run will behave the same way as --json.

I can add documentation in a follow up if this approach gets merged.

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.

Targeted PR against the correct branch (see CONTRIBUTING.md).
Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
Code follows the module structure standards and Go style guide.
Wrote unit and integration tests.
Updated relevant documentation (docs/) or specification (x/<module>/spec/).
Added relevant godoc comments.
Provide a commit message to be used for the changelog entry in the PR description for review.
Re-reviewed Files changed in the Github PR explorer.
Review Codecov Report in the comment section below once CI passes.

chatton · 2024-01-08T16:11:43Z

modules/core/04-channel/client/cli/tx.go

+				msgUpgradeInit := types.NewMsgChannelUpgradeInit(ch.PortId, ch.ChannelId, types.NewUpgradeFields(ch.Ordering, ch.ConnectionHops, versionStr), clientCtx.GetFromAddress().String())
+				msgs = append(msgs, msgUpgradeInit)


I left ordering and connection hops unchanged, and only the version string changes. Technically we can allow these fields to be updated, but I think the version string will be all that we need 99% of the time. As far as bulk creating MsgChanUpgradeInits is concerned

colin-axner

Thank you @chatton! Looks great to me!

colin-axner · 2024-01-08T17:01:12Z

modules/core/04-channel/client/cli/tx.go

+		Use:   "upgrade-channels",
+		Short: "Upgrade IBC channels",
+		Long: `Submit a governance proposal to upgrade all open channels whose port matches a specified pattern 
+(the default is transfer), optionally, specific an exact list of channel IDs with a comma separated list.`,


Suggested change

(the default is transfer), optionally, specific an exact list of channel IDs with a comma separated list.`,

(the default is transfer), optionally, an exact list of comma separated channel IDs may be specified.`,

should we specify in this docstring that if channel IDs are specified then it must match the specified port? or leave that for the longer documentation?

charleenfei

just one doc related comment, thanks @chatton!

charleenfei · 2024-01-08T17:40:43Z

modules/core/04-channel/client/cli/tx.go

+		Use:   "upgrade-channels",
+		Short: "Upgrade IBC channels",
+		Long: `Submit a governance proposal to upgrade all open channels whose port matches a specified pattern 
+(the default is transfer), optionally, specific an exact list of channel IDs with a comma separated list.`,


should we specify in this docstring that if channel IDs are specified then it must match the specified port? or leave that for the longer documentation?

crodriguezvega

Thank you for the quick turnaround, @chatton! I just left a suggestion to maybe simplify some code.

crodriguezvega · 2024-01-08T19:08:37Z

modules/core/04-channel/client/cli/tx.go

+			depositStr := args[0]
+			versionStr := args[1]
+
+			metadata, err := cmd.Flags().GetString(flagMetadata)
+			if err != nil {
+				return err
+			}
+
+			summary, err := cmd.Flags().GetString(flagSummary)
+			if err != nil {
+				return err
+			}
+
+			title, err := cmd.Flags().GetString(flagTitle)
+			if err != nil {
+				return err
+			}
+
+			portPattern, err := cmd.Flags().GetString(flagPortPattern)
+			if err != nil {
+				return err
+			}
+
+			commaSeparatedChannelIDs, err := cmd.Flags().GetString(flagChannelIDs)
+			if err != nil {
+				return err
+			}
+
+			displayJSON, err := cmd.Flags().GetBool(flagJSON)
+			if err != nil {
+				return err
+			}
+
+			expidited, err := cmd.Flags().GetBool(flagExpedited)
+			if err != nil {
+				return err
+			}


I think we can maybe simplify all this code about the gov flags with a call ReadGovPropFlags like here, including the deposit parameter. And that function will return us a govv1.MsgSubmitProposal where we can add the messages.

yea, could also add AddGovPropFlagsToCmd though we do set a sensible default here so fine either way (maybe add and override default value?)

great suggestions, will implement these.

crodriguezvega · 2024-01-08T20:17:42Z

modules/core/04-channel/client/cli/tx.go

+	return cmd
+}
+
+// channelShouldBeUpgraded returns a boolean indicated whether or not the given channel should be upgraded based


Suggested change

// channelShouldBeUpgraded returns a boolean indicated whether or not the given channel should be upgraded based

// channelShouldBeUpgraded returns a boolean indicating whether or not the given channel should be upgraded based

chatton · 2024-01-09T10:18:51Z

just one doc related comment, thanks @chatton!

I think we can leave this for the longer docs

chatton · 2024-01-09T10:30:17Z

modules/core/04-channel/client/cli/cli.go

@@ -50,7 +50,8 @@ func NewTxCmd() *cobra.Command {
 	}

 	txCmd.AddCommand(
-		NewPruneAcknowledgementsTxCmd(),
+		newUpgradeChannelsTxCmd(),


unexported this to stay consistent, I don't think this needs to be exported.

(cherry picked from commit 11b301b)

…5555) (cherry picked from commit 11b301b) Co-authored-by: Cian Hatton <cian@interchain.io>

chatton added 3 commits January 8, 2024 14:26

chore: wip cli

024cb07

chore: create UpgradeChannels cobra command

d6b3759

chore: added support for comma separated list of channel IDs

505a6af

chatton requested review from colin-axner, AdityaSripal and damiannolan as code owners January 8, 2024 16:00

chatton commented Jan 8, 2024

View reviewed changes

colin-axner approved these changes Jan 8, 2024

View reviewed changes

charleenfei approved these changes Jan 8, 2024

View reviewed changes

charleenfei added the channel-upgradability Channel upgradability feature label Jan 8, 2024

crodriguezvega approved these changes Jan 8, 2024

View reviewed changes

DimitrisJim mentioned this pull request Jan 9, 2024

docs: channel upgrade timeout #5546

Merged

9 tasks

chatton added 3 commits January 9, 2024 10:23

chore: addressing PR feedback

91569f1

chore: merge main

9ebdbe8

chore: merge main

91e87d2

chatton commented Jan 9, 2024

View reviewed changes

chore: additional PR feedback

70758b0

chatton mentioned this pull request Jan 9, 2024

Docs for channel upgrades CLI #5553

Merged

9 tasks

chatton merged commit 11b301b into main Jan 9, 2024
61 checks passed

chatton deleted the cian/issue#5540-add-cli-to-submit-gov-proposal-to-upgrade-multiple-channels branch January 9, 2024 11:17

crodriguezvega added the backport-to-v8.1.x label Jan 9, 2024

mergify bot pushed a commit that referenced this pull request Jan 9, 2024

Add cli to submit gov proposal to upgrade multiple channels (#5548)

1c5a755

(cherry picked from commit 11b301b)

mergify bot mentioned this pull request Jan 9, 2024

Add cli to submit gov proposal to upgrade multiple channels (backport #5548) #5555

Merged

damiannolan pushed a commit that referenced this pull request Jan 9, 2024

Add cli to submit gov proposal to upgrade multiple channels (#5548) (#…

cd5586d

…5555) (cherry picked from commit 11b301b) Co-authored-by: Cian Hatton <cian@interchain.io>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cli to submit gov proposal to upgrade multiple channels #5548

Add cli to submit gov proposal to upgrade multiple channels #5548

chatton commented Jan 8, 2024

chatton Jan 8, 2024 •

edited

Loading

colin-axner left a comment

colin-axner Jan 8, 2024

charleenfei Jan 8, 2024

charleenfei left a comment

charleenfei Jan 8, 2024

crodriguezvega left a comment

crodriguezvega Jan 8, 2024

DimitrisJim Jan 9, 2024

chatton Jan 9, 2024

crodriguezvega Jan 8, 2024

chatton commented Jan 9, 2024

chatton Jan 9, 2024

		msgUpgradeInit := types.NewMsgChannelUpgradeInit(ch.PortId, ch.ChannelId, types.NewUpgradeFields(ch.Ordering, ch.ConnectionHops, versionStr), clientCtx.GetFromAddress().String())
		msgs = append(msgs, msgUpgradeInit)

	(the default is transfer), optionally, specific an exact list of channel IDs with a comma separated list.`,
	(the default is transfer), optionally, an exact list of comma separated channel IDs may be specified.`,

	// channelShouldBeUpgraded returns a boolean indicated whether or not the given channel should be upgraded based
	// channelShouldBeUpgraded returns a boolean indicating whether or not the given channel should be upgraded based

Add cli to submit gov proposal to upgrade multiple channels #5548

Add cli to submit gov proposal to upgrade multiple channels #5548

Conversation

chatton commented Jan 8, 2024

Description

chatton Jan 8, 2024 • edited Loading

Choose a reason for hiding this comment

colin-axner left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

charleenfei left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crodriguezvega left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chatton commented Jan 9, 2024

Choose a reason for hiding this comment

chatton Jan 8, 2024 •

edited

Loading