decouple transaction payment and currency #6912

weichweich · 2020-08-18T12:19:57Z

polkadot companion: paritytech/polkadot#1784

Hi, we wanted to use the transaction payment pallet with a multi-currency pallet. We therefore needed to remove the Currency dependency from the transaction payment pallet. Is this something that could get merged?

This PR introduces an OnChargeTransaction trait that must be implemented and added to the pallet_transaction_payment::Trait. OnChargeTransaction implements how transaction fees are withdrawn and deposited. To keep the original behavior, I implemented OnChargeTransaction for a CurrencyAdapter which does the same as the implementation before.

These changes would break the API, because all chains need to change the implementation of the pallet_transaction_payment::Trait.

Before:

impl pallet_transaction_payment::Trait for Runtime {
	type Currency = Balances;
	type OnTransactionPayment = ();
	...
}

After:

use pallet_transaction_payment::CurrencyAdapter;

impl pallet_transaction_payment::Trait for Runtime {
	type OnChargeTransaction = CurrencyAdapter<Balances, ()>;
	...
}

cla-bot-2020 · 2020-08-18T12:20:00Z

@weichweich it looks like you have not signed our contributor license aggreement yet. Please visit this link to sign our agreement. This pull request cannot be merged until the agrement is signed.

cla-bot-2020 · 2020-08-18T12:39:13Z

@weichweich, Your signature has been received.

shawntabrizi · 2020-08-18T15:38:13Z

Where can I see the multi-currency pallet you want to integrate?

Have you considered implementing your own payment pallet for your needs rather than modifying ours?

weichweich · 2020-08-19T07:02:56Z

Where can I see the multi-currency pallet you want to integrate?

It's the token pallet from the ORML.

Have you considered implementing your own payment pallet for your needs rather than modifying ours?

Yes I considered that, but adjusting your payment pallet was easier, I think. I would rather maintain our changed version of the transaction payment pallet than writing everything from scratch.

For us it would just be more convenient to have this as part of substrate and maybe someone wants a more flexible payment pallet too? 😅 But it's also an option to maintain our own version.

gavofyork · 2020-08-19T15:40:09Z

Is it not possible to implement a Currency trait with an adaptor to the (multicurrency) Balance type (plus some other type that gives an index or key or whatever)? (The Currency trait is used to generalise over something that provides fungible tokens throughout Substrate Frame, so I don't see why we'd want to abstract it to something less specific here and not elsewhere.)

xlc · 2020-08-19T22:34:49Z

It is possible to implement Currency trait, but I the imbalance features makes it complicated. Maybe we can have a simpler version like this? https://github.com/open-web3-stack/open-runtime-module-library/blob/fa1eef43ad87f3b20f5e3bfaea84bf9ffaa8df18/traits/src/currency.rs#L174

weichweich · 2020-08-20T06:46:29Z

Is it not possible to implement a Currency trait with an adaptor to the (multicurrency) Balance type (plus some other type that gives an index or key or whatever)? (The Currency trait is used to generalise over something that provides fungible tokens throughout Substrate Frame, so I don't see why we'd want to abstract it to something less specific here and not elsewhere.)

As xlc said, the imbalances make this rather complicated. We also want to be able to pay the transaction fees in different currencies depending on the transaction (e.g. if you transfer a multi-currency, you pay the fees in the same currency). That's why we can't just drop in a currency implementation. We inspect the call and then decide which currency is used to pay the fees.

gavofyork · 2020-08-24T09:32:47Z

I see. Fair enough, then.

gavofyork · 2020-08-24T09:38:13Z

I'm ok with the general direction. Haven't done a deep review of the logic though.

xlc · 2020-09-03T03:43:16Z

@weichweich FYI orml-tokens now can implement trait Currency open-web3-stack/open-runtime-module-library#262

frame/transaction-payment/src/payment.rs

athei

I think this is almost ready. My only complaint is about naming and documentation on the trait.

frame/transaction-payment/src/payment.rs

improved documentation Co-authored-by: Alexander Theißen <alex.theissen@me.com>

