-
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: Implement generate-only option for commands that create txs #2165
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2165 +/- ##
===========================================
- Coverage 64.23% 63.95% -0.28%
===========================================
Files 140 140
Lines 8575 8612 +37
===========================================
Hits 5508 5508
- Misses 2690 2727 +37
Partials 377 377 |
f98d1d6
to
1144625
Compare
025b9a7
to
f9c63dc
Compare
f9c63dc
to
bc2473e
Compare
bc2473e
to
413938b
Compare
I think this is a bit redundant with "offline" (pubkey-only) keys in gaiacli - did you look at that code? This is definitely something we want regardless - maybe offline keys should automatically trigger |
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.
A bit of repeated code, also see comment re: offline keys.
The new CLI flag builds an unsigned transaction and writes it to STDOUT. Likewise, REST clients can now append generate_only=true to a request's query arguments list and expect a JSON response carrying the unsigned transaction. Closes: #966
a74b9fa
to
8639580
Compare
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.
utACK. Just add the test that's missing to CLI to test the msg.Signature length
bdbd556
to
8c32a8f
Compare
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.
tested ACK. Left a minor note on the pending log.
Also, during testing, I found that I can generate txs without an amount. Is this desired/expected behavior? I would assume this logic should be identical to actually generating a real tx (except without signing and sending)...in other words, it should go through the same validation checks. No?
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.
tested ACK. Thanks!
I don't know if we need validation on transaction generation. I think users should be able to generate w/o all required data and then fill in manually if desired. cc @alexanderbez Anyone else have thoughts here? |
@jackzampolin it provides more consistent behavior, but I poised it as a question more or less and not something that is necessary. I could be swayed the other way. In other words, when I see |
Seems like we're all set and ready then |
PR on sign command too is almost ready |
@cwgoes are you happy with the changes? |
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.
utACK, thanks @alessio
The new CLI
--generate-only
flag builds an unsigned transaction and writes it to STDOUT. Likewise, REST clients can now appendgenerate_only=true
to a request's query arguments list and expect a JSON response carrying the unsigned transaction.Closes: #966
docs/
)PENDING.md
with issue #Files changed
in the github PR explorerFor Admin Use: