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

Add Event to Pallet Asset-Tx-Payment #11690

Merged
merged 5 commits into from Jul 29, 2022
Merged

Add Event to Pallet Asset-Tx-Payment #11690

merged 5 commits into from Jul 29, 2022

Conversation

hzy1919
Copy link
Contributor

@hzy1919 hzy1919 commented Jun 16, 2022 •
edited

Follow up to #11618
Fixes #11683
Fixes #11595
cumulus companion: paritytech/cumulus#1377

polkadot address: 15wPGN5vysDW1HYWfZRjQQhv2ktpfLMMuiJNMRKcodwPfM14

@hzy1919 hzy1919 mentioned this pull request Jun 16, 2022
Merged
shawntabrizi
shawntabrizi reviewed Jun 17, 2022
View reviewed changes
frame/transaction-payment/asset-tx-payment/src/lib.rs
Comment on lines +231 to +232
// asset_id for the transaction payment
Option<ChargeAssetIdOf<T>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

I am very confused why this PR is introducing this.

It makes sense, but it should have existed already. There is no way that adding an event should require adding more data into a signed extension

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I add this inorder to get 'asset_id' ,otherwise need to add parameter 'self' into the interface 'post_dispatch' in my humble opinion. Maybe there is a other better solution.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi , @shawntabrizi What do you think about this now? Would you like to share your solution ? 😃

Copy link
Member

Choose a reason for hiding this comment

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

@shawntabrizi that is not adding any new data to the signed extension. The only thing this is doing is that it adds a new return value to pre_dispatch that is then passed to post_dispatch

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, @bkchr you explain it more clearly . 👍

@kianenigma kianenigma added A0-please_review Pull request needs code review. B7-runtimenoteworthy C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit labels Jun 21, 2022
@stale
Copy link

stale bot commented Jul 27, 2022

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Jul 27, 2022
bkchr
bkchr approved these changes Jul 28, 2022
View reviewed changes
frame/transaction-payment/asset-tx-payment/src/lib.rs
Comment on lines +231 to +232
// asset_id for the transaction payment
Option<ChargeAssetIdOf<T>>,
Copy link
Member

Choose a reason for hiding this comment

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

@shawntabrizi that is not adding any new data to the signed extension. The only thing this is doing is that it adds a new return value to pre_dispatch that is then passed to post_dispatch

@bkchr bkchr requested a review from shawntabrizi July 28, 2022 08:34
@stale stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Jul 28, 2022
@bkchr
Copy link
Member

bkchr commented Jul 28, 2022

@hzy1919 can you please merge master?

@hzy1919
Copy link
Contributor Author

hzy1919 commented Jul 29, 2022

@hzy1919 can you please merge master?

@bkchr yeah, No problem!

@hzy1919
Copy link
Contributor Author

hzy1919 commented Jul 29, 2022

@bkchr I have merged master of Substrate and cumulus . I think the CI need to be manually activated again.

@shawntabrizi
Copy link
Contributor

bot rebase

@paritytech-processbot
Copy link

Rebased

@bkchr
Copy link
Member

bkchr commented Jul 29, 2022

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot
Copy link

Merge cancelled due to error. Error: Statuses failed for e438503

@bkchr
Copy link
Member

bkchr commented Jul 29, 2022

@hzy1919 can you please add a cumulus companion?

@hzy1919
Copy link
Contributor Author

hzy1919 commented Jul 29, 2022

@hzy1919 can you please add a cumulus companion?

@bkchr I had added a cumulus companion before. And I pushed a new commit to cumulus a moment ago.

@bkchr
Copy link
Member

bkchr commented Jul 29, 2022

@hzy1919 can you please add a cumulus companion?

@bkchr I had added a cumulus companion before. And I pushed a new commit to cumulus a moment ago.

Ha! Yeah! 😅

@bkchr
Copy link
Member

bkchr commented Jul 29, 2022

bot merge

@paritytech-processbot
Copy link

Waiting for commit status.

@paritytech-processbot
Copy link

Merge cancelled due to error. Error: Statuses failed for e438503

@paritytech-processbot
Copy link

Error: Response error (status 404 Not Found): 

{"documentation_url":"https://docs.github.com/rest/reference/orgs#check-organization-membership-for-a-user","message":"User does not exist or is not a member of the organization"}

@ggwpez
Copy link
Member

ggwpez commented Jul 29, 2022

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 3e0554a into paritytech:master Jul 29, 2022
@shawntabrizi
Copy link
Contributor

@hzy1919 you can add a polkadot address for a tip

@hzy1919
Copy link
Contributor Author

hzy1919 commented Jul 30, 2022

@hzy1919 you can add a polkadot address for a tip

@shawntabrizi Thanks . I have added my polkadot address in the PR description.

BTW, there is another tip mentioned here ，and it hasn't been executed , I don't know if it's still available or not . 😄

@shawntabrizi
Copy link
Contributor

@hzy1919 everything should be tipped now. feel free to remind me whenever if stuff like that happens. sorry!

@shawntabrizi
Copy link
Contributor

/tip small

@substrate-tip-bot
Copy link

@shawntabrizi A small tip was successfully submitted for hzy1919 (15wPGN5vysDW1HYWfZRjQQhv2ktpfLMMuiJNMRKcodwPfM14 on polkadot).

https://polkadot.js.org/apps/?rpc=wss%3A%2F%2Frpc.polkadot.io#/treasury/tips

@paritytech paritytech deleted a comment from substrate-tip-bot bot Jul 30, 2022
@hzy1919
Copy link
Contributor Author

hzy1919 commented Jul 30, 2022 •
edited

@hzy1919 everything should be tipped now. feel free to remind me whenever if stuff like that happens. sorry!

@shawntabrizi OK , It dosen't matter. :D

DaviRain-Su pushed a commit to octopus-network/substrate that referenced this pull request Aug 23, 2022
@hzy1919 @DaviRain-Su
Add Event to Pallet Asset-Tx-Payment (paritytech#11690)
9a24492 
* Add Event to Pallet Asset-Tx-Payment

* add asset_id into the Event

Co-authored-by: parity-processbot <>
@hbulgarini hbulgarini mentioned this pull request Aug 24, 2022
Closed
@librelois librelois mentioned this pull request Sep 15, 2022
Closed
2 tasks
@hzy1919 hzy1919 mentioned this pull request Oct 26, 2022
Closed
ark0f pushed a commit to gear-tech/substrate that referenced this pull request Feb 27, 2023
@hzy1919 @ark0f
Add Event to Pallet Asset-Tx-Payment (paritytech#11690)
ca3f060 
* Add Event to Pallet Asset-Tx-Payment

* add asset_id into the Event

Co-authored-by: parity-processbot <>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@shawntabrizi shawntabrizi shawntabrizi approved these changes

@bkchr bkchr bkchr approved these changes

Assignees
No one assigned
Labels
A0-please_review Pull request needs code review. C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add an event to Asset Transaction Payment Add TransactionPayment event to Pallet Tranascation Payment
5 participants
@hzy1919 @bkchr @shawntabrizi @ggwpez @kianenigma