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

fix: force genOrBroadcastFn even when max-msgs != 0 #364

Conversation

0Tech
Copy link
Collaborator

@0Tech 0Tech commented Oct 29, 2021

Description

If cli tx withdraw-all-rewards sends empty tx, the tx would be
rejected by validators because it has no msgs. However, the cli sends
empty tx only if one sets --max-msgs=0 which means it does not split
the msgs. When you set max-msgs with any positive number, the cli
does not send any tx, which may confuse you because you cannot get the
feedback indicating the address has no delegations.
This patch addresses the problem, by forcing genOrBroadcastFn when the
number of total msgs generated is zero. It will provide the same user
experience with any max-msgs.

closes: #363


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 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.
  • Wrote unit and integration tests
  • Updated relevant documentation (docs/) or specification (x/<module>/spec/)
  • Added relevant godoc comments.
  • Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
  • Re-reviewed Files changed in the Github PR explorer
  • Review Codecov Report in the comment section below once CI passes

@0Tech 0Tech added the A: bug Something isn't working label Oct 29, 2021
@0Tech 0Tech self-assigned this Oct 29, 2021
@0Tech 0Tech force-pushed the 0Tech/fix/withdraw-all-rewards-null branch from a819a84 to 5785fd0 Compare October 29, 2021 10:28
@0Tech 0Tech marked this pull request as ready for review October 29, 2021 10:32
@0Tech 0Tech force-pushed the 0Tech/fix/withdraw-all-rewards-null branch from 975ff3d to b342b79 Compare October 29, 2021 11:21
@codecov
Copy link

codecov bot commented Oct 29, 2021

Codecov Report

❗ No coverage uploaded for pull request base (release/v0.43.x@2c45298). Click here to learn what that means.
The diff coverage is n/a.

❗ Current head eed98a2 differs from pull request most recent head b899dae. Consider uploading reports for the commit b899dae to get more accurate results
Impacted file tree graph

@@                Coverage Diff                 @@
##             release/v0.43.x     #364   +/-   ##
==================================================
  Coverage                   ?   53.17%           
==================================================
  Files                      ?      642           
  Lines                      ?    67250           
  Branches                   ?        0           
==================================================
  Hits                       ?    35763           
  Misses                     ?    28545           
  Partials                   ?     2942           

@egonspace
Copy link

I think it's a good solution. However, it seems that the target branch should be release/v0.43.x. If so, it will be automatically applied in main branch by the github action.

@0Tech 0Tech changed the base branch from main to release/v0.43.x November 1, 2021 02:09
@0Tech 0Tech marked this pull request as draft November 1, 2021 02:10
@0Tech 0Tech force-pushed the 0Tech/fix/withdraw-all-rewards-null branch from b342b79 to eed98a2 Compare November 1, 2021 02:34
Copy link
Contributor

@dudong2 dudong2 left a comment

Choose a reason for hiding this comment

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

LGTM

@dudong2 dudong2 self-requested a review November 1, 2021 05:38
Copy link

@egonspace egonspace left a comment

Choose a reason for hiding this comment

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

LGTM

@0Tech
Copy link
Collaborator Author

0Tech commented Nov 1, 2021

I have submitted the corresponding PR to cosmos-sdk. I will apply this PR after they respond.

If cli tx withdraw-all-rewards sends empty tx, the tx would be
rejected by validators because it has no msgs. However, the cli sends
empty tx only if one sets `--max-msgs=0` which means it does not split
the msgs. When you set `max-msgs` with any positive number, the cli
does not send any tx, which may confuse you because you cannot get the
feedback indicating the address has no delegations.
This patch addresses the problem, by forcing genOrBroadcastFn when the
number of total msgs generated is zero. It will provide the same user
experience with any `max-msgs`.
@0Tech 0Tech force-pushed the 0Tech/fix/withdraw-all-rewards-null branch from eed98a2 to b899dae Compare November 1, 2021 07:05
@0Tech 0Tech changed the title fix: emit error in tx withdraw-all-rewards when the signer has no delegations fix: force genOrBroadcastFn even when max-msgs != 0 Nov 1, 2021
@0Tech 0Tech marked this pull request as ready for review November 2, 2021 03:58
@egonspace egonspace merged commit 4fbe47a into Finschia:release/v0.43.x Nov 2, 2021
@0Tech 0Tech deleted the 0Tech/fix/withdraw-all-rewards-null branch November 2, 2021 04:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: bug Something isn't working release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Empty response for invalid address on withdraw-all-rewards
3 participants