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

Moving pallet-asset-tx-payment from cumulus to substrate #10127

Merged
merged 34 commits into from
Nov 19, 2021
Merged

Moving pallet-asset-tx-payment from cumulus to substrate #10127

merged 34 commits into from
Nov 19, 2021

Conversation

georgesdib
Copy link
Contributor

@georgesdib georgesdib commented Oct 30, 2021

Fixes paritytech/cumulus#688.

Moving pallet-asset-tx-payment into substrate given there is nothing cumulus specific in it.

@apopiak @nukemandan
related cumulus PR building on this one: paritytech/cumulus#712

Polkadot address: 131dPecTmpTC1p1ofemufqFBJo9vNbV7dkgN7vWwKnaSMkC4

@apopiak
Copy link
Contributor

apopiak commented Nov 3, 2021

Thank you for your contribution! I took the liberty of moving the pallet into transaction-payment because it is an extension of that pallet.

@apopiak apopiak added the A0-please_review Pull request needs code review. label Nov 3, 2021
@apopiak apopiak 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 Nov 3, 2021
@apopiak
Copy link
Contributor

apopiak commented Nov 3, 2021

@shawntabrizi Do we want to add this pallet to the node runtime? Will break transaction format, but would probably be good to make sure that it keeps working with any changes.

@apopiak apopiak requested a review from tomusdrw November 3, 2021 09:44
@bkchr
Copy link
Member

bkchr commented Nov 3, 2021

The Substrate node is just for testing, for nothing else. So, it should be fine to add this.

@apopiak
Copy link
Contributor

apopiak commented Nov 3, 2021

@georgesdib
Could you please add the new pallet to the node runtime

I had to change the Balance type to u128.
Also harmonised that pallet's version
@georgesdib
Copy link
Contributor Author

@georgesdib
Could you please add the new pallet to the node runtime

Done, however I had to do quite a few changes, and some (like changing Balance from u64 to u128) could be controversial.

Copy link
Contributor

@apopiak apopiak left a comment

Choose a reason for hiding this comment

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

The SignedExtension is replaced, not added.

bin/node/cli/src/service.rs Outdated Show resolved Hide resolved
bin/node/cli/src/service.rs Outdated Show resolved Hide resolved
bin/node/runtime/src/lib.rs Outdated Show resolved Hide resolved
bin/node/runtime/src/lib.rs Outdated Show resolved Hide resolved
bin/node/runtime/src/lib.rs Show resolved Hide resolved
bin/node/test-runner-example/src/lib.rs Outdated Show resolved Hide resolved
@apopiak
Copy link
Contributor

apopiak commented Nov 4, 2021

@jacogr I imagine this PR would have an impact on how Polkadot-js handles the default Substrate node.

@apopiak apopiak added the A3-in_progress Pull request is in progress. No review needed at this stage. label Nov 4, 2021
@jacogr
Copy link
Contributor

jacogr commented Nov 4, 2021

No impact, the metadata defined which extensions should be used. (As long as it knows the type and in this case it does... assuming no changes to the format of the extension...)

@georgesdib
Copy link
Contributor Author

georgesdib commented Nov 11, 2021

How is this related to this PR?

It's not. I have reverted the impl-serde accidental upgrade.

@georgesdib
Copy link
Contributor Author

All checks are green, @kianenigma @tomusdrw look good to merge?

@apopiak apopiak added D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed. and removed D9-needsaudit 👮 PR contains changes to fund-managing logic that should be properly reviewed and externally audited labels Nov 12, 2021
Copy link
Contributor

@kianenigma kianenigma left a comment

Choose a reason for hiding this comment

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

Looks good to me!

georgesdib and others added 2 commits November 18, 2021 16:37
Reverting changes which broke cargo fmt
@georgesdib
Copy link
Contributor Author

@apopiak we have all approvals, shall we merge it?

@apopiak
Copy link
Contributor

apopiak commented Nov 19, 2021

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 4581dd9 into paritytech:master Nov 19, 2021
@georgesdib georgesdib deleted the asset-tx-payment-from-cumulus branch November 19, 2021 09:30
@bkchr
Copy link
Member

bkchr commented Nov 19, 2021

/tip small

@substrate-tip-bot
Copy link

Please fix the following problems before calling the tip bot again:

  • Contributor did not properly post their Polkadot or Kusama address. Make sure the pull request has: "{network} address: {address}".

@bkchr
Copy link
Member

bkchr commented Nov 19, 2021

@georgesdib if you add your address, I can ping the bot another time.

@georgesdib
Copy link
Contributor Author

@georgesdib if you add your address, I can ping the bot another time.

Done, thank you very much!

@bkchr
Copy link
Member

bkchr commented Nov 19, 2021

/tip small

@substrate-tip-bot
Copy link

A small tip was successfully submitted for georgesdib (131dPecTmpTC1p1ofemufqFBJo9vNbV7dkgN7vWwKnaSMkC4 on polkadot).

https://polkadot.js.org/apps/#/treasury/tips

grishasobol pushed a commit to gear-tech/substrate that referenced this pull request Mar 28, 2022
…h#10127)

* Moving `pallet-asset-tx-payment` from cumulus

* move pallet-asset-tx-payment into transaction payment directory

* cargo +nightly fmt

* Adding `pallet-asset-tx-payment` to node runtime
I had to change the Balance type to u128.
Also harmonised that pallet's version

* Updating cargo.lock after merge

* forgot this

* Adding tx-payment signature

* Missed one more

* `transaction-payment` replaced in`SignedExtension`
by `asset-tx-payment` and not added

* Fixing benches

* add test to verify that we don't charge on post-dispatch if we didn't on pre-dispatch

* add (failing) test for asset tx payment of unsigned extrinsics

* fix test by removing debug_assert

* cargo +nightly fmt

* typo in `Cargo.lock`

* Object defined twice in lock file

* cargo update

* remove todo

* Apply formatting suggestions from code review

Co-authored-by: Kian Paimani <[email protected]>

* Refactoring `post_dispatch` of `asset-tx-payment`
to reuse `post_dispatch` of `transaction-payment` if the fee asset is
native
Removing unneeded imports.

* Removing redundant `TODO`

* Reverting an accidental bump of `impl-serde`
 from `0.3.1` to `0.3.2`

* Revert unneeded changes to `cargo.lock`

* Update frame/transaction-payment/asset-tx-payment/src/payment.rs

Co-authored-by: Kian Paimani <[email protected]>

* Fixing cargo fmt

Reverting changes which broke cargo fmt

Co-authored-by: Alexander Popiak <[email protected]>
Co-authored-by: Alexander Popiak <[email protected]>
Co-authored-by: Kian Paimani <[email protected]>
MRamanenkau added a commit to Cerebellum-Network/pos-network-node that referenced this pull request Oct 14, 2022
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
…h#10127)

* Moving `pallet-asset-tx-payment` from cumulus

* move pallet-asset-tx-payment into transaction payment directory

* cargo +nightly fmt

* Adding `pallet-asset-tx-payment` to node runtime
I had to change the Balance type to u128.
Also harmonised that pallet's version

* Updating cargo.lock after merge

* forgot this

* Adding tx-payment signature

* Missed one more

* `transaction-payment` replaced in`SignedExtension`
by `asset-tx-payment` and not added

* Fixing benches

* add test to verify that we don't charge on post-dispatch if we didn't on pre-dispatch

* add (failing) test for asset tx payment of unsigned extrinsics

* fix test by removing debug_assert

* cargo +nightly fmt

* typo in `Cargo.lock`

* Object defined twice in lock file

* cargo update

* remove todo

* Apply formatting suggestions from code review

Co-authored-by: Kian Paimani <[email protected]>

* Refactoring `post_dispatch` of `asset-tx-payment`
to reuse `post_dispatch` of `transaction-payment` if the fee asset is
native
Removing unneeded imports.

* Removing redundant `TODO`

* Reverting an accidental bump of `impl-serde`
 from `0.3.1` to `0.3.2`

* Revert unneeded changes to `cargo.lock`

* Update frame/transaction-payment/asset-tx-payment/src/payment.rs

Co-authored-by: Kian Paimani <[email protected]>

* Fixing cargo fmt

Reverting changes which broke cargo fmt

Co-authored-by: Alexander Popiak <[email protected]>
Co-authored-by: Alexander Popiak <[email protected]>
Co-authored-by: Kian Paimani <[email protected]>
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. D5-nicetohaveaudit ⚠️ PR contains trivial changes to logic that should be properly reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Move pallet-asset-tx-payment into Substrate
6 participants