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 (extension based version) #3712

Closed
wants to merge 20 commits into from

Conversation

muharem
Copy link
Contributor

@muharem muharem commented Mar 15, 2024

Experiment with a new Transaction Extension model that utilizes tx extensions to enable transaction fee sponsorship by an agent.

To see an example, refer to the meta_tx_works test case in substrate/frame/transaction-payment/src/meta_tx.rs file.

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

RFC: #4123

georgepisaltu and others added 17 commits March 8, 2024 13:29
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>
Signed-off-by: georgepisaltu <[email protected]>

// The part of `TxExtension` that has to be provided and signed by user who wants
// the transaciton fee to be sponsored by someone else.
type BaseTxExtension = (
Copy link
Contributor

Choose a reason for hiding this comment

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

@kianenigma
Copy link
Contributor

This is a pretty good #3901 type of work, glad to see a PoC.

@shawntabrizi
Copy link
Member

shawntabrizi commented Apr 1, 2024

Am I correct that this closes #266 ?

cc @xlc

@xlc
Copy link
Contributor

xlc commented Apr 2, 2024

Does this solve the challenges I listed in #266?

The common way to implement replay attack prevention mechanism is by having a sequence number / nonce that increments for each new message.

However, this is incompatible on the default storage model of Substrate. The nonce have to be stored onchain and all the onchain storages must be covered with a deposit (ED). For some use cases, the signing account will not hold any balances at all and therefore shouldn't really be able to cover the storage deposit for nonce storage.

i.e. how is the nonce and ED handled?

#[codec(decode_bound())]
pub struct SetFeeAgent<V: Verify>
where
V: Codec + Debug + Sync + Send + Clone + Eq + PartialEq + StaticTypeInfo,
Copy link
Contributor

Choose a reason for hiding this comment

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

better to avoid all those duplicated Codec + Debug + Sync + Send + Clone + Eq + PartialEq + StaticTypeInfo

@acatangiu
Copy link
Contributor

Possibly dumb question:

Why do this through transaction extensions? Even the tx ext set we have now is quite complicated and is seen as some "black magic" by many developers on Polkadot 😆.

Would this not be cleaner, clearer, less complicated to understand if it were implemented in a pallet?
Even the use-case itself seems to me a bit exotic to warrant "implementing it for everyone" through transaction extensions. I think beneficiaries of this feature would be perfectly satisfied going through some dedicated utility extrinsic, while the rest of Polkadot would appreciate less rather than even more complexity..

wdyt?

@xlc
Copy link
Contributor

xlc commented Apr 2, 2024

@acatangiu I don't think that's possible but I hope I am wrong

@muharem
Copy link
Contributor Author

muharem commented Apr 2, 2024

@xlc no, it does not. but I read the thread now, and I like what @shawntabrizi proposed. Basically implementation wont have any workaround for non-registered users, but it should provide a way to setup this feature, where for such users the provider reference can be used to register an account with a nonce.

@acatangiu yes, it's possible, and I am working on such option. Here I wanted to try what we can do with new Transaction Extensions and reuse some of them. With a solution based on custom pallet, I think we can still reuse them. I should have answers with examples soon, if we can reuse them and if it worths it.

@georgepisaltu
Copy link
Contributor

Why do this through transaction extensions? Even the tx ext set we have now is quite complicated and is seen as some "black magic" by many developers on Polkadot 😆.

It's about to get even more complicated as we shift the signature checking logic from the Checkable -> Applyable logic into its own (new-school) transaction extension (see POC impl in #3898 , which is a follow up to this PR). This will happen as we phase out (old-school) signed transactions in favor of (new-school) general transactions.

Would this not be cleaner, clearer, less complicated to understand if it were implemented in a pallet?

It would, for sure. As @muharem said, this is more of an experiment of what it would look like with this new interface. We haven't compared the extension vs pallet approaches yet, but I am confident it would be simpler if implemented in a pallet. The origin "swap" that the transaction extension does can happen in a particular pallet's extrinsic which would then call into the actual extrinsic you want to run with the updated origin.

Moreover, this extension we're building here (and any extension we end up using) would obviously be part of the pipeline for all transactions, but would only do useful work when the feature is used (sponsoring a transaction in this case). An extension like CheckTxVersion is perfect as a transaction extension because all transactions need to run this check, while something like this sponsored fee extension would run very rarely. Such use cases that don't serve all transactions are IMO not suited to the transaction extension model and should have dedicated pallet logic. This pallet logic can then be abstracted away by clients.

The closest example to this idea would be what pallet_utility does in batch or dispatch_as wrt wrapping calls and altering the origin: these extrinsics offer such low-level functionality that it's bad UX if end users ever need to manually construct a dispatch_as IMO, a client should do it for them where appropriate.

If the argument for an extension is that it would be easier to use, I'd counter with the fact that it would probably be even worse if we had an extension for dispatch_as that clients need to understand and deal with it every time they construct a transaction. Right now, they don't even worry about its existence because they don't have to, only people concerned are the ones that use it.

/// considered before regular transactions.
#[derive(Encode, Decode, Clone, Eq, PartialEq, TypeInfo)]
#[scale_info(skip_type_params(T))]
pub struct ChargeTransactionPayment<T: Config>(#[codec(compact)] BalanceOf<T>);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

no changes to the API of this type. get_priority method decoupled to the separate Priority type to be reused. the withdraw_fee and can_withdraw_fee just dropped.

@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/5913834

@muharem muharem changed the title FRAME: Sponsored Transaction Fee FRAME: Meta Transaction (extension based version) Apr 15, 2024
@muharem
Copy link
Contributor Author

muharem commented Apr 15, 2024

I have posted an RFC issue with the current solution and a solution based on a pallet, with some considerations. Please check it out and share you thoughts.
#4123
cc: @kianenigma @xlc @acatangiu @georgepisaltu

@Polkadot-Forum
Copy link

This pull request has been mentioned on Polkadot Forum. There might be relevant details there:

https://forum.polkadot.network/t/2024-04-23-technical-fellowship-opendev-call/7592/1

Base automatically changed from george/restore-gav-tx-ext to master October 18, 2024 18:29
@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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants