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

[pallet-balances] burn_allow_death extrinsic #3964

Merged

Conversation

Dinonard
Copy link
Contributor

@Dinonard Dinonard commented Apr 3, 2024

Adds an additional extrinsic call to the pallet-balances to burn tokens.
Depending on the keep_alive flag, the call might or might not reap the account.

Required modification of the fungible's Mutate trait, burn_from function to allow the Preservation argument.

TODO

  • run benchmarks & update weights
  • make sure prdoc is required & properly formatted

Related issue: #3943

@Dinonard Dinonard mentioned this pull request Apr 3, 2024
2 tasks
@Dinonard Dinonard requested a review from a team as a code owner April 3, 2024 11:41
#[pallet::weight(T::WeightInfo::burn_allow_death())]
pub fn burn_allow_death(
origin: OriginFor<T>,
#[pallet::compact] value: T::Balance,
Copy link
Member

Choose a reason for hiding this comment

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

An additional call is overkill, but a keep_alive: bool similar to thetransfer_all would be fine i think.

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 don't mind doing that, but will need to change this function signature:
https://github.com/paritytech/polkadot-sdk/blob/master/substrate/frame/support/src/traits/tokens/fungible/regular.rs#L260

It might end being a slightly lager change, hope that's ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated per request.

Will need help/suggestion with the remaining TODOs in the PR summary:

  • running benchmarks & updating weights (I cannot trigger the job)
  • check whether prdoc is ok

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 know you said in another PR that current review process isn't sustainable (I assume due to human resources), but is there someone else we can ask to take a look at the PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ggwpez sorry for the tag but could you please give a timeline when this can be checked out? Can we get someone else to review it?

Copy link
Member

Choose a reason for hiding this comment

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

Yea, it is best to ask in chat for review. We have the open #fellowship-open-channel:parity.io channel for that. I invited you.
Otherwise it can get lost or not have any priority...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks a lot!

@Dinonard Dinonard requested a review from a team as a code owner April 3, 2024 14:28
@paritytech-review-bot paritytech-review-bot bot requested a review from a team April 3, 2024 14:29
@ggwpez ggwpez requested review from a team and ggwpez and removed request for a team April 30, 2024 10:36
substrate/frame/balances/src/benchmarking.rs Outdated Show resolved Hide resolved
substrate/frame/balances/src/lib.rs Outdated Show resolved Hide resolved
substrate/frame/balances/src/lib.rs Show resolved Hide resolved
@ggwpez ggwpez added the T1-FRAME This PR/Issue is related to core FRAME, the framework. label Apr 30, 2024
@ggwpez
Copy link
Member

ggwpez commented Apr 30, 2024

bot bench-all pallet --pallet=pallet_balances

@command-bot
Copy link

command-bot bot commented Apr 30, 2024

@ggwpez https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6092354 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench-all/bench-all.sh" --pallet=pallet_balances. Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 4-02c5a23e-ef01-4cbd-aba7-be284195e69c to cancel this command or bot cancel to cancel all commands in this pull request.

@command-bot
Copy link

command-bot bot commented Apr 30, 2024

@ggwpez Command "$PIPELINE_SCRIPTS_DIR/commands/bench-all/bench-all.sh" --pallet=pallet_balances has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6092354 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6092354/artifacts/download.

Dinonard and others added 6 commits April 30, 2024 14:11
…s/pallet_balances.rs

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
…trinsics' into dinonard/pallet-balances-burn-extrinsics
@command-bot
Copy link

command-bot bot commented May 6, 2024

@mordamax Command "$PIPELINE_SCRIPTS_DIR/commands/bench-all/bench-all.sh" --pallet=pallet_balances has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6140797 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6140797/artifacts/download.

@Dinonard
Copy link
Contributor Author

Dinonard commented May 7, 2024

Thanks a lot, all the weight files have been updated!

Now just to get a second review 🙂

@liamaharon
Copy link
Member

@Dinonard thanks for this! if you want a tip, you can put your Polkadot address in the PR description like

polkadot address: <SS58 Address>

then tag me and I'll initiate it.

@Dinonard
Copy link
Contributor Author

Dinonard commented May 7, 2024

@liamaharon thanks! I will pass since I'm already part of Astar team, and taking a tip for this seems wrong 🙂

It seems I was premature in celebrating benchmark success though, rococo is still missing 🙃.

Can you please trigger it?
I assume only bot bench polkadot-pallet --pallet=pallet_balances --runtime=rococo-dev?

@liamaharon
Copy link
Member

liamaharon commented May 7, 2024

Hmm yeah it's also missing westend weights. @mordamax any ideas how we can bench all runtimes?

@Dinonard
Copy link
Contributor Author

Dinonard commented May 8, 2024

The only files missing weight updates are:

  • polkadot/runtime/rococo/src/weights/pallet_balances_balances.rs
  • polkadot/runtime/rococo/src/weights/pallet_balances_nis_counterpart_balances.rs

hence my suggestion to try bot bench polkadot-pallet --pallet=pallet_balances --runtime=rococo-dev.
(not sure it will work 🙃 )

@mordamax
Copy link
Contributor

mordamax commented May 8, 2024

that issue is known, we have it to fix in our backlog,
lets try ) im not sure either

