validate contract states

controller/tests/contract_self_spend_custom.rs

@@ -200,10 +200,7 @@ fn wallet_contract_self_spend_custom_tx() -> Result<(), libwallet::Error> {
 	Ok(())
 }
 
-#[test]
+fn contract_min_confirmations_impl(test_dir: &'static str) -> Result<(), libwallet::Error> {
-fn contract_min_confirmations() -> Result<(), libwallet::Error> {
-	let test_dir = "test_output/contract_min_confirmations";
-	setup(test_dir);
 	let (wallets, _, stopper, bh) = create_wallets(
 		vec![
 			vec![("default", 7)],
@@ -295,6 +292,14 @@ fn contract_min_confirmations() -> Result<(), libwallet::Error> {
 
 	stopper.store(false, Ordering::Relaxed);
 	thread::sleep(Duration::from_millis(200));
+	Ok(())
+}
+
+#[test]
+fn contract_min_confirmations() -> Result<(), libwallet::Error> {
+	let test_dir = "test_output/contract_min_confirmations";
+	setup(test_dir);
+	contract_min_confirmations_impl(test_dir)?;
 	clean_output_dir(test_dir);
 	Ok(())
 }

controller/tests/contract_srs.rs

@@ -70,6 +70,41 @@ fn contract_srs_tx_impl(test_dir: &'static str) -> Result<(), libwallet::Error>
 		recv_mask,
 		PathBuf::from(test_dir),
 		|api, m| {
+			let child_index = {
+				let mut wallet = api.wallet_inst.lock();
+				let wallet = wallet.lc_provider()?.wallet_inst()?;
+				let parent = wallet.parent_key_id();
+				wallet.current_child_index(&parent)?
+			};
+			let (_, txs) = api.retrieve_txs(m, false, None, None, None)?;
+			let mut invalid = slate.clone();
+			invalid.state = SlateState::Unknown;
+			let err = api
+				.contract_sign(
+					m,
+					&invalid,
+					&ContractSetupArgsAPI {
+						selection_args: common::contract_selection_args(),
+						net_change: Some(5_000_000_000),
+						..Default::default()
+					},
+				)
+				.unwrap_err();
+			assert!(matches!(
+				err,
+				libwallet::Error::GenericError(ref msg)
+					if msg == "Cannot advance a contract slate in state UN"
+			));
+			let current_child_index = {
+				let mut wallet = api.wallet_inst.lock();
+				let wallet = wallet.lc_provider()?.wallet_inst()?;
+				let parent = wallet.parent_key_id();
+				wallet.current_child_index(&parent)?
+			};
+			let (_, current_txs) = api.retrieve_txs(m, false, None, None, None)?;
+			assert_eq!(current_child_index, child_index);
+			assert_eq!(current_txs.len(), txs.len());
+
 			let wrong_args = ContractSetupArgsAPI {
 				selection_args: common::contract_selection_args(),
 				net_change: Some(-5_000_000_000),
@@ -88,7 +123,31 @@ fn contract_srs_tx_impl(test_dir: &'static str) -> Result<(), libwallet::Error>
 				net_change: Some(5_000_000_000),
 				..Default::default()
 			};
-			slate = api.contract_sign(m, &slate, args)?;
+			let incoming = slate.clone();
+			slate = api.contract_sign(m, &incoming, args)?;
+
+			let child_index = {
+				let mut wallet = api.wallet_inst.lock();
+				let wallet = wallet.lc_provider()?.wallet_inst()?;
+				let parent = wallet.parent_key_id();
+				wallet.current_child_index(&parent)?
+			};
+			let (_, txs) = api.retrieve_txs(m, false, None, None, None)?;
+			let err = api.contract_sign(m, &incoming, args).unwrap_err();
+			assert!(matches!(
+				err,
+				libwallet::Error::GenericError(ref msg)
+					if msg == &format!("Slate with id:{} has already been signed.", incoming.id)
+			));
+			let current_child_index = {
+				let mut wallet = api.wallet_inst.lock();
+				let wallet = wallet.lc_provider()?.wallet_inst()?;
+				let parent = wallet.parent_key_id();
+				wallet.current_child_index(&parent)?
+			};
+			let (_, current_txs) = api.retrieve_txs(m, false, None, None, None)?;
+			assert_eq!(current_child_index, child_index);
+			assert_eq!(current_txs.len(), txs.len());
 			Ok(())
 		},
 	)?;

libwallet/src/contract/actions/README.md

@@ -73,8 +73,6 @@ Separating side effects until the 'save_step' part would make these functions mu
 
 #### Tests
 
- - signing the same slate twice, which `verify_not_signed` guards
- - invalid or inconsistent slates and unexpected states
  - slate contents at each step, not only the end state
  - the foreign API entry points for `new` and `sign`
  - Slatepack version negotiation with the last supported wallet release

libwallet/src/contract/actions/sign.rs

@@ -98,6 +98,8 @@ where
 {
 	let mut sl = slate.clone();
 	contract::utils::verify_not_signed(w, sl.id)?;
+	// Reject terminal and unknown states before setup reserves keys.
+	contract::slate::next_state(&sl.state)?;
 
 	// Ensure net_change has been provided
 	let expected_net_change =

libwallet/src/contract/slate.rs

@@ -438,26 +438,27 @@ where
 	Ok(())
 }
 
-/// Transition the slate state to the next one
+/// Return the next contract slate state
-pub fn transition_state(slate: &mut Slate) -> Result<(), Error> {
+pub(super) fn next_state(state: &SlateState) -> Result<SlateState, Error> {
-	// We don't really use these states right now apart from leaving it to derive expected net_change.
+	let next = match state {
-	// This suggests these can't be used for manipulation. It doesn't hurt to think a bit more if that's the case.
-	let new_state = match slate.state {
 		SlateState::Invoice1 => SlateState::Invoice2,
 		SlateState::Invoice2 => SlateState::Invoice3,
 		SlateState::Standard1 => SlateState::Standard2,
 		SlateState::Standard2 => SlateState::Standard3,
-		// Unknown, or a slate that has already reached its final state, is not something
+		// Unknown and final states cannot be advanced.
-		// we can advance. We have signed by this point and the wallet state is persisted
+		s => {
-		// next, so report it rather than reporting a Standard3 that never happened.
-		ref s => {
 			return Err(Error::GenericError(format!(
 				"Cannot advance a contract slate in state {}",
 				s
 			)))
 		}
 	};
-	slate.state = new_state;
+	Ok(next)
+}
+
+/// Transition the slate state to the next one
+pub(super) fn transition_state(slate: &mut Slate) -> Result<(), Error> {
+	slate.state = next_state(&slate.state)?;
 	// NOTE: It's possible to never reach the step3. A self-spend has only 2 steps: new -> sign.
 	Ok(())
 }