feat: tx sign and new signer
#1590
Conversation
The new `sign` subcommand allows for signing a passed transaction envelope. And can choose a different source_account than the signing key's corresponding public key.
Additionally, `signer::{Transaction, TransactionHash, Blob}` traits which simplifies the interface. Using blanket implementations, any type that implements `Blob`, will implement, `TransactionHash` and any type that implements `TransactionHash` implements `Transaction`, which uses the hash. This will allow for types to opt in to how they want to sign the transaction.
There was a problem hiding this comment.
A few issues with this command, elaborated inline:
- Signing shouldn't require specifying a source account.
- Signing shouldn't be able to alter the source account.
- Signing shouldn't require additional user confirmation, given the command's only purpose is to sign.
- Trait mechanics. See inline. I think @Ifropc might have commented on this in the past too.
Co-authored-by: Leigh McCulloch <351529+leighmcculloch@users.noreply.github.com>
Remove Blob trait. Add to TransactionHash trait to include hint. Move print to top level signer and fix test. Also remove the prompt for this PR since the sign command is already approval for signing.
|
I've been pushing some tweaks directly to this PR to try something out, but I had to step away midway through. Will return, but maybe don't look too hard at the moment. Happy to also revert or force push the commits away. |
There was a problem hiding this comment.
I've pushed a series of changes that I think simplify the type system around the new logic being added. The new logic is largely unchanged, just moved out of the trait system.
I was going to leave a comment with the suggestion, but wanted to try it out before suggesting, and then discovered some things along the way.
The changes involved in brief:
- Removing traits
Stellar,TransactionHash,Transaction. Part of the design of theStellarSignerthat was already in this PR was an enum containing the different keys, and it then called each function on its key type anyway. So we actually don't need the implementations of different keys to sit behind a common trait, at least not yet with what's presented in this PR. Also the traits were implemented at two layers (both StellarSigner and LocalKey), which was confusing. I think we can bring back traits when they add value, but it seems like the design of StellarSigner already satisfies. - Removing terms like "accounts" in any of the signing logic, since these aren't accounts, just keys.
- Updated the signing code used by other fns to call through to the StellarSigner. Doing this only required ~4 lines of code because of how the StellarSigner was already designed, and it means after this PR merges we still only have one signing implementation for txs.
I'm approving this PR, but now that I've contributed changes, it would be ideal if @fnando you could 👀 the PR as a whole and sign off before we merge.
|
@leighmcculloch Thanks for the clean up. I do think it's ready to be merged and we can iterate when the demands change. |
willemneal
changed the title
tx sign and new traits for signingtx sign and new signer
The new
signsubcommand allows for signing a passed transaction envelope.Additionally,
signer::{Transaction, TransactionHash}traits which simplifies the interface.Transactionhas just a single method which returns aDecoratedSignature.TransactionHashbreaks this up into two methods to get aSignatureand aSignatureHint.