bot bench polkadot-pallet --pallet=pallet_balances --runtime=rococo-dev

@command-bot
Copy link

command-bot bot commented May 8, 2024

@mordamax option '--runtime ' argument 'rococo-dev' is invalid. Allowed choices are rococo, westend.

@mordamax
Copy link
Contributor

mordamax commented May 8, 2024

bot bench polkadot-pallet --pallet=pallet_balances --runtime=rococo

@command-bot
Copy link

command-bot bot commented May 8, 2024

@mordamax https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6156561 was started for your command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=rococo --target_dir=polkadot --pallet=pallet_balances. Check out https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/pipelines?page=1&scope=all&username=group_605_bot to know what else is being executed currently.

Comment bot cancel 1-c26120b1-4f7e-40f2-9c19-05d76c505cc4 to cancel this command or bot cancel to cancel all commands in this pull request.

…=rococo --target_dir=polkadot --pallet=pallet_balances
@command-bot
Copy link

command-bot bot commented May 8, 2024

@mordamax Command "$PIPELINE_SCRIPTS_DIR/commands/bench/bench.sh" --subcommand=pallet --runtime=rococo --target_dir=polkadot --pallet=pallet_balances has finished. Result: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6156561 has finished. If any artifacts were generated, you can download them from https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/6156561/artifacts/download.

@mordamax
Copy link
Contributor

mordamax commented May 8, 2024

@Dinonard looks good now. I will need to make sure those ones are part of bench-all pallet 👍

@liamaharon liamaharon added this pull request to the merge queue May 8, 2024
Merged via the queue into paritytech:master with commit c3e57c1 May 8, 2024
146 of 147 checks passed
@Dinonard
Copy link
Contributor Author

Dinonard commented May 8, 2024

Thank you all for the support!

paritytech-ci pushed a commit that referenced this pull request May 8, 2024
Adds an additional extrinsic call to the `pallet-balances` to _burn_
tokens.
Depending on the `keep_alive` flag, the call might or might not reap the
account.

Required modification of the _fungible's_ `Mutate` trait, `burn_from`
function to allow the `Preservation` argument.

**TODO**
- [x] run benchmarks & update weights
- [x] make sure prdoc is required & properly formatted

Related issue: #3943

---------

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: command-bot <>
TomaszWaszczyk pushed a commit to TomaszWaszczyk/polkadot-sdk that referenced this pull request May 27, 2024
Adds an additional extrinsic call to the `pallet-balances` to _burn_
tokens.
Depending on the `keep_alive` flag, the call might or might not reap the
account.

Required modification of the _fungible's_ `Mutate` trait, `burn_from`
function to allow the `Preservation` argument.

**TODO**
- [x] run benchmarks & update weights
- [x] make sure prdoc is required & properly formatted

Related issue: paritytech#3943

---------

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: command-bot <>
hitchhooker pushed a commit to ibp-network/polkadot-sdk that referenced this pull request Jun 5, 2024
Adds an additional extrinsic call to the `pallet-balances` to _burn_
tokens.
Depending on the `keep_alive` flag, the call might or might not reap the
account.

Required modification of the _fungible's_ `Mutate` trait, `burn_from`
function to allow the `Preservation` argument.

**TODO**
- [x] run benchmarks & update weights
- [x] make sure prdoc is required & properly formatted

Related issue: paritytech#3943

---------

Co-authored-by: Oliver Tale-Yazdi <oliver.tale-yazdi@parity.io>
Co-authored-by: command-bot <>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T1-FRAME This PR/Issue is related to core FRAME, the framework.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants