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.

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: Improve withdraw-all-rewards UX #9781

Merged
merged 8 commits into from
Jul 28, 2021
11 changes: 9 additions & 2 deletions x/distribution/client/cli/tx.go
Original file line number Diff line number Diff line change
Expand Up @@ -126,11 +126,12 @@ func NewWithdrawAllRewardsCmd() *cobra.Command {
Short: "withdraw all delegations rewards for a delegator",
Long: strings.TrimSpace(
fmt.Sprintf(`Withdraw all rewards for a single delegator.
Note that if you use this command with --%[2]s=%[3]s or --%[2]s=%[4]s, the %[5]s flag will automatically be set to 0.

Example:
$ %s tx distribution withdraw-all-rewards --from mykey
$ %[1]s tx distribution withdraw-all-rewards --from mykey
`,
version.AppName,
version.AppName, flags.FlagBroadcastMode, flags.BroadcastSync, flags.BroadcastAsync, FlagMaxMessagesPerTx,
),
),
Args: cobra.NoArgs,
Expand Down Expand Up @@ -167,6 +168,12 @@ $ %s tx distribution withdraw-all-rewards --from mykey
}

chunkSize, _ := cmd.Flags().GetInt(FlagMaxMessagesPerTx)
if clientCtx.BroadcastMode != flags.BroadcastBlock && chunkSize > 0 {
cmd.Printf("Cannot use broadcast mode %[1]s with %[2]s != 0. Forcing %[2]s to 0",
clientCtx.BroadcastMode, FlagMaxMessagesPerTx)
chunkSize = 0
Copy link
Contributor

@amaury1093 amaury1093 Jul 28, 2021

Choose a reason for hiding this comment

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

is it worth erroring here? Instead of a bit sliently changing the user inputted flag, as the user might not notice this line.

This will also force the user to add the --FlagMaxMessagesPerTx=0 flag when not using broadcast mode block.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AmauryM To be honest, I think that would result in a bad UX. As per this comment, the block broadcast mode should be disincentivized, and the sync and async should be used instead. If we require the users to set a flag for the modes sync and async, that might make them simply use the block mode instead. For this reason, I think that changing the value and warning them with that log is sufficient.

Another thing that could be done is simply set the default value of FlagMaxMessagesPerTx to be 0, and error in case async or sync are used and that value is not zero.

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool. I'm putting automerge on, thanks!

Copy link
Contributor Author

@RiccardoM RiccardoM Jul 28, 2021

Choose a reason for hiding this comment

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

@AmauryM I've swapped the behavior by setting the default chunks to 0 and returning an error when async or sync is used with a FlagMaxMessagesPerTx greater than 0

}

return newSplitAndApply(tx.GenerateOrBroadcastTxCLI, clientCtx, cmd.Flags(), msgs, chunkSize)
},
}
Expand Down