Skip to content

Commit

Permalink
refactor!: change GetSigners return type to []sdk.AccAddress (#9915)
Browse files Browse the repository at this point in the history
<!--
The default pull request template is for types feat, fix, or refactor.
For other templates, add one of the following parameters to the url:
- template=docs.md
- template=other.md
-->

## Description
Changes the `sdk.Msg` interface method `GetSigners() []string` to `GetSigners() []sdk.AccAddress`

Closes: #9885 


+change GetSigner return type
+defer address checking to each msg's `ValidateBasic`  method
+clean up consistency/redundancy issues in validate basic


---

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

- [ ] included the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] added `!` to the type prefix if API or client breaking change
- [ ] targeted the correct branch (see [PR Targeting](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#pr-targeting))
- [ ] provided a link to the relevant issue or specification
- [ ] followed the guidelines for [building modules](https://github.com/cosmos/cosmos-sdk/blob/master/docs/building-modules)
- [ ] included the necessary unit and integration [tests](https://github.com/cosmos/cosmos-sdk/blob/master/CONTRIBUTING.md#testing)
- [ ] added a changelog entry to `CHANGELOG.md`
- [ ] included comments for [documenting Go code](https://blog.golang.org/godoc)
- [ ] updated the relevant documentation or specification
- [ ] reviewed "Files changed" and left comments if necessary
- [ ] confirmed all CI checks have passed

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

- [ ] confirmed the correct [type prefix](https://github.com/commitizen/conventional-commit-types/blob/v3.0.0/index.json) in the PR title
- [ ] confirmed `!` in the type prefix if API or client breaking change
- [ ] confirmed all author checklist items have been addressed 
- [ ] reviewed state machine logic
- [ ] reviewed API design and naming
- [ ] reviewed documentation is accurate
- [ ] reviewed tests and test coverage
- [ ] manually tested (if applicable)
  • Loading branch information
technicallyty authored Aug 13, 2021
1 parent 6a0eb50 commit 1dba673
Show file tree
Hide file tree
Showing 28 changed files with 211 additions and 291 deletions.
1 change: 0 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,6 @@ Ref: https://keepachangelog.com/en/1.0.0/
* [\#9519](https://github.com/cosmos/cosmos-sdk/pull/9519) `DeleteDeposits` renamed to `DeleteAndBurnDeposits`, `RefundDeposits` renamed to `RefundAndDeleteDeposits`
* (codec) [\#9521](https://github.com/cosmos/cosmos-sdk/pull/9521) Removed deprecated `clientCtx.JSONCodec` from `client.Context`.
* (codec) [\#9521](https://github.com/cosmos/cosmos-sdk/pull/9521) Rename `EncodingConfig.Marshaler` to `Codec`.
* [\#9418](https://github.com/cosmos/cosmos-sdk/pull/9418) `sdk.Msg`'s `GetSigners()` method updated to return `[]string`.
* [\#9594](https://github.com/cosmos/cosmos-sdk/pull/9594) `RESTHandlerFn` argument is removed from the `gov/NewProposalHandler`.
* [\#9594](https://github.com/cosmos/cosmos-sdk/pull/9594) `types/rest` package moved to `testutil/rest`.
* [\#9432](https://github.com/cosmos/cosmos-sdk/pull/9432) `ConsensusParamsKeyTable` moved from `params/keeper` to `params/types`
Expand Down
3 changes: 1 addition & 2 deletions baseapp/baseapp.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ import (
"github.com/cosmos/cosmos-sdk/store/rootmulti"
sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
sdktx "github.com/cosmos/cosmos-sdk/types/tx"
"github.com/cosmos/cosmos-sdk/x/auth/migrations/legacytx"
)

Expand Down Expand Up @@ -511,7 +510,7 @@ func validateBasicTxMsgs(msgs []sdk.Msg) error {
}

for _, msg := range msgs {
err := sdktx.ValidateMsg(msg)
err := msg.ValidateBasic()
if err != nil {
return err
}
Expand Down
30 changes: 15 additions & 15 deletions baseapp/baseapp_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -717,10 +717,10 @@ func (msg msgCounter) String() string { return "TODO" }
func (msg msgCounter) ProtoMessage() {}

// Implements Msg
func (msg msgCounter) Route() string { return routeMsgCounter }
func (msg msgCounter) Type() string { return "counter1" }
func (msg msgCounter) GetSignBytes() []byte { return nil }
func (msg msgCounter) GetSigners() []string { return nil }
func (msg msgCounter) Route() string { return routeMsgCounter }
func (msg msgCounter) Type() string { return "counter1" }
func (msg msgCounter) GetSignBytes() []byte { return nil }
func (msg msgCounter) GetSigners() []sdk.AccAddress { return nil }
func (msg msgCounter) ValidateBasic() error {
if msg.Counter >= 0 {
return nil
Expand Down Expand Up @@ -762,10 +762,10 @@ func (msg msgCounter2) String() string { return "TODO" }
func (msg msgCounter2) ProtoMessage() {}

// Implements Msg
func (msg msgCounter2) Route() string { return routeMsgCounter2 }
func (msg msgCounter2) Type() string { return "counter2" }
func (msg msgCounter2) GetSignBytes() []byte { return nil }
func (msg msgCounter2) GetSigners() []string { return nil }
func (msg msgCounter2) Route() string { return routeMsgCounter2 }
func (msg msgCounter2) Type() string { return "counter2" }
func (msg msgCounter2) GetSignBytes() []byte { return nil }
func (msg msgCounter2) GetSigners() []sdk.AccAddress { return nil }
func (msg msgCounter2) ValidateBasic() error {
if msg.Counter >= 0 {
return nil
Expand All @@ -779,13 +779,13 @@ type msgKeyValue struct {
Value []byte
}

func (msg msgKeyValue) Reset() {}
func (msg msgKeyValue) String() string { return "TODO" }
func (msg msgKeyValue) ProtoMessage() {}
func (msg msgKeyValue) Route() string { return routeMsgKeyValue }
func (msg msgKeyValue) Type() string { return "keyValue" }
func (msg msgKeyValue) GetSignBytes() []byte { return nil }
func (msg msgKeyValue) GetSigners() []string { return nil }
func (msg msgKeyValue) Reset() {}
func (msg msgKeyValue) String() string { return "TODO" }
func (msg msgKeyValue) ProtoMessage() {}
func (msg msgKeyValue) Route() string { return routeMsgKeyValue }
func (msg msgKeyValue) Type() string { return "keyValue" }
func (msg msgKeyValue) GetSignBytes() []byte { return nil }
func (msg msgKeyValue) GetSigners() []sdk.AccAddress { return nil }
func (msg msgKeyValue) ValidateBasic() error {
if msg.Key == nil {
return sdkerrors.Wrap(sdkerrors.ErrInvalidRequest, "key cannot be nil")
Expand Down
2 changes: 1 addition & 1 deletion client/tx/tx.go
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ func GenerateOrBroadcastTxWithFactory(clientCtx client.Context, txf Factory, msg
// Right now, we're factorizing that call inside this function.
// ref: https://github.com/cosmos/cosmos-sdk/pull/9236#discussion_r623803504
for _, msg := range msgs {
if err := tx.ValidateMsg(msg); err != nil {
if err := msg.ValidateBasic(); err != nil {
return err
}
}
Expand Down
2 changes: 1 addition & 1 deletion server/mock/tx.go
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ func (tx kvstoreTx) ValidateBasic() error {
return nil
}

func (tx kvstoreTx) GetSigners() []string {
func (tx kvstoreTx) GetSigners() []sdk.AccAddress {
return nil
}

Expand Down
2 changes: 1 addition & 1 deletion server/rosetta/converter.go
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ func (c converter) Ops(status string, msg sdk.Msg) ([]*rosettatypes.Operation, e
op := &rosettatypes.Operation{
Type: opName,
Status: &status,
Account: &rosettatypes.AccountIdentifier{Address: signer},
Account: &rosettatypes.AccountIdentifier{Address: signer.String()},
Metadata: meta,
}

Expand Down
23 changes: 18 additions & 5 deletions testutil/testdata/tx.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package testdata

import (
"encoding/json"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"

"github.com/stretchr/testify/require"

Expand Down Expand Up @@ -62,12 +63,24 @@ func (msg *TestMsg) GetSignBytes() []byte {
}
return sdk.MustSortJSON(bz)
}
func (msg *TestMsg) GetSigners() []string {
return msg.Signers
func (msg *TestMsg) GetSigners() []sdk.AccAddress {
signers := make([]sdk.AccAddress, 0, len(msg.Signers))
for _, addr := range msg.Signers {
a, _ := sdk.AccAddressFromBech32(addr)
signers = append(signers, a)
}
return signers
}
func (msg *TestMsg) ValidateBasic() error {
for _, addr := range msg.Signers {
if _, err := sdk.AccAddressFromBech32(addr); err != nil {
return sdkerrors.ErrInvalidAddress.Wrapf("invalid signer address: %s", err)
}
}
return nil
}
func (msg *TestMsg) ValidateBasic() error { return nil }

var _ sdk.Msg = &MsgCreateDog{}

func (msg *MsgCreateDog) GetSigners() []string { return []string{} }
func (msg *MsgCreateDog) ValidateBasic() error { return nil }
func (msg *MsgCreateDog) GetSigners() []sdk.AccAddress { return []sdk.AccAddress{} }
func (msg *MsgCreateDog) ValidateBasic() error { return nil }
30 changes: 3 additions & 27 deletions types/tx/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -97,14 +97,9 @@ func (t *Tx) GetSigners() []sdk.AccAddress {

for _, msg := range t.GetMsgs() {
for _, addr := range msg.GetSigners() {
if !seen[addr] {
signer, err := sdk.AccAddressFromBech32(addr)
if err != nil {
panic(err)
}

signers = append(signers, signer)
seen[addr] = true
if !seen[addr.String()] {
signers = append(signers, addr)
seen[addr.String()] = true
}
}
}
Expand Down Expand Up @@ -195,22 +190,3 @@ func RegisterInterfaces(registry codectypes.InterfaceRegistry) {
registry.RegisterInterface("cosmos.tx.v1beta1.Tx", (*sdk.Tx)(nil))
registry.RegisterImplementations((*sdk.Tx)(nil), &Tx{})
}

// ValidateMsg calls the `sdk.Msg.ValidateBasic()`
// also validates all the signers are valid bech32 addresses.
func ValidateMsg(msg sdk.Msg) error {
err := msg.ValidateBasic()
if err != nil {
return err
}

signers := msg.GetSigners()
for _, signer := range signers {
_, err = sdk.AccAddressFromBech32(signer)
if err != nil {
return err
}
}

return nil
}
43 changes: 0 additions & 43 deletions types/tx/types_test.go

This file was deleted.

4 changes: 2 additions & 2 deletions types/tx_msg.go
Original file line number Diff line number Diff line change
Expand Up @@ -15,10 +15,10 @@ type (
// doesn't require access to any other information.
ValidateBasic() error

// Signers returns the bech32-encoded addrs of signers that must sign.
// Signers returns the addrs of signers that must sign.
// CONTRACT: All signatures must be present to be valid.
// CONTRACT: Returns addrs in some deterministic order.
GetSigners() []string
GetSigners() []AccAddress
}

// Fee defines an interface for an application application-defined concrete
Expand Down
2 changes: 1 addition & 1 deletion types/tx_msg_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ func (s *testMsgSuite) TestMsg() {

msg := testdata.NewTestMsg(accAddr)
s.Require().NotNil(msg)
s.Require().Equal([]string{accAddr.String()}, msg.GetSigners())
s.Require().True(accAddr.Equals(msg.GetSigners()[0]))
s.Require().Equal("TestMsg", msg.Route())
s.Require().Equal("Test message", msg.Type())
s.Require().Nil(msg.ValidateBasic())
Expand Down
2 changes: 1 addition & 1 deletion x/auth/ante/basic.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import (
authsigning "github.com/cosmos/cosmos-sdk/x/auth/signing"
)

// ValidateBasicDecorator will call tx.ValidateBasic, ValidateMsg(for each msg inside tx)
// ValidateBasicDecorator will call tx.ValidateBasic, msg.ValidateBasic(for each msg inside tx)
// and return any non-nil error.
// If ValidateBasic passes, decorator calls next AnteHandler in chain. Note,
// ValidateBasicDecorator decorator will not get executed on ReCheckTx since it
Expand Down
10 changes: 3 additions & 7 deletions x/auth/migrations/legacytx/stdtx.go
Original file line number Diff line number Diff line change
Expand Up @@ -143,13 +143,9 @@ func (tx StdTx) GetSigners() []sdk.AccAddress {

for _, msg := range tx.GetMsgs() {
for _, addr := range msg.GetSigners() {
if !seen[addr] {
signer, err := sdk.AccAddressFromBech32(addr)
if err != nil {
panic(err)
}
signers = append(signers, signer)
seen[addr] = true
if !seen[addr.String()] {
signers = append(signers, addr)
seen[addr.String()] = true
}
}
}
Expand Down
22 changes: 7 additions & 15 deletions x/auth/vesting/types/msgs.go
Original file line number Diff line number Diff line change
Expand Up @@ -30,20 +30,11 @@ func (msg MsgCreateVestingAccount) Type() string { return TypeMsgCreateVestingAc

// ValidateBasic Implements Msg.
func (msg MsgCreateVestingAccount) ValidateBasic() error {
from, err := sdk.AccAddressFromBech32(msg.FromAddress)
if err != nil {
return err
if _, err := sdk.AccAddressFromBech32(msg.FromAddress); err != nil {
return sdkerrors.ErrInvalidAddress.Wrapf("invalid 'from' address: %s", err)
}
to, err := sdk.AccAddressFromBech32(msg.ToAddress)
if err != nil {
return err
}
if err := sdk.VerifyAddressFormat(from); err != nil {
return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "invalid sender address: %s", err)
}

if err := sdk.VerifyAddressFormat(to); err != nil {
return sdkerrors.Wrapf(sdkerrors.ErrInvalidAddress, "invalid recipient address: %s", err)
if _, err := sdk.AccAddressFromBech32(msg.ToAddress); err != nil {
return sdkerrors.ErrInvalidAddress.Wrapf("invalid 'to' address: %s", err)
}

if !msg.Amount.IsValid() {
Expand All @@ -68,6 +59,7 @@ func (msg MsgCreateVestingAccount) GetSignBytes() []byte {
}

// GetSigners returns the expected signers for a MsgCreateVestingAccount.
func (msg MsgCreateVestingAccount) GetSigners() []string {
return []string{msg.FromAddress}
func (msg MsgCreateVestingAccount) GetSigners() []sdk.AccAddress {
addr, _ := sdk.AccAddressFromBech32(msg.FromAddress)
return []sdk.AccAddress{addr}
}
6 changes: 2 additions & 4 deletions x/authz/keeper/keeper.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,10 +79,8 @@ func (k Keeper) DispatchActions(ctx sdk.Context, grantee sdk.AccAddress, msgs []
if len(signers) != 1 {
return nil, sdkerrors.ErrInvalidRequest.Wrap("authorization can be given to msg with only one signer")
}
granter, err := sdk.AccAddressFromBech32(signers[0])
if err != nil {
return nil, err
}
granter := signers[0]

// if granter != grantee then check authorization.Accept, otherwise we implicitly accept.
if !granter.Equals(grantee) {
authorization, _ := k.GetCleanAuthorization(ctx, grantee, granter, sdk.MsgTypeURL(msg))
Expand Down
28 changes: 15 additions & 13 deletions x/authz/msgs.go
Original file line number Diff line number Diff line change
Expand Up @@ -42,19 +42,20 @@ func NewMsgGrant(granter sdk.AccAddress, grantee sdk.AccAddress, a Authorization
}

// GetSigners implements Msg
func (msg MsgGrant) GetSigners() []string {
return []string{msg.Granter}
func (msg MsgGrant) GetSigners() []sdk.AccAddress {
granter, _ := sdk.AccAddressFromBech32(msg.Granter)
return []sdk.AccAddress{granter}
}

// ValidateBasic implements Msg
func (msg MsgGrant) ValidateBasic() error {
granter, err := sdk.AccAddressFromBech32(msg.Granter)
if err != nil {
return sdkerrors.Wrap(sdkerrors.ErrInvalidAddress, "invalid granter address")
return sdkerrors.ErrInvalidAddress.Wrapf("invalid granter address: %s", err)
}
grantee, err := sdk.AccAddressFromBech32(msg.Grantee)
if err != nil {
return sdkerrors.Wrap(sdkerrors.ErrInvalidAddress, "invalid granter address")
return sdkerrors.ErrInvalidAddress.Wrapf("invalid grantee address: %s", err)
}

if granter.Equals(grantee) {
Expand Down Expand Up @@ -126,19 +127,20 @@ func NewMsgRevoke(granter sdk.AccAddress, grantee sdk.AccAddress, msgTypeURL str
}

// GetSigners implements Msg
func (msg MsgRevoke) GetSigners() []string {
return []string{msg.Granter}
func (msg MsgRevoke) GetSigners() []sdk.AccAddress {
granter, _ := sdk.AccAddressFromBech32(msg.Granter)
return []sdk.AccAddress{granter}
}

// ValidateBasic implements MsgRequest.ValidateBasic
func (msg MsgRevoke) ValidateBasic() error {
granter, err := sdk.AccAddressFromBech32(msg.Granter)
if err != nil {
return sdkerrors.Wrap(sdkerrors.ErrInvalidAddress, "invalid granter address")
return sdkerrors.ErrInvalidAddress.Wrapf("invalid granter address: %s", err)
}
grantee, err := sdk.AccAddressFromBech32(msg.Grantee)
if err != nil {
return sdkerrors.Wrap(sdkerrors.ErrInvalidAddress, "invalid grantee address")
return sdkerrors.ErrInvalidAddress.Wrapf("invalid grantee address: %s", err)
}

if granter.Equals(grantee) {
Expand Down Expand Up @@ -201,15 +203,15 @@ func (msg MsgExec) GetMessages() ([]sdk.Msg, error) {
}

// GetSigners implements Msg
func (msg MsgExec) GetSigners() []string {
return []string{msg.Grantee}
func (msg MsgExec) GetSigners() []sdk.AccAddress {
grantee, _ := sdk.AccAddressFromBech32(msg.Grantee)
return []sdk.AccAddress{grantee}
}

// ValidateBasic implements Msg
func (msg MsgExec) ValidateBasic() error {
_, err := sdk.AccAddressFromBech32(msg.Grantee)
if err != nil {
return sdkerrors.Wrap(sdkerrors.ErrInvalidAddress, "invalid grantee address")
if _, err := sdk.AccAddressFromBech32(msg.Grantee); err != nil {
return sdkerrors.ErrInvalidAddress.Wrapf("invalid grantee address: %s", err)
}

if len(msg.Msgs) == 0 {
Expand Down
Loading

0 comments on commit 1dba673

Please sign in to comment.