This repository has been archived by the owner on Nov 15, 2023. It is now read-only.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Asset Pallet: Support repeated destroys to safely destroy large assets #12310
Asset Pallet: Support repeated destroys to safely destroy large assets #12310
Changes from 19 commits
6a9d9d1
7db1fd5
cbe14b2
001eaef
1beae07
c9ce171
3067e38
7c4f610
aca497f
634345a
93d886d
2d3b650
5a612d1
b961255
91e3c18
60f954a
ce00b5b
5492b6d
bc20a6c
6ceb592
2edac52
aa2aecf
10fdd90
139d2e9
597f532
a09e762
70930ac
dc4a243
05d778d
62d4f13
e9e108b
5df4a62
544ac52
9206acf
29c7745
2986f29
6ac84a1
1f2930f
791aa7f
450a698
3daef79
ea34cf5
84cbb47
082a10c
e84d5ae
78cde15
c6470fb
85e50b9
c97e850
df27e0b
ca9fa2e
9609f13
26d27cc
72ec8f8
ee30cf8
87a7877
773212e
a4be57f
111da0e
9c06bb8
4eef069
486814f
c185cb0
91095af
9ef3c2b
6165576
7811b62
cd3e28d
aee596e
30814df
9b653a5
57fe747
7ba1ef5
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removal of that method would be a breaking change for the existing integrations, for those who use our pallet and for other similar assets pallets like orml. What could we do about this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's true.
Though my understanding from @joepetrowski is that this particular functionality isn't really used in the wild yet.
What do you propose we do about it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we could keep that destroy method for compatibility, but change the implementation and allow to call it if there are less than T::RemoveItemsLimit items to remove? In that case, this method would consist of the items amount check and will simply call
do_start_destroy(), do_destroy_accounts(), do_destroy_approvals(), do_finish_destroy()
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that makes sense. Though it's always to be a bit annoying maintaining 2 ways of doing the same thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
agree
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I weighed it in with @joepetrowski and since it seems no one is using the destroy extrinsic yet, it should be fine to break it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://statemint.subscan.io/event?address=&module=assets&event=destroyed&startDate=&endDate=&startBlock=&endBlock=&timeType=date&version=9290
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joepetrowski @tonyalaribe What if we simply put
start_destroy()
,destroy_accounts()
,destroy_approvals()
andfinish_destroy()
into the traits file and make those methods available for other pallets to use?