-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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!: add 'amount' field to withdrawal responses #11457
feat!: add 'amount' field to withdrawal responses #11457
Conversation
Would appreciate some guidance on where to test this - the unit tests for withdrawal don't submit a tx (they call the fn directly on the keeper), and the simapp tests discard the tx response. Additionally w.r.t docs - I don't see any relevant docs to update (the proto generated docs are obviously automatically updated). Anything I have missed? |
Codecov Report
@@ Coverage Diff @@
## master #11457 +/- ##
==========================================
+ Coverage 65.88% 65.93% +0.05%
==========================================
Files 675 675
Lines 69830 69832 +2
==========================================
+ Hits 46006 46047 +41
+ Misses 21123 21092 -31
+ Partials 2701 2693 -8
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
You can use integration tests https://github.com/cosmos/cosmos-sdk/blob/master/x/distribution/client/testutil/suite.go#L452-L517 |
This PR is stalled on #11469 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
We can backport this IIUC. |
8808115
to
44902bb
Compare
…d MsgWithdrawValidatorCommissionResponse
44902bb
to
9d68416
Compare
Updated failing test, and rebased upon master. Should be good to go. |
@AmauryM additions to proto structs are safe, but I'm not 100% sure if these are stored in any Tendermint objects. So I'm going to merge this w/o backporting. If we deem this can be backported w/o app state changes, then we can always do so later. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but I'm not 100% sure if these are stored in any Tendermint objects.
@alexanderbez Yes, they are in the data
field in abci's {Check,DeliverTx}Response
. do you know if that constitues a consensus-breaking change?
Fine to include it in v0.46, so okay for the merge. Just would like to add proto comments for clients.
@@ -56,7 +56,10 @@ message MsgWithdrawDelegatorReward { | |||
} | |||
|
|||
// MsgWithdrawDelegatorRewardResponse defines the Msg/WithdrawDelegatorReward response type. | |||
message MsgWithdrawDelegatorRewardResponse {} | |||
message MsgWithdrawDelegatorRewardResponse { | |||
repeated cosmos.base.v1beta1.Coin amount = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
repeated cosmos.base.v1beta1.Coin amount = 1 | |
// Since: cosmos-sdk 0.46 | |
repeated cosmos.base.v1beta1.Coin amount = 1 |
@@ -70,7 +73,10 @@ message MsgWithdrawValidatorCommission { | |||
} | |||
|
|||
// MsgWithdrawValidatorCommissionResponse defines the Msg/WithdrawValidatorCommission response type. | |||
message MsgWithdrawValidatorCommissionResponse {} | |||
message MsgWithdrawValidatorCommissionResponse { | |||
repeated cosmos.base.v1beta1.Coin amount = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
repeated cosmos.base.v1beta1.Coin amount = 1 | |
// Since: cosmos-sdk 0.46 | |
repeated cosmos.base.v1beta1.Coin amount = 1 |
@joe-bowman could you by any chance make the tiny tweaks @AmauryM suggested? I merged prior to his review 😄 |
Description
Closes: #11456
Updates distr/tx.proto protobufs to add an
amount
field of typesrepeated sdk.Coin
todistrtypes.MsgWithdrawDelegatorRewardResponse
anddistrtypes.MsgWithdrawValidatorCommissionResponse
.x/distribution/keeper/msg_server.go
populate return values ofWithdrawValidatorCommission
andWithdrawDelegatorReward
withamount
.Author Checklist
All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.
I have...
!
to the type prefix if API or client breaking changeCHANGELOG.md
Reviewers Checklist
All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.
I have...
!
in the type prefix if API or client breaking change