Use the generated DispatchError instead of the hardcoded Substrate one #394

jsdw · 2022-01-14T16:39:02Z

The intention of this PR is to use the version of DispatchError generated from the metadata, rather than using a baked in version which may not be compatible with the node being targeted.

A rough summary of the changes:

Our Error type becomes Error<E> (actually, GenericError<RuntimeError<E>> under the hood) where E is the generated DispatchError.
A related type, BasicError (actually GenericError<Infallible> under the hood) also exists for the cases where we don't intend to return a Runtime error (ie most cases).
An E param was added where necessary to accomodate this new generic parameter in the error type (although using BasicError negates the need for it in many cases).
We previously had logic to obtain nicer error information (strings for name, docs) at runtime when converting from a DispatchError to our error type. This logic has moved to compile-time, so that we can obtain more details from the generated DispatchError more ergonomically
Debug has been added as a default trait for generated types to impl, mainly so that the generated DispatchError and its sub types can be debug printed inside our Error type.

… param there

…nto sp_version::RuntimeVersion

…ittle nicer to work with

dvdplm

Overall LGTM, nice work!

dvdplm · 2022-01-20T09:43:26Z

src/error.rs

+            GenericError::Invalid(e) => GenericError::Invalid(e),
+            GenericError::InvalidMetadata(e) => GenericError::InvalidMetadata(e),
+            GenericError::Metadata(e) => GenericError::Metadata(e),
+            GenericError::Runtime(e) => GenericError::Runtime(f(e)),


Maybe a comment here to point the reader to the salient spot?

dvdplm · 2022-01-20T09:55:03Z

src/subscription.rs

@@ -117,12 +118,12 @@ impl<'a, T: Config> EventSubscription<'a, T> {
    }

    /// Filters events by type.
-    pub fn filter_event<E: Event>(&mut self) {
-        self.event = Some((E::PALLET, E::EVENT));
+    pub fn filter_event<Ev: Event>(&mut self) {


I for one very much appreciate these renames of the generics. :)

dvdplm · 2022-01-20T10:46:46Z

codegen/src/api/errors.rs

+        })
+        .collect::<Result<Vec<(_, _)>, _>>()?;
+
+    let errors = pallet_errors


Maybe we can unify these two chains into a single? Like, why collect the pallet_errors first and then build the errors Vec?

It might be a bit tricky; we end up with Result<_> Items just prior to that collect, so the collecting I think is just there to give a chance to bail if we encounter any errors! I'll see whether I can do it in a clean way!

I settled on standard for loops; it felt tidier:

let mut pallet_errors = vec![]; for pallet in &metadata.pallets { let error = match &pallet.error { Some(err) => err, None => continue }; let type_def_variant = get_type_def_variant(error.ty.id())?; for var in type_def_variant.variants().iter() { pallet_errors.push(ErrorMetadata { pallet_index: pallet.index, error_index: var.index(), pallet: pallet.name.clone(), error: var.name().clone(), variant: var.clone(), }); } }

dvdplm · 2022-01-20T10:47:17Z

codegen/src/api/errors.rs

