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

Add a tx sign command #1490

Closed
Ifropc opened this issue Jul 25, 2024 · 8 comments · Fixed by #1604, #1592 or #1590
Closed

Add a tx sign command #1490

Ifropc opened this issue Jul 25, 2024 · 8 comments · Fixed by #1604, #1592 or #1590
Assignees

Comments

@Ifropc
Copy link
Contributor

Ifropc commented Jul 25, 2024

Currently, there is no command for signing transactions. This issue was created in a follow-up to #1406 to discuss design on sign command

@github-project-automation github-project-automation bot moved this to Backlog (Not Ready) in DevX Jul 25, 2024
@Ifropc
Copy link
Contributor Author

Ifropc commented Jul 25, 2024

I also took a look at @willemneal PR (#1406 ), I think we should make some design choices before we commit to the implementation. I totally agree that we can't break a user later on, so we really should be careful about our design choices for new commands.
Leigh brought up all the cases we would want to support from signing in this comment
Maybe we should start small and support only signing with 1 key that's also a source account?
I also thing that signing should be stateless and shouldn't mutate XDR (expect obviously adding signatures)
In addition, we do have some functions with signature capabilities (e.g. invoke ) which in my opinion should be kept as is without adding any new functionalities.
For simple cases, developer should be able to just call it with --identity the way it's done right now.
For any other scenarios (complex) developer should just pass signed XDR.
We only support complex scenarios (that Leigh mentioned in the PR comment) in sign command.
For example, let's say I want to deploy an asset contract, I'd write something like this:
stellar contract asset deploy --asset USDC: --source GMULTISIG --build-only --no-sign | stellar tx sign --sign-with-key alice | stellar tx sign --sign-with-key S2 | stellar tx sign --sign-with-key "hello world" --hd-path 0 | stellar tx sign --sign-with-ledger | stellar tx sign --sign-with-lab | stellar tx send --dry-run | stellar tx send --yes

The command above will:
Form contract asset deploy transaction, with transaction source account set to GMULTISIG and no signatures. GMULTISIG is a multisig account with total signer weights of 5

  1. Sign with first key (identity alice)
  2. Sign with second key (plane secret key S2...)
  3. Sign with third key (derived from seed phrase)
  4. Sign with 4th key (using ledger)
  5. Sign with 5th key (using lab, e.g. Freighter wallet)
  6. Dry-run send (returns status code 0 and XDR on success; on failure returns non-0 status code and error)
  7. Send transaction (will require confirmation, are you sure you want to send this transaction? bypassing with passing --yes)

Finally, as far at the sign goes, I think what is currently in PR generally looks good. I'd organize flags like this:

stellar tx sign <xdr> [OPTIONS] [SIGN_METHOD] [METHOD FLAGS]
Options (all are optional):
(General options)
--network-passphrase
--network
--global
--config-dir
Sign method (is mandatory):
--sign-with-key <secret key/identity/seed pharse/key alias>
   --hd-path <- mandatory ONLY if key passed is a seed phrase
--with-ledger
   --ledger-account (optional) 
--any-other-method

Flags we can add in the first iteration (as it's already implemented), or later on (to separate PRs):

general option flag
--auth-only <expires in ledgers> 

Proposed design gives us flexibility into adding new methods without affecting other methods (e.g. --with-secret doesn't need to know about --hd-path, while for --with-seed --hd-path is mandatory), and adding signing capabilities to all signing methods (e.g. everything Leigh is listed we can try to support with flags)

@leighmcculloch
Copy link
Member

Fyi there is also this issue to add signer support directly to other commands. I think this is still important for simple usability such as someone who wants to run commands and always use their hardware device:

@leighmcculloch
Copy link
Member

I like the idea expressed above to keep tx sign only ever signing with one signer, and then supporting using the command multiple times, that looks like a nice way to keep the command simpler, easier to use and understand, while retaining the flexibility and range of scenarios it supports.

@leighmcculloch
Copy link
Member

--with-secret
--with-seed
--hd-path <- note that it's not a valid flag for any other method and mandatory for --with-seed

I suggest we make the options --sign-with-* so that they lead with a scope that makes it clear that the 'with' is related to signing, and so we can use the same options on the tx sign command as well as elsewhere and they can remain consistent.

@leighmcculloch
Copy link
Member

I suggest we keep --with-secret and --with-seed together as a single option --sign-with-key because today on --source they are supported on the one option so it keeps --source and --sign-with-key working the same way and supporting the same inputs. We also support key aliases on the source option and so we should have key aliases work on it also.

@Ifropc
Copy link
Contributor Author

Ifropc commented Jul 26, 2024

Synced with @willemneal today, I think we are all on the same page for this implementation. A quick recap:

For all tx generating commands (any command that has a --source-account flag, or more generally commands that are using soroban_cli::commands::config::Args) PLUS new tx sign we will:

  1. Support the same set of --sign-with-* flags.
  2. . --sign-with-* is optional for all signing commands, and mandatory for sign command
  3. Initial implementation supports only --sign-with-key that accepts the same argument as --source-account
  4. --sign-with-* may also accept additional mandatory/optional parameters. Currently, it's only hd_path for --sign-with-key <passphrase>
  5. If --sign-with-* flags is provided, command will require confirmation from the user, unless --yes flag is provided (optional flag for all --sign-with-* commands)

For tx generating commands, the following is true:
--source-account is always used as a source account and,

  1. Used to auto-sign, unless --sign-with-* is passed. In that case transaction is NOT signed with --source-account key, but instead is signed with key provided in --sign-with
  2. --build-only flag doesn't auto-sign with source account (current behavior), and is intended to later sign generated XDR with sign command

Future improvements:
Don't allow user to sign with key using pubnet (due to secret key being exposed in a command history). User should only be allowed to sign with alternative options, such as hardware/software wallet, or a file.

auth capabilities were covered in original PR, but I suggested to move it in another PR. Issue to track and discuss it: #1493

User stories:

  1. I want to simply do it old way
    stellar [command] --source-account alice
    Does exactly what it was doing before
  2. I want to create transaction with different source account (Add support for building transactions with a different signer to the source account #1264 )
    stellar [command] --source-account alice --sign-with bob
  3. I want to sign my transaction using different method/on a different machine
    stellar [command] --source-account alice --build-only | stellar tx sign --sign-with-ledger | xargs -I{} curl my_remove_signer/sign/{}
  4. I want to (multisig) sign and send
    stellar [command] --source-account alice --build-only | stellar tx sign --sign-with-ledger | stellar tx sign --sign-with-key bob | stellar tx send
  5. I want to sign only (some) auths (TBD: Add a tx auth command #1493)

cc @leighmcculloch @janewang

@Ifropc
Copy link
Contributor Author

Ifropc commented Jul 26, 2024

The only open question is: do we want to support --source-account in tx sign?
No:
Not adding --source-account makes sign more clear from user perspective (because technically we don't need source account for signing)
Yes:
Adding --source-account will allow us to have unified api across all commands that involve signing and allow for better code re-using.

Personally, I vote to not add --source-account flag, but open for discussion

@leighmcculloch
Copy link
Member

I understand the desire of code reuse but in terms of semantics not supporting source account on tx sign makes the most sense.

Let's not invest in code deduplication where it sacrifices the UX.

@Ifropc Ifropc linked a pull request Jul 29, 2024 that will close this issue
@Ifropc Ifropc moved this from Backlog (Not Ready) to In Progress in DevX Jul 30, 2024
This was linked to pull requests Oct 2, 2024
@github-project-automation github-project-automation bot moved this from Needs Review to Done in DevX Oct 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
3 participants