Skip to content
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

Implement --generate-only option for commands that create transactions #966

Closed
ebuchman opened this issue May 8, 2018 · 17 comments
Closed
Assignees

Comments

@ebuchman
Copy link
Member

ebuchman commented May 8, 2018

The gaiacli should support workflow for offline signing of transactions.

Additionally, it should support signing without saving a private key to disk (ie. tendermint/go-crypto#57)!

This flow should also allow an HD path to be specified so you can pick an account by passing the seed phrase and the HD path (at least for secp256k1 keys).

Description

As a gaiacli user
I want to generate an unsigned Tx
So that I can review the contents of the transaction before signing it.

Acceptance Criteria

Given transaction parameters
When I run the command to send the transaction with the --generate-only flag on
Then a text representation of the unsigned transaction is printed out.

@jackzampolin
Copy link
Member

@sunnya97 and I had a conversation about this today. We could follow the generate | sign | submit workflow. There are also a number spots in the CLI where we would need to implement this (thinking of staking and gov too).

I think the ideal way to deal with this would be to leave the existing functionality, and implement new commands for this workflow. I think the list of affected commands would be:

gaiacli stake create-validator
gaiacli stake edit-validator
gaiacli stake delegate
gaiacli stake unbond begin
gaiacli stake unbond complete
gaiacli stake redelegate begin
gaiacli stake redelegate complete
gaiacli stake unrevoke
gaiacli gov submit-proposal 
gaiacli gov deposit
gaiacli gov vote
gaiacli send

One way to implement this could be to leave the above commands and then create 3 new subcommands:

gaiacli new-tx
gaiacli sign-tx
gaiacli submit-tx

The new-tx command would have subcommands for each of the transactions that are currently created by the cli, sign-tx would take the text/JSON as an argument and sign it. This output would then be able to passed to the submit-tx command, which would just take a signed tx as an argument. This way it would be possible to construct complex transactions using this flow and | them together. This may not be possible given the way we separate code into modules and it might take too large of a refactor.

Another possible way to implement this would be to implement a new-tx sub command for the affected commands above (that generates the tx JSON to pass to gaiacli sign-tx), and leave the root command (e.g. gaiacli send) unchanged.

Would love to hear some thoughts on this as it's still tagged prelaunch

@rigelrozanski
Copy link
Contributor

Alternatively, maybe a bit more easy to implement - we could add some common flags:
--only-generate
--only-sign
--only-submit
to do the exact same thing except without new commands - I think we'd actually be able to easily abstract the desired functionality separation logic between commands real easily in the sdk.

@jackzampolin
Copy link
Member

jackzampolin commented Aug 9, 2018

@rigelrozanski The --generate-only, -g flag I really like! Would you be opposed to then having a gaiacli sign and gaiacli submit for the other two, or do both of those need to be specific to each transaction?

@alexanderbez
Copy link
Contributor

I don't believe sign and submit need to be specific to each transaction and/or module. As long as things are super intuitive and easy for the end user.

@jackzampolin
Copy link
Member

Cool. I think we go with this then.

@jackzampolin
Copy link
Member

jackzampolin commented Aug 9, 2018

Care should be taken in implementation such that the commands can be |ed together:

gaiacli send --from <key> --to <address> --generate-only | gaiacli sign <key> | gaiacli send

Or the result could be saved to a file, edited and then read from a file:

gaiacli send --from <key> --to <address> --generate-only > ./transaction.json
gaiacli sign <key> --from-file @transaction.json > ./signed_transaction.json
gaiacli send --from-file @signed_transaction.json

@jackzampolin jackzampolin changed the title cli for offline signing Implement --generate-only option for commands that create transactions Aug 9, 2018
@jackzampolin
Copy link
Member

I'm also going to open issues for both gaiacli sign and gaiacli send to track work on those in a separate issue.

@rigelrozanski
Copy link
Contributor

awesome

@alessio
Copy link
Contributor

alessio commented Aug 23, 2018

Sketching description and acceptance criteria:

Description

As a gaiacli user
I want to generate an unsigned Tx
So that I can review the contents of the transaction before signing it.

Acceptance Criteria

Given a transaction
When I run its respective command with the --generate-only flag on
Then a text representation of the unsigned transaction is printed out.

@jackzampolin
Copy link
Member

jackzampolin commented Aug 23, 2018

I would say

Given a cli command that generates a transaction
When I run its respective command with the --generate-only flag
Then a sign-able text representation of the unsigned transaction is printed out

@alessio
Copy link
Contributor

alessio commented Aug 23, 2018

Agreed.
Feel free to improve/amend it and paste it into the issue's description 👍

[EDIT]
Mmm, I may have to disagree with myself here :)

The Given clause should be used to lay the context/set of inputs out. In that case, the inputs consist of transaction parameters.

@alessio
Copy link
Contributor

alessio commented Aug 23, 2018

I'm on this, a brief preview of the output follows:

alessio@bizet:~/.../github.com/cosmos/cosmos-sdk$ gaiacli send --home=/tmp/.test_gaiacli --node=tcp://0.0.0.0:39199 --chain-id=test-chain-pXd1DH --generate-only --amount=10steak --to=cosmosaccaddr1pfdd6y6tt8r4vqr0s092dscssa85kssrmsk49p --from=foo
{"ChainID":"test-chain-pXd1DH","AccountNumber":"0","Sequence":"0","Fee":{"amount":[{"denom":"","amount":"0"}],"gas":"200000"},"Msgs":[{"type":"cosmos-sdk/Send","value":{"inputs":[{"address":"cosmosaccaddr1ujjwtf5sp9f70vhk0p4ealetnvg0f007dxrjc5","coins":[{"denom":"steak","amount":"10"}]}],"outputs":[{"address":"cosmosaccaddr1pfdd6y6tt8r4vqr0s092dscssa85kssrmsk49p","coins":[{"denom":"steak","amount":"10"}]}]}}],"Memo":""}
alessio@bizet:~/.../github.com/cosmos/cosmos-sdk$

@jackzampolin
Copy link
Member

@alessio Looks great! Think this will be a nice feature. Can we switch the variable names to lower case and - separated (chain-id, account-number)?

@alessio
Copy link
Contributor

alessio commented Aug 24, 2018 via email

@rigelrozanski
Copy link
Contributor

@alessio it would be great if you could open up a WIP PR for your work on this issue? (3.v. in https://github.com/cosmos/cosmos-sdk/blob/develop/CONTRIBUTING.md#contributing)

alessio added a commit that referenced this issue Aug 28, 2018
alessio added a commit that referenced this issue Aug 28, 2018
alessio added a commit that referenced this issue Aug 28, 2018
alessio added a commit that referenced this issue Sep 4, 2018
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
@hukkin
Copy link

hukkin commented May 20, 2019

Additionally, it should support signing without saving a private key to disk (ie. tendermint/go-crypto#57)!

Does anyone know if this ever happened? Based on the output of gaiacli tx sign -h and gaiacli tx send -h it seems to me that for the --from flag we still need Name or address of private key with which to sign, meaning that the private key itself must be on the disk.

@alexanderbez
Copy link
Contributor

@hukkinj1 Afaik, we do not support singing txs with a private key held ephemerally at the moment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants