refactor(mint): move ValidateBasic logic to msgServer
#15760
Conversation
github-prbot
requested review from
a team,
mark-rushakoff and
likhita-809
and removed request for
a team
| if ms.authority != req.Authority { | ||
| return nil, errors.Wrapf(govtypes.ErrInvalidSigner, "invalid authority; expected %s, got %s", ms.authority, req.Authority) | ||
| func (ms msgServer) UpdateParams(goCtx context.Context, msg *types.MsgUpdateParams) (*types.MsgUpdateParamsResponse, error) { | ||
| if _, err := sdk.AccAddressFromBech32(msg.Authority); err != nil { |
There was a problem hiding this comment.
If we check the authority equality after, do we really need to verify if the input is a valid address? I actually do not think so, imho the inequality error message will be sufficiently clear.
There was a problem hiding this comment.
isnt the one stored on disk decoded as well?
There was a problem hiding this comment.
oh it seems in this module we dont. there seems to be a mix of some modules decoding and others not. Which should we go with?
There was a problem hiding this comment.
Not sure I follow. We always check the given authority compared to the module authority.
The address validity check is useful is CLI, but my point was if you submit a transaction with an address "foo", that it returns an error with "foo is an invalid address" or "expected cosmos1abc, got: foo" are both explicit, so we could remove that first check in the msg server.
There was a problem hiding this comment.
I have the same opinion as julien, as we always have checks for module authority. It covers the address validity check too returning "invalid authority".
There was a problem hiding this comment.
Alright, let me submit a PR simplifying that for all modules afterward.
julienrbrt
added
the
A:automerge
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