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

Improve Dispatch Errors #878

Merged
merged 12 commits into from
Mar 23, 2023
Merged

Improve Dispatch Errors #878

merged 12 commits into from
Mar 23, 2023

Conversation

jsdw
Copy link
Collaborator

@jsdw jsdw commented Mar 22, 2023

Dispatch errors are a bit pants; we only decode the Module variant, and then we cloned a bunch of details out of metadata into it. This PR makes the following changes:

  • Expose more dispatch error details; this is important since token errors are now separate from module errors, and are quite a likely thing people might want to react to.
  • Use the DispatchError in our dry_run result too, so we get the same level of detail there (for dispatch erros at least).
  • Re-work ModuleError API; don't pull/clone things from metadata; call that on-demand instead.
  • Use DecodeAsType to decode DispatchErrors based on shape, so we don't have to worry about indexes changing if variants are added.
  • Mark all errors #[non_exhaustive] to future proof a little against new variants being added (with DecodeAsType, this at least gives us a path to add new variants and deprecate old ones without worrying about index stuff).

@jsdw jsdw marked this pull request as ready for review March 22, 2023 15:21
@jsdw jsdw marked this pull request as draft March 22, 2023 15:23
@jsdw jsdw marked this pull request as ready for review March 22, 2023 16:25
Other(String),
}

impl From<&str> for Error {
Copy link
Member

Choose a reason for hiding this comment

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

I think this is better, &str implies only static &str which is not that flexible to use.

Suggested change
impl From<&str> for Error {
impl<T: AsRef<str>> From<T> for Error {

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

But impl From<T> I think would conflict with all of the other From<InnerError> impls we have on Error :(

Let me double check that..

Copy link
Collaborator Author

@jsdw jsdw Mar 22, 2023

Choose a reason for hiding this comment

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

Ok nar that didn't work; conflicting impls!

I also did a test and this code worked fine:

fn test() {
    let s = String::from("hello");
    let r = s.as_str();
    let _e = Error::from(r);
}

So I think the From<&str> impl does not require &'static str :)

(now I look at it I am surprised that I didn't have to provide an explicit lifetime there)

Copy link
Member

@niklasad1 niklasad1 Mar 22, 2023

Choose a reason for hiding this comment

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

Ok, I did exactly the same for a while ago and rustc inferred it as 'static maybe just me then :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I put an explicit lifetime on this to make it clear, and because I'm surprised I didn't need one already!

subxt/src/error/mod.rs Outdated Show resolved Hide resolved
subxt/src/error/mod.rs Outdated Show resolved Hide resolved
Copy link
Member

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Collaborator

@lexnv lexnv left a comment

Choose a reason for hiding this comment

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

LGTM!

@jsdw jsdw merged commit b5194be into master Mar 23, 2023
@jsdw jsdw deleted the jsdw-better-dispatch-error branch March 23, 2023 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants