codegen for root level error by tadeohepperle · Pull Request #930 · paritytech/subxt

tadeohepperle · 2023-04-25T08:41:55Z

fixes #734.

Added an enum Error to the generated code. It has one variant for each pallet that exposes an error type.
It implements a new trait called RootError. The RootError trait allows the generated root Error type to be constructed from a pallet index (u8) and 4 error bytes:

pub trait RootError {
    fn root_error(pallet_index: &u8, error: &[u8; 4]) -> Result<Self, Error>;
}

The ModuleError struct now exposes a function as_root_error<E: RootError>(&self) -> Result<E, Error> that trys to convert the ModuleError into the generates top level enum Error.

This makes it possible to check if a certain subxt error is a certain error from a certain pallet:

fn is_err(error: subxt::Error, target: api::Error) -> bool {
    let subxt::Error::Runtime(DispatchError::Module(module_err) = subxt_error else {
        return false
    }
    module_err.as_root_error<api::Error>() == target
}

jsdw · 2023-04-25T10:11:14Z

codegen/src/api/mod.rs

+        };
+
+        let root_error_match_arms = self.metadata.pallets.iter().filter_map(|p| {
+            let variant_index = p.index;


We should compare based on variant names and not indexes; indexes can chanbe between nodes (as pallets tend to live in different indexes on different chains), so comparing on names is more portable :)

Ah okay, so a conversion from index to name should happen inside the as_root_error() function of this struct:

pub struct ModuleError { metadata: Metadata, raw: RawModuleError, }

using it's metadata field?

There's a details() function on the ModuleError already which will give you the pallet name that the error came from, so I'd probably call that inside the as_root_error function and then hand the name to the RootError trait so then the codegen can use the name to find the correct variant that was generated.

jsdw · 2023-04-25T10:12:55Z

codegen/src/api/mod.rs

+
+            p.error.as_ref().map(|_|
+                quote! {
+                    #variant_index => {


Just as a nit; I did "if arms" and not "match arms" for the RootEvent stuff, so I think we should be consistent one way or the other :)

The reason I did "if arms" was so that the impl below that uses this output is cleaner (match arms mean you have part of the match syntax here and part of it below which imo is a little less clean).

Ah okay, I liked the match style more, because it is less generated code, but I can change it to if arms

I'm not strongly against the match style; the "if" one just felt nicer code wise; either way both impls should be identical to be consistent :)

jsdw · 2023-04-25T10:13:17Z

codegen/src/api/mod.rs

+                    }
+                }
+
+


Nit; unnecessary newline :)

jsdw · 2023-04-25T10:21:16Z

subxt/src/error/mod.rs

+pub trait RootError: Sized {
+    /// Given details of the pallet error we want to decode
+    fn root_error(pallet_index: &u8, error: &[u8; 4]) -> Result<Self, Error>;
+}


I'd more closely mirror the events version which looks like:

pub trait RootEvent: Sized { fn root_event( pallet_bytes: &[u8], pallet_name: &str, pallet_event_ty: u32, metadata: &Metadata, ) -> Result<Self, Error>; }

So something more like this maybe:

pub trait RootError: Sized { fn root_error( pallet_bytes: [u8; 4], pallet_name: &str, pallet_event_ty: u32, metadata: &Metadata, ) -> Result<Self, Error>; }

Reasons:

We should avoid relying on Decode::decode if we can and instead use #mod_name::Error::decode_with_metadata to decode it. Why? because Decode::decode will just decode the bytes based on the static codegenned type, whereas decode_with_metadata will decode into the error enum based on matching up the encoded variant names and fields with the error struct we've codegenned, making it more robust to changes in eg variant order or field order or whatever.

Using a string instead of index for pallet; same as above, just allows more robust matching across nodes where pallets are likely in a different order.

Just generally to be consistent with RootError :)

jsdw · 2023-04-25T10:25:09Z

subxt/src/metadata/metadata_type.rs

+    /// Returns a reference to [`PalletMetadata`].
+    pub fn pallet_by_index(&self, pallet_index: u8) -> Result<&PalletMetadata, MetadataError> {
+        self.inner
+            .pallets
+            .values()
+            .find(|pallet_metadata| pallet_metadata.index == pallet_index)
+            .ok_or(MetadataError::PalletNotFound)
+    }


Maybe won't be needed when moving to using names over indexes

You're right, I removed the function.

tadeohepperle · 2023-04-25T15:39:25Z

I was not able to find a way to produce a ModuleError in an integration test, if one of you @jsdw @niklasad1 @lexnv knows how I can get a ModuleError returned from a substrate node, please let me know, then I add a test. Otherwise we can merge without test, it compiles at least.

lexnv · 2023-04-26T18:07:44Z

