-
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
feat: Add tx encode and decode endpoints #13789
Conversation
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #13789 +/- ##
==========================================
+ Coverage 56.46% 56.64% +0.18%
==========================================
Files 648 647 -1
Lines 56221 55289 -932
==========================================
- Hits 31747 31321 -426
+ Misses 21909 21414 -495
+ Partials 2565 2554 -11
|
…ita/add-tx/encode-endpoint
…ita/add-tx/encode-endpoint
…ita/add-tx/encode-endpoint
…ita/add-tx/encode-endpoint
…ita/add-tx/encode-endpoint
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 document this only works with proto encoded txs, secondly is there a way we can add an amino way of encoding txs to the rest api? It could be something we attach to 1317 like before. Then we would support both tax types.
cc @ValarDragon for visibility 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.
Great work @likhita-809. We need a changelog here as well.
Before the merge could we add amino encoding. this would help users quite a lot |
@tac0turtle amino in which direction? We cannot convert an Amino-encoded Tx into a proto Tx. We can convert a proto Tx to an amino-encoded Tx though. But is there a use case for the latter? |
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.
LGTM overall, I'm proposing an optimized way of doing encoding below
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.
LGTM
…ita/add-tx/encode-endpoint
can we expose an endpoint allowing users to encode amino txs? I would think the use case is multisigs and any sort of ledger support |
Just to be clear, you are asking to encode a proto Tx to amino Tx? This won't help any of the 2 use cases you mentioned:
What ledger receives is a StdSignDoc. We can add an endpoint from unsigned proto Tx to StdSignDoc if that's useful, but it's not an amino-encoded tx.
Amino is used in multisigs to get the address. Not sure how a proto Tx to amino Tx encoder would help. |
what Im thinking is not related to proto tx. if someone would like to encode a amino tx they cant use this endpoint, so can we offer an endpoint to help them? |
I'm still not understanding what you mean by "encode a amino tx". Maybe you can write the request and response types of the endpoint? Also, how about we merge this PR, and discuss this additional endpoint in an issue? |
This issue asked about adding an endpoint to encode all versions, proto and amino. Right now this is only supporting proto. |
Ah ok, so it's an endpoint that encodes/decodes amino JSON <-> amino binary? |
Do we want to block this PR on that or create a new separate PR for those endpoints? |
lets create a new pr for that, but lets leave the same issue opened |
SGTM. @likhita-809 are you okay to work on that new PR too? |
yeah, sure |
* add grpc endpoint for encoding proto tx's (cherry picked from commit bcff22a)
Description
ref: #13085
ref: #10856
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