refactor(x/auth): v2 adaptable tx instead of double decode #15910
Conversation
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
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