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

FRAME: Meta Transaction (pallet based version) #4122

Closed
wants to merge 26 commits into from

Conversation

muharem
Copy link
Contributor

@muharem muharem commented Apr 15, 2024

Meta transaction implementation based on a pallet's call and configurable via transaction extensions.

To see an example, refer to the sign_and_execute_meta_tx test case in substrate/frame/meta-tx/src/tests.rs file.

This implementation serves to demonstrate the concept and not production ready.

Based on: #3685
RFC: #4123
Alternative solution: #3712

TODOs:

  • benchmarks
  • more tests

georgepisaltu and others added 20 commits March 8, 2024 13:29
Signed-off-by: georgepisaltu <george.pisaltu@parity.io>
Signed-off-by: georgepisaltu <george.pisaltu@parity.io>
Signed-off-by: georgepisaltu <george.pisaltu@parity.io>
Signed-off-by: georgepisaltu <george.pisaltu@parity.io>
Signed-off-by: georgepisaltu <george.pisaltu@parity.io>
Signed-off-by: georgepisaltu <george.pisaltu@parity.io>
Signed-off-by: georgepisaltu <george.pisaltu@parity.io>
Signed-off-by: georgepisaltu <george.pisaltu@parity.io>
Signed-off-by: georgepisaltu <george.pisaltu@parity.io>
Signed-off-by: georgepisaltu <george.pisaltu@parity.io>
Signed-off-by: georgepisaltu <george.pisaltu@parity.io>
Signed-off-by: georgepisaltu <george.pisaltu@parity.io>
Signed-off-by: georgepisaltu <george.pisaltu@parity.io>
…tx-ext

Signed-off-by: georgepisaltu <george.pisaltu@parity.io>
Signed-off-by: georgepisaltu <george.pisaltu@parity.io>
// TODO: to get the len we have to encode the original `meta_tx`.
let len = 0;
let mut ctx = T::TxContext::default();

Copy link
Contributor Author

Choose a reason for hiding this comment

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

we can use set the DispatchTransaction bound for Config::TxExtension and use it's impl for everything below. here, for the demonstration purpose, I'm not utilizing it.

Copy link
Contributor

Choose a reason for hiding this comment

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

DispatchTransaction is meant to work only for "legacy" extensions which use a () context. I believe the current impl is the right thing 👍 .

@@ -0,0 +1,53 @@
[package]
Copy link
Contributor

Choose a reason for hiding this comment

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

This whole crate is great, and should live in frame/examples if it remains in any form.

/// Signature type for meta transactions.
type Signature: Parameter + Verify<Signer = Self::PublicKey>;
/// Public key type used for signature verification.
type PublicKey: IdentifyAccount<AccountId = Self::AccountId>;
Copy link
Contributor

Choose a reason for hiding this comment

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

unrelated to this PR: Kind of a pitty that we can't fetch PublicKey and Signature from AccountId. The reverse is possible. You can always go from Signature (eg. MultiSignature) to PublicKey and then AccountId via IdentifyAccount.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in this scope I do not even need a public key type, but have no other way to ensure the signature type implemented over the account id the system operates with.

Copy link
Contributor

@georgepisaltu georgepisaltu left a comment

Choose a reason for hiding this comment

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

Looks great 👍

Comment on lines 41 to 43
// TODO: The `MetaTx` type is similar to `sp_runtime::generic::UncheckedExtrinsic`. However,
// `MetaTx` cannot replace generic::UncheckedExtrinsic because we need to box the call type,
// given that `MetaTx` is used as an argument type for a call.
Copy link
Contributor

Choose a reason for hiding this comment

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

We could probably have this "destructured" in the extrinsic argument list if it looks funny because of the UncheckedExtrinsic similarity. I think it's fine.

Comment on lines 63 to 66
pub enum Proof<Address, Signature> {
Signed(Address, Signature),
// TODO `General` as in `sp_runtime::generic::Preamble`.
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can branch off the UncheckedExtrinsic and Preamble logic a bit here and consider introducing a variant for any type of custom validation logic supported by the pallet. In TransactionExtensions, they would be individual extensions in the pipeline, one for each validation type, each wrapping a General transaction.

Here, we care only about the call and validation type, which is nice.

// TODO: to get the len we have to encode the original `meta_tx`.
let len = 0;
let mut ctx = T::TxContext::default();

Copy link
Contributor

Choose a reason for hiding this comment

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

DispatchTransaction is meant to work only for "legacy" extensions which use a () context. I believe the current impl is the right thing 👍 .

let post_info = res.unwrap_or_else(|err| err.post_info);
let pd_res = res.map(|_| ()).map_err(|e| e.error);

T::TxExtension::post_dispatch(pre, &info, &post_info, len, &pd_res, &ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

This would allow a graceful and harmless failure in cases like the one described here when a post_dispatch fail is critical.

pub struct MetaTx<Address, Signature, Call, TxExtension> {
proof: Proof<Address, Signature>,
call: Box<Call>,
tx_ext: TxExtension,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason why we can't get away with just a CheckNonce here? The other stuff will be checked when the agent submits the tx anyway, we just want the replay protection for Alice.

If we want to move off the extension approach completely, we could do the nonce check and inc without the extension even, just run the frame_system::Account mutation inside the extrinsic, but the signed payload would need to include the nonce. Maybe it's too complicated, just a thought.

@muharem muharem added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label May 7, 2024
@paritytech-cicd-pr
Copy link

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: cargo-clippy
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6152091

@muharem
Copy link
Contributor Author

muharem commented May 7, 2024

Clippy CI check fails on the file that is not edited by this PR. I will wait for the #3685 to be updated from master, to update this branch, which should address the issue.

Currently version of this branch compiles, tests pass.

@paritytech-review-bot paritytech-review-bot bot requested a review from a team October 23, 2024 11:30
@jasl
Copy link
Contributor

jasl commented Oct 27, 2024

Kindly ping.
Since Token Extension has merged, can it push Meta Transaction forward?

@muharem
Copy link
Contributor Author

muharem commented Oct 28, 2024

@jasl I am working on it

@muharem
Copy link
Contributor Author

muharem commented Nov 10, 2024

in favor of #6428

@muharem muharem closed this Nov 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants