refactor(distribution): move ValidateBasic logic to msgServer
#15755
Conversation
julienrbrt
force-pushed
the
julien/validate-distribution
branch
from
99cebe9 to
16d36c7
Compare
x/distribution/keeper/msg_server.go
Outdated
|
|
||
| return &types.MsgCommunityPoolSpendResponse{}, nil | ||
| } | ||
|
|
||
| func (k msgServer) DepositValidatorRewardsPool(goCtx context.Context, req *types.MsgDepositValidatorRewardsPool) (*types.MsgDepositValidatorRewardsPoolResponse, error) { | ||
| ctx := sdk.UnwrapSDKContext(goCtx) | ||
| if err := k.validateAuthority(req.Authority); err != nil { |
There was a problem hiding this comment.
I am a bit confused with the name authority here. It turns out, it isn't about the module authority (otherwise, the message would have been a bit useless).
I got confused because of the name, so I think people can get confused when sending the message. Changing Authority by Depositor or Sender would make more sense. cc @alexanderbez wdyt?
If this message should really be tied to a gov / any other authority, then it wasn't checked that msg the authority did match with the distribution authority (which is a bug then).
In any case, we can still update this as it is not released yet.
There was a problem hiding this comment.
I think the functionality is to be able to deposit validator rewards pool from not just authority, it can be any other depositer. The changes you made makes sense to me.
Not sure, if there is more underlying meaning here.
github-prbot
requested review from
a team,
aaronc and
testinginprod
and removed request for
a team
| "amount", req.Amount.String(), | ||
| "validator", req.ValidatorAddress, | ||
| ) | ||
|
|
||
| return &types.MsgDepositValidatorRewardsPoolResponse{}, nil | ||
| } | ||
|
|
||
| func (k *Keeper) validateAuthority(authority string) error { | ||
| if _, err := k.authKeeper.StringToBytes(authority); err != nil { |
There was a problem hiding this comment.
Same thoughts as here: #15760 (comment)
It is due to that: #15755 (comment) I'd like feedback on the naming and the expected behavior before merging. |
x/distribution/keeper/msg_server.go
Outdated
|
|
||
| return &types.MsgCommunityPoolSpendResponse{}, nil | ||
| } | ||
|
|
||
| func (k msgServer) DepositValidatorRewardsPool(goCtx context.Context, req *types.MsgDepositValidatorRewardsPool) (*types.MsgDepositValidatorRewardsPoolResponse, error) { | ||
| ctx := sdk.UnwrapSDKContext(goCtx) | ||
| if err := k.validateAuthority(req.Authority); err != nil { |
There was a problem hiding this comment.
I think the functionality is to be able to deposit validator rewards pool from not just authority, it can be any other depositer. The changes you made makes sense to me.
Not sure, if there is more underlying meaning here.
| msg := types.NewMsgFundCommunityPool(tc.amount, tc.depositor) | ||
| _, err := f.msgServer.FundCommunityPool(f.ctx, msg) | ||
| if tc.expErr { | ||
| require.NotNil(t, err) |
There was a problem hiding this comment.
nit: can we also match the error msg if possible?
Description
ref: #15648
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.mdReviewers 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