Refactor transaction building combinators (#3057)

* tx combinators now take operate on Result to allow for more robust errors handling
replace with_fee() and with_lock_height() with a more flexible with_features()

* pass kernel features in as arg to build::transaction()

* fix chain tests

* fix pool tests

* do not pass kernel around in the combinators
just set it once on the tx when building

* build::partial_transaction now takes a existing tx to build on

chain/tests/mine_simple_chain.rs

@@ -16,7 +16,7 @@ use self::chain::types::{NoopAdapter, Tip};
 use self::chain::Chain;
 use self::core::core::hash::Hashed;
 use self::core::core::verifier_cache::LruVerifierCache;
-use self::core::core::{Block, BlockHeader, OutputIdentifier, Transaction};
+use self::core::core::{Block, BlockHeader, KernelFeatures, OutputIdentifier, Transaction};
 use self::core::global::ChainTypes;
 use self::core::libtx::{self, build, ProofBuilder};
 use self::core::pow::Difficulty;
@@ -562,10 +562,10 @@ fn spend_in_fork_and_compact() {
 		let key_id31 = ExtKeychainPath::new(1, 31, 0, 0, 0).to_identifier();
 
 		let tx1 = build::transaction(
+			KernelFeatures::Plain { fee: 20000 },
 			vec![
 				build::coinbase_input(consensus::REWARD, key_id2.clone()),
 				build::output(consensus::REWARD - 20000, key_id30.clone()),
-				build::with_fee(20000),
 			],
 			&kc,
 			&pb,
@@ -580,10 +580,10 @@ fn spend_in_fork_and_compact() {
 		chain.validate(false).unwrap();
 
 		let tx2 = build::transaction(
+			KernelFeatures::Plain { fee: 20000 },
 			vec![
 				build::input(consensus::REWARD - 20000, key_id30.clone()),
 				build::output(consensus::REWARD - 40000, key_id31.clone()),
-				build::with_fee(20000),
 			],
 			&kc,
 			&pb,

chain/tests/test_coinbase_maturity.rs

@@ -15,6 +15,7 @@
 use self::chain::types::NoopAdapter;
 use self::chain::ErrorKind;
 use self::core::core::verifier_cache::LruVerifierCache;
+use self::core::core::KernelFeatures;
 use self::core::global::{self, ChainTypes};
 use self::core::libtx::{self, build, ProofBuilder};
 use self::core::pow::Difficulty;
@@ -99,10 +100,10 @@ fn test_coinbase_maturity() {
 		// here we build a tx that attempts to spend the earlier coinbase output
 		// this is not a valid tx as the coinbase output cannot be spent yet
 		let coinbase_txn = build::transaction(
+			KernelFeatures::Plain { fee: 2 },
 			vec![
 				build::coinbase_input(amount, key_id1.clone()),
 				build::output(amount - 2, key_id2.clone()),
-				build::with_fee(2),
 			],
 			&keychain,
 			&builder,
@@ -182,10 +183,10 @@ fn test_coinbase_maturity() {
 			// here we build a tx that attempts to spend the earlier coinbase output
 			// this is not a valid tx as the coinbase output cannot be spent yet
 			let coinbase_txn = build::transaction(
+				KernelFeatures::Plain { fee: 2 },
 				vec![
 					build::coinbase_input(amount, key_id1.clone()),
 					build::output(amount - 2, key_id2.clone()),
-					build::with_fee(2),
 				],
 				&keychain,
 				&builder,

core/src/core/transaction.rs

@@ -463,42 +463,17 @@ impl TxKernel {
 
 	/// Build an empty tx kernel with zero values.
 	pub fn empty() -> TxKernel {
+		TxKernel::with_features(KernelFeatures::Plain { fee: 0 })
+	}
+
+	/// Build an empty tx kernel with the provided kernel features.
+	pub fn with_features(features: KernelFeatures) -> TxKernel {
 		TxKernel {
-			features: KernelFeatures::Plain { fee: 0 },
+			features,
 			excess: Commitment::from_vec(vec![0; 33]),
 			excess_sig: secp::Signature::from_raw_data(&[0; 64]).unwrap(),
 		}
 	}
-
-	/// Builds a new tx kernel with the provided fee.
-	/// Will panic if we cannot safely do this on the existing kernel.
-	/// i.e. Do not try and set a fee on a coinbase kernel.
-	pub fn with_fee(self, fee: u64) -> TxKernel {
-		match self.features {
-			KernelFeatures::Plain { .. } => {
-				let features = KernelFeatures::Plain { fee };
-				TxKernel { features, ..self }
-			}
-			KernelFeatures::HeightLocked { lock_height, .. } => {
-				let features = KernelFeatures::HeightLocked { fee, lock_height };
-				TxKernel { features, ..self }
-			}
-			KernelFeatures::Coinbase => panic!("fee not supported on coinbase kernel"),
-		}
-	}
-
-	/// Builds a new tx kernel with the provided lock_height.
-	/// Will panic if we cannot safely do this on the existing kernel.
-	/// i.e. Do not try and set a lock_height on a coinbase kernel.
-	pub fn with_lock_height(self, lock_height: u64) -> TxKernel {
-		match self.features {
-			KernelFeatures::Plain { fee } | KernelFeatures::HeightLocked { fee, .. } => {
-				let features = KernelFeatures::HeightLocked { fee, lock_height };
-				TxKernel { features, ..self }
-			}
-			KernelFeatures::Coinbase => panic!("lock_height not supported on coinbase kernel"),
-		}
-	}
 }
 
 /// Enum of possible tx weight verification options -
@@ -684,6 +659,13 @@ impl TransactionBody {
 		self
 	}
 
+	/// Builds a new TransactionBody replacing any existing kernels with the provided kernel.
+	pub fn replace_kernel(mut self, kernel: TxKernel) -> TransactionBody {
+		self.kernels.clear();
+		self.kernels.push(kernel);
+		self
+	}
+
 	/// Total fee for a TransactionBody is the sum of fees of all fee carrying kernels.
 	pub fn fee(&self) -> u64 {
 		self.kernels
@@ -1012,8 +994,8 @@ impl Transaction {
 		}
 	}
 
-	/// Builds a new transaction with the provided output added. Existing
-	/// outputs, if any, are kept intact.
+	/// Builds a new transaction with the provided kernel added. Existing
+	/// kernels, if any, are kept intact.
 	/// Sort order is maintained.
 	pub fn with_kernel(self, kernel: TxKernel) -> Transaction {
 		Transaction {
@@ -1022,6 +1004,14 @@ impl Transaction {
 		}
 	}
 
+	/// Builds a new transaction replacing any existing kernels with the provided kernel.
+	pub fn replace_kernel(self, kernel: TxKernel) -> Transaction {
+		Transaction {
+			body: self.body.replace_kernel(kernel),
+			..self
+		}
+	}
+
 	/// Get inputs
 	pub fn inputs(&self) -> &Vec<Input> {
 		&self.body.inputs