Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.

Conversation

@bragov4ik
Copy link

@bragov4ik bragov4ik commented Jun 27, 2023

Description

Extend WithPostDispatchInfo to allow easier specification of pays_fee when converting to DispatchErrorWithPostInfo.

At the time of PR that added with_weight (#5458), pays_fee field was not present, so it makes sense to add add such function.
For example, instead of

let mut e = DispatchErrorWithPostInfo::from(Error::<T>::InvalidParameters);
e.pays_fee = Pays::No;
e

one can write

Error::<T>::InvalidParameters.with_pays_fee(Pays::No)

My motivation in making this change is inconvenience when working with errors returned from other functions. In particular, I had the following case:

Self::fallible_function().map_err(|e| {
	let mut e = DispatchErrorWithPostInfo::from(e);
	e.pays_fee = Pays::No;
	e
})?;

and this should be cleaner and easier to read:

Self::fallible_function().map_err(|e| e.with_pays_fee(Pays::No))?;
  • I haven't found an issue about this and I'm not sure if it should be created, as it's mostly convenience PR.
  • Apparently, I don't have permissions to add labels, but I suppose it's something like A0, B0, C1
  • It doesn't change existing functions, only adds an extra one. Shouldn't break API, I guess. Correct me if I'm wrong please :)

@cla-bot-2021
Copy link

cla-bot-2021 bot commented Jun 27, 2023

User @bragov4ik, please sign the CLA here.

@bragov4ik
Copy link
Author

Also I wanted to implement WithPostDispatchInfo for DispatchErrorWithPostInfo to allow builder pattern for the latter (e.with_pays_fee(Pays::No).with_weight(1234)). However, that impl conflicted with existing <T: Into<DispatchError>> one ("upstream crates may add a new impl of trait" was the error).

One of the ways I see to fix it is to impl for DispatchError instead, but it might break some stuff, so I'm not sure about it :)

@bragov4ik bragov4ik marked this pull request as ready for review June 27, 2023 13:55
@bragov4ik bragov4ik requested review from a team June 27, 2023 13:55
@stale
Copy link

stale bot commented Jul 27, 2023

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A3-stale label Jul 27, 2023
@bragov4ik
Copy link
Author

Not sure why the CI doesn't pass with (lack of feature in sp-keystore) error. I didn't touch any features 🤔

@stale stale bot removed the A3-stale label Jul 31, 2023
@KiChjang KiChjang added A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Aug 17, 2023
post_info: PostDispatchInfo { actual_weight: None, pays_fee },
error: self.into(),
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like to me that we really should add a new function called with_post_info, so that callers can freely decide whether or not they want to use the default or modify any of the fields.

These methods should also exist in PostDispatchInfo rather than here as well, as it's not clear that the function actually returns a DispatchErrorWtihPostInfo after calling it.

@paritytech-ci paritytech-ci requested a review from a team August 17, 2023 04:16
@juangirini
Copy link
Contributor

Not sure why the CI doesn't pass with (lack of feature in sp-keystore) error. I didn't touch any features 🤔

It seems like the Polkadot companion is off, I will re-run the job.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

A0-please_review Pull request needs code review. B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants