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

Jump to bottom

feat: Improve withdraw-all-rewards UX #9781

Merged
merged 8 commits into from
Jul 28, 2021
Merged

feat: Improve withdraw-all-rewards UX #9781

Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
adc8a83
Improved withdraw-all-rewards UX by forcing BroadcastMode to block if…
RiccardoM Jul 27, 2021
5a69de4
Updated command documentation
RiccardoM Jul 27, 2021
3d7f6b8
Updated how to handle withdraw-all-rewards with different broadcast m…
RiccardoM Jul 28, 2021
94cacd2
Merge branch 'master' into improve-withdraw-all-rewards-ux
RiccardoM Jul 28, 2021
3ebce6f
Update x/distribution/client/cli/tx.go
RiccardoM Jul 28, 2021
b66e6ed
Merge branch 'master' of github.com:cosmos/cosmos-sdk into improve-wi…
RiccardoM Jul 28, 2021
5a40a2c
Added CHANGELOG entry
RiccardoM Jul 28, 2021
ca9e1d5
Merge remote-tracking branch 'desmos/improve-withdraw-all-rewards-ux'…
RiccardoM Jul 28, 2021
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
11 changes: 9 additions & 2 deletions x/distribution/client/cli/tx.go
View file
Open in desktop
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.Println("Cannot use broadcast mode %[1]s with %[2]s != 0. Forcing %[2]s to 0",
RiccardoM marked this conversation as resolved.
Show resolved Hide resolved
clientCtx.BroadcastMode, FlagMaxMessagesPerTx)
chunkSize = 0
Copy link
Contributor

@amaury1093 amaury1093 Jul 28, 2021 •
edited
Loading

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 •
edited
Loading

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