Clean up Api Error and indicate future status clearly (#424)

* remove finalitytimeout from is_valid

rename is_supported to is_valid

rename is_valid to is_expected

some more clean up

add unexpected Tx status enum

add some comment

* clean up traits

Author: haerdib

examples/examples/custom_nonce.rs

@@ -20,8 +20,8 @@ use kitchensink_runtime::{BalancesCall, Runtime, RuntimeCall};
 use sp_keyring::AccountKeyring;
 use sp_runtime::{generic::Era, MultiAddress};
 use substrate_api_client::{
-	rpc::JsonrpseeClient, Api, AssetTipExtrinsicParams, GenericAdditionalParams, GetHeader,
-	SubmitAndWatch, XtStatus,
+	rpc::JsonrpseeClient, Api, AssetTipExtrinsicParams, Error, GenericAdditionalParams, GetHeader,
+	SubmitAndWatch, UnexpectedTxStatus, XtStatus,
 };
 
 #[tokio::main]
@@ -58,11 +58,12 @@ async fn main() {
 	println!("[+] Composed Extrinsic:\n {:?}\n", xt);
 
 	// Send and watch extrinsic until InBlock.
-	match api.submit_and_watch_extrinsic_until(xt, XtStatus::InBlock) {
-		Err(error) => {
-			println!("Retrieved error {:?}", error);
-			assert!(format!("{:?}", error).contains("Future"));
+	let result = api.submit_and_watch_extrinsic_until(xt, XtStatus::InBlock);
+	println!("Returned Result {:?}", result);
+	match result {
+		Err(Error::UnexpectedTxStatus(UnexpectedTxStatus::Future)) => {
+			// All good, we expected a Future Error.
 		},
-		_ => panic!("Expected an error upon a future extrinsic"),
+		_ => panic!("Expected a future error"),
 	}
 }

node-api/src/lib.rs

@@ -21,7 +21,6 @@ use alloc::{borrow::ToOwned, vec::Vec};
 use codec::{Decode, Encode};
 
 pub use decoder::*;
-pub use error::*;
 pub use events::*;
 pub use metadata::*;
 pub use storage::*;

src/api/error.rs

@@ -15,30 +15,45 @@
 
 */
 
-use crate::{api::XtStatus, rpc::Error as RpcClientError};
+use crate::{api::UnexpectedTxStatus, rpc::Error as RpcClientError};
 use ac_node_api::{
+	error::DispatchError,
 	metadata::{InvalidMetadataError, MetadataError},
-	DispatchError,
 };
-use alloc::{boxed::Box, string::String};
+use alloc::boxed::Box;
 
 pub type Result<T> = core::result::Result<T, Error>;
 
 #[derive(Debug, derive_more::From)]
 pub enum Error {
+	/// Could not fetch the genesis hash from node.
 	FetchGenesisHash,
+	/// Expected a signer, but none is assigned.
 	NoSigner,
+	/// Rpc Client Error.
 	RpcClient(RpcClientError),
+	/// Metadata Error.
 	Metadata(MetadataError),
+	/// Invalid Metadata Error.
 	InvalidMetadata(InvalidMetadataError),
+	/// Node Api Error.
 	NodeApi(ac_node_api::error::Error),
-	StorageValueDecode(codec::Error),
-	UnsupportedXtStatus(XtStatus),
+	/// Encode / Decode Error.
+	Codec(codec::Error),
+	/// Could not convert NumberOrHex with try_from.
 	TryFromIntError,
+	/// Node Api Dispatch Error.
 	Dispatch(DispatchError),
-	Extrinsic(String),
+	/// Encountered unexpected tx status during watch process.
+	UnexpectedTxStatus(UnexpectedTxStatus),
+	/// Could not send update because the Stream has been closed unexpectedly.
 	NoStream,
-	NoBlockHash,
-	NoBlock,
+	/// Could not find the expected extrinsic.
+	ExtrinsicNotFound,
+	/// Could not find the expected block hash.
+	BlockHashNotFound,
+	/// Could not find the expected block.
+	BlockNotFound,
+	/// Any custom Error.
 	Other(Box<dyn core::error::Error + Send + Sync + 'static>),
 }

src/api/mod.rs

@@ -67,6 +67,18 @@ pub enum XtStatus {
 	Finalized = 6,
 }
 
+/// TxStatus that is not expected during the watch process. Will be returned
+/// as unexpected error if encountered due to the potential danger of endless loops.
+#[derive(Debug, PartialEq, Eq, Copy, Clone)]
+pub enum UnexpectedTxStatus {
+	Future,
+	Retracted,
+	FinalityTimeout,
+	Usurped,
+	Dropped,
+	Invalid,
+}
+
 /// Possible transaction status events.
 // Copied from `sc-transaction-pool`
 // (https://github.com/paritytech/substrate/blob/dddfed3d9260cf03244f15ba3db4edf9af7467e9/client/transaction-pool/api/src/lib.rs)
@@ -114,15 +126,24 @@ impl<Hash, BlockHash> TransactionStatus<Hash, BlockHash> {
 		}
 	}
 
-	pub fn is_supported(&self) -> bool {
-		matches!(
-			self,
+	pub fn is_expected(&self) -> Result<()> {
+		match self {
 			TransactionStatus::Ready
-				| TransactionStatus::Broadcast(_)
-				| TransactionStatus::InBlock(_)
-				| TransactionStatus::FinalityTimeout(_)
-				| TransactionStatus::Finalized(_)
-		)
+			| TransactionStatus::Broadcast(_)
+			| TransactionStatus::InBlock(_)
+			| TransactionStatus::Finalized(_) => Ok(()),
+			TransactionStatus::Future => Err(Error::UnexpectedTxStatus(UnexpectedTxStatus::Future)),
+			TransactionStatus::Retracted(_) =>
+				Err(Error::UnexpectedTxStatus(UnexpectedTxStatus::Retracted)),
+			TransactionStatus::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)),
+		}
 	}
 
 	/// Returns true if the input status has been reached (or overreached)
@@ -184,20 +205,20 @@ mod tests {
 	}
 
 	#[test]
-	fn test_transaction_status_is_supported() {
+	fn test_transaction_status_is_expected() {
 		// Supported.
-		assert!(TransactionStatus::Ready.is_supported());
-		assert!(TransactionStatus::Broadcast(vec![]).is_supported());
-		assert!(TransactionStatus::InBlock(H256::random()).is_supported());
-		assert!(TransactionStatus::FinalityTimeout(H256::random()).is_supported());
-		assert!(TransactionStatus::Finalized(H256::random()).is_supported());
+		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::Finalized(H256::random()).is_expected().is_ok());
 
 		// Not supported.
-		assert!(!TransactionStatus::Future.is_supported());
-		assert!(!TransactionStatus::Retracted(H256::random()).is_supported());
-		assert!(!TransactionStatus::Usurped(H256::random()).is_supported());
-		assert!(!TransactionStatus::Dropped.is_supported());
-		assert!(!TransactionStatus::Invalid.is_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());
+		assert!(TransactionStatus::Invalid.is_expected().is_err());
 	}
 
 	#[test]

src/api/rpc_api/author.rs

@@ -22,7 +22,7 @@ use crate::{
 use ac_compose_macros::rpc_params;
 use ac_node_api::EventDetails;
 use ac_primitives::{ExtrinsicParams, FrameSystemConfig};
-use alloc::{format, string::ToString, vec::Vec};
+use alloc::vec::Vec;
 use codec::Encode;
 use log::*;
 use serde::de::DeserializeOwned;
@@ -208,24 +208,22 @@ where
 
 		while let Some(transaction_status) = subscription.next() {
 			let transaction_status = transaction_status?;
-			if transaction_status.is_supported() {
-				if transaction_status.reached_status(watch_until) {
+			match transaction_status.is_expected() {
+				Ok(_) =>
+					if transaction_status.reached_status(watch_until) {
+						subscription.unsubscribe()?;
+						let block_hash = transaction_status.get_maybe_block_hash();
+						return Ok(ExtrinsicReport::new(
+							tx_hash,
+							block_hash.copied(),
+							transaction_status,
+							None,
+						))
+					},
+				Err(e) => {
 					subscription.unsubscribe()?;
-					let block_hash = transaction_status.get_maybe_block_hash();
-					return Ok(ExtrinsicReport::new(
-						tx_hash,
-						block_hash.copied(),
-						transaction_status,
-						None,
-					))
-				}
-			} else {
-				subscription.unsubscribe()?;
-				let error = Error::Extrinsic(format!(
-					"Unsupported transaction status: {transaction_status:?}, stopping watch process."
-
-				));
-				return Err(error)
+					return Err(e)
+				},
 			}
 		}
 		Err(Error::NoStream)
@@ -237,7 +235,6 @@ impl<Signer, Client, Params, Runtime> SubmitAndWatchUntilSuccess<Client, Runtime
 where
 	Client: Subscribe + Request,
 	Params: ExtrinsicParams<Runtime::Index, Runtime::Hash>,
-	Runtime::Hashing: HashTrait<Output = Runtime::Hash>,
 	Runtime: FrameSystemConfig + GetRuntimeBlockType,
 	Runtime::RuntimeBlock: BlockTrait + DeserializeOwned,
 	Runtime::Hashing: HashTrait<Output = Runtime::Hash>,
@@ -267,7 +264,7 @@ where
 		let mut report =
 			self.submit_and_watch_opaque_extrinsic_until(encoded_extrinsic, xt_status)?;
 
-		let block_hash = report.block_hash.ok_or(Error::NoBlockHash)?;
+		let block_hash = report.block_hash.ok_or(Error::BlockHashNotFound)?;
 		let extrinsic_index =
 			self.retrieve_extrinsic_index_from_block(block_hash, report.extrinsic_hash)?;
 		let block_events = self.fetch_events_from_block(block_hash)?;
@@ -293,7 +290,7 @@ where
 		block_hash: Runtime::Hash,
 		extrinsic_hash: Runtime::Hash,
 	) -> Result<u32> {
-		let block = self.get_block(Some(block_hash))?.ok_or(Error::NoBlock)?;
+		let block = self.get_block(Some(block_hash))?.ok_or(Error::BlockNotFound)?;
 		let xt_index = block
 			.extrinsics()
 			.iter()
@@ -302,7 +299,7 @@ where
 				trace!("Looking for: {:?}, got xt_hash {:?}", extrinsic_hash, xt_hash);
 				extrinsic_hash == xt_hash
 			})
-			.ok_or(Error::Extrinsic("Could not find extrinsic hash".to_string()))?;
+			.ok_or(Error::ExtrinsicNotFound)?;
 		Ok(xt_index as u32)
 	}
 
@@ -311,7 +308,7 @@ where
 		let key = utils::storage_key("System", "Events");
 		let event_bytes = self
 			.get_opaque_storage_by_key_hash(key, Some(block_hash))?
-			.ok_or(Error::NoBlock)?;
+			.ok_or(Error::BlockNotFound)?;
 		let events =
 			Events::<Runtime::Hash>::new(self.metadata().clone(), Default::default(), event_bytes);
 		Ok(events)