Skip to content

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

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.

Sign up for GitHub

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

Merged
jsdw merged 8 commits into from
Feb 15, 2022
Merged

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

Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a95c77d
ModuleError is now a tuple variant
jsdw Feb 7, 2022
b73bdcd
Merge branch 'master' into jsdw-module-error
jsdw Feb 15, 2022
61b8034
more robust pointer to DispatchError
jsdw Feb 15, 2022
c1d1ff8
backward compat
jsdw Feb 15, 2022
2b76adb
clippy
jsdw Feb 15, 2022
e3e0d41
cargo fmt
jsdw Feb 15, 2022
99ea75b
abort_call_site and simplify path comparison
jsdw Feb 15, 2022
eec70a0
clippy
jsdw Feb 15, 2022
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
4 changes: 2 additions & 2 deletions codegen/src/api/errors.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,8 +81,8 @@ pub fn generate_error_details(metadata: &RuntimeMetadataV14) -> ErrorDetails {
},
dispatch_error_impl_fn: quote! {
pub fn details(&self) -> Option<ErrorDetails> {
if let Self::Module { index, error } = self {
match (index, error) {
if let Self::Module (module_error) = self {

@ascjones ascjones Feb 14, 2022

Copy link
Copy Markdown
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 :)

@jsdw jsdw Feb 14, 2022

Copy link
Copy Markdown
Contributor 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!

@ascjones ascjones Feb 14, 2022

Copy link
Copy Markdown
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.

@jsdw jsdw Feb 14, 2022

Copy link
Copy Markdown
Contributor 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..

@jsdw jsdw Feb 14, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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
}
Expand Down