Apply suggestions from code review Co-authored-by: Alexander Theißen <alex.theissen@me.com>

athei

Looks good to me now. You need to create a polkadot companion. If there are errors unrelated to your changes in the gitlab-check-polkadot-companion-build CI job you may need to merge master in this PR.

…currency

frame/transaction-payment/src/payment.rs

kianenigma · 2020-10-12T12:09:39Z

frame/transaction-payment/src/payment.rs

+		if fee.is_zero() {
+			return Ok(None);
+		}


This is already checked in the call site, but anyways good.

kianenigma · 2020-10-12T12:10:33Z

frame/transaction-payment/src/payment.rs

+	/// Note: The `fee` already includes the `tip`.
+	fn withdraw_fee(
+		who: &T::AccountId,
+		_call: &T::Call,


why are we passing the call here if it is not used? what is the future proof argument?

I guess the answer is this:

We also want to be able to pay the transaction fees in different currencies depending on the transaction (e.g. if you transfer a multi-currency, you pay the fees in the same currency). That's why we can't just drop in a currency implementation. We inspect the call and then decide which currency is used to pay the fees.

But honestly I don't get how you can achieve this in your multi-currency runtime. You still have one currency types passed in as C here. How can you implement something where you deduct from different currencies based on the call?

I am have not worked with such a multi currency pallet yet, so I might be missing some background info here.

We have another implementation of the OnChargeTransaction Trait. Our implementation doesn't require a Currency, but a multi currency.

frame/transaction-payment/src/payment.rs

kianenigma

All in all looks good, I just want to see a few questions that I had answered as well.

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

…currency

thiolliere

looks good to me

frame/transaction-payment/src/lib.rs

frame/transaction-payment/src/payment.rs

thiolliere · 2020-10-29T13:36:27Z

Audit is done, sorry for delay.
Can you merge master ?
Also I think this comment is relevant: #6912 (comment)

…currency

thiolliere · 2020-10-30T12:26:58Z

bot merge

ghost · 2020-10-30T12:27:03Z

Trying merge.

* adjustments for substrate PR paritytech/substrate#6912 * Update runtime/kusama/src/lib.rs Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com> * Apply suggestions from code review Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com> * update substrate Co-authored-by: Guillaume Thiolliere <gui.thiolliere@gmail.com>

weichweich added 11 commits August 18, 2020 09:29

wip: setup types

370ab13

fix types

96137cb

make tx payment pallet independent from balances

006ffcd

fix dependent tests

052d951

comments

78b503b

restructure a bit and include more info

9bb1b46

clean up ugly phantom

1d08e13

reduce complexity

af9bda7

minor doc improvements

96af556

use shorthand

308c220

doc

c724904

paritytech-ci added the D2-breaksapi label Aug 18, 2020

gavofyork added the A0-please_review Pull request needs code review. label Aug 19, 2020

gavofyork added B3-apinoteworthy 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 Aug 24, 2020

fix line lenght and style

99eb079

gnunicorn requested review from kianenigma and thiolliere and removed request for kianenigma October 1, 2020 14:16

athei reviewed Oct 5, 2020

View reviewed changes

frame/transaction-payment/src/payment.rs Show resolved Hide resolved

frame/transaction-payment/src/payment.rs Show resolved Hide resolved

athei reviewed Oct 5, 2020

View reviewed changes

frame/transaction-payment/src/payment.rs Outdated Show resolved Hide resolved

frame/transaction-payment/src/payment.rs Show resolved Hide resolved

weichweich and others added 2 commits October 5, 2020 13:14

Apply suggestions from code review

ada1d06

improved documentation Co-authored-by: Alexander Theißen <alex.theissen@me.com>

Improve naming and documentation

623b449

Apply suggestions from code review Co-authored-by: Alexander Theißen <alex.theissen@me.com>

athei approved these changes Oct 5, 2020

View reviewed changes

Merge remote-tracking branch 'upstream/master' into aw-tx-payment-no-…

b657448

…currency

weichweich mentioned this pull request Oct 5, 2020

Companion for #6912 paritytech/polkadot#1784

Merged

wischli pushed a commit to KILTprotocol/substrate that referenced this pull request Oct 6, 2020

paritytech#6912 decouple transaction payment and currency

e5a2a23

kianenigma reviewed Oct 12, 2020

View reviewed changes

frame/transaction-payment/src/payment.rs Outdated Show resolved Hide resolved

kianenigma reviewed Oct 12, 2020

View reviewed changes

frame/transaction-payment/src/payment.rs Show resolved Hide resolved

kianenigma reviewed Oct 12, 2020

View reviewed changes

weichweich and others added 2 commits October 12, 2020 21:39

Apply suggestions from code review

7bad69c

Co-authored-by: Kian Paimani <5588131+kianenigma@users.noreply.github.com>

Merge remote-tracking branch 'upstream/master' into aw-tx-payment-no-…

1bda8a8

…currency

thiolliere approved these changes Oct 13, 2020

View reviewed changes

frame/transaction-payment/src/lib.rs Outdated Show resolved Hide resolved

frame/transaction-payment/src/payment.rs Show resolved Hide resolved

thiolliere added D1-audited 👍 PR contains changes to fund-managing 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 29, 2020

weichweich added 4 commits October 30, 2020 08:25

Merge remote-tracking branch 'upstream/master' into aw-tx-payment-no-…

4f77443

…currency

always call withdraw_fee

2367a3e

move NegativeImbalanceOf to payment module

e5f1c64

fix unused import

1b73d2b

ghost merged commit 5d30787 into paritytech:master Oct 30, 2020

weichweich deleted the aw-tx-payment-no-currency branch October 30, 2020 12:59

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

decouple transaction payment and currency #6912

decouple transaction payment and currency #6912

weichweich commented Aug 18, 2020 •

edited

cla-bot-2020 bot commented Aug 18, 2020

cla-bot-2020 bot commented Aug 18, 2020

shawntabrizi commented Aug 18, 2020

weichweich commented Aug 19, 2020

gavofyork commented Aug 19, 2020 •

edited

xlc commented Aug 19, 2020

weichweich commented Aug 20, 2020

gavofyork commented Aug 24, 2020

gavofyork commented Aug 24, 2020

xlc commented Sep 3, 2020

athei left a comment

athei left a comment •

edited

kianenigma Oct 12, 2020

kianenigma Oct 12, 2020

kianenigma Oct 12, 2020

weichweich Oct 13, 2020

kianenigma left a comment

thiolliere left a comment

thiolliere commented Oct 29, 2020

thiolliere commented Oct 30, 2020

ghost commented Oct 30, 2020

decouple transaction payment and currency #6912

decouple transaction payment and currency #6912

Conversation

weichweich commented Aug 18, 2020 • edited

cla-bot-2020 bot commented Aug 18, 2020

cla-bot-2020 bot commented Aug 18, 2020

shawntabrizi commented Aug 18, 2020

weichweich commented Aug 19, 2020

gavofyork commented Aug 19, 2020 • edited

xlc commented Aug 19, 2020

weichweich commented Aug 20, 2020

gavofyork commented Aug 24, 2020

gavofyork commented Aug 24, 2020

xlc commented Sep 3, 2020

athei left a comment

Choose a reason for hiding this comment

athei left a comment • edited

Choose a reason for hiding this comment

kianenigma Oct 12, 2020

Choose a reason for hiding this comment

kianenigma Oct 12, 2020

Choose a reason for hiding this comment

kianenigma Oct 12, 2020

Choose a reason for hiding this comment

weichweich Oct 13, 2020

Choose a reason for hiding this comment

kianenigma left a comment

Choose a reason for hiding this comment

thiolliere left a comment

Choose a reason for hiding this comment

thiolliere commented Oct 29, 2020

thiolliere commented Oct 30, 2020

ghost commented Oct 30, 2020

weichweich commented Aug 18, 2020 •

edited

gavofyork commented Aug 19, 2020 •

edited

athei left a comment •

edited