Skip to content
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.

Sign up for GitHub

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

feat: CLI cmd for MsgRegisterCounterpartyAddress #987

Merged
merged 7 commits into from
Feb 24, 2022
Merged

feat: CLI cmd for MsgRegisterCounterpartyAddress #987

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
ed50980
feat: CLI cmd for MsgRegisterCounterpartyAddress
seantking Feb 23, 2022
7cec663
fix: examples
seantking Feb 23, 2022
67e1286
Update modules/apps/29-fee/client/cli/tx.go
seantking Feb 23, 2022
019c2d4
Update modules/apps/29-fee/client/cli/tx.go
seantking Feb 23, 2022
0cd34bc
chore: remove print
seantking Feb 23, 2022
af51847
Merge branch 'ics29-fee-middleware' into sean/issue#964-cli-register-…
seantking Feb 24, 2022
3371d51
nit: update address for counterparty
seantking Feb 24, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions modules/apps/29-fee/client/cli/cli.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@ func NewTxCmd() *cobra.Command {

txCmd.AddCommand(
NewPayPacketFeeAsyncTxCmd(),
NewRegisterCounterpartyAddress(),
)

return txCmd
Expand Down
27 changes: 26 additions & 1 deletion modules/apps/29-fee/client/cli/tx.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ func NewPayPacketFeeAsyncTxCmd() *cobra.Command {
Use: "pay-packet-fee [src-port] [src-channel] [sequence]",
Short: "Pay a fee to incentivize an existing IBC packet",
Long: strings.TrimSpace(`Pay a fee to incentivize an existing IBC packet.`),
Example: fmt.Sprintf("%s tx pay-packet-fee transfer channel-0 1 --recv-fee 10stake --ack-fee 10stake --timeout-fee 10stake", version.AppName),
Example: fmt.Sprintf("%s tx ibc-fee pay-packet-fee transfer channel-0 1 --recv-fee 10stake --ack-fee 10stake --timeout-fee 10stake", version.AppName),
Copy link
Contributor

@crodriguezvega crodriguezvega Feb 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How do you feel about doing instead tx ibc fee (or tx ibc incentivise)? We have under tx currently ibc and ibc-transfer, should we try to group things under the ibc subcommand? Maybe this requires some discussion to think about the direction we want to go regarding cli commands and if refactoring would be needed for the commands we currently have.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for raising this point. I think it's worthy of discussion. I think it's worth merging in this PR as-is for now and following up.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be difficult to do since then certain applications maintained by us can be in ibc subcommand. But third-party ibc applications won't be. This will make it confusing if those apps are in same binary, since it won't be clear why some ibc apps get to be in ibc subcommand and others aren't

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @AdityaSripal. Yes, @colin-axner also shared the same concern with me. I will just open an issue for now to work on it in the future. So as @seantking suggests it's good to merge as is.

Args: cobra.ExactArgs(3),
RunE: func(cmd *cobra.Command, args []string) error {
clientCtx, err := client.GetClientTxContext(cmd)
Expand Down Expand Up @@ -97,3 +97,28 @@ func NewPayPacketFeeAsyncTxCmd() *cobra.Command {

return cmd
}

// NewRegisterCounterpartyAddress returns the command to create a MsgRegisterCounterpartyAddress
func NewRegisterCounterpartyAddress() *cobra.Command {
cmd := &cobra.Command{
Use: "register-counterparty [address] [counterparty-address] [channel-id]",
Short: "Register a counterparty relayer address on a given channel.",
Long: strings.TrimSpace(`Register a counterparty relayer address on a given channel.`),
Example: fmt.Sprintf("%s tx ibc-fee register-counterparty cosmos1rsp837a4kvtgp2m4uqzdge0zzu6efqgucm0qdh cosmoss1sp921a4tttgpln6rqhdqe0zzu6efqgucm0qdh channel-0", version.AppName),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: can we have a different bech32 prefix on the counterparty. Use an osmo address as an example

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point

Args: cobra.ExactArgs(3),
RunE: func(cmd *cobra.Command, args []string) error {
clientCtx, err := client.GetClientTxContext(cmd)
if err != nil {
return err
}

msg := types.NewMsgRegisterCounterpartyAddress(args[0], args[1], args[2])
Copy link
Contributor

@crodriguezvega crodriguezvega Feb 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to validate args[0] and args[1] to make sure they are valid addresses? Maybe it's also possible to check that the channel in arg[2] is a channel that actually can be incentivised? Just to fail fast before submitting the message...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

msg.ValidateBasic is called on the message which will do validation on the strings as appropriate


return tx.GenerateOrBroadcastTxCLI(clientCtx, cmd.Flags(), msg)
},
}

flags.AddTxFlagsToCmd(cmd)

return cmd
}