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

Fix pallet_xcm::execute #4490

Merged
merged 3 commits into from
Jan 18, 2022
Merged

Fix pallet_xcm::execute #4490

merged 3 commits into from
Jan 18, 2022

Conversation

athei
Copy link
Member

@athei athei commented Dec 8, 2021

The max_weight argument is listed in the weight annotation and therefore the weight is already paid by including the extrinsic due to weight fee collection. Therefore we should credit it here.

We should also use weight correction here. I only use it in case we actually do the xcm execution. One could also use it in case we error out before reaching this point. This would add more complexity to the function. Not sure if worth.

I would need to add some additional tests if we consider merging this. Just wanted to make sure that my assumptions about this functions are correct before investing more work.

@athei athei added A0-please_review Pull request needs code review. B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited. labels Dec 8, 2021
Copy link
Member

@shawntabrizi shawntabrizi left a comment

Choose a reason for hiding this comment

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

Yup, this is a good addition to refund weight.

@shawntabrizi
Copy link
Member

shawntabrizi commented Dec 9, 2021

Actually I think there is a bit more handling needed here.

	/// How much weight was used by the XCM execution attempt.
	pub fn weight_used(&self) -> Weight {
		match self {
			Outcome::Complete(w) => *w,
			Outcome::Incomplete(w, _) => *w,
			Outcome::Error(_) => 0,
		}
	}

We probably should not be doing a weight refund returning an Error. So maybe you want to check for that or we update the logic above to return an option or Weight::MAX

@athei
Copy link
Member Author

athei commented Dec 9, 2021

We probably should not be doing a weight refund returning an Error.

Why? In case of Error the XCM execution didn't even start according to documentation. Also, the weight_used() function would be faulty if it doesn't return the weight used. Why do you suggest not trusting this functions?

Do you also agree with the other change (crediting the max_weight)?

@shawntabrizi
Copy link
Member

My only thought is that we don't support refunds when extrinsics error, and generally that provides an even larger negative incentive for people to submit any transactions which may potentially error.

You can think of the larger the potential fee, the more someone will be careful about sending a message which succeeds. Ideally, the blockchain is only consistent of successful transactions, but instead, we must store bad ones too since we need to charge a fee for that.

Anyway, practically speaking, you are right though, the weight returned here should be safe.

@gavofyork gavofyork merged commit d2a156d into master Jan 18, 2022
@gavofyork gavofyork deleted the at-xcm-weight branch January 18, 2022 16:39
Wizdave97 pushed a commit to ComposableFi/polkadot that referenced this pull request Feb 3, 2022
* Make `pallet_xcm::execute` supply the proper weight credit

* Use weight correction

Co-authored-by: Shawn Tabrizi <shawntabrizi@gmail.com>
@louismerlin louismerlin added D1-audited 👍 PR contains changes to critical logic that has been properly reviewed and externally audited. and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited. labels Oct 18, 2022
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. C1-low PR touches the given topic and has a low impact on builders. D1-audited 👍 PR contains changes to critical logic that has been properly reviewed and externally audited.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants