Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DispatchError::Module is now a tuple variant in latest Substrate #439

Merged
merged 8 commits into from
Feb 15, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 67 additions & 10 deletions codegen/src/api/errors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,9 @@ use proc_macro2::{
Span as Span2,
TokenStream as TokenStream2,
};
use proc_macro_error::abort_call_site;
use quote::quote;
use scale_info::TypeDef;

/// Tokens which allow us to provide static error information in the generated output.
pub struct ErrorDetails {
Expand Down Expand Up @@ -71,17 +73,48 @@ pub fn generate_error_details(metadata: &RuntimeMetadataV14) -> ErrorDetails {
}
});

ErrorDetails {
type_def: quote! {
pub struct ErrorDetails {
pub pallet: &'static str,
pub error: &'static str,
pub docs: &'static str,
}
},
dispatch_error_impl_fn: quote! {
let dispatch_error_def = metadata
.types
.types()
.iter()
.find(|&ty| ty.ty().path().segments() == ["sp_runtime", "DispatchError"])
.unwrap_or_else(|| {
abort_call_site!("sp_runtime::DispatchError type expected in metadata")
})
.ty()
.type_def();

// Slightly older versions of substrate have a `DispatchError::Module { index, error }`
// variant. Newer versions have something like a `DispatchError::Module (Details)` variant.
// We check to see which type of variant we're dealing with based on the metadata, and
// generate the correct code to handle either older or newer substrate versions.
let module_variant_is_struct = if let TypeDef::Variant(details) = dispatch_error_def {
let module_variant = details
.variants()
.iter()
.find(|variant| variant.name() == "Module")
.unwrap_or_else(|| {
abort_call_site!("DispatchError::Module variant expected in metadata")
});
let are_fields_named = module_variant
.fields()
.get(0)
.unwrap_or_else(|| {
abort_call_site!(
"DispatchError::Module expected to contain 1 or more fields"
)
})
.name()
.is_some();
are_fields_named
} else {
false
};

let dispatch_error_impl_fn = if module_variant_is_struct {
quote! {
pub fn details(&self) -> Option<ErrorDetails> {
if let Self::Module { index, error } = self {
if let Self::Module { error, index } = self {
match (index, error) {
#( #match_body_items ),*,
_ => None
Expand All @@ -90,7 +123,31 @@ pub fn generate_error_details(metadata: &RuntimeMetadataV14) -> ErrorDetails {
None
}
}
}
} else {
quote! {
pub fn details(&self) -> Option<ErrorDetails> {
if let Self::Module (module_error) = self {
Copy link
Contributor

Choose a reason for hiding this comment

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

One downside of this is that this codegen is now not backwards compatible with older versions of substrate. Not sure what a nice solution to this is. Of course we can always detect whether it is the tuple variant of the old struct variant, but then that is not a nice solution :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Mmm yeah; the only solution that comes to mind is doing a proper check for it in the metadata and conditionally outputting one or the other. Perhaps that's worth doing though so that this passes CI now (and continues to pass when the change finally lands)?

Side note: I'm wondering why the change hasn't made it to the binary that CI uses yet. Any ideas? I may have to look into that!

Copy link
Contributor

Choose a reason for hiding this comment

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

the only solution that comes to mind is doing a proper check for it in the metadata and conditionally outputting one or the other.

I agree, unfortunately that seems to be the only way to do this.

I'm wondering why the change hasn't made it to the binary that CI uses yet. Any ideas?

I thought it automatically uses a recent build from master? If that was working right it should be there. Unless something is being cached somewhere.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Asked in the CI/CD room. We rely on a binary made available at https://releases.parity.io/substrate/x86_64-debian:stretch/latest/substrate/substrate from a nightly job on substrate gitlab, but the binary is from a commit ~21 days old..

Copy link
Collaborator Author

@jsdw jsdw Feb 14, 2022

Choose a reason for hiding this comment

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

The nightly job could certainly build and run the latest binary from master, but doing so would slow down the regular CI a bunch I suspect, so it would keep failing (not sure how much caching would end up helping though), so I avoided it.

match (module_error.index, module_error.error) {
#( #match_body_items ),*,
_ => None
}
} else {
None
}
}
}
};

ErrorDetails {
type_def: quote! {
pub struct ErrorDetails {
pub pallet: &'static str,
pub error: &'static str,
pub docs: &'static str,
}
},
dispatch_error_impl_fn,
}
}

Expand Down
2 changes: 1 addition & 1 deletion codegen/src/api/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,7 @@ impl RuntimeGenerator {
#types_mod

/// The default error type returned when there is a runtime issue.
pub type DispatchError = self::runtime_types::sp_runtime::DispatchError;
pub type DispatchError = #types_mod_ident::sp_runtime::DispatchError;

// Statically generate error information so that we don't need runtime metadata for it.
#error_type
Expand Down