codegen/src/api/errors.rs

+    Ok(quote! {
+        #docs
+        pub type Error = #error_type;
+


Suggested change

Nit: no need for this extra line

lexnv · 2023-04-26T18:10:34Z

subxt/src/error/dispatch_error.rs

        self.raw
    }
+
+    /// attempts to decode the ModuleError into a value implementing the trait `RootError`


Suggested change

/// attempts to decode the ModuleError into a value implementing the trait `RootError`

/// Attempts to decode the ModuleError into a value implementing the trait `RootError`

Nit: we should start public documentation with upper case, for code comments this is fine tho

jsdw · 2023-04-27T15:15:23Z

subxt/src/error/mod.rs

+pub trait RootError: Sized {
+    /// Given details of the pallet error we want to decode
+    fn root_error(
+        pallet_bytes: &[u8; 4],


Nit: Not a big deal at all but I'd either go for [u8; 4] (no need for reference since it's only 4 bytes) or &[u8] (gives a little room for growth if the error type grows from [u8;4] for some reason) :)

jsdw

Added a test and it all looks to work well to me; nice job!

lexnv · 2023-05-02T08:20:24Z

codegen/src/api/mod.rs

                    should_gen_docs,
                )?;

+                let errors = errors::generate_error_type_alias(&type_gen, pallet, should_gen_docs)?;


nit: We could name this something like error_alias to be a bit more explicit, however you prefer here

lexnv · 2023-05-02T08:26:24Z

codegen/src/api/mod.rs

+        let outer_error_variants = self.metadata.pallets.iter().filter_map(|p| {
+            let variant_name = format_ident!("{}", p.name);
+            let mod_name = format_ident!("{}", p.name.to_string().to_snake_case());
+            let index = proc_macro2::Literal::u8_unsuffixed(p.index);
+
+            p.error.as_ref().map(|_| {
+                quote! {
+                    #[codec(index = #index)]
+                    #variant_name(#mod_name::Error),
+                }
+            })
+        });
+
+        let outer_error = quote! {
+            #default_derives
+            pub enum Error {
+                #( #outer_error_variants )*
+            }
+        };


Leaving here my thoughts, not necessarily to be done for this PR:

This part of the code seems highly similar to the generation of the outer Event enum.
There might be a few things we could do here to reduce code duplicate, improve reliability and maintenance:

add a function generate_outer_enum

take in the name of the enum (either Event / Error or other names we might add in the future)

#[derive(ToTokens)] enum OuterEnumName { Error, Event, }

lexnv

Nicely done!

The PR looks clean, I've left a few ideas that we could tackle in other PRs 👍

code gen for root error

60db3e5

tadeohepperle requested a review from a team as a code owner April 25, 2023 08:41

tadeohepperle and others added 3 commits April 25, 2023 10:42

Merge branch 'master' into tadeo-hepperle-root-level-error-codegen

e4f29df

cargo fmt

77c6fa6

polkadot.rs regenerated

fd192e2

jsdw reviewed Apr 25, 2023

View reviewed changes

codegen/src/api/mod.rs Outdated

}

}

Copy link

Collaborator

jsdw Apr 25, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit; unnecessary newline :)

jsdw reviewed Apr 25, 2023

View reviewed changes

tadeohepperle added 2 commits April 25, 2023 16:28

use pallet name and decode with metadata

e030846

remove pallet by name fn

7f4104f

Merge branch 'master' into tadeo-hepperle-root-level-error-codegen

00ecee6

lexnv reviewed Apr 26, 2023

View reviewed changes

Merge branch 'master' into tadeo-hepperle-root-level-error-codegen

da0647c

jsdw reviewed Apr 27, 2023

View reviewed changes

test that we can decode a ModuleError via as_root_error

e0359ae

jsdw approved these changes Apr 27, 2023

View reviewed changes

tadeohepperle and others added 2 commits April 28, 2023 09:54

nits

0106d46

Merge branch 'master' into tadeo-hepperle-root-level-error-codegen

7cea08b

lexnv reviewed May 2, 2023

View reviewed changes

lexnv approved these changes May 2, 2023

View reviewed changes

tadeohepperle merged commit 265f16f into master May 2, 2023

tadeohepperle deleted the tadeo-hepperle-root-level-error-codegen branch May 2, 2023 15:33

jsdw mentioned this pull request Jun 1, 2023

Prep for 0.29.0 release #992

Merged

	/// attempts to decode the ModuleError into a value implementing the trait `RootError`
	/// Attempts to decode the ModuleError into a value implementing the trait `RootError`

Conversation

tadeohepperle commented Apr 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tadeohepperle Apr 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tadeohepperle commented Apr 25, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsdw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lexnv left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tadeohepperle Apr 25, 2023 •

edited

Loading