-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
refactor: Update GetSigners
to return []string
#9418
Changes from 17 commits
d7d37e9
f7f56d7
d62d5d5
f5fa9a2
cd28bd4
74118f7
d60f84d
6859083
3dee427
964acf4
cd4a867
87555ed
5a0a874
b572a65
26623ee
71114b7
72e5bfc
f926018
52ec9b0
72c93f5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -20,6 +20,23 @@ import ( | |
authsigning "github.com/cosmos/cosmos-sdk/x/auth/signing" | ||
) | ||
|
||
func ValidateMsg(msg sdk.Msg) error { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why do we need this? I thought all Msg's ValidateBasic already checks that the signers are correct bech32 addresses. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think it may not cover signer addresses in few cases, can you check this There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we have a godoc for ValidateMsg? |
||
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 | ||
} | ||
|
||
// GenerateOrBroadcastTxCLI will either generate and print and unsigned transaction | ||
// or sign it and broadcast it returning an error upon failure. | ||
func GenerateOrBroadcastTxCLI(clientCtx client.Context, flagSet *pflag.FlagSet, msgs ...sdk.Msg) error { | ||
|
@@ -35,7 +52,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 := msg.ValidateBasic(); err != nil { | ||
if err := ValidateMsg(msg); err != nil { | ||
amaury1093 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return err | ||
} | ||
} | ||
|
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.
baseapp should not import anything from client, it creates messy code. Is there somewhere else we can put ValidateMsg?
Edit: Maybe
types/tx
?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.
how about
types/utils.go
?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.
I believe we're moving away of putting anything in the
types
root package, it's too large already.Edit: or put it in
types/bech32
, and call itbech32.ValidateMsgSigners
We can also create a new
types/msg
package. @robert-zaremba @aaronc @ryanchristo Do you have any opinions on this?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.
I don't think it is fine to put it in
types/bech32
, since it is callingmsg.ValidateBasic()
inside. For now I kept it in thetypes/tx
.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.
I would like to avoid adding these as new public functions if possible and that means we don't need to think about packages as much if they're private. Since we are deprecating global bech32's soon that will avoid another API breaking change.