From a2d0983ccc845e0c0dc609fe30c519cdb1afa2af Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Tue, 19 Nov 2024 13:54:01 +0000 Subject: [PATCH 1/5] test/int/ledger: test wrapping a raw tx by elsewho --- crates/tests/src/integration/ledger_tests.rs | 212 +++++++++++++++++++ 1 file changed, 212 insertions(+) diff --git a/crates/tests/src/integration/ledger_tests.rs b/crates/tests/src/integration/ledger_tests.rs index 7649c72413..1b3d97eb62 100644 --- a/crates/tests/src/integration/ledger_tests.rs +++ b/crates/tests/src/integration/ledger_tests.rs @@ -24,6 +24,7 @@ use namada_node::shell::SnapshotSync; use namada_node::storage::DbSnapshot; use namada_sdk::account::AccountPublicKeysMap; use namada_sdk::collections::HashMap; +use namada_sdk::error::TxSubmitError; use namada_sdk::migrations; use namada_sdk::queries::RPC; use namada_sdk::token::{self, DenominatedAmount}; @@ -2353,6 +2354,217 @@ fn scheduled_migration() -> Result<()> { Ok(()) } +/// Test that a raw transaction can be wrapped and signed by someone else who +/// can pay for the gas fees for this tx. +/// +/// 1. Create a new account +/// 2. Credit the new account with some tokens to reveal its PK and have tiny +/// amount left +/// 3. Reveal the PK of the new account +/// 4. Check that the new account doesn't have sufficient balance to submit a +/// transfer tx +/// 5. Dump a raw tx of a transfer from the new account +/// 6. Sign the raw transaction +/// 7. Wrap the raw transaction by another account and submit it +#[test] +fn wrap_tx_by_elsewho() -> Result<()> { + let (node, _services) = setup::setup()?; + + // 1. Create a new account + let key_alias = "new-account"; + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Wallet, + apply_use_device(vec![ + "gen", + "--alias", + key_alias, + "--unsafe-dont-encrypt", + ]), + ) + }); + assert!(captured.result.is_ok()); + + // 2. Credit the new account with some tokens to reveal its PK and have tiny + // amount left + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + apply_use_device(vec![ + "transparent-transfer", + "--source", + ALBERT, + "--target", + key_alias, + "--token", + NAM, + "--amount", + // transfer enough to cover reveal-pk gas fees (0.5) and to + // have only 0.000001 left after + "0.500001", + ]), + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains(TX_APPLIED_SUCCESS)); + + // 3. Reveal the PK of the new account + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + apply_use_device(vec!["reveal-pk", "--public-key", key_alias]), + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains(TX_APPLIED_SUCCESS)); + + // Assert that there's only the smallest possible non-zero amount left + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec!["balance", "--owner", key_alias, "--token", NAM], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains("nam: 0.000001")); + + // 4. Check that the new account doesn't have sufficient balance to submit a + // transfer tx + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + apply_use_device(vec![ + "transparent-transfer", + "--source", + key_alias, + "--target", + ALBERT, + "--token", + NAM, + "--amount", + "0.000001", + ]), + ) + }); + assert!(captured.result.is_err()); + assert_matches!( + captured + .result + .unwrap_err() + .downcast_ref::() + .unwrap(), + namada_sdk::error::Error::Tx(TxSubmitError::BalanceTooLowForFees( + _, + _, + _, + _ + )) + ); + + // 5. Dump a raw tx of a transfer from the new account + let output_folder = node.test_dir.path(); + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + apply_use_device(vec![ + "transparent-transfer", + "--source", + key_alias, + "--target", + ALBERT, + "--token", + NAM, + "--amount", + "0.000001", + "--gas-payer", + CHRISTEL_KEY, + "--dump-tx", + "--output-folder-path", + &output_folder.to_str().unwrap(), + ]), + ) + }); + assert!(captured.result.is_ok()); + + let tx = find_files_with_ext(output_folder, "tx") + .unwrap() + .first() + .expect("Offline tx should be found.") + .to_path_buf() + .display() + .to_string(); + + // 6. Sign the raw transaction + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + apply_use_device(vec![ + "utils", + "sign-offline", + "--data-path", + &tx, + "--secret-keys", + &key_alias, + "--output-folder-path", + &output_folder.to_str().unwrap(), + ]), + ) + }); + assert!(captured.result.is_ok()); + + let sig_files = find_files_with_ext(output_folder, "sig").unwrap(); + assert_eq!(sig_files.len(), 1); + let offline_sig = sig_files.first().unwrap().to_str().unwrap(); + + // 7. Wrap the raw transaction by another account and submit it + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + apply_use_device(vec![ + "tx", + "--tx-path", + &tx, + "--signatures", + &offline_sig, + "--signing-keys", + CHRISTEL_KEY, + ]), + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains(TX_APPLIED_SUCCESS)); + + // Assert changed balances + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec!["balance", "--owner", ALBERT, "--token", NAM], + ) + }); + assert!(captured.contains("nam: 1979999.5\n")); + + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + vec!["balance", "--owner", key_alias, "--token", NAM], + ) + }); + assert!(captured.result.is_ok()); + assert!(captured.contains("nam: 0\n")); + + Ok(()) +} + fn make_migration_json() -> (Hash, tempfile::NamedTempFile) { let file = tempfile::Builder::new().tempfile().expect("Test failed"); let updates = [migrations::DbUpdateType::Add { From 3e6a07578a4cd52edcec8b1db41bb702a9689c8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Wed, 20 Nov 2024 11:05:24 +0000 Subject: [PATCH 2/5] cli/tx: allow to use `--signatures` together with `--signing-keys` --- crates/apps_lib/src/cli.rs | 28 ++++++++-------------------- 1 file changed, 8 insertions(+), 20 deletions(-) diff --git a/crates/apps_lib/src/cli.rs b/crates/apps_lib/src/cli.rs index 1df8c79cd5..f364c5b1a6 100644 --- a/crates/apps_lib/src/cli.rs +++ b/crates/apps_lib/src/cli.rs @@ -7546,26 +7546,14 @@ pub mod args { )) .conflicts_with_all([EXPIRATION_OPT.name]), ) - .arg( - SIGNING_KEYS - .def() - .help(wrap!( - "Sign the transaction with the key for the given \ - public key, public key hash or alias from your \ - wallet." - )) - .conflicts_with_all([SIGNATURES.name]), - ) - .arg( - SIGNATURES - .def() - .help(wrap!( - "List of file paths containing a serialized signature \ - to be attached to a transaction. Requires to provide \ - a gas payer." - )) - .conflicts_with_all([SIGNING_KEYS.name]), - ) + .arg(SIGNING_KEYS.def().help(wrap!( + "Sign the transaction with the key for the given public key, \ + public key hash or alias from your wallet." + ))) + .arg(SIGNATURES.def().help(wrap!( + "List of file paths containing a serialized signature to be \ + attached to a transaction. Requires to provide a gas payer." + ))) .arg( WRAPPER_SIGNATURE_OPT .def() From bf217aaffffbc51be426da31afc6c17957385b84 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Wed, 20 Nov 2024 11:08:51 +0000 Subject: [PATCH 3/5] changelog: add #4055 --- .changelog/unreleased/bug-fixes/4055-wrap-tx-by-elsewho.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/bug-fixes/4055-wrap-tx-by-elsewho.md diff --git a/.changelog/unreleased/bug-fixes/4055-wrap-tx-by-elsewho.md b/.changelog/unreleased/bug-fixes/4055-wrap-tx-by-elsewho.md new file mode 100644 index 0000000000..f363190c16 --- /dev/null +++ b/.changelog/unreleased/bug-fixes/4055-wrap-tx-by-elsewho.md @@ -0,0 +1,2 @@ +- CLI: Allow to wrap a raw tx signed by someone else. + ([\#4055](https://github.com/anoma/namada/pull/4055)) \ No newline at end of file From d69279fa4df049dfc844063b89c8b10c9a99c46c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Wed, 20 Nov 2024 16:00:44 +0000 Subject: [PATCH 4/5] cli/tx: don't require signers if signatures are already provided --- crates/sdk/src/signing.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/crates/sdk/src/signing.rs b/crates/sdk/src/signing.rs index c5f3da2b0f..b0ff3ff119 100644 --- a/crates/sdk/src/signing.rs +++ b/crates/sdk/src/signing.rs @@ -170,11 +170,15 @@ pub async fn tx_signers( args: &args::Tx, default: Option
, ) -> Result, Error> { - let signer = if !&args.signing_keys.is_empty() { + let signer = if !args.signing_keys.is_empty() { return Ok(args.signing_keys.clone()); - } else { + } else if args.signatures.is_empty() { // Otherwise use the signer determined by the caller default + } else { + // If explicit signature(s) are provided signing keys are not required + // anymore + return Ok(vec![]); }; // Now actually fetch the signing key and apply it From dc0904f5e04dc7baefda909344064bd1286911ab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tom=C3=A1=C5=A1=20Zemanovi=C4=8D?= Date: Wed, 20 Nov 2024 16:01:29 +0000 Subject: [PATCH 5/5] test/int/ledger_tests: use a `--gas-payer` instead of `--signing-keys` --- crates/tests/src/integration/ledger_tests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/tests/src/integration/ledger_tests.rs b/crates/tests/src/integration/ledger_tests.rs index 1b3d97eb62..0a5a39cc01 100644 --- a/crates/tests/src/integration/ledger_tests.rs +++ b/crates/tests/src/integration/ledger_tests.rs @@ -2534,7 +2534,7 @@ fn wrap_tx_by_elsewho() -> Result<()> { &tx, "--signatures", &offline_sig, - "--signing-keys", + "--gas-payer", CHRISTEL_KEY, ]), )