feat(x/tx): add basic handler types + sign mode direct #14787
Conversation
aaronc
changed the title
There was a problem hiding this comment.
I prefer this SignModeHandler interface over the old one (with Modes and DefaultMode).
My only doubt about merging this now, is how long the 2 sign mode handlers will co-exist. Tx and signing stuff is already confusing, and this adds more complexity. For example, Evmos has their own handler, and I guess for hubl to work on their chain, they also need to add a new x/tx sign mode handler.
I would feel more comfortable if e.g. we did a spike to see if removing the old sign mode handler and replacing with this one right now would be viable (no unknown surprises). We would at least have the peace of mind that if we prioritized it, we could make the switch asap.
| @@ -0,0 +1,28 @@ | |||
| package std | |||
There was a problem hiding this comment.
Why does std mean here? Could we put it in signing directly?
There was a problem hiding this comment.
std just means standard or default. default is a reserved keyword but I wanted to do that
We can't put it in signing because this depends on all the other sign modes and all sign modes depend on signing.
| if s.CoinMetadataQueryFn == nil { | ||
| return nil, fmt.Errorf("missing %T needed for SIGN_MODE_TEXTUAL", s.CoinMetadataQueryFn) | ||
| } |
There was a problem hiding this comment.
This should go in textual.NewSignModeHandler. I believe in tests there are a couple of textual.NewSignModeHandler(nil), which should simply be replaced by textual.SignModeHandler{}.
I can do that in a follow-up if you want to keep this PR focused on tx signing structure.
There was a problem hiding this comment.
Can you do that in a follow-up @AmauryM ?
| // BodyHasUnknownNonCriticals should be set to true if the transaction has been | ||
| // decoded and found to have unknown non-critical fields. This is only needed | ||
| // for amino JSON signing. | ||
| BodyHasUnknownNonCriticals bool |
There was a problem hiding this comment.
What should the amino handler do if this is true? Currently we just ignore right?
There was a problem hiding this comment.
I think we reject but we should check
| // to specific value renderers for SIGN_MODE_TEXTUAL. | ||
| type Textual struct { | ||
| type SignModeHandler struct { |
|
I think we should consider this tx semantic version 2 - it's not duplication it's the next version. We can deprecate and remove the existing sign mode handler stuff once we can support amino JSON with protoreflect which hopefully will be ready soon |
|
I understand this is the next version. I think it's better to do it in this order:
rather than doing 2, 1, 3. Or more precisely, 2+3 should be done close to each other, this way the co-existence phase is as short as possible. BTW 1 can be shipped without 2+3, i.e. plugged into the old system, that's why I'm proposing to do it first. If we decide that these 3 pieces will be merged atomically for the next release v048, then it's fine to merge this PR now. |
|
The reason I'd prefer to do this sooner is because it gives a structure to implement the amino JSON support and to start migrating the other stuff. For instance, we can start migrating aux and ante handler stuff if we have this |
|
Sure, let's fix the last couple of comments and merge this |
|
I did the one refactoring you suggested @AmauryM. There's another part that you said could be dealt with in a follow-up and then the one question about the |
|
This needs a review from someone in @cosmos/sdk-core-dev |
Co-authored-by: Amaury <1293565+amaurym@users.noreply.github.com>
Description
Closes: #XXXX
Work is currently proceeding on sign mode textual in the new
x/txmodule, we are starting to look into #10993, and autocli/hubl will soon want tx support. The intention of this PR is to introduce a bit more structure tox/txin terms of how sign modes are added and also to introduce the very basic sign mode direct as a building block.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