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 SignedExtension matching logic and remove SkipCheckIfFeeless bits #1283

Merged
merged 9 commits into from
Nov 23, 2023

Conversation

jsdw
Copy link
Collaborator

@jsdw jsdw commented Nov 22, 2023

Closes #1280.

  • SkipCheckIfFeeless is now transparent to the outside world in the latest polkadot-sdk, so remove logic specific to that.
  • But, remove SignedExtension::IDENTIFIER and replace with SignedExtension::matches(..) function which is able to do much more custom/intelligent matching to work out which of our signed extensions to use at which point (in part, just in case we have more complex things to do later, and in part just to provide the flexibility now for anybody that wants it).
  • Tidy some bounds (Debug/Clone only for the signed exts the user might handle ie the ones that are Decode targets) and no need for EncodeAsType/DecodeAsType on most of the exts now.
  • Just return ExtrinsicParamsError rather than type Error: Into<ExtrinsicParamsError>.

@jsdw jsdw changed the title Improve SignedExtension matching logic Improve SignedExtension matching logic and remove SkipCheckIfFeeless bits Nov 22, 2023
@jsdw jsdw marked this pull request as ready for review November 22, 2023 17:48
@jsdw jsdw requested a review from a team as a code owner November 22, 2023 17:48
@@ -258,22 +243,11 @@ impl<T: Config> ChargeAssetTxPayment<T> {
}

/// Parameters to configure the [`ChargeAssetTxPayment`] signed extension.
#[derive(Debug)]
Copy link
Member

@niklasad1 niklasad1 Nov 23, 2023

Choose a reason for hiding this comment

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

why removed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it would never have worked anyway afaik, because T: Config doesn't impl Debug. Maybe I was a bit overzealous in removing a bunch of derives for these things though; I did initially replace them all with working ones and then backed out of it!

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.

I like it, it simplifies the signed extension impls.

Copy link
Contributor

@tadeohepperle tadeohepperle left a comment

Choose a reason for hiding this comment

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

I don't really understand the reasons why SkipCheckIfFeeless was introduced and why we don't need it as a singed extension in subxt anymore.

@niklasad1
Copy link
Member

niklasad1 commented Nov 23, 2023

I don't really understand the reasons why SkipCheckIfFeeless was introduced and why we don't need it as a singed extension in subxt anymore.

AFAIU, SkipCheckIfFeeless was introduced as "wrapper signed extension" that wrapped something like "ChargeTxPayment", initially the name of the signed extension was "SkipCheckIfFeeless" but that was reverted to propagate the name of the signed extension that was wrapped.

Such SkipCheckIfFeeLess, its name will be "ChargeTxPayment" and not "SkipCheckIfFeeLess"

@jsdw
Copy link
Collaborator Author

jsdw commented Nov 23, 2023

I don't really understand the reasons why SkipCheckIfFeeless was introduced and why we don't need it as a singed extension in subxt anymore.

Yup, a couple of Substrate PRs made the name return the innet signed ext name, and the typeinfo match the inner typeinfo, so SkipCheckIfFeeless is now invisible to us and can be ignored again :)

@jsdw jsdw requested a review from a team as a code owner November 23, 2023 11:39
@jsdw jsdw merged commit 2c52885 into master Nov 23, 2023
9 of 10 checks passed
@jsdw jsdw deleted the jsdw-signed-ext-matching branch November 23, 2023 15:44
@jsdw jsdw mentioned this pull request Dec 7, 2023
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.

Alter SignedExtension and related to cater for more advanced exts like SkipCheckIfFeeless
4 participants