Implement state handling for Retracted status (#807)

* - Move Retractec from UnexpectedTxStatus to XtStatus
- Provide is_final method for TransactionStatus
- Return from watch method as soon as a final state is reached
- Provide separate method to populate events for an ExtrinsicReport

* Fix unittest

* Remove unreachable handling of states with is_final() == True

* Improve testing and error handling

* Improve tests

* Change API for `populate_events` function

* Fix failing test

* Split populate_events into two separate methods

* Add documentation

* Rename function

* Expand Error type for event related problems

* Improve documentation

Co-authored-by: Bigna Härdi <[email protected]>

---------

Co-authored-by: Bigna Härdi <[email protected]>

src/api/error.rs

@@ -56,6 +56,10 @@ pub enum Error {
 	BlockHashNotFound,
 	/// Could not find the expected block.
 	BlockNotFound,
+	/// Operation needs events but events are missing.
+	EventsMissing,
+	/// Operation wants to add events but they are already present.
+	EventsAlreadyPresent,
 	/// Any custom Error.
 	Other(Box<dyn ErrorT + Send + Sync + 'static>),
 }

src/api/mod.rs

@@ -15,7 +15,8 @@
 
 */
 
-use ac_node_api::events::RawEventDetails;
+use crate::error::FailedExtrinsicError;
+use ac_node_api::{events::RawEventDetails, EventDetails, Metadata};
 use alloc::{string::String, vec::Vec};
 use codec::{Decode, Encode};
 use serde::{Deserialize, Serialize};
@@ -60,6 +61,29 @@ impl<Hash: Encode + Decode> ExtrinsicReport<Hash> {
 	) -> Self {
 		Self { extrinsic_hash, block_hash, status, events }
 	}
+
+	pub fn add_events(&mut self, events: Vec<EventDetails<Hash>>) {
+		self.events = Some(events.into_iter().map(|event| event.to_raw()).collect());
+	}
+
+	/// Checks the status of the extrinsic by evaluating the events attached to the report.
+	/// Returns an error if the events are missing or if one of the events indicates a problem.
+	pub fn check_events_for_dispatch_error(&self, metadata: &Metadata) -> Result<()> {
+		if self.events.is_none() {
+			return Err(Error::EventsMissing)
+		}
+		// Check if the extrinsic was successful or not.
+		let events = self.events.as_ref().unwrap();
+		for event in events {
+			if let Some(dispatch_error) = event.get_associated_dispatch_error(metadata) {
+				return Err(Error::FailedExtrinsic(FailedExtrinsicError::new(
+					dispatch_error,
+					self.encode(),
+				)))
+			}
+		}
+		Ok(())
+	}
 }
 
 /// Simplified TransactionStatus to allow the user to choose until when to watch
@@ -70,6 +94,7 @@ pub enum XtStatus {
 	Ready = 1,
 	Broadcast = 2,
 	InBlock = 3,
+	Retracted = 4,
 	Finalized = 6,
 }
 
@@ -78,7 +103,6 @@ pub enum XtStatus {
 #[derive(Debug, PartialEq, Eq, Copy, Clone)]
 pub enum UnexpectedTxStatus {
 	Future,
-	Retracted,
 	FinalityTimeout,
 	Usurped,
 	Dropped,
@@ -119,36 +143,32 @@ pub enum TransactionStatus<Hash: Encode + Decode, BlockHash: Encode + Decode> {
 impl<Hash: Encode + Decode, BlockHash: Encode + Decode> TransactionStatus<Hash, BlockHash> {
 	pub fn as_u8(&self) -> u8 {
 		match self {
-			TransactionStatus::Future => 0,
-			TransactionStatus::Ready => 1,
-			TransactionStatus::Broadcast(_) => 2,
-			TransactionStatus::InBlock(_) => 3,
-			TransactionStatus::Retracted(_) => 4,
-			TransactionStatus::FinalityTimeout(_) => 5,
-			TransactionStatus::Finalized(_) => 6,
-			TransactionStatus::Usurped(_) => 7,
-			TransactionStatus::Dropped => 8,
-			TransactionStatus::Invalid => 9,
+			Self::Future => 0,
+			Self::Ready => 1,
+			Self::Broadcast(_) => 2,
+			Self::InBlock(_) => 3,
+			Self::Retracted(_) => 4,
+			Self::FinalityTimeout(_) => 5,
+			Self::Finalized(_) => 6,
+			Self::Usurped(_) => 7,
+			Self::Dropped => 8,
+			Self::Invalid => 9,
 		}
 	}
 
 	pub fn is_expected(&self) -> Result<()> {
 		match self {
-			TransactionStatus::Ready
-			| TransactionStatus::Broadcast(_)
-			| TransactionStatus::InBlock(_)
-			| TransactionStatus::Finalized(_) => Ok(()),
-			TransactionStatus::Future => Err(Error::UnexpectedTxStatus(UnexpectedTxStatus::Future)),
-			TransactionStatus::Retracted(_) =>
-				Err(Error::UnexpectedTxStatus(UnexpectedTxStatus::Retracted)),
-			TransactionStatus::FinalityTimeout(_) =>
+			Self::Ready
+			| Self::Broadcast(_)
+			| Self::InBlock(_)
+			| Self::Retracted(_)
+			| Self::Finalized(_) => Ok(()),
+			Self::Future => Err(Error::UnexpectedTxStatus(UnexpectedTxStatus::Future)),
+			Self::FinalityTimeout(_) =>
 				Err(Error::UnexpectedTxStatus(UnexpectedTxStatus::FinalityTimeout)),
-			TransactionStatus::Usurped(_) =>
-				Err(Error::UnexpectedTxStatus(UnexpectedTxStatus::Usurped)),
-			TransactionStatus::Dropped =>
-				Err(Error::UnexpectedTxStatus(UnexpectedTxStatus::Dropped)),
-			TransactionStatus::Invalid =>
-				Err(Error::UnexpectedTxStatus(UnexpectedTxStatus::Invalid)),
+			Self::Usurped(_) => Err(Error::UnexpectedTxStatus(UnexpectedTxStatus::Usurped)),
+			Self::Dropped => Err(Error::UnexpectedTxStatus(UnexpectedTxStatus::Dropped)),
+			Self::Invalid => Err(Error::UnexpectedTxStatus(UnexpectedTxStatus::Invalid)),
 		}
 	}
 
@@ -160,13 +180,26 @@ impl<Hash: Encode + Decode, BlockHash: Encode + Decode> TransactionStatus<Hash,
 
 	pub fn get_maybe_block_hash(&self) -> Option<&BlockHash> {
 		match self {
-			TransactionStatus::InBlock(block_hash) => Some(block_hash),
-			TransactionStatus::Retracted(block_hash) => Some(block_hash),
-			TransactionStatus::FinalityTimeout(block_hash) => Some(block_hash),
-			TransactionStatus::Finalized(block_hash) => Some(block_hash),
+			Self::InBlock(block_hash) => Some(block_hash),
+			Self::Retracted(block_hash) => Some(block_hash),
+			Self::FinalityTimeout(block_hash) => Some(block_hash),
+			Self::Finalized(block_hash) => Some(block_hash),
 			_ => None,
 		}
 	}
+
+	/// Returns true if the Transaction reached its final Status
+	// See https://github.com/paritytech/polkadot-sdk/blob/289f5bbf7a45dc0380904a435464b15ec711ed03/substrate/client/transaction-pool/api/src/lib.rs#L161
+	pub fn is_final(&self) -> bool {
+		matches!(
+			self,
+			Self::Usurped(_)
+				| Self::Finalized(_)
+				| Self::FinalityTimeout(_)
+				| Self::Invalid
+				| Self::Dropped
+		)
+	}
 }
 
 // Exact structure from
@@ -216,11 +249,11 @@ mod tests {
 		assert!(TransactionStatus::Ready.is_expected().is_ok());
 		assert!(TransactionStatus::Broadcast(vec![]).is_expected().is_ok());
 		assert!(TransactionStatus::InBlock(H256::random()).is_expected().is_ok());
+		assert!(TransactionStatus::Retracted(H256::random()).is_expected().is_ok());
 		assert!(TransactionStatus::Finalized(H256::random()).is_expected().is_ok());
 
 		// Not supported.
 		assert!(TransactionStatus::Future.is_expected().is_err());
-		assert!(TransactionStatus::Retracted(H256::random()).is_expected().is_err());
 		assert!(TransactionStatus::FinalityTimeout(H256::random()).is_expected().is_err());
 		assert!(TransactionStatus::Usurped(H256::random()).is_expected().is_err());
 		assert!(TransactionStatus::Dropped.is_expected().is_err());

src/api/rpc_api/author.rs

@@ -15,7 +15,6 @@
 
 use crate::{
 	api::{rpc_api::events::FetchEvents, Error, Result},
-	error::FailedExtrinsicError,
 	rpc::{HandleSubscription, Request, Subscribe},
 	Api, ExtrinsicReport, TransactionStatus, XtStatus,
 };
@@ -211,6 +210,12 @@ pub trait SubmitAndWatch {
 		encoded_extrinsic: &Bytes,
 		watch_until: XtStatus,
 	) -> Result<ExtrinsicReport<Self::Hash>>;
+
+	/// Query the events for the specified `report` and attaches them to the mutable report.
+	/// If the function fails events might still be added to the report.
+	///
+	/// This method is blocking if the sync-api feature is activated
+	async fn populate_events(&self, report: &mut ExtrinsicReport<Self::Hash>) -> Result<()>;
 }
 
 #[maybe_async::maybe_async(?Send)]
@@ -276,29 +281,20 @@ where
 		if watch_until < XtStatus::InBlock {
 			return Ok(report)
 		}
+		self.populate_events(&mut report).await?;
+		report.check_events_for_dispatch_error(self.metadata())?;
+		return Ok(report);
+	}
+
+	async fn populate_events(&self, report: &mut ExtrinsicReport<Self::Hash>) -> Result<()> {
+		if report.events.is_some() {
+			return Err(Error::EventsAlreadyPresent)
+		}
 		let block_hash = report.block_hash.ok_or(Error::BlockHashNotFound)?;
 		let extrinsic_events =
 			self.fetch_events_for_extrinsic(block_hash, report.extrinsic_hash).await?;
-
-		// Check if the extrinsic was succesfull or not.
-		let mut maybe_dispatch_error = None;
-		for event in &extrinsic_events {
-			if let Some(dispatch_error) = event.get_associated_dispatch_error() {
-				maybe_dispatch_error = Some(dispatch_error);
-				break
-			}
-		}
-
-		report.events = Some(extrinsic_events.into_iter().map(|event| event.to_raw()).collect());
-
-		if let Some(dispatch_error) = maybe_dispatch_error {
-			return Err(Error::FailedExtrinsic(FailedExtrinsicError::new(
-				dispatch_error,
-				report.encode(),
-			)))
-		}
-
-		Ok(report)
+		report.add_events(extrinsic_events);
+		Ok(())
 	}
 
 	async fn submit_and_watch_extrinsic_until_without_events<

testing/async/examples/author_tests.rs

@@ -58,17 +58,18 @@ async fn main() {
 		test_submit_and_watch_until_broadcast(&api, transfer_call.clone(), signer_nonce + 3),
 		test_submit_and_watch_until_in_block(&api, transfer_call.clone(), signer_nonce + 4),
 		test_submit_and_watch_until_finalized(&api, transfer_call.clone(), signer_nonce + 5),
+		test_submit_and_watch_until_retracted(&api, transfer_call.clone(), signer_nonce + 6),
 		// Test some _watch_untils_without_events. We don't need to test all, because it is tested implicitly by `submit_and_watch_extrinsic_until`
 		// as internal call.
 		test_submit_and_watch_extrinsic_until_ready_without_events(
 			&api,
 			transfer_call.clone(),
-			signer_nonce + 6
+			signer_nonce + 7
 		),
 		test_submit_and_watch_extrinsic_until_in_block_without_events(
 			&api,
 			transfer_call.clone(),
-			signer_nonce + 7
+			signer_nonce + 8
 		)
 	);
 }
@@ -125,7 +126,7 @@ async fn test_submit_and_watch_until_in_block(
 	let report = api.submit_and_watch_extrinsic_until(xt, XtStatus::InBlock).await.unwrap();
 	assert!(report.block_hash.is_some());
 	assert!(matches!(report.status, TransactionStatus::InBlock(_)));
-	assert_associated_events_match_expected(report.events.unwrap());
+	assert_associated_events_match_expected(&report.events.unwrap());
 	println!("Success: submit_and_watch_extrinsic_until {:?}", report.status);
 }
 
@@ -139,7 +140,23 @@ async fn test_submit_and_watch_until_finalized(
 	let report = api.submit_and_watch_extrinsic_until(xt, XtStatus::Finalized).await.unwrap();
 	assert!(report.block_hash.is_some());
 	assert!(matches!(report.status, TransactionStatus::Finalized(_)));
-	assert_associated_events_match_expected(report.events.unwrap());
+	assert_associated_events_match_expected(&report.events.unwrap());
+	println!("Success: submit_and_watch_extrinsic_until {:?}", report.status);
+}
+
+async fn test_submit_and_watch_until_retracted(
+	api: &MyApi,
+	transfer_call: RuntimeCall,
+	nonce: Index,
+) {
+	std::thread::sleep(std::time::Duration::from_secs(1));
+	let xt = api.compose_extrinsic_offline(transfer_call, nonce);
+	// We wait for `Retracted`` but we cannot simulate this in a test. Therefore we will receive the status after `Retracted`
+	// which is `Finalized`
+	let report = api.submit_and_watch_extrinsic_until(xt, XtStatus::Retracted).await.unwrap();
+	assert!(report.block_hash.is_some());
+	assert!(matches!(report.status, TransactionStatus::Finalized(_)));
+	assert_associated_events_match_expected(&report.events.unwrap());
 	println!("Success: submit_and_watch_extrinsic_until {:?}", report.status);
 }
 
@@ -167,16 +184,30 @@ async fn test_submit_and_watch_extrinsic_until_in_block_without_events(
 	// Wait a little, otherwise we may run into future
 	std::thread::sleep(std::time::Duration::from_secs(1));
 	let xt = api.compose_extrinsic_offline(transfer_call, nonce);
-	let report = api
+	let mut report = api
 		.submit_and_watch_extrinsic_until_without_events(xt, XtStatus::InBlock)
 		.await
 		.unwrap();
 	println!("Extrinsic got successfully included in Block!");
 	assert!(report.block_hash.is_some());
 	assert!(report.events.is_none());
+
+	// Should fail without events
+	assert!(report.check_events_for_dispatch_error(&api.metadata()).is_err());
+
+	// Now we fetch the events separately
+	api.populate_events(&mut report).await.unwrap();
+	assert!(report.events.is_some());
+	assert!(report.check_events_for_dispatch_error(&api.metadata()).is_ok());
+	let events = report.events.as_ref().unwrap();
+	assert_associated_events_match_expected(&events);
+
+	// Can populate events only once
+	let result = api.populate_events(&mut report).await;
+	assert!(result.is_err());
 }
 
-fn assert_associated_events_match_expected(events: Vec<RawEventDetails<Hash>>) {
+fn assert_associated_events_match_expected(events: &[RawEventDetails<Hash>]) {
 	// First event
 	assert_eq!(events[0].pallet_name(), "Balances");
 	assert_eq!(events[0].variant_name(), "Withdraw");