From 71cd4222518dd9de4304396dbbee141e3466cd32 Mon Sep 17 00:00:00 2001 From: Chris Beck Date: Mon, 25 Apr 2022 17:52:09 -0600 Subject: [PATCH] Make `TransactionBuilder::new(` take the fee `Amount` this cleans up the constructor --- android-bindings/src/bindings.rs | 5 +- api/src/convert/tx.rs | 5 +- consensus/service/src/validators.rs | 40 +++--- fog/distribution/src/main.rs | 13 +- fog/sample-paykit/src/client.rs | 8 +- libmobilecoin/src/transaction.rs | 8 +- mobilecoind/src/payments.rs | 11 +- mobilecoind/src/service.rs | 15 ++- transaction/core/test-utils/src/lib.rs | 8 +- transaction/std/src/transaction_builder.rs | 148 +++++++++++++-------- 10 files changed, 152 insertions(+), 109 deletions(-) diff --git a/android-bindings/src/bindings.rs b/android-bindings/src/bindings.rs index 0f39c52cf5..c6aac93220 100644 --- a/android-bindings/src/bindings.rs +++ b/android-bindings/src/bindings.rs @@ -1603,12 +1603,13 @@ pub unsafe extern "C" fn Java_com_mobilecoin_lib_TransactionBuilder_init_1jni( // FIXME #1595: The token id should be a parameter and not hard coded to Mob // here let token_id = Mob::ID; + let fee_amount = Amount::new(Mob::MINIMUM_FEE, token_id); let tx_builder = TransactionBuilder::new_with_box( block_version, - token_id, + fee_amount, fog_resolver.clone(), memo_builder_box, - ); + )?; Ok(env.set_rust_field(obj, RUST_OBJ_FIELD, tx_builder)?) }) diff --git a/api/src/convert/tx.rs b/api/src/convert/tx.rs index 67f98fb120..45459dbf0a 100644 --- a/api/src/convert/tx.rs +++ b/api/src/convert/tx.rs @@ -72,10 +72,11 @@ mod tests { let mut transaction_builder = TransactionBuilder::new( block_version, - Mob::ID, + Amount::new(Mob::MINIMUM_FEE, Mob::ID), MockFogResolver::default(), EmptyMemoBuilder::default(), - ); + ) + .unwrap(); let ring: Vec = minted_outputs.clone(); let public_key = RistrettoPublic::try_from(&minted_outputs[0].public_key).unwrap(); diff --git a/consensus/service/src/validators.rs b/consensus/service/src/validators.rs index 816a24c5c7..3c716979d0 100644 --- a/consensus/service/src/validators.rs +++ b/consensus/service/src/validators.rs @@ -575,10 +575,11 @@ mod combine_tests { let mut transaction_builder = TransactionBuilder::new( block_version, - Mob::ID, + Amount::new(Mob::MINIMUM_FEE, Mob::ID), MockFogResolver::default(), EmptyMemoBuilder::default(), - ); + ) + .unwrap(); transaction_builder.add_input(input_credentials); transaction_builder.set_fee(0).unwrap(); transaction_builder @@ -634,10 +635,11 @@ mod combine_tests { let mut transaction_builder = TransactionBuilder::new( block_version, - Mob::ID, + Amount::new(Mob::MINIMUM_FEE, Mob::ID), MockFogResolver::default(), EmptyMemoBuilder::default(), - ); + ) + .unwrap(); // Create InputCredentials to spend the TxOut. let onetime_private_key = recover_onetime_private_key( @@ -739,10 +741,11 @@ mod combine_tests { let mut transaction_builder = TransactionBuilder::new( block_version, - Mob::ID, + Amount::new(Mob::MINIMUM_FEE, Mob::ID), MockFogResolver::default(), EmptyMemoBuilder::default(), - ); + ) + .unwrap(); transaction_builder.add_input(input_credentials); transaction_builder.set_fee(0).unwrap(); transaction_builder @@ -780,10 +783,11 @@ mod combine_tests { let mut transaction_builder = TransactionBuilder::new( block_version, - Mob::ID, + Amount::new(Mob::MINIMUM_FEE, Mob::ID), MockFogResolver::default(), EmptyMemoBuilder::default(), - ); + ) + .unwrap(); transaction_builder.add_input(input_credentials); transaction_builder.set_fee(0).unwrap(); transaction_builder @@ -844,10 +848,11 @@ mod combine_tests { let mut transaction_builder = TransactionBuilder::new( block_version, - Mob::ID, + Amount::new(Mob::MINIMUM_FEE, Mob::ID), MockFogResolver::default(), EmptyMemoBuilder::default(), - ); + ) + .unwrap(); transaction_builder.add_input(input_credentials); transaction_builder.set_fee(0).unwrap(); transaction_builder @@ -935,10 +940,11 @@ mod combine_tests { let mut transaction_builder = TransactionBuilder::new( block_version, - Mob::ID, + Amount::new(Mob::MINIMUM_FEE, Mob::ID), MockFogResolver::default(), EmptyMemoBuilder::default(), - ); + ) + .unwrap(); transaction_builder.add_input(input_credentials); transaction_builder.set_fee(0).unwrap(); transaction_builder @@ -977,10 +983,11 @@ mod combine_tests { let mut transaction_builder = TransactionBuilder::new( block_version, - Mob::ID, + Amount::new(Mob::MINIMUM_FEE, Mob::ID), MockFogResolver::default(), EmptyMemoBuilder::default(), - ); + ) + .unwrap(); transaction_builder.add_input(input_credentials); transaction_builder.set_fee(0).unwrap(); transaction_builder @@ -1042,10 +1049,11 @@ mod combine_tests { let mut transaction_builder = TransactionBuilder::new( block_version, - Mob::ID, + Amount::new(Mob::MINIMUM_FEE, Mob::ID), MockFogResolver::default(), EmptyMemoBuilder::default(), - ); + ) + .unwrap(); transaction_builder.add_input(input_credentials); transaction_builder.set_fee(0).unwrap(); transaction_builder diff --git a/fog/distribution/src/main.rs b/fog/distribution/src/main.rs index 5bac5b32ab..d7c6c0a8ec 100755 --- a/fog/distribution/src/main.rs +++ b/fog/distribution/src/main.rs @@ -715,17 +715,18 @@ fn build_tx( // Use token id for first spendable tx out let token_id = spendable_txouts.first().unwrap().amount.token_id; + // FIXME: This needs to be the fee for the current token, not MOB. + // However, bootstrapping non MOB tokens is not supported right now. + let fee_amount = Amount::new(MOB_FEE.load(Ordering::SeqCst), token_id); + // Create tx_builder. let mut tx_builder = TransactionBuilder::new( block_version, - token_id, + fee_amount, fog_resolver, EmptyMemoBuilder::default(), - ); - - // FIXME: This needs to be the fee for the current token, not MOB. - // However, bootstrapping non MOB tokens is not supported right now. - tx_builder.set_fee(MOB_FEE.load(Ordering::SeqCst)).unwrap(); + ) + .unwrap(); // Unzip each vec of tuples into a tuple of vecs. let mut rings_and_proofs: Vec<(Vec, Vec)> = rings diff --git a/fog/sample-paykit/src/client.rs b/fog/sample-paykit/src/client.rs index 04b8de2682..0d2a8f769d 100644 --- a/fog/sample-paykit/src/client.rs +++ b/fog/sample-paykit/src/client.rs @@ -606,9 +606,13 @@ fn build_transaction_helper( memo_builder.set_sender_credential(SenderMemoCredential::from(source_account_key)); memo_builder.enable_destination_memo(); - TransactionBuilder::new(block_version, amount.token_id, fog_resolver, memo_builder) + TransactionBuilder::new( + block_version, + Amount::new(fee, amount.token_id), + fog_resolver, + memo_builder, + )? }; - tx_builder.set_fee(fee)?; let input_amount = inputs .iter() diff --git a/libmobilecoin/src/transaction.rs b/libmobilecoin/src/transaction.rs index 3aeb58b3fc..0bf2f669e7 100644 --- a/libmobilecoin/src/transaction.rs +++ b/libmobilecoin/src/transaction.rs @@ -378,14 +378,12 @@ pub extern "C" fn mc_transaction_builder_create( let mut transaction_builder = TransactionBuilder::new_with_box( block_version, - token_id, + Amount::new(fee, token_id), fog_resolver, memo_builder_box, - ); + ) + .expect("Could not create transaction builder"); - transaction_builder - .set_fee(fee) - .expect("failure not expected"); transaction_builder.set_tombstone_block(tombstone_block); Some(transaction_builder) }) diff --git a/mobilecoind/src/payments.rs b/mobilecoind/src/payments.rs index ebf4ef613d..f2dec02984 100644 --- a/mobilecoind/src/payments.rs +++ b/mobilecoind/src/payments.rs @@ -869,17 +869,16 @@ impl, Vec)> = rings diff --git a/mobilecoind/src/service.rs b/mobilecoind/src/service.rs index 261a8c84db..37e2aa7092 100644 --- a/mobilecoind/src/service.rs +++ b/mobilecoind/src/service.rs @@ -2917,10 +2917,11 @@ mod test { let monitor_id = mobilecoind_db.add_monitor(&data).unwrap(); let mut transaction_builder = TransactionBuilder::new( BLOCK_VERSION, - Mob::ID, + Amount::new(Mob::MINIMUM_FEE, Mob::ID), MockFogResolver::default(), EmptyMemoBuilder::default(), - ); + ) + .unwrap(); let (tx_out, tx_confirmation) = transaction_builder .add_output(Amount::new(10, Mob::ID), &receiver.subaddress(0), &mut rng) .unwrap(); @@ -5230,10 +5231,11 @@ mod test { let mut transaction_builder = TransactionBuilder::new( BLOCK_VERSION, - Mob::ID, + Amount::new(Mob::MINIMUM_FEE, Mob::ID), MockFogResolver::default(), EmptyMemoBuilder::default(), - ); + ) + .unwrap(); let (tx_out, _tx_confirmation) = transaction_builder .add_output( Amount::new(10, Mob::ID), @@ -5342,10 +5344,11 @@ mod test { let mut transaction_builder = TransactionBuilder::new( BLOCK_VERSION, - Mob::ID, + Amount::new(Mob::MINIMUM_FEE, Mob::ID), MockFogResolver::default(), EmptyMemoBuilder::default(), - ); + ) + .unwrap(); let (tx_out, _tx_confirmation) = transaction_builder .add_output( Amount::new(10, Mob::ID), diff --git a/transaction/core/test-utils/src/lib.rs b/transaction/core/test-utils/src/lib.rs index 93be96272a..1193a6747d 100644 --- a/transaction/core/test-utils/src/lib.rs +++ b/transaction/core/test-utils/src/lib.rs @@ -151,10 +151,11 @@ pub fn create_transaction_with_amount_and_comparer< let mut transaction_builder = TransactionBuilder::new( block_version, - sender_amount.token_id, + Amount::new(fee, sender_amount.token_id), MockFogResolver::default(), EmptyMemoBuilder::default(), - ); + ) + .unwrap(); // The first transaction in the origin block should contain enough outputs to // use as mixins. @@ -206,9 +207,6 @@ pub fn create_transaction_with_amount_and_comparer< // Tombstone block transaction_builder.set_tombstone_block(tombstone_block); - // Fee - transaction_builder.set_fee(fee).unwrap(); - // Build and return the transaction transaction_builder.build_with_sorter::(rng).unwrap() } diff --git a/transaction/std/src/transaction_builder.rs b/transaction/std/src/transaction_builder.rs index e99aeb9b86..9b27275824 100644 --- a/transaction/std/src/transaction_builder.rs +++ b/transaction/std/src/transaction_builder.rs @@ -80,51 +80,47 @@ impl TransactionBuilder { /// Initializes a new TransactionBuilder. /// /// # Arguments + /// * `block_version` - The block version rules to use when building this + /// transaction + /// * `fee` - The fee (and token id) to use for this transaction. Note: The + /// fee token id cannot be changed later, and before mixed transactions + /// feature, every input and output must have this token id. /// * `fog_resolver` - Source of validated fog keys to use with this /// transaction /// * `memo_builder` - An object which creates memos for the TxOuts in this /// transaction pub fn new( block_version: BlockVersion, - token_id: TokenId, + fee: Amount, fog_resolver: FPR, memo_builder: MB, - ) -> Self { - TransactionBuilder::new_with_box( - block_version, - token_id, - fog_resolver, - Box::new(memo_builder), - ) + ) -> Result { + TransactionBuilder::new_with_box(block_version, fee, fog_resolver, Box::new(memo_builder)) } /// Initializes a new TransactionBuilder, using a Box /// instead of statically typed /// /// # Arguments - /// * `block_version` - The block version to use when building a transaction - /// * `fee_token_id` - The token id of the fee output + /// * `block_version` - The block version rules to use when building this + /// transaction + /// * `fee` - The fee (and token id) to use for this transaction. Note: The + /// fee token id cannot be changed later, and before mixed transactions + /// feature, every input and output must have this token id. /// * `fog_resolver` - Source of validated fog keys to use with this /// transaction /// * `memo_builder` - An object which creates memos for the TxOuts in this /// transaction pub fn new_with_box( block_version: BlockVersion, - fee_token_id: TokenId, + fee: Amount, fog_resolver: FPR, mut memo_builder: Box, - ) -> Self { - // HACK: make sure that the memo builder + ) -> Result { + // make sure that the memo builder // is initialized to the same fee as the transaction builder - // It might be better to require the user to call `set_fee` at some point - // instead of allowing that they might never call that. - // It is also janky that we default to Mob::MINIMUM_FEE even though the - // token id may not be Mob, but changing that for now will break tests. - let fee = Amount::new(Mob::MINIMUM_FEE, fee_token_id); - memo_builder - .set_fee(fee) - .expect("memo builder should not complain at this point"); - TransactionBuilder { + memo_builder.set_fee(fee)?; + Ok(TransactionBuilder { block_version, input_credentials: Vec::new(), outputs_and_shared_secrets: Vec::new(), @@ -133,7 +129,7 @@ impl TransactionBuilder { fog_resolver, fog_tombstone_block_limit: u64::max_value(), memo_builder: Some(memo_builder), - } + }) } /// Add an Input to the transaction. @@ -732,10 +728,11 @@ pub mod transaction_builder_tests { ) -> Result { let mut transaction_builder = TransactionBuilder::new( block_version, - token_id, + Amount::new(Mob::MINIMUM_FEE, token_id), fog_resolver.clone(), EmptyMemoBuilder::default(), - ); + ) + .unwrap(); let input_value = 1000; let output_value = 10; @@ -803,8 +800,13 @@ pub mod transaction_builder_tests { let membership_proofs = input_credentials.membership_proofs.clone(); let key_image = KeyImage::from(&input_credentials.onetime_private_key); - let mut transaction_builder = - TransactionBuilder::new(block_version, token_id, fpr, EmptyMemoBuilder::default()); + let mut transaction_builder = TransactionBuilder::new( + block_version, + Amount::new(Mob::MINIMUM_FEE, token_id), + fpr, + EmptyMemoBuilder::default(), + ) + .unwrap(); transaction_builder.add_input(input_credentials); let (_txout, confirmation) = transaction_builder @@ -882,10 +884,11 @@ pub mod transaction_builder_tests { let mut transaction_builder = TransactionBuilder::new( block_version, - token_id, + Amount::new(Mob::MINIMUM_FEE, token_id), fog_resolver, EmptyMemoBuilder::default(), - ); + ) + .unwrap(); transaction_builder.add_input(input_credentials); let (_txout, confirmation) = transaction_builder @@ -972,10 +975,11 @@ pub mod transaction_builder_tests { let mut transaction_builder = TransactionBuilder::new( block_version, - token_id, + Amount::new(Mob::MINIMUM_FEE, token_id), fog_resolver.clone(), EmptyMemoBuilder::default(), - ); + ) + .unwrap(); let input_credentials = get_input_credentials(block_version, amount, &sender, &fog_resolver, &mut rng); @@ -1047,10 +1051,11 @@ pub mod transaction_builder_tests { { let mut transaction_builder = TransactionBuilder::new( block_version, - token_id, + Amount::new(Mob::MINIMUM_FEE, token_id), fog_resolver.clone(), EmptyMemoBuilder::default(), - ); + ) + .unwrap(); transaction_builder.set_tombstone_block(2000); @@ -1081,10 +1086,11 @@ pub mod transaction_builder_tests { { let mut transaction_builder = TransactionBuilder::new( block_version, - token_id, + Amount::new(Mob::MINIMUM_FEE, token_id), fog_resolver.clone(), EmptyMemoBuilder::default(), - ); + ) + .unwrap(); transaction_builder.set_tombstone_block(500); @@ -1144,10 +1150,11 @@ pub mod transaction_builder_tests { { let mut transaction_builder = TransactionBuilder::new( block_version, - token_id, + Amount::new(Mob::MINIMUM_FEE, token_id), fog_resolver.clone(), EmptyMemoBuilder::default(), - ); + ) + .unwrap(); transaction_builder.set_tombstone_block(2000); @@ -1324,10 +1331,11 @@ pub mod transaction_builder_tests { let mut transaction_builder = TransactionBuilder::new( block_version, - token_id, + Amount::new(Mob::MINIMUM_FEE, token_id), fog_resolver.clone(), memo_builder, - ); + ) + .unwrap(); transaction_builder.set_tombstone_block(2000); @@ -1484,10 +1492,11 @@ pub mod transaction_builder_tests { let mut transaction_builder = TransactionBuilder::new( block_version, - token_id, + Amount::new(Mob::MINIMUM_FEE, token_id), fog_resolver.clone(), memo_builder, - ); + ) + .unwrap(); transaction_builder.set_tombstone_block(2000); transaction_builder.set_fee(Mob::MINIMUM_FEE * 4).unwrap(); @@ -1646,10 +1655,11 @@ pub mod transaction_builder_tests { let mut transaction_builder = TransactionBuilder::new( block_version, - token_id, + Amount::new(Mob::MINIMUM_FEE, token_id), fog_resolver.clone(), memo_builder, - ); + ) + .unwrap(); transaction_builder.set_tombstone_block(2000); @@ -1807,10 +1817,11 @@ pub mod transaction_builder_tests { let mut transaction_builder = TransactionBuilder::new( block_version, - token_id, + Amount::new(Mob::MINIMUM_FEE, token_id), fog_resolver.clone(), memo_builder, - ); + ) + .unwrap(); transaction_builder.set_tombstone_block(2000); @@ -1956,10 +1967,11 @@ pub mod transaction_builder_tests { let mut transaction_builder = TransactionBuilder::new( block_version, - token_id, + Amount::new(Mob::MINIMUM_FEE, token_id), fog_resolver.clone(), memo_builder, - ); + ) + .unwrap(); transaction_builder.set_tombstone_block(2000); @@ -2130,10 +2142,11 @@ pub mod transaction_builder_tests { let mut transaction_builder = TransactionBuilder::new( block_version, - token_id, + Amount::new(Mob::MINIMUM_FEE, token_id), fog_resolver.clone(), memo_builder, - ); + ) + .unwrap(); transaction_builder.set_tombstone_block(2000); @@ -2321,10 +2334,11 @@ pub mod transaction_builder_tests { let mut transaction_builder = TransactionBuilder::new( block_version, - token_id, + Amount::new(Mob::MINIMUM_FEE, token_id), fog_resolver.clone(), memo_builder, - ); + ) + .unwrap(); transaction_builder.set_tombstone_block(2000); @@ -2434,8 +2448,13 @@ pub mod transaction_builder_tests { ) .unwrap(); - let mut transaction_builder = - TransactionBuilder::new(block_version, token_id, fpr, EmptyMemoBuilder::default()); + let mut transaction_builder = TransactionBuilder::new( + block_version, + Amount::new(Mob::MINIMUM_FEE, token_id), + fpr, + EmptyMemoBuilder::default(), + ) + .unwrap(); transaction_builder.add_input(input_credentials); let wrong_value = 999; @@ -2599,10 +2618,11 @@ pub mod transaction_builder_tests { let mut transaction_builder = TransactionBuilder::new( block_version, - token_id, + Amount::new(Mob::MINIMUM_FEE, token_id), fog_resolver.clone(), memo_builder, - ); + ) + .unwrap(); let input_credentials = get_input_credentials( block_version, @@ -2698,8 +2718,13 @@ pub mod transaction_builder_tests { let block_version = BlockVersion::try_from(block_version).unwrap(); let memo_builder = EmptyMemoBuilder::default(); - let mut transaction_builder = - TransactionBuilder::new(block_version, Mob::ID, fog_resolver.clone(), memo_builder); + let mut transaction_builder = TransactionBuilder::new( + block_version, + Amount::new(Mob::MINIMUM_FEE, Mob::ID), + fog_resolver.clone(), + memo_builder, + ) + .unwrap(); let input_credentials = get_input_credentials(block_version, amount1, &sender, &fog_resolver, &mut rng); @@ -2825,8 +2850,13 @@ pub mod transaction_builder_tests { let mut test_fn = |block_version, tx_out1_amount| -> Result<_, _> { let memo_builder = EmptyMemoBuilder::default(); - let mut transaction_builder = - TransactionBuilder::new(block_version, Mob::ID, fog_resolver.clone(), memo_builder); + let mut transaction_builder = TransactionBuilder::new( + block_version, + Amount::new(Mob::MINIMUM_FEE, Mob::ID), + fog_resolver.clone(), + memo_builder, + ) + .unwrap(); let input_credentials = get_input_credentials(block_version, amount1, &sender, &fog_resolver, &mut rng);