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

Obtain DispatchError::Module info dynamically #453

Merged
merged 6 commits into from Feb 17, 2022
Merged

Conversation

jsdw
Copy link
Collaborator

@jsdw jsdw commented Feb 16, 2022

Static error info is troublesome if the node you interact with uses a different pallet index for the pallet you're working with. This PR moves error info back to being obtained via runtime metadata, but keeps the details() fn in codegen for now to provide a nicer API for obtaining it.

closes #443.

@jsdw jsdw requested review from ascjones and a team February 16, 2022 17:06
Copy link
Member

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

Using this API is a bit clunky, e.g.

            .wait_for_finalized_success()
            .await
            .map_err(|e| match e {
                subxt::Error::Runtime(err) => {
                    let inner = err.inner();
                    match inner.details(self.api.client.metadata()) {
                        Ok(Some(details)) =>
                            eyre::eyre!(
                                "Runtime: {} -> {}: {:?}",
                                details.pallet(),
                                details.error(),
                                details.description(),
                            ),
                        Ok(None) => eyre::eyre!("Runtime: {:?}", inner),
                        Err(e) => e.into(),
                    }
                }
                err => err.into(),
            })?;

Perhaps we could integrate it directly into wait_for_success, by defining a trait with fn module_error_indices(&self) -> (u8, u8) which is implemented by the macro where at the moment you define fn details(&metadata: Metadata). In this context we already have the self.client so can easily grab a reference to the metadata. Then we can construct a tuple (E, Option<ErrorMetadata>).

The slight wrinkle is what to do if the error cannot be resolved from the metadata, perhaps it would be better to roll that in to a None on the right hand side rather than having a nested Option<Result<ErrorMetadata>> or Option<Result<ErrorMetadata>>. Because the original error is still present on the left hand side, so can still match that case with (RuntimeError::ModuleError(_), None)

codegen/src/api/errors.rs Outdated Show resolved Hide resolved
@jsdw
Copy link
Collaborator Author

jsdw commented Feb 17, 2022

Oooh interesting! I definitely agree that the API is clunky!

Your suggestion has got me thinking; if we define the trait you suggest, we could extend the GenericError enum and add a specific Module variant that will contain the ErrorMetadata (or something resolved from it, depending on how easily lifetimes can be worked with) if the error returned can be resolved to that. What do you reckon?

@ascjones
Copy link
Member

Oooh interesting! I definitely agree that the API is clunky!

It wouldn't be a problem, except that you have to use the same pattern for every extrinsic call, so would have to use a similar helper method everywhere.

we could extend the GenericError enum and add a specific Module variant that will contain the ErrorMetadata (or something resolved from it, depending on how easily lifetimes can be worked with) if the error returned can be resolved to that. What do you reckon?

Yes that would be very nice, we are usually very interested in what the module error actually is.

@jsdw
Copy link
Collaborator Author

jsdw commented Feb 17, 2022

Ok, I've added a Module variant, and any DispatchError::Module's are now converted into that if possible. It does have to allocate some strings and such, but this may be preferable to having the error type carry a lifetime around (though something to ponder perhaps). If we fail to find the error details from the metadata we emit a metadata error rather than emitting the original module error (but this seems fair; the runtime metadata should contain the details).

Finally, if the original Module variant contains extra information beyond the pallet and error index, at present that'll be lost. When we drop backward compat we could more easily embed the new module error struct into this variant that's been added to preserve all of the info.

All that said, this API is much nicer to work with!

Copy link
Member

@ascjones ascjones left a comment

Choose a reason for hiding this comment

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

Lovely ❤️


impl<'client, T: Config, E: Decode, Evs: Decode> TransactionProgress<'client, T, E, Evs> {
impl<'client, T: Config, E: Decode + HasModuleError, Evs: Decode>
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe worth moving to a where-clause at this point?

@dvdplm dvdplm merged commit e866d74 into master Feb 17, 2022
@dvdplm dvdplm deleted the jsdw-dynamic-error-info branch February 17, 2022 13:45
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.

Dynamic DispatchError::Module details
3 participants