Improve SignedExtension matching logic and remove SkipCheckIfFeeless bits (paritytech#1283)

* First pass making matching on signed exts more general and handlng SkipCheckifFeeless

* remove unneeded derives (only exts we can decode into are handled by the user)

* No SkipCheckIfFeeless in integration tests either

* Cargo fmt

* Remove SkipCheckIfFeeless specific logic

* clippy

* matches to just return bool, not result

* remove now-invalid comment

* return error from find if .iter() errors

Author: jsdw

subxt/examples/setup_config_custom.rs

@@ -1,6 +1,6 @@
 use codec::Encode;
 use subxt::client::OfflineClientT;
-use subxt::config::{Config, ExtrinsicParams, ExtrinsicParamsEncoder};
+use subxt::config::{Config, ExtrinsicParams, ExtrinsicParamsEncoder, ExtrinsicParamsError};
 use subxt_signer::sr25519::dev;
 
 #[subxt::subxt(
@@ -66,14 +66,13 @@ impl CustomExtrinsicParamsBuilder {
 // Describe how to fetch and then encode the params:
 impl<T: Config> ExtrinsicParams<T> for CustomExtrinsicParams<T> {
     type OtherParams = CustomExtrinsicParamsBuilder;
-    type Error = std::convert::Infallible;
 
     // Gather together all of the params we will need to encode:
     fn new<Client: OfflineClientT<T>>(
         _nonce: u64,
         client: Client,
         other_params: Self::OtherParams,
-    ) -> Result<Self, Self::Error> {
+    ) -> Result<Self, ExtrinsicParamsError> {
         Ok(Self {
             genesis_hash: client.genesis_hash(),
             tip: other_params.tip,

subxt/examples/setup_config_signed_extension.rs

@@ -1,9 +1,11 @@
 use codec::Encode;
 use scale_encode::EncodeAsType;
+use scale_info::PortableRegistry;
 use subxt::client::OfflineClientT;
 use subxt::config::signed_extensions;
 use subxt::config::{
     Config, DefaultExtrinsicParamsBuilder, ExtrinsicParams, ExtrinsicParamsEncoder,
+    ExtrinsicParamsError,
 };
 use subxt_signer::sr25519::dev;
 
@@ -34,10 +36,6 @@ impl Config for CustomConfig {
             signed_extensions::CheckMortality<Self>,
             signed_extensions::ChargeAssetTxPayment<Self>,
             signed_extensions::ChargeTransactionPayment,
-            signed_extensions::SkipCheckIfFeeless<
-                Self,
-                signed_extensions::ChargeAssetTxPayment<Self>,
-            >,
             // And add a new one of our own:
             CustomSignedExtension,
         ),
@@ -51,20 +49,21 @@ pub struct CustomSignedExtension;
 // Give the extension a name; this allows `AnyOf` to look it
 // up in the chain metadata in order to know when and if to use it.
 impl<T: Config> signed_extensions::SignedExtension<T> for CustomSignedExtension {
-    const NAME: &'static str = "CustomSignedExtension";
     type Decoded = ();
+    fn matches(identifier: &str, _type_id: u32, _types: &PortableRegistry) -> bool {
+        identifier == "CustomSignedExtension"
+    }
 }
 
 // Gather together any params we need for our signed extension, here none.
 impl<T: Config> ExtrinsicParams<T> for CustomSignedExtension {
     type OtherParams = ();
-    type Error = std::convert::Infallible;
 
     fn new<Client: OfflineClientT<T>>(
         _nonce: u64,
         _client: Client,
         _other_params: Self::OtherParams,
-    ) -> Result<Self, Self::Error> {
+    ) -> Result<Self, ExtrinsicParamsError> {
         Ok(CustomSignedExtension)
     }
 }
@@ -87,8 +86,8 @@ impl ExtrinsicParamsEncoder for CustomSignedExtension {
 pub fn custom(
     params: DefaultExtrinsicParamsBuilder<CustomConfig>,
 ) -> <<CustomConfig as Config>::ExtrinsicParams as ExtrinsicParams<CustomConfig>>::OtherParams {
-    let (a, b, c, d, e, f, g, h) = params.build();
-    (a, b, c, d, e, f, g, h, ())
+    let (a, b, c, d, e, f, g) = params.build();
+    (a, b, c, d, e, f, g, ())
 }
 
 #[tokio::main]

subxt/src/blocks/extrinsic_types.rs

@@ -13,7 +13,7 @@ use crate::{
 };
 
 use crate::config::signed_extensions::{
-    ChargeAssetTxPayment, ChargeTransactionPayment, CheckNonce, SkipCheckIfFeeless,
+    ChargeAssetTxPayment, ChargeTransactionPayment, CheckNonce,
 };
 use crate::config::SignedExtension;
 use crate::dynamic::DecodedValue;
@@ -660,24 +660,24 @@ impl<'a, T: Config> ExtrinsicSignedExtensions<'a, T> {
         })
     }
 
-    fn find_by_name(&self, name: &str) -> Option<ExtrinsicSignedExtension<'_, T>> {
-        let signed_extension = self
-            .iter()
-            .find_map(|e| e.ok().filter(|e| e.name() == name))?;
-        Some(signed_extension)
-    }
-
     /// Searches through all signed extensions to find a specific one.
     /// If the Signed Extension is not found `Ok(None)` is returned.
     /// If the Signed Extension is found but decoding failed `Err(_)` is returned.
     pub fn find<S: SignedExtension<T>>(&self) -> Result<Option<S::Decoded>, Error> {
-        self.find_by_name(S::NAME)
-            .map(|s| {
-                s.as_signed_extra::<S>().map(|e| {
-                    e.expect("signed extra name is correct, because it was found before; qed.")
-                })
-            })
-            .transpose()
+        for ext in self.iter() {
+            // If we encounter an error while iterating, we won't get any more results
+            // back, so just return that error as we won't find the signed ext anyway.
+            let ext = ext?;
+            match ext.as_signed_extension::<S>() {
+                // We found a match; return it:
+                Ok(Some(e)) => return Ok(Some(e)),
+                // No error, but no match either; next!
+                Ok(None) => continue,
+                // Error? return it
+                Err(e) => return Err(e),
+            }
+        }
+        Ok(None)
     }
 
     /// The tip of an extrinsic, extracted from the ChargeTransactionPayment or ChargeAssetTxPayment
@@ -696,20 +696,13 @@ impl<'a, T: Config> ExtrinsicSignedExtensions<'a, T> {
                     .flatten()
                     .map(|e| e.tip())
             })
-            .or_else(|| {
-                self.find::<SkipCheckIfFeeless<T, ChargeAssetTxPayment<T>>>()
-                    .ok()
-                    .flatten()
-                    .map(|skip_check| skip_check.inner_signed_extension().tip())
-            })
     }
 
     /// The nonce of the account that submitted the extrinsic, extracted from the CheckNonce signed extension.
     ///
     /// Returns `None` if `nonce` was not found or decoding failed.
     pub fn nonce(&self) -> Option<u64> {
-        let nonce = self.find::<CheckNonce>().ok()??.0;
-        Some(nonce)
+        self.find::<CheckNonce>().ok()?
     }
 }
 
@@ -744,20 +737,20 @@ impl<'a, T: Config> ExtrinsicSignedExtension<'a, T> {
         self.as_type()
     }
 
-    /// Decodes the `extra` bytes of this Signed Extension into a static type.
-    fn as_type<E: DecodeAsType>(&self) -> Result<E, Error> {
-        let value = E::decode_as_type(&mut &self.bytes[..], self.ty_id, self.metadata.types())?;
-        Ok(value)
-    }
-
-    /// Decodes the `extra` bytes of this Signed Extension into its associated `Decoded` type.
-    /// Returns `Ok(None)` if the identitfier of this Signed Extension object does not line up with the `NAME` constant of the provided Signed Extension type.
-    pub fn as_signed_extra<S: SignedExtension<T>>(&self) -> Result<Option<S::Decoded>, Error> {
-        if self.identifier != S::NAME {
+    /// Decodes the bytes of this Signed Extension into its associated `Decoded` type.
+    /// Returns `Ok(None)` if the data we have doesn't match the Signed Extension we're asking to
+    /// decode with.
+    pub fn as_signed_extension<S: SignedExtension<T>>(&self) -> Result<Option<S::Decoded>, Error> {
+        if !S::matches(self.identifier, self.ty_id, self.metadata.types()) {
             return Ok(None);
         }
         self.as_type::<S::Decoded>().map(Some)
     }
+
+    fn as_type<E: DecodeAsType>(&self) -> Result<E, Error> {
+        let value = E::decode_as_type(&mut &self.bytes[..], self.ty_id, self.metadata.types())?;
+        Ok(value)
+    }
 }
 
 #[cfg(test)]

subxt/src/config/default_extrinsic_params.rs

@@ -17,7 +17,6 @@ pub type DefaultExtrinsicParams<T> = signed_extensions::AnyOf<
         signed_extensions::CheckMortality<T>,
         signed_extensions::ChargeAssetTxPayment<T>,
         signed_extensions::ChargeTransactionPayment,
-        signed_extensions::SkipCheckIfFeeless<T, signed_extensions::ChargeAssetTxPayment<T>>,
     ),
 >;
 
@@ -132,9 +131,6 @@ impl<T: Config> DefaultExtrinsicParamsBuilder<T> {
         let charge_transaction_params =
             signed_extensions::ChargeTransactionPaymentParams::tip(self.tip);
 
-        let skip_check_params =
-            signed_extensions::SkipCheckIfFeelessParams::from(charge_asset_tx_params.clone());
-
         (
             (),
             (),
@@ -143,7 +139,6 @@ impl<T: Config> DefaultExtrinsicParamsBuilder<T> {
             check_mortality_params,
             charge_asset_tx_params,
             charge_transaction_params,
-            skip_check_params,
         )
     }
 }

subxt/src/config/extrinsic_params.rs

@@ -10,39 +10,41 @@
 use crate::{client::OfflineClientT, Config};
 use core::fmt::Debug;
 
-/// An error that can be emitted when trying to construct
-/// an instance of [`ExtrinsicParams`].
+/// An error that can be emitted when trying to construct an instance of [`ExtrinsicParams`],
+/// encode data from the instance, or match on signed extensions.
 #[derive(thiserror::Error, Debug)]
 #[non_exhaustive]
 pub enum ExtrinsicParamsError {
-    /// A signed extension was encountered that we don't know about.
-    #[error("Error constructing extrinsic parameters: Unknown signed extension '{0}'")]
+    /// Cannot find a type id in the metadata. The context provides some additional
+    /// information about the source of the error (eg the signed extension name).
+    #[error("Cannot find type id '{type_id} in the metadata (context: {context})")]
+    MissingTypeId {
+        /// Type ID.
+        type_id: u32,
+        /// Some arbitrary context to help narrow the source of the error.
+        context: &'static str,
+    },
+    /// A signed extension in use on some chain was not provided.
+    #[error("The chain expects a signed extension with the name {0}, but we did not provide one")]
     UnknownSignedExtension(String),
-    /// Cannot find the type id of a signed extension in the metadata.
-    #[error("Cannot find extension's '{0}' type id '{1} in the metadata")]
-    MissingTypeId(String, u32),
-    /// User provided a different signed extension than the one expected.
-    #[error("Provided a different signed extension for '{0}', the metadata expect '{1}'")]
-    ExpectedAnotherExtension(String, String),
-    /// The inner type of a signed extension is not present in the metadata.
-    #[error("The inner type of the signed extension '{0}' is not present in the metadata")]
-    MissingInnerSignedExtension(String),
-    /// The inner type of the signed extension is not named.
-    #[error("The signed extension's '{0}' type id '{1}' does not have a name in the metadata")]
-    ExpectedNamedTypeId(String, u32),
-    /// Some custom error.s
+    /// Some custom error.
     #[error("Error constructing extrinsic parameters: {0}")]
-    Custom(CustomError),
+    Custom(CustomExtrinsicParamsError),
 }
 
 /// A custom error.
-type CustomError = Box<dyn std::error::Error + Send + Sync + 'static>;
+pub type CustomExtrinsicParamsError = Box<dyn std::error::Error + Send + Sync + 'static>;
 
 impl From<std::convert::Infallible> for ExtrinsicParamsError {
     fn from(value: std::convert::Infallible) -> Self {
         match value {}
     }
 }
+impl From<CustomExtrinsicParamsError> for ExtrinsicParamsError {
+    fn from(value: CustomExtrinsicParamsError) -> Self {
+        ExtrinsicParamsError::Custom(value)
+    }
+}
 
 /// This trait allows you to configure the "signed extra" and
 /// "additional" parameters that are a part of the transaction payload
@@ -53,15 +55,12 @@ pub trait ExtrinsicParams<T: Config>: ExtrinsicParamsEncoder + Sized + 'static {
     /// help construct your [`ExtrinsicParams`] object.
     type OtherParams;
 
-    /// The type of error returned from [`ExtrinsicParams::new()`].
-    type Error: Into<ExtrinsicParamsError>;
-
-    /// Construct a new instance of our [`ExtrinsicParams`]
+    /// Construct a new instance of our [`ExtrinsicParams`].
     fn new<Client: OfflineClientT<T>>(
         nonce: u64,
         client: Client,
         other_params: Self::OtherParams,
-    ) -> Result<Self, Self::Error>;
+    ) -> Result<Self, ExtrinsicParamsError>;
 }
 
 /// This trait is expected to be implemented for any [`ExtrinsicParams`], and