substrate
substrate copied to clipboard
paritytech
Add CallbackHandle to pallet-assets
This PR adds an option to register callbacks for pallet-assets which are invoked when a new asset is created or an existing one destroyed.
Fixes issue #12279.
It's a simple change but requires a small runtime code update. Benchmarks shouldn't be impacted in case default empty implementation is used.
Note: Will prepare UT & companion PRs if/once changes are confirmed to be ok.
TODO
- [ ] unit test
- [ ] Companion PRs
![cla-bot-2021[bot] avatar](https://avatars.githubusercontent.com/in/110390?v=4 "Published by a pub.dev verified publisher"){kind=small}
Any comment maybe? :slightly_smiling_face:
I'd like to jump on UTs and companion PRs if there are no objections (I know the change is minimal).
@Dinonard Thanks for making this PR! I've thinking a bit about this and it raises two questions:
- Is this not a usecase that could be covered by subscribing to the generated events?
- How will these callbacks interact with extrinsic weights? For eg, what if the caller of the function does very expensive calculations or storage operations in the callback? Won't we be unable to generate a correct weight in that state? And won't that be a possible attack vector?
cc @joepetrowski @jsidorenko
Hi @tonyalaribe!
- Is this not a usecase that could be covered by subscribing to the generated events?
Event subscription is for interacting with the client, it's not part of the state transition function, so I don't believe this is applicable here.
- How will these callbacks interact with extrinsic weights?
In case current pallet-assets users don't use this callback, they can just keep the weight as they are. In case they decide to introduce new custom logic, they have to rerun the benchmarks themselves and use the new generated weights instead.
The CI pipeline was cancelled due to failure one of the required jobs. The job name - cargo-check-benches The job logs - https://gitlab.parity.io/parity/mirrors/substrate/-/jobs/1969865
@ggwpez sorry if this is a silly question, but could you tell me if and what I can do next?
Should I prepare cumulus companion?
I see that contributing guide references polkadot companions, doesn't mention cumulus though.
@ggwpez sorry if this is a silly question, but could you tell me if and what I can do next?
Should I prepare
cumuluscompanion? I see that contributing guide referencespolkadotcompanions, doesn't mentioncumulusthough.
Sorry did not see.
We are waiting for audit. I will give them another ping. Better not change any code anymore then.
Your Cumulus companion looks good! Exactly how its done :laughing:
PS: Please dont force push - otherwise its difficult to see the change. You can just use git merge origin/master after pulling master.
Thank you!
PS: Please dont force push - otherwise its difficult to see the change. You can just use
git merge origin/masterafter pulling master.
My bad, will avoid force push in the future. I saw it's explained in the contributing guidelines :see_no_evil:
You can setup some helpers in your mock.rs for this: ...
Good to know! I will avoid changing this since you asked not to change any code anymore because of the audit (not sure if it's relevant since this is just a mock file but better safe than sorry).
Error: Github API says "Allow edits from maintainers" is not enabled for https://github.com/paritytech/cumulus/pull/1947. The bot would use that permission to push the lockfile update after merging this PR. Please check https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork.
![paritytech-processbot[bot] avatar](https://avatars.githubusercontent.com/in/144942?v=4 "Published by a pub.dev verified publisher"){kind=small}
Error: Github API says "Allow edits from maintainers" is not enabled for https://github.com/paritytech/cumulus/pull/1947. The bot would use that permission to push the lockfile update after merging this PR. Please check https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork.
Can this be merged? slightly_smiling_face
@Dinonard Well, i tried. You see the error message from the bot:
Error: Github API says "Allow edits from maintainers" is not enabled for https://github.com/paritytech/cumulus/pull/1947. The bot would use that permission to push the lockfile update after merging this PR. Please check https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork.
@Dinonard Well, i tried. You see the error message from the bot:
Thanks!
Not sure what to make of that error but in cumulus companion it says:
Rule "Runtime files" needs in total 2 DISTINCT approvals, but 1 were given.
Quite strange given that that there are two distinct approvals.
Quite strange given that that there are two distinct approvals.
You need another approval from someone in the polkadot-review group.
Error: Github API says "Allow edits from maintainers" is not enabled for https://github.com/paritytech/cumulus/pull/1947. The bot would use that permission to push the lockfile update after merging this PR. Please check https://docs.github.com/en/github/collaborating-with-pull-requests/working-with-forks/allowing-changes-to-a-pull-request-branch-created-from-a-fork.
You still did not update the Cumulus PR to be edit-able by maintainers @Dinonard
You still did not update the Cumulus PR to be edit-able by maintainers @Dinonard
Do you know how we can enable "allow edits by maintainers" from a fork that lives under an org account? This thread suggests that that is not possible. Worst case, we'll have to close this PR and open a new one from an individual account fork, or you manually merge this.
…or you manually merge this.
Yea. Then we know this for the future.
Thanks!
I initially checked with @hoonsubin since I thought it might be due to me not having admin privileges under AstarNetwork/cumulus.
Should I modify the companion PR by updating the .lock file to point to latest master?
This pull request has been mentioned on Polkadot Forum. There might be relevant details there:
https://forum.polkadot.network/t/polkadot-release-analysis-v0-9-37/1736/1
