-
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
R4R: Add tx/encode endpoint and CLI command #3523
Conversation
This also might be interesting as a microservice... |
x/bank/client/cli/encode.go
Outdated
@@ -0,0 +1,76 @@ | |||
package cli |
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.
Both this and the rest endpoint should live under the x/auth
package. The same applies to the broadcast
command/endpoint too, though I reckon we can get this in as it is since such refactoring would be out of context of the PR. /cc @jackzampolin @cwgoes @alexanderbez
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.
Yes, both should be moved to the auth module. Preferably in this PR.
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.
Cool, I've moved both to the auth module.
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.
ACK'd
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.
After a significant discussion offline with the SDK team, I don't think this is something we want to have added. cc @cwgoes
@jackzampolin could you provide a bit more detail on why? Thanks! |
@ducembarr we deem that such functionality should not exist in the SDK as it's something we do not intend on maintaining. I believe it would be a great addition as a microservice elsewhere or part of a wallet or block explorer. |
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.
After a longer discussion this endpoint does have significant utility for users who need to push high volumes of transactions and is a good stopgap until we have amino implementations in other languages
Codecov Report
@@ Coverage Diff @@
## develop #3523 +/- ##
===========================================
- Coverage 57.12% 57.09% -0.03%
===========================================
Files 190 191 +1
Lines 14956 14967 +11
===========================================
+ Hits 8544 8546 +2
- Misses 5872 5877 +5
- Partials 540 544 +4 |
/tx/encode: | ||
post: | ||
tags: | ||
- ICS20 |
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.
Is this from a copy pasta?
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.
ICS20 seemed the most reasonable since it's related to creating and broadcasting transactions.
x/bank/client/cli/encode.go
Outdated
@@ -0,0 +1,76 @@ | |||
package cli |
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.
Yes, both should be moved to the auth module. Preferably in this PR.
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.
few changes are required
Ref #3560, which can be separate from this PR, but we should definitely do this. |
7aaf7e9
to
a75c687
Compare
a75c687
to
6791329
Compare
@alessio @alexanderbez I've responded to PR comments; please re-review. Thanks! |
👍 💯 |
This PR adds a
/tx/encode
endpoint to Gaia to take a JSON-ified transaction, serialize it using the Amino wire format, and return the serialized bytes encoded to a base64 string. The wire format is necessary to determine a transaction's hash, so it's useful for client applications that want to build their own transactions without being dependent on wire serialization changes. I've also included an equivalentgaiacli tx encode
command.Targeted PR against correct branch (see CONTRIBUTING.md)
Linked to github-issue with discussion and accepted design OR link to spec that describes this work.
Wrote tests
Updated relevant documentation (
docs/
)Added entries in
PENDING.md
with issue #rereviewed
Files changed
in the github PR explorerFor Admin Use: