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

[pallet_assets] How to destroy an asset with potentially u32::MAX accounts? #12275

Closed
Chralt98 opened this issue Sep 15, 2022 · 8 comments
Closed
Labels
J2-unconfirmed Issue might be valid, but it’s not yet known. Z7-question Issue is a question. Closer should answer.

Comments

@Chralt98
Copy link

Hey valuable Substrate maintainers,

I found out, that the witness limits the number of deletions here, but when there are too many accounts for one asset, then a sufficiently high number for the accounts in the witness param would lead to a weight which exceeds the maximum block limit. What could be done, if there are u32::MAX accounts with non-zero balances? Then, we couldn't destroy the asset right? Or is the consumed weight with u32::MAX below the maximum block limit? I don't think so.

What can I do to destroy a portion of all accounts? For example one could call an extrinsic multiple times with a small witness to destroy all u32::MAX accounts.

u32::MAX should only be an example to get a better understanding of this problem

@github-actions github-actions bot added the J2-unconfirmed Issue might be valid, but it’s not yet known. label Sep 15, 2022
@kianenigma
Copy link
Contributor

cc @jsidorenko

@kianenigma kianenigma added the Z7-question Issue is a question. Closer should answer. label Sep 15, 2022
@kianenigma kianenigma moved this to Backlog in Runtime / FRAME Sep 15, 2022
@jsidorenko
Copy link
Contributor

cc @tonyalaribe

@jsidorenko
Copy link
Contributor

What if we change the assets deletion process a bit:

  1. we mark an asset as soft-deleted and thus prevent any transfers
  2. we create a method to wipe user balances for such a token by passing either the addresses or the number of accounts to wipe. This would allow calling the same method multiple times
  3. and finally, completely delete the token

@ggwpez
Copy link
Member

ggwpez commented Sep 16, 2022

  1. we mark an asset as soft-deleted and thus prevent any transfers

freeze_asset could be enough for that.

@joepetrowski
Copy link
Contributor

It's a bit of an edge case, but freeze_asset should really be used in advance of deleting an asset anyway in order to provide the correct witness data. Otherwise, a simple transfer getting between your transaction construction and inclusion will invalidate the destroy call.

@tonyalaribe
Copy link
Contributor

tonyalaribe commented Sep 16, 2022

So an alternate solution would be to destroy the maximum number of accounts, and then emit a PartiallyDestroyed event to encourage reexecuting the action to complete destroying the asset. Like the refund and dissolve pattern in crowdloans

[Joe edit: changed master to commit to save in the future]

@jsidorenko
Copy link
Contributor

yeah, that could work as well

@tonyalaribe
Copy link
Contributor

I'll create a PR for this.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
J2-unconfirmed Issue might be valid, but it’s not yet known. Z7-question Issue is a question. Closer should answer.
Projects
Status: Done
Development

No branches or pull requests

6 participants