refactor(x/auth): v2 adaptable tx instead of double decode #15910
Conversation
github-prbot
requested review from
a team,
mark-rushakoff and
samricotta
and removed request for
a team
What about this seems convoluted? I don't have an alternative right now. Can you suggest one or make a specific criticism? |
It's just the fact that we need to pull things out with an interface - a lot of indirection when there's really only one concrete type that can implement this interface. I guess it's okay for now. But does seem like evidence of the point I was making earlier today about ante handler/sdk.Tx convolution |
|
Thanks. I agree with the points you made about how things look now and I think we should consider them as we move towards x/tx away from x/auth/tx. I updated the PR with some more background. |
| // GetSignBytesAdapter returns the sign bytes for a given transaction and sign mode. It accepts the arguments expected | ||
| // for signing in x/auth/tx and converts them to the arguments expected by the txsigning.HandlerMap, then applies | ||
| // HandlerMap.GetSignBytes to get the sign bytes. | ||
| func GetSignBytesAdapter( |
There was a problem hiding this comment.
nit: changelog for public api break
There was a problem hiding this comment.
This was introduced in https://github.com/cosmos/cosmos-sdk/pull/15822/files#diff-0100bcd16617d3f0e0a8eadcbdf052dfec465cb61c23daea4d201c95c2e20004 so still Unreleased, do we still need a Changelog entry?
There was a problem hiding this comment.
No, just make sure we update any previous CHANGELOG entry referring to this
| txSignerInfos := make([]*txv1beta1.SignerInfo, len(authInfo.SignerInfos)) | ||
| for i, signerInfo := range authInfo.SignerInfos { | ||
| modeInfo := &txv1beta1.ModeInfo{} | ||
| adaptModeInfo(signerInfo.ModeInfo, modeInfo) | ||
| txSignerInfo := &txv1beta1.SignerInfo{ | ||
| PublicKey: &anypb.Any{ | ||
| TypeUrl: signerInfo.PublicKey.TypeUrl, | ||
| Value: signerInfo.PublicKey.Value, | ||
| }, | ||
| Sequence: signerInfo.Sequence, | ||
| ModeInfo: modeInfo, | ||
| } | ||
| txSignerInfos[i] = txSignerInfo | ||
| } |
There was a problem hiding this comment.
did we purposefully want to disable being able to submit a transaction without a public key here @tac0turtle or what this just a simple mistake?
There was a problem hiding this comment.
quickly reading the pr as i dont have context for work done two years ago it seems its needed.
There was a problem hiding this comment.
apologies and makes sense
the thing we ran into was that signerInfo.PublicKey will panic as there's a nil public key. Since this comment I've dug a bit deeper and I'm guessing this is just a simple mistake.
we can't change now since it would be consensus breaking, but its a very small optimization so I was more just curious if we were moving towards requiring the public key be included in each tx instead of using the state
Description
Removes double decoding in SigVerificationAnteHandler which was introduced in #15822.
Introduces an adapter on the
wrapperstruct in tx builder to return a x/tx/signing.TxData struct for interoperability with x/tx.Background
The idealized version of the SDK doesn't need this because the entire transaction stack is powered by x/tx instead of x/auth/tx. However this would be a giant refactor and diff, and I think it's OK to depend on both modules for a period of transition time. I propose that shipping smaller, incremental changes, even if they include throw away code like this, will move the SDK to that idealized state faster than a large "all at once" refactor.
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