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

refactor(mint): move ValidateBasic logic to msgServer #15760

Merged
merged 5 commits into from
Apr 10, 2023
Merged

refactor(mint): move ValidateBasic logic to msgServer #15760

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
d018f39
refactor(mint): move `ValidateBasic` logic to `msgServer`
julienrbrt Apr 8, 2023
6c3486d
Merge branch 'main' into julien/validate-mint
julienrbrt Apr 9, 2023
7e039fe
Merge branch 'main' into julien/validate-mint
julienrbrt Apr 10, 2023
459fddd
Merge branch 'main' into julien/validate-mint
julienrbrt Apr 10, 2023
dd02bab
Merge branch 'main' into julien/validate-mint
mergify[bot] Apr 10, 2023
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
16 changes: 12 additions & 4 deletions x/mint/keeper/msg_server.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,21 @@ func NewMsgServerImpl(k Keeper) types.MsgServer {
}

// UpdateParams updates the params.
func (ms msgServer) UpdateParams(goCtx context.Context, req *types.MsgUpdateParams) (*types.MsgUpdateParamsResponse, error) {
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 {
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

isnt the one stored on disk decoded as well?

Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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".

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, let me submit a PR simplifying that for all modules afterward.

return nil, errors.Wrap(err, "invalid authority address")
}

if ms.authority != msg.Authority {
return nil, errors.Wrapf(govtypes.ErrInvalidSigner, "invalid authority; expected %s, got %s", ms.authority, msg.Authority)
}

if err := msg.Params.Validate(); err != nil {
return nil, err
}

ctx := sdk.UnwrapSDKContext(goCtx)
if err := ms.SetParams(ctx, req.Params); err != nil {
if err := ms.SetParams(ctx, msg.Params); err != nil {
return nil, err
}

Expand Down
9 changes: 8 additions & 1 deletion x/mint/keeper/msg_server_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,12 +12,19 @@ func (s *IntegrationTestSuite) TestUpdateParams() {
expectErr bool
}{
{
name: "set invalid authority",
name: "set invalid authority (not an address)",
request: &types.MsgUpdateParams{
Authority: "foo",
},
expectErr: true,
},
{
name: "set invalid authority (not defined authority)",
request: &types.MsgUpdateParams{
Authority: "cosmos139f7kncmglres2nf3h4hc4tade85ekfr8sulz5",
},
expectErr: true,
},
{
name: "set invalid params",
request: &types.MsgUpdateParams{
Expand Down
15 changes: 0 additions & 15 deletions x/mint/types/msgs.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
package types

import (
"cosmossdk.io/errors"

sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/x/auth/migrations/legacytx"
)
Expand All @@ -22,16 +20,3 @@ func (m MsgUpdateParams) GetSigners() []sdk.AccAddress {
addr, _ := sdk.AccAddressFromBech32(m.Authority)
return []sdk.AccAddress{addr}
}

// ValidateBasic does a sanity check on the provided data.
func (m MsgUpdateParams) ValidateBasic() error {
if _, err := sdk.AccAddressFromBech32(m.Authority); err != nil {
return errors.Wrap(err, "invalid authority address")
}

if err := m.Params.Validate(); err != nil {
return err
}

return nil
}