From cd02987cffb34a0be3e4cabc158912c71dc5d2c3 Mon Sep 17 00:00:00 2001 From: jowparks Date: Mon, 22 Jan 2024 13:00:03 -0800 Subject: [PATCH] moves change notes construction inside of `build` function of `ProposedTransaction`. Makes it so no new method is required for build layer (#4572) --- ironfish-rust-nodejs/index.d.ts | 2 +- ironfish-rust-nodejs/src/structs/transaction.rs | 6 ++++++ ironfish-rust/src/transaction/mod.rs | 13 ++++++++++--- ironfish-rust/src/transaction/outputs.rs | 4 ++++ ironfish-rust/src/transaction/tests.rs | 6 ++---- 5 files changed, 23 insertions(+), 8 deletions(-) diff --git a/ironfish-rust-nodejs/index.d.ts b/ironfish-rust-nodejs/index.d.ts index acb5fc40e8..4514adfd9a 100644 --- a/ironfish-rust-nodejs/index.d.ts +++ b/ironfish-rust-nodejs/index.d.ts @@ -223,7 +223,7 @@ export class Transaction { * aka: self.value_balance - intended_transaction_fee - change = 0 */ post(spenderHexKey: string, changeGoesTo: string | undefined | null, intendedTransactionFee: bigint): Buffer - build(proofGenerationKeyStr: string, viewKeyStr: string, outgoingViewKeyStr: string, publicAddressStr: string, intendedTransactionFee: bigint): Buffer + build(proofGenerationKeyStr: string, viewKeyStr: string, outgoingViewKeyStr: string, publicAddressStr: string, intendedTransactionFee: bigint, changeGoesTo?: string | undefined | null): Buffer setExpiration(sequence: number): void } export class FoundBlockResult { diff --git a/ironfish-rust-nodejs/src/structs/transaction.rs b/ironfish-rust-nodejs/src/structs/transaction.rs index 223655aeb0..97d181fe01 100644 --- a/ironfish-rust-nodejs/src/structs/transaction.rs +++ b/ironfish-rust-nodejs/src/structs/transaction.rs @@ -321,6 +321,7 @@ impl NativeTransaction { outgoing_view_key_str: String, public_address_str: String, intended_transaction_fee: BigInt, + change_goes_to: Option, ) -> Result { let view_key = ViewKey::from_hex(&view_key_str).map_err(to_napi_err)?; let outgoing_view_key = @@ -328,6 +329,10 @@ impl NativeTransaction { let public_address = PublicAddress::from_hex(&public_address_str).map_err(to_napi_err)?; let proof_generation_key = ProofGenerationKey::from_hex(&proof_generation_key_str) .map_err(|_| to_napi_err("PublicKeyPackage hex to bytes failed"))?; + let change_address = match change_goes_to { + Some(address) => Some(PublicAddress::from_hex(&address).map_err(to_napi_err)?), + None => None, + }; let unsigned_transaction = self .transaction .build( @@ -336,6 +341,7 @@ impl NativeTransaction { outgoing_view_key, public_address, intended_transaction_fee.get_i64().0, + change_address, ) .map_err(to_napi_err)?; diff --git a/ironfish-rust/src/transaction/mod.rs b/ironfish-rust/src/transaction/mod.rs index 38912549f0..149fb43dde 100644 --- a/ironfish-rust/src/transaction/mod.rs +++ b/ironfish-rust/src/transaction/mod.rs @@ -191,7 +191,7 @@ impl ProposedTransaction { Ok(()) } - pub fn add_change_notes( + fn add_change_notes( &mut self, change_goes_to: Option, public_address: PublicAddress, @@ -223,7 +223,6 @@ impl ProposedTransaction { change_notes.push(change_note); } } - for change_note in change_notes { self.add_output(change_note)?; } @@ -237,7 +236,14 @@ impl ProposedTransaction { outgoing_view_key: OutgoingViewKey, public_address: PublicAddress, intended_transaction_fee: i64, + change_goes_to: Option, ) -> Result { + // skip adding change notes if this is special case of a miners fee transaction + let is_miners_fee = self.outputs.iter().any(|output| output.get_is_miners_fee()); + if !is_miners_fee { + self.add_change_notes(change_goes_to, public_address, intended_transaction_fee)?; + } + // The public key after randomization has been applied. This is used // during signature verification. Referred to as `rk` in the literature // Calculated from the authorizing key and the public_key_randomness. @@ -330,7 +336,6 @@ impl ProposedTransaction { let public_address = spender_key.public_address(); let i64_fee = i64::try_from(intended_transaction_fee)?; - self.add_change_notes(change_goes_to, public_address, i64_fee)?; let unsigned = self.build( spender_key.sapling_proof_generation_key(), @@ -338,6 +343,7 @@ impl ProposedTransaction { spender_key.outgoing_view_key().clone(), public_address, i64_fee, + change_goes_to, )?; unsigned.sign(spender_key) } @@ -378,6 +384,7 @@ impl ProposedTransaction { spender_key.outgoing_view_key().clone(), spender_key.public_address(), *self.value_balances.fee(), + None, )?; unsigned.sign(spender_key) } diff --git a/ironfish-rust/src/transaction/outputs.rs b/ironfish-rust/src/transaction/outputs.rs index 52d7e6aca1..d7f1690277 100644 --- a/ironfish-rust/src/transaction/outputs.rs +++ b/ironfish-rust/src/transaction/outputs.rs @@ -60,6 +60,10 @@ impl OutputBuilder { self.is_miners_fee = true; } + pub(crate) fn get_is_miners_fee(&self) -> bool { + self.is_miners_fee + } + /// Get the value_commitment from this proof as an edwards Point. /// /// This integrates the value and randomness into a single point, using an diff --git a/ironfish-rust/src/transaction/tests.rs b/ironfish-rust/src/transaction/tests.rs index a8f79fa9da..07f42b5eaf 100644 --- a/ironfish-rust/src/transaction/tests.rs +++ b/ironfish-rust/src/transaction/tests.rs @@ -227,11 +227,8 @@ fn test_proposed_transaction_build() { transaction.add_output(out_note).unwrap(); assert_eq!(transaction.outputs.len(), 1); - let public_address = spender_key.public_address(); + let public_address: crate::PublicAddress = spender_key.public_address(); let intended_fee = 1; - transaction - .add_change_notes(Some(public_address), public_address, intended_fee) - .expect("should be able to add change notes"); let unsigned_transaction = transaction .build( @@ -240,6 +237,7 @@ fn test_proposed_transaction_build() { spender_key.outgoing_view_key().clone(), spender_key.public_address(), intended_fee, + Some(public_address), ) .expect("should be able to build unsigned transaction");