+}
+
+#[derive(Clone, Debug)]
+pub struct ErrorMetadata {


Missing docs here. Also, what do you think about moving this to the top?

dvdplm · 2022-01-20T10:48:05Z

codegen/src/api/errors.rs

+    pub error_index: u8,
+    pub pallet: String,
+    pub error: String,
+    variant: scale_info::Variant<scale_info::form::PortableForm>,


dq: what is the reason for storing the variant for?

ah, for the description()?

Yup, that's it! I'm not sure why I did/left this now. I've made it be just the doc string, which simplifies things a bit!

dvdplm · 2022-01-20T10:49:34Z

codegen/src/api/errors.rs

+
+impl ErrorMetadata {
+    pub fn description(&self) -> String {
+        self.variant.docs().join("\n")


So if the node is compiled to strip the docs, this will be empty? Worth detecting and add some kind of default, "No docs available in the metadata"?

In the empty case docs will just be "", which seems good enough (that can be detected and handled down the line in whichever way things prefer)

dvdplm · 2022-01-20T10:49:58Z

codegen/src/api/errors.rs

+}
+
+#[derive(Debug)]
+pub enum InvalidMetadataError {


Missing docs.

Ah; doesn't need to be pub :)

dvdplm · 2022-01-20T10:54:25Z

examples/custom_type_derives.rs

@@ -16,7 +16,7 @@

 #[subxt::subxt(
    runtime_metadata_path = "examples/polkadot_metadata.scale",
-    generated_type_derives = "Clone, Debug"
+    generated_type_derives = "Clone"


A comment here and on runtime_metadata_path would be good.

Looking further, I think this whole example could use some attention. I am not sure what it actually does. :/
(Not a concern for this PR ofc)

I gave the exampel a wee tidy; it's purpose appears solely to be showing off the generated_type_derives usage, so I made that a little clearer (and I didn't see a need for the strange way of calling clone offhand).

niklasad1

nice, LGTM

paritytech#394) * WIP DispatchError generic param * main crate now compiling with new E generic param for DispatchError * Remove E param from RpcClient since it doesn't really need it * Point to generated DispatchError in codegen so no need for additional param there * More error hunting; cargo check --all-targets passes now * Use our own RuntimeVersion struct (for now) to avoid error decoding into sp_version::RuntimeVersion * cargo fmt * fix docs and expose private documented thing that I think should be pub * move error info to compile time so that we can make DispatchError a little nicer to work with * cargo fmt * clippy * Rework error handling to remove <E> param in most cases * fix Error doc ambiguity (hopefully) * doc tweaks * docs: remove dismbiguation thing that isn't needed now * One more Error<E> that can be a BasicError * rewrite pallet errors thing into normal loops to tidy * tidy errors codegen a little * tidy examples/custom_type_derives.rs a little * cargo fmt * silcnce clippy in example

* Add error information back into metadata to roll back removal in #394 * Go back to obtaining runtime error info * re-do codegen too to check that it's all gravy * Convert DispatchError module errors into a module variant to make them easier to work with * Fix broken doc link

jsdw added 7 commits January 13, 2022 16:22

WIP DispatchError generic param

92c2c1f

main crate now compiling with new E generic param for DispatchError

55c1dcb

Remove E param from RpcClient since it doesn't really need it

2a8f094

Point to generated DispatchError in codegen so no need for additional…

2f19f9c

… param there

More error hunting; cargo check --all-targets passes now

5d0a56a

Use our own RuntimeVersion struct (for now) to avoid error decoding i…

940c770

…nto sp_version::RuntimeVersion

cargo fmt

8235ddc

This was referenced Jan 17, 2022

Invalid transaction version #392

Closed

Use released substrate dependencies #375

Merged

jsdw added 8 commits January 17, 2022 14:28

Merge branch 'master' into jsdw-dispatch-error

8425253

fix docs and expose private documented thing that I think should be pub

3748160

move error info to compile time so that we can make DispatchError a l…

63fbde8

…ittle nicer to work with

cargo fmt

b6e13bd

clippy

a458565

Rework error handling to remove <E> param in most cases

ad38bdb

fix Error doc ambiguity (hopefully)

8c7a207

doc tweaks

6309a1f

jsdw marked this pull request as ready for review January 19, 2022 16:38

jsdw added 2 commits January 19, 2022 17:09

Merge branch 'master' into jsdw-dispatch-error

f6f27db

docs: remove dismbiguation thing that isn't needed now

19fe205

dvdplm approved these changes Jan 20, 2022

View reviewed changes

jsdw added 6 commits January 20, 2022 11:55

One more Error<E> that can be a BasicError

5e6b82f

rewrite pallet errors thing into normal loops to tidy

19df387

tidy errors codegen a little

e1918eb

tidy examples/custom_type_derives.rs a little

d8e40fa

cargo fmt

424acb5

silcnce clippy in example

d7e5914

niklasad1 approved these changes Jan 20, 2022

View reviewed changes

jsdw merged commit 6437959 into master Jan 20, 2022

jsdw deleted the jsdw-dispatch-error branch January 20, 2022 16:35

jsdw mentioned this pull request Jan 21, 2022

[track] Release subxt to crates.io #345

Closed

18 tasks

ascjones mentioned this pull request Feb 9, 2022

Dynamic DispatchError::Module details #443

Closed

jsdw added a commit that referenced this pull request Feb 16, 2022

Add error information back into metadata to roll back removal in #394

ad6f19f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use the generated DispatchError instead of the hardcoded Substrate one #394

Use the generated DispatchError instead of the hardcoded Substrate one #394

jsdw commented Jan 14, 2022 •

edited

Loading

dvdplm left a comment

dvdplm Jan 20, 2022

dvdplm Jan 20, 2022

dvdplm Jan 20, 2022

jsdw Jan 20, 2022 •

edited

Loading

jsdw Jan 20, 2022

dvdplm Jan 20, 2022

dvdplm Jan 20, 2022

dvdplm Jan 20, 2022

jsdw Jan 20, 2022 •

edited

Loading

dvdplm Jan 20, 2022

jsdw Jan 20, 2022

dvdplm Jan 20, 2022

jsdw Jan 20, 2022

dvdplm Jan 20, 2022

dvdplm Jan 20, 2022

jsdw Jan 20, 2022 •

edited

Loading

niklasad1 left a comment

Use the generated DispatchError instead of the hardcoded Substrate one #394

Use the generated DispatchError instead of the hardcoded Substrate one #394

Conversation

jsdw commented Jan 14, 2022 • edited Loading

dvdplm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw Jan 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw Jan 20, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsdw Jan 20, 2022 • edited Loading

Choose a reason for hiding this comment

niklasad1 left a comment

Choose a reason for hiding this comment

jsdw commented Jan 14, 2022 •

edited

Loading

jsdw Jan 20, 2022 •

edited

Loading

jsdw Jan 20, 2022 •

edited

Loading

jsdw Jan 20, 2022 •

edited

Loading