diff --git a/.changelog/unreleased/bug-fixes/4120-fix-offline-wrapper-sig.md b/.changelog/unreleased/bug-fixes/4120-fix-offline-wrapper-sig.md new file mode 100644 index 0000000000..842cda5abb --- /dev/null +++ b/.changelog/unreleased/bug-fixes/4120-fix-offline-wrapper-sig.md @@ -0,0 +1,3 @@ +- Fixed a bug that was preventing generating wrapper signatures + offline. Removed a redundant `sign-tx` client command. + ([\#4120](https://github.com/anoma/namada/pull/4120)) \ No newline at end of file diff --git a/crates/apps_lib/src/cli.rs b/crates/apps_lib/src/cli.rs index 36e0de0d7c..9949b6e98b 100644 --- a/crates/apps_lib/src/cli.rs +++ b/crates/apps_lib/src/cli.rs @@ -391,7 +391,6 @@ pub mod cmds { let query_metadata = Self::parse_with_ctx(matches, QueryMetaData); let add_to_eth_bridge_pool = Self::parse_with_ctx(matches, AddToEthBridgePool); - let sign_tx = Self::parse_with_ctx(matches, SignTx); let shielded_sync = Self::parse_with_ctx(matches, ShieldedSync); let gen_ibc_shielding = Self::parse_with_ctx(matches, GenIbcShieldingTransfer); @@ -451,7 +450,6 @@ pub mod cmds { .or(query_native_supply) .or(query_staking_rewards_rate) .or(query_account) - .or(sign_tx) .or(shielded_sync) .or(gen_ibc_shielding) .or(utils) @@ -546,7 +544,6 @@ pub mod cmds { QueryPgf(QueryPgf), QueryValidatorState(QueryValidatorState), QueryRewards(QueryRewards), - SignTx(SignTx), ShieldedSync(ShieldedSync), GenIbcShieldingTransfer(GenIbcShieldingTransfer), } @@ -7573,7 +7570,6 @@ pub mod args { "The file path containing a serialized signature of \ the entire transaction for gas payment." )) - .requires(SIGNATURES.name) .conflicts_with(FEE_PAYER_OPT.name), ) .arg(OUTPUT_FOLDER_PATH.def().help(wrap!( diff --git a/crates/apps_lib/src/cli/client.rs b/crates/apps_lib/src/cli/client.rs index 6542a55b63..e54f45beb5 100644 --- a/crates/apps_lib/src/cli/client.rs +++ b/crates/apps_lib/src/cli/client.rs @@ -796,18 +796,6 @@ impl CliApi { let namada = ctx.to_sdk(client, io); rpc::query_account(&namada, args).await; } - Sub::SignTx(SignTx(args)) => { - let chain_ctx = ctx.borrow_mut_chain_or_exit(); - let ledger_address = - chain_ctx.get(&args.tx.ledger_address); - let client = client.unwrap_or_else(|| { - C::from_tendermint_address(&ledger_address) - }); - client.wait_until_node_is_synced(&io).await?; - let args = args.to_sdk(&mut ctx)?; - let namada = ctx.to_sdk(client, io); - tx::sign_tx(&namada, args).await?; - } } } cli::NamadaClient::WithoutContext(cmd_box) => { diff --git a/crates/apps_lib/src/client/tx.rs b/crates/apps_lib/src/client/tx.rs index 55871f0e40..8f1ee930c4 100644 --- a/crates/apps_lib/src/client/tx.rs +++ b/crates/apps_lib/src/client/tx.rs @@ -283,9 +283,27 @@ pub async fn submit_custom( where ::Error: std::fmt::Display, { - let (tx, signing_data) = args.build(namada).await?; + let (mut tx, signing_data) = args.build(namada).await?; + + if args.tx.dump_tx { + return tx::dump_tx(namada.io(), &args.tx, tx); + } + if args.tx.dump_wrapper_tx { + // Attach the provided inner signatures to the tx (if any) + let signatures = args + .tx + .signatures + .iter() + .map(|bytes| { + tx::SignatureIndex::try_from_json_bytes(bytes).map_err(|err| { + error::Error::Encode(error::EncodingError::Serde( + err.to_string(), + )) + }) + }) + .collect::>>()?; + tx.add_signatures(signatures); - if args.tx.dump_tx || args.tx.dump_wrapper_tx { return tx::dump_tx(namada.io(), &args.tx, tx); } @@ -1139,82 +1157,6 @@ where Ok(()) } -pub async fn sign_tx( - namada: &N, - args::SignTx { - tx: tx_args, - tx_data, - owner, - }: args::SignTx, -) -> Result<(), error::Error> -where - ::Error: std::fmt::Display, -{ - let tx = if let Ok(transaction) = Tx::try_from_json_bytes(tx_data.as_ref()) - { - transaction - } else { - edisplay_line!(namada.io(), "Couldn't decode the transaction."); - return Err(error::Error::Other( - "Couldn't decode the transaction.".to_string(), - )); - }; - - let default_signer = Some(owner.clone()); - let signing_data = aux_signing_data( - namada, - &tx_args, - Some(owner.clone()), - default_signer, - false, - ) - .await?; - - let mut wallet = namada.wallet_mut().await; - - let mut secret_keys = vec![]; - for public_key in &signing_data.public_keys { - let secret_key = - signing::find_key_by_pk(&mut wallet, &tx_args, public_key)?; - secret_keys.push(secret_key); - } - - if let Some(account_public_keys_map) = signing_data.account_public_keys_map - { - let signatures = tx.compute_section_signature( - &secret_keys, - &account_public_keys_map, - Some(owner), - ); - - for signature in &signatures { - let filename = format!( - "offline_signature_{}_{}.tx", - tx.raw_header_hash(), - signature.pubkey, - ); - let output_path = match &tx_args.output_folder { - Some(path) => path.join(filename), - None => filename.into(), - }; - let signature_path = File::create(&output_path) - .expect("Should be able to create signature file."); - signature.to_writer_json(signature_path).expect( - "Signature should be serializable and the file writeable.", - ); - - display_line!( - namada.io(), - "Signature for {} serialized at {}", - signature.pubkey, - output_path.display() - ); - } - } - - Ok(()) -} - pub async fn submit_reveal_pk( namada: &N, args: args::RevealPk, diff --git a/crates/apps_lib/src/client/utils.rs b/crates/apps_lib/src/client/utils.rs index f75bc3b6f1..4c3fcb0ea0 100644 --- a/crates/apps_lib/src/client/utils.rs +++ b/crates/apps_lib/src/client/utils.rs @@ -1099,6 +1099,7 @@ pub async fn sign_offline( if tx.header.wrapper().is_some() { // Wrapper signature must be computed over the raw signatures too tx.add_signatures(signatures); + tx.protocol_filter(); let wrapper_signature = Authorization::new( tx.sechashes(), [(0, wrapper_signer)].into_iter().collect(), @@ -1124,10 +1125,10 @@ pub async fn sign_offline( println!("Wrapper signature serialized at {}", signature_path); } else { - println!( - "A gas payer was provided but the transaction is not a \ - wrapper: skipping the wrapper signature" + eprintln!( + "A gas payer was provided but the transaction is not a wrapper" ); + safe_exit(1); } } } diff --git a/crates/sdk/src/tx.rs b/crates/sdk/src/tx.rs index 17eb672cfa..678926865b 100644 --- a/crates/sdk/src/tx.rs +++ b/crates/sdk/src/tx.rs @@ -3757,8 +3757,15 @@ pub async fn build_custom( let signing_data = if let Some(wrapper_signature) = &tx_args.wrapper_signature { + if tx.header.wrapper().is_none() { + return Err(Error::Other( + "A wrapper signature was provided but the transaction is not \ + a wrapper" + .to_string(), + )); + } // Attach the provided signatures to the tx without the need to produce - // any mroe signatures + // any more signatures let signatures = tx_args.signatures.iter().try_fold( vec![], |mut acc, bytes| -> Result> { diff --git a/crates/tests/src/integration/ledger_tests.rs b/crates/tests/src/integration/ledger_tests.rs index d609aaddd7..c447ed2701 100644 --- a/crates/tests/src/integration/ledger_tests.rs +++ b/crates/tests/src/integration/ledger_tests.rs @@ -2483,8 +2483,8 @@ fn wrap_tx_by_elsewho() -> Result<()> { NAM, "--amount", "0.000001", - "--gas-payer", - CHRISTEL_KEY, + // Force to ignore the balance check + "--force", "--dump-tx", "--output-folder-path", &output_folder.to_str().unwrap(), @@ -2566,6 +2566,514 @@ fn wrap_tx_by_elsewho() -> 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 +/// 8. Offline sign the wrapper +/// 9. Load the dumped wrapper with the signatures and submit it +#[test] +fn offline_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", + "--force", + "--dump-tx", + "--output-folder-path", + &output_folder.to_str().unwrap(), + ]), + ) + }); + assert!(captured.result.is_ok()); + + let tx_path_buf = find_files_with_ext(output_folder, "tx") + .unwrap() + .first() + .expect("Offline tx should be found.") + .to_owned(); + let tx = tx_path_buf.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_path = sig_files.first().unwrap(); + let offline_sig = offline_sig_path.to_str().unwrap(); + + // 7. Wrap the raw transaction by another account and dump the wrapper + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + apply_use_device(vec![ + "tx", + "--tx-path", + &tx, + "--signatures", + &offline_sig, + "--gas-payer", + CHRISTEL_KEY, + "--dump-wrapper-tx", + "--output-folder-path", + &output_folder.to_str().unwrap(), + ]), + ) + }); + assert!(captured.result.is_ok()); + let wrapper_tx = find_files_with_ext(output_folder, "tx") + .unwrap() + .into_iter() + .find(|wrapper_tx| wrapper_tx != &tx_path_buf) + .expect("Offline wrapper tx should be found.") + .to_path_buf() + .display() + .to_string(); + + // 8. Sign the wrapper offline + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + apply_use_device(vec![ + "utils", + "sign-offline", + "--data-path", + &wrapper_tx, + "--secret-key", + CHRISTEL_KEY, + "--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(), 2); + let offline_wrapper_sig = sig_files + .into_iter() + .find(|wrapper_sig| wrapper_sig != offline_sig_path) + .unwrap() + .to_str() + .unwrap() + .to_owned(); + + // 9. Submit the wrapped tx + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + apply_use_device(vec![ + "tx", + "--tx-path", + &wrapper_tx, + // We've attached the inner signatures to the tx when we + // wrapped it, so we only need to provide the wrapper signature + // here + "--gas-signature", + &offline_wrapper_sig, + ]), + ) + }); + 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(()) +} + +/// Test that a wrapper transaction can be dumped and later signed offline. +/// +/// 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 wrapper tx of a transfer from the new account +/// 6. Sign both the wrapper and raw transaction +/// 7. Load the dumped wrapper with the signatures and submit it +#[test] +fn offline_wrapper_tx() -> 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 wrapper 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-wrapper-tx", + "--output-folder-path", + &output_folder.to_str().unwrap(), + ]), + ) + }); + assert!(captured.result.is_ok()); + + let tx_path_buf = find_files_with_ext(output_folder, "tx") + .unwrap() + .first() + .expect("Offline tx should be found.") + .to_owned(); + let tx = tx_path_buf.to_path_buf().display().to_string(); + + // 6. Sign both the wrapper and raw transaction + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + apply_use_device(vec![ + "utils", + "sign-offline", + "--data-path", + &tx, + "--secret-keys", + &key_alias, + "--secret-key", + CHRISTEL_KEY, + "--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(), 2); + + let offline_sig = sig_files + .iter() + .find(|path| { + path.file_name() + .unwrap() + .to_str() + .unwrap() + .contains("offline_signature") + }) + .unwrap() + .to_str() + .unwrap() + .to_owned(); + let offline_wrapper_sig = sig_files + .iter() + .find(|path| { + path.file_name() + .unwrap() + .to_str() + .unwrap() + .contains("offline_wrapper_signature") + }) + .unwrap() + .to_str() + .unwrap() + .to_owned(); + + // 7. Submit the wrapped tx + let captured = CapturedOutput::of(|| { + run( + &node, + Bin::Client, + apply_use_device(vec![ + "tx", + "--tx-path", + &tx, + "--signatures", + &offline_sig, + "--gas-signature", + &offline_wrapper_sig, + ]), + ) + }); + 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(()) +} + /// Test for PoS validator metadata validation. /// /// 1. Run the ledger node.