-
Notifications
You must be signed in to change notification settings - Fork 3.7k
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!: deprecate sdk.Msg.GetSigners #15284
Conversation
…-deprecate-get-signers-2
…-deprecate-get-signers-2
…-deprecate-get-signers-2
…-deprecate-get-signers-2
…-deprecate-get-signers-2
@@ -943,7 +955,7 @@ func (app *BaseApp) runMsgs(ctx sdk.Context, msgs []sdk.Msg, mode execMode) (*sd | |||
} | |||
|
|||
// create message events | |||
msgEvents := createEvents(msgResult.GetEvents(), msg) | |||
msgEvents := createEvents(app.cdc, msgResult.GetEvents(), msg, msgsV2[i]) |
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.
Shouldn't we compare the two lengths of msgs and msgsV2 for preventing an out of bound?
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.
We can, but could we also document that part of the contract of the methods creating msgsV2 is that it has the same length as msgs?
|
||
option go_package = "github.com/cosmos/cosmos-sdk/baseapp/testutil"; | ||
|
||
message MsgCounter { | ||
option (cosmos.msg.v1.signer) = "signer"; |
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.
We should add docs for that in the UPGRADING.md and somewhere in the docs website.
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.
@@ -133,6 +134,36 @@ var ( | |||
_ servertypes.Application = (*SimApp)(nil) | |||
) | |||
|
|||
// stdAccAddressCodec is a temporary address codec that we will use until we |
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.
does this have to be in app.go or can it be somewhere users can import to get the needed information?
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.
We can just refactor this to use address.Bech32Codec
. Does how I've updated it in the latest commit look okay for now? It's still using the global but we can refactor that later.
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.
left one nit but shouldnt be a blocker
Added #16294 around updating UPGRADING.md. Going to add this to the merge queue. I think it will be easier to address other issues in follow-ups given the massive size of this PR already. |
Description
Closes: #11275
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.md
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...
!
in the type prefix if API or client breaking change