From 00aaedc9dbb10edf4b5f404fc1cfbe423f385186 Mon Sep 17 00:00:00 2001 From: Willem Wyndham Date: Tue, 17 Sep 2024 15:06:47 -0400 Subject: [PATCH] fix: address PR review 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. --- Cargo.lock | 48 ---------- FULL_HELP_DOCS.md | 4 +- .../soroban-test/tests/it/integration/tx.rs | 9 +- cmd/soroban-cli/Cargo.toml | 1 - cmd/soroban-cli/src/commands/tx/hash.rs | 2 +- cmd/soroban-cli/src/commands/tx/mod.rs | 2 +- cmd/soroban-cli/src/commands/tx/sign.rs | 13 ++- cmd/soroban-cli/src/config/secret.rs | 49 ++++++++--- cmd/soroban-cli/src/config/sign_with.rs | 36 ++------ cmd/soroban-cli/src/print.rs | 3 +- cmd/soroban-cli/src/signer.rs | 2 +- cmd/soroban-cli/src/signer/types.rs | 87 +++++-------------- cmd/soroban-cli/src/utils.rs | 40 +-------- 13 files changed, 85 insertions(+), 211 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 4cfcf436b..788fc8e59 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -878,31 +878,6 @@ version = "0.8.20" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "22ec99545bb0ed0ea7bb9b8e1e9122ea386ff8a48c0922e43f36d45ab09e0e80" -[[package]] -name = "crossterm" -version = "0.28.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "829d955a0bb380ef178a640b91779e3987da38c9aea133b20614cfed8cdea9c6" -dependencies = [ - "bitflags 2.6.0", - "crossterm_winapi", - "mio", - "parking_lot", - "rustix 0.38.34", - "signal-hook", - "signal-hook-mio", - "winapi", -] - -[[package]] -name = "crossterm_winapi" -version = "0.9.1" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "acdd7c62a3665c7f6830a51635d9ac9b23ed385797f70a83bb8bafe9c572ab2b" -dependencies = [ - "winapi", -] - [[package]] name = "crunchy" version = "0.2.2" @@ -3323,7 +3298,6 @@ checksum = "4569e456d394deccd22ce1c1913e6ea0e54519f577285001215d33557431afe4" dependencies = [ "hermit-abi 0.3.9", "libc", - "log", "wasi", "windows-sys 0.52.0", ] @@ -4571,27 +4545,6 @@ version = "1.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "0fda2ff0d084019ba4d7c6f371c95d8fd75ce3524c3cb8fb653a3023f6323e64" -[[package]] -name = "signal-hook" -version = "0.3.17" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "8621587d4798caf8eb44879d42e56b9a93ea5dcd315a6487c357130095b62801" -dependencies = [ - "libc", - "signal-hook-registry", -] - -[[package]] -name = "signal-hook-mio" -version = "0.2.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "34db1a06d485c9142248b7a054f034b349b212551f3dfd19c94d45a754a217cd" -dependencies = [ - "libc", - "mio", - "signal-hook", -] - [[package]] name = "signal-hook-registry" version = "1.4.2" @@ -4737,7 +4690,6 @@ dependencies = [ "clap-markdown", "clap_complete", "crate-git-revision 0.0.4", - "crossterm", "csv", "directories", "dirs", diff --git a/FULL_HELP_DOCS.md b/FULL_HELP_DOCS.md index ecd5c97cf..b0ecf7514 100644 --- a/FULL_HELP_DOCS.md +++ b/FULL_HELP_DOCS.md @@ -1331,14 +1331,12 @@ Calculate the hash of a transaction envelope from stdin Sign a transaction envolope appending the signature to the envelope -**Usage:** `stellar tx sign [OPTIONS] --source-account ` +**Usage:** `stellar tx sign [OPTIONS]` ###### **Options:** * `--sign-with-key ` — Sign with a local key. Can be an identity (--sign-with-key alice), a secret key (--sign-with-key SC36…), or a seed phrase (--sign-with-key "kite urban…"). If using seed phrase, `--hd-path` defaults to the `0` path * `--hd-path ` — If using a seed phrase to sign, sets which hierarchical deterministic path to use, e.g. `m/44'/148'/{hd_path}`. Example: `--hd-path 1`. Default: `0` -* `--yes` — If one of `--sign-with-*` flags is provided, don't ask to confirm to sign a transaction -* `--source-account ` — Account that signs the transaction. Alias `source`. Can be an identity (--source alice), a secret key (--source SC36…), or a seed phrase (--source "kite urban…") * `--rpc-url ` — RPC server endpoint * `--network-passphrase ` — Network passphrase to sign the transaction sent to the rpc server * `--network ` — Name of network to use from config diff --git a/cmd/crates/soroban-test/tests/it/integration/tx.rs b/cmd/crates/soroban-test/tests/it/integration/tx.rs index 5acbff9a9..1eab10684 100644 --- a/cmd/crates/soroban-test/tests/it/integration/tx.rs +++ b/cmd/crates/soroban-test/tests/it/integration/tx.rs @@ -1,3 +1,4 @@ +use soroban_rpc::GetTransactionResponse; use soroban_sdk::xdr::{Limits, ReadXdr, TransactionEnvelope, WriteXdr}; use soroban_test::{AssertExt, TestEnv}; @@ -71,14 +72,12 @@ async fn send() { ); let rpc_result = send_manually(sandbox, &tx_env).await; - - println!("Transaction sent: {rpc_result}"); + assert_eq!(rpc_result.status, "SUCCESS"); } -async fn send_manually(sandbox: &TestEnv, tx_env: &TransactionEnvelope) -> String { +async fn send_manually(sandbox: &TestEnv, tx_env: &TransactionEnvelope) -> GetTransactionResponse { let client = soroban_rpc::Client::new(&sandbox.rpc_url).unwrap(); - let res = client.send_transaction_polling(tx_env).await.unwrap(); - serde_json::to_string_pretty(&res).unwrap() + client.send_transaction_polling(tx_env).await.unwrap() } fn sign_manually(sandbox: &TestEnv, tx_env: &TransactionEnvelope) -> TransactionEnvelope { diff --git a/cmd/soroban-cli/Cargo.toml b/cmd/soroban-cli/Cargo.toml index 67b3f9be4..1f381eb42 100644 --- a/cmd/soroban-cli/Cargo.toml +++ b/cmd/soroban-cli/Cargo.toml @@ -123,7 +123,6 @@ humantime = "2.1.0" phf = { version = "0.11.2", features = ["macros"] } semver = "1.0.0" glob = "0.3.1" -crossterm = "0.28.1" # For hyper-tls [target.'cfg(unix)'.dependencies] diff --git a/cmd/soroban-cli/src/commands/tx/hash.rs b/cmd/soroban-cli/src/commands/tx/hash.rs index 8d8ec6d82..dfffb623e 100644 --- a/cmd/soroban-cli/src/commands/tx/hash.rs +++ b/cmd/soroban-cli/src/commands/tx/hash.rs @@ -1,6 +1,6 @@ use hex; -use crate::{commands::global, config::network, utils::transaction_hash}; +use crate::{commands::global, config::network, signer::types::transaction_hash}; #[derive(thiserror::Error, Debug)] pub enum Error { diff --git a/cmd/soroban-cli/src/commands/tx/mod.rs b/cmd/soroban-cli/src/commands/tx/mod.rs index a64417127..1f57cb79f 100644 --- a/cmd/soroban-cli/src/commands/tx/mod.rs +++ b/cmd/soroban-cli/src/commands/tx/mod.rs @@ -34,7 +34,7 @@ impl Cmd { match self { Cmd::Simulate(cmd) => cmd.run(global_args).await?, Cmd::Hash(cmd) => cmd.run(global_args)?, - Cmd::Sign(cmd) => cmd.run().await?, + Cmd::Sign(cmd) => cmd.run(global_args).await?, }; Ok(()) } diff --git a/cmd/soroban-cli/src/commands/tx/sign.rs b/cmd/soroban-cli/src/commands/tx/sign.rs index 1047a91f7..913422b34 100644 --- a/cmd/soroban-cli/src/commands/tx/sign.rs +++ b/cmd/soroban-cli/src/commands/tx/sign.rs @@ -1,4 +1,5 @@ use crate::{ + commands::global, config::{locator, network, sign_with}, xdr::{self, Limits, TransactionEnvelope, WriteXdr}, }; @@ -30,17 +31,21 @@ pub struct Cmd { impl Cmd { #[allow(clippy::unused_async)] - pub async fn run(&self) -> Result<(), Error> { + pub async fn run(&self, global_args: &global::Args) -> Result<(), Error> { let txn_env = super::xdr::tx_envelope_from_stdin()?; - let envelope = self.sign_tx_env(txn_env).await?; + let envelope = self.sign_tx_env(txn_env, global_args.quiet).await?; println!("{}", envelope.to_xdr_base64(Limits::none())?.trim()); Ok(()) } - pub async fn sign_tx_env(&self, tx: TransactionEnvelope) -> Result { + pub async fn sign_tx_env( + &self, + tx: TransactionEnvelope, + quiet: bool, + ) -> Result { Ok(self .sign_with - .sign_txn_env(tx, &self.locator, &self.network.get(&self.locator)?) + .sign_txn_env(tx, &self.locator, &self.network.get(&self.locator)?, quiet) .await?) } } diff --git a/cmd/soroban-cli/src/config/secret.rs b/cmd/soroban-cli/src/config/secret.rs index fcba3ed77..0293c056a 100644 --- a/cmd/soroban-cli/src/config/secret.rs +++ b/cmd/soroban-cli/src/config/secret.rs @@ -3,11 +3,16 @@ use serde::{Deserialize, Serialize}; use std::{io::Write, str::FromStr}; use stellar_strkey::ed25519::{PrivateKey, PublicKey}; +use crate::print::Print; +use crate::signer::types::transaction_hash; +use crate::xdr::{self, DecoratedSignature}; use crate::{ signer::{self, LocalKey}, utils, }; +use super::network::Network; + #[derive(thiserror::Error, Debug)] pub enum Error { #[error("invalid secret key")] @@ -125,12 +130,21 @@ impl Secret { )?) } - pub fn signer(&self, index: Option, prompt: bool) -> Result { - match self { - Secret::SecretKey { .. } | Secret::SeedPhrase { .. } => Ok(StellarSigner::Local( - LocalKey::new(self.key_pair(index)?, prompt), - )), - } + pub fn signer( + &self, + index: Option, + prompt: bool, + quiet: bool, + ) -> Result { + let kind = match self { + Secret::SecretKey { .. } | Secret::SeedPhrase { .. } => { + SignerKind::Local(LocalKey::new(self.key_pair(index)?, prompt)) + } + }; + Ok(StellarSigner { + kind, + printer: Print::new(quiet), + }) } pub fn key_pair(&self, index: Option) -> Result { @@ -153,15 +167,28 @@ impl Secret { } } -pub enum StellarSigner { +pub struct StellarSigner { + kind: SignerKind, + printer: Print, +} + +pub enum SignerKind { Local(LocalKey), } #[async_trait::async_trait] -impl signer::Blob for StellarSigner { - async fn sign_blob(&self, blob: &[u8]) -> Result, signer::types::Error> { - match self { - StellarSigner::Local(signer) => signer.sign_blob(blob).await, +impl signer::Transaction for StellarSigner { + async fn sign_txn( + &self, + txn: &xdr::Transaction, + network: &Network, + ) -> Result { + let tx_hash = transaction_hash(txn, &network.network_passphrase)?; + let hex_hash = hex::encode(tx_hash); + self.printer + .infoln(format!("Signing transaction with hash: {hex_hash}")); + match &self.kind { + SignerKind::Local(key) => key.sign_txn(txn, network).await, } } } diff --git a/cmd/soroban-cli/src/config/sign_with.rs b/cmd/soroban-cli/src/config/sign_with.rs index b0eb0900d..a2bc796bc 100644 --- a/cmd/soroban-cli/src/config/sign_with.rs +++ b/cmd/soroban-cli/src/config/sign_with.rs @@ -6,7 +6,6 @@ use crate::{ xdr::TransactionEnvelope, }; use clap::arg; -use stellar_strkey::ed25519::PublicKey; use super::{ locator, @@ -36,11 +35,7 @@ pub enum Error { #[group(skip)] pub struct Args { /// Sign with a local key. Can be an identity (--sign-with-key alice), a secret key (--sign-with-key SC36…), or a seed phrase (--sign-with-key "kite urban…"). If using seed phrase, `--hd-path` defaults to the `0` path. - #[arg( - long, - conflicts_with = "sign_with_lab", - env = "STELLAR_SIGN_WITH_KEY" - )] + #[arg(long, conflicts_with = "sign_with_lab", env = "STELLAR_SIGN_WITH_KEY")] pub sign_with_key: Option, /// Sign with labratory #[arg( @@ -54,22 +49,11 @@ pub struct Args { #[arg(long, conflicts_with = "sign_with_lab")] /// If using a seed phrase to sign, sets which hierarchical deterministic path to use, e.g. `m/44'/148'/{hd_path}`. Example: `--hd-path 1`. Default: `0` pub hd_path: Option, - - /// If one of `--sign-with-*` flags is provided, don't ask to confirm to sign a transaction - #[arg(long)] - pub yes: bool, - - /// Account that signs the transaction. Alias `source`. Can be an identity (--source alice), a secret key (--source SC36…), or a seed phrase (--source "kite urban…"). - #[arg(long, visible_alias = "source", env = "STELLAR_ACCOUNT")] - pub source_account: String, } impl Args { pub fn secret(&self, locator: &locator::Args) -> Result { - let account = self - .sign_with_key - .as_deref() - .unwrap_or(&self.source_account); + let account = self.sign_with_key.as_deref().ok_or(Error::NoSignWithKey)?; Ok(locator.account(account)?) } @@ -78,27 +62,19 @@ impl Args { tx: TransactionEnvelope, locator: &locator::Args, network: &Network, + quiet: bool, ) -> Result { let secret = self.secret(locator)?; - let signer = secret.signer(self.hd_path, !self.yes)?; - let source_account = self.source_account(locator)?; - self.sign_tx_env_with_signer(&signer, &source_account, tx, network) - .await + let signer = secret.signer(self.hd_path, false, quiet)?; + self.sign_tx_env_with_signer(&signer, tx, network).await } pub async fn sign_tx_env_with_signer( &self, signer: &(impl Transaction + std::marker::Sync), - source_account: &PublicKey, tx_env: TransactionEnvelope, network: &Network, ) -> Result { - Ok(sign_txn_env(signer, source_account, tx_env, &network).await?) - } - - pub fn source_account(&self, locator: &locator::Args) -> Result { - Ok(locator - .account(&self.source_account)? - .public_key(self.hd_path)?) + Ok(sign_txn_env(signer, tx_env, network).await?) } } diff --git a/cmd/soroban-cli/src/print.rs b/cmd/soroban-cli/src/print.rs index f772eb649..2a95267d0 100644 --- a/cmd/soroban-cli/src/print.rs +++ b/cmd/soroban-cli/src/print.rs @@ -3,8 +3,7 @@ use std::{env, fmt::Display}; use soroban_env_host::xdr::{Error as XdrError, Transaction}; use crate::{ - config::network::Network, - utils::{explorer_url_for_transaction, transaction_hash}, + config::network::Network, signer::types::transaction_hash, utils::explorer_url_for_transaction, }; const TERMS: &[&str] = &["Apple_Terminal", "vscode"]; diff --git a/cmd/soroban-cli/src/signer.rs b/cmd/soroban-cli/src/signer.rs index 99dc3038f..4f66a93c4 100644 --- a/cmd/soroban-cli/src/signer.rs +++ b/cmd/soroban-cli/src/signer.rs @@ -11,7 +11,7 @@ use soroban_env_host::xdr::{ }; pub mod types; -pub use types::{Blob, LocalKey, Transaction, TransactionHash}; +pub use types::{LocalKey, Transaction, TransactionHash}; #[derive(thiserror::Error, Debug)] pub enum Error { diff --git a/cmd/soroban-cli/src/signer/types.rs b/cmd/soroban-cli/src/signer/types.rs index 0f619015e..12ba97ddd 100644 --- a/cmd/soroban-cli/src/signer/types.rs +++ b/cmd/soroban-cli/src/signer/types.rs @@ -1,4 +1,3 @@ -use crossterm::event::{read, Event, KeyCode}; use ed25519_dalek::ed25519::signature::Signer; use sha2::{Digest, Sha256}; @@ -33,7 +32,7 @@ pub enum Error { pub fn transaction_hash( txn: &xdr::Transaction, network_passphrase: &str, -) -> Result<[u8; 32], Error> { +) -> Result<[u8; 32], xdr::Error> { let signature_payload = TransactionSignaturePayload { network_id: hash(network_passphrase), tagged_transaction: TransactionSignaturePayloadTaggedTransaction::Tx(txn.clone()), @@ -42,46 +41,15 @@ pub fn transaction_hash( Ok(hash) } -/// A trait for signing arbitrary byte arrays -#[async_trait::async_trait] -pub trait Blob { - /// Sign an abritatry byte array - async fn sign_blob(&self, blob: &[u8]) -> Result, Error>; -} - #[async_trait::async_trait] pub trait TransactionHash { - /// Sign a transaction hash with the given source account + /// Sign a transaction hash with the given signer /// # Errors /// Returns an error if the source account is not found - async fn sign_txn_hash( - &self, - source_account: &stellar_strkey::ed25519::PublicKey, - txn: [u8; 32], - ) -> Result; -} -#[async_trait::async_trait] -impl TransactionHash for T -where - T: Blob + Send + Sync, -{ - async fn sign_txn_hash( - &self, - source_account: &stellar_strkey::ed25519::PublicKey, - txn: [u8; 32], - ) -> Result { - eprintln!( - "{} about to sign hash: {}", - source_account.to_string(), - hex::encode(txn) - ); - let tx_signature = self.sign_blob(&txn).await?; - Ok(DecoratedSignature { - // TODO: remove this unwrap. It's safe because we know the length of the array - hint: SignatureHint(source_account.0[28..].try_into().unwrap()), - signature: Signature(tx_signature.try_into()?), - }) - } + async fn sign_txn_hash(&self, txn: [u8; 32]) -> Result; + + /// Return the signature hint required for a `DecoratedSignature`` + fn hint(&self) -> SignatureHint; } /// A trait for signing Stellar transactions and Soroban authorization entries @@ -95,7 +63,6 @@ pub trait Transaction { /// Returns an error if the source account is not found async fn sign_txn( &self, - source_account: &stellar_strkey::ed25519::PublicKey, txn: &xdr::Transaction, network: &Network, ) -> Result; @@ -108,25 +75,25 @@ where { async fn sign_txn( &self, - source_account: &stellar_strkey::ed25519::PublicKey, txn: &xdr::Transaction, Network { network_passphrase, .. }: &Network, ) -> Result { let hash = transaction_hash(txn, network_passphrase)?; - self.sign_txn_hash(source_account, hash).await + let hint = self.hint(); + let signature = self.sign_txn_hash(hash).await?; + Ok(DecoratedSignature { hint, signature }) } } pub async fn sign_txn_env( signer: &(impl Transaction + std::marker::Sync), - source_account: &stellar_strkey::ed25519::PublicKey, txn_env: TransactionEnvelope, network: &Network, ) -> Result { match txn_env { TransactionEnvelope::Tx(TransactionV1Envelope { tx, signatures }) => { - let decorated_signature = signer.sign_txn(source_account, &tx, network).await?; + let decorated_signature = signer.sign_txn(&tx, network).await?; let mut sigs = signatures.to_vec(); sigs.push(decorated_signature); Ok(TransactionEnvelope::Tx(TransactionV1Envelope { @@ -144,6 +111,7 @@ pub(crate) fn hash(network_passphrase: &str) -> xdr::Hash { pub struct LocalKey { key: ed25519_dalek::SigningKey, + #[allow(dead_code)] prompt: bool, } @@ -154,30 +122,17 @@ impl LocalKey { } #[async_trait::async_trait] -impl Blob for LocalKey { - async fn sign_blob(&self, data: &[u8]) -> Result, Error> { - if self.prompt { - eprintln!("Press 'y' or 'Y' for yes, any other key for no:"); - match read_key() { - 'y' | 'Y' => { - eprintln!("Signing now..."); - } - _ => return Err(Error::UserCancelledSigning), - }; - } - let sig = self.key.sign(data); - Ok(sig.to_bytes().to_vec()) +impl TransactionHash for LocalKey { + async fn sign_txn_hash(&self, txn: [u8; 32]) -> Result { + let sig = self.key.sign(&txn); + Ok(Signature(sig.to_bytes().to_vec().try_into()?)) } -} -pub fn read_key() -> char { - loop { - if let Event::Key(key) = read().unwrap() { - match key.code { - KeyCode::Char(c) => return c, - KeyCode::Esc => return '\x1b', // escape key - _ => (), - } - } + fn hint(&self) -> SignatureHint { + SignatureHint( + self.key.verifying_key().to_bytes()[28..] + .try_into() + .unwrap(), + ) } } diff --git a/cmd/soroban-cli/src/utils.rs b/cmd/soroban-cli/src/utils.rs index f8ebb8b37..75a18d5e1 100644 --- a/cmd/soroban-cli/src/utils.rs +++ b/cmd/soroban-cli/src/utils.rs @@ -1,13 +1,10 @@ -use ed25519_dalek::Signer; use phf::phf_map; use sha2::{Digest, Sha256}; use stellar_strkey::ed25519::PrivateKey; use soroban_env_host::xdr::{ - Asset, ContractIdPreimage, DecoratedSignature, Error as XdrError, Hash, HashIdPreimage, - HashIdPreimageContractId, Limits, ScMap, ScMapEntry, ScVal, Signature, SignatureHint, - Transaction, TransactionEnvelope, TransactionSignaturePayload, - TransactionSignaturePayloadTaggedTransaction, TransactionV1Envelope, WriteXdr, + Asset, ContractIdPreimage, Error as XdrError, Hash, HashIdPreimage, HashIdPreimageContractId, + Limits, ScMap, ScMapEntry, ScVal, WriteXdr, }; pub use soroban_spec_tools::contract as contract_spec; @@ -21,17 +18,6 @@ pub fn contract_hash(contract: &[u8]) -> Result { Ok(Hash(Sha256::digest(contract).into())) } -/// # Errors -/// -/// Might return an error -pub fn transaction_hash(tx: &Transaction, network_passphrase: &str) -> Result<[u8; 32], XdrError> { - let signature_payload = TransactionSignaturePayload { - network_id: Hash(Sha256::digest(network_passphrase).into()), - tagged_transaction: TransactionSignaturePayloadTaggedTransaction::Tx(tx.clone()), - }; - Ok(Sha256::digest(signature_payload.to_xdr(Limits::none())?).into()) -} - static EXPLORERS: phf::Map<&'static str, &'static str> = phf_map! { "Test SDF Network ; September 2015" => "https://stellar.expert/explorer/testnet", "Public Global Stellar Network ; September 2015" => "https://stellar.expert/explorer/public", @@ -49,28 +35,6 @@ pub fn explorer_url_for_contract(network: &Network, contract_id: &str) -> Option .map(|base_url| format!("{base_url}/contract/{contract_id}")) } -/// # Errors -/// -/// Might return an error -pub fn sign_transaction( - key: &ed25519_dalek::SigningKey, - tx: &Transaction, - network_passphrase: &str, -) -> Result { - let tx_hash = transaction_hash(tx, network_passphrase)?; - let tx_signature = key.sign(&tx_hash); - - let decorated_signature = DecoratedSignature { - hint: SignatureHint(key.verifying_key().to_bytes()[28..].try_into()?), - signature: Signature(tx_signature.to_bytes().try_into()?), - }; - - Ok(TransactionEnvelope::Tx(TransactionV1Envelope { - tx: tx.clone(), - signatures: vec![decorated_signature].try_into()?, - })) -} - /// # Errors /// /// Might return an error