Assets Pallet: reintroduce fungibles::Destroy trait by tonyalaribe · Pull Request #12690 · paritytech/substrate

tonyalaribe · 2022-11-11T13:18:29Z

This PR is an offshoot of #12310 , to maintain a way for users of the asset pallet still remove fungible assets.
It's a separate PR to allow it be reviewed easily without bloating #12310 further. But they should be both merged in together.

…ly-destroy-large-assets

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

…has begun the process; Add live check for other extrinsics

…hooks

…arge-assets

Co-authored-by: Muharem Ismailov <ismailov.m.h@gmail.com>

frame/support/src/traits/tokens/fungibles.rs

gilescope · 2022-11-11T16:48:44Z

frame/support/src/traits/tokens/fungibles.rs

+	/// - `id`: The identifier of the asset to be destroyed. This must identify an existing asset.
+	///
+	/// Each call Emits the `Event::DestroyedAccounts` event.
+	fn destroy_accounts(id: Self::AssetId, max_items: u32) -> Result<u32, DispatchError>;


left wondering what the u32 returned is. Is it the amount left or destroyed? Be good to be explicit about what it's returning.

That's true. I'll add it to the comment.

gilescope

Looks good aside from already mentioned typo.

muharem · 2022-11-11T17:00:59Z

frame/support/src/traits/tokens/fungibles.rs

-	/// Destroy an existing fungible asset.
+	/// Start the destruction an existing fungible asset.
 	/// * `id`: The `AssetId` to be destroyed.
 	/// * `witness`: Any witness data that needs to be provided to complete the operation


no witness arg anymore

Thanks for spotting this. I've taken it out.

muharem · 2022-11-11T17:01:18Z

frame/support/src/traits/tokens/fungibles.rs

@@ -281,9 +275,41 @@ pub trait Destroy<AccountId>: Inspect<AccountId> {
 	///
 	/// If successful, this function will return the actual witness data from the destroyed asset.


this part of the doc also needs to be updated

Yeah. Thanks again. Also taken it out now.

muharem · 2022-11-11T17:02:58Z

frame/support/src/traits/tokens/fungibles.rs

+	/// asset is in a `Destroying` state
+	///
+	/// Due to weight restrictions, this function may need to be called multiple
+	/// times to fully destroy all accounts. It will destroy `RemoveItemsLimit` accounts at a


there is no RemoveItemsLimit in this context

Yeah, I've updated the docs. The max_items value does the job in this case. Thanks for spotting it.

muharem · 2022-11-11T17:03:37Z

frame/support/src/traits/tokens/fungibles.rs

+	/// time.
+	///
+	/// - `id`: The identifier of the asset to be destroyed. This must identify an existing asset.
+	///


doc for max_items is missing

Yeah, i've added that now. Thanks

muharem · 2022-11-11T17:03:57Z

frame/support/src/traits/tokens/fungibles.rs

+	///
+	/// - `id`: The identifier of the asset to be destroyed. This must identify an existing asset.
+	///
+	/// Each call Emits the `Event::DestroyedAccounts` event.


we don't know about the event in this context

True. I wasn't sure how to word it. But i've removed the reference.

muharem

please update the docs, the docs from the implementation is not relevant for the trait

muharem · 2022-11-14T12:18:55Z

frame/support/src/traits/tokens/fungibles.rs

-		witness: Self::DestroyWitness,
-		maybe_check_owner: Option<AccountId>,
-	) -> Result<Self::DestroyWitness, DispatchError>;
+	/// - `id`: The identifier of the asset to be destroyed. This must identify an existing asset.


minor note, the formatting is different, with what you have above, I think the rust docs can be used as a reference https://doc.rust-lang.org/rust-by-example/meta/doc.html

I also might be important because polkadot js displays the docs in UI, but not sure what it exactly expects

jsidorenko · 2022-11-15T10:38:51Z

bot rebase

paritytech-processbot · 2022-11-15T10:39:00Z

Error: Command 'Command { std: "git" "merge" "origin/master" "--no-ff" "--no-edit", kill_on_drop: false }' failed with status Some(1); output: no output

tonyalaribe · 2022-11-15T11:32:29Z

bot rebase

paritytech-processbot · 2022-11-15T11:32:34Z

Branch is already up-to-date

tonyalaribe and others added 30 commits September 20, 2022 11:59

Support repeated destroys to safely destroy large assets

6a9d9d1

require freezing accounts before destroying

7db1fd5

support only deleting asset as final stage when there's no assets left

cbe14b2

Merge branch 'master' into aa/safely-destroy-large-assets

001eaef

pre: introduce the RemoveKeyLimit config parameter

1beae07

debug_ensure empty account in the right if block

c9ce171

update to having separate max values for accounts and approvals

3067e38

add tests and use RemoveKeyLimit constant

7c4f610

add useful comments to the extrinsics, and calculate returned weight

aca497f

Merge branch 'master' of github.com:paritytech/substrate into aa/safe…

634345a

…ly-destroy-large-assets

add benchmarking for start_destroy and finish destroy

93d886d

push failing benchmark logic

2d3b650

add benchmark tests for new functions

5a612d1

update weights via local benchmarks

b961255

remove extra weight file

91e3c18

Update frame/assets/src/lib.rs

60f954a

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

Update frame/assets/src/types.rs

ce00b5b

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

Update frame/assets/src/lib.rs

5492b6d

Co-authored-by: joe petrowski <25483142+joepetrowski@users.noreply.github.com>

effect some changes from codereview

bc20a6c

use NotFrozen error

6ceb592

remove origin checks, as anyone can complete destruction after owner …

2edac52

…has begun the process; Add live check for other extrinsics

fix comments about Origin behaviour

aa2aecf

add AssetStatus docs

10fdd90

modularize logic to allow calling logic in on_idle and on_initialize …

139d2e9

…hooks

introduce simple migration for assets details

597f532

Merge remote-tracking branch 'origin/master' into aa/safely-destroy-l…

a09e762

…arge-assets

reintroduce logging in the migrations

70930ac

move deposit_Event out of the mutate block

dc4a243

Update frame/assets/src/functions.rs

05d778d

Co-authored-by: Muharem Ismailov <ismailov.m.h@gmail.com>

Update frame/assets/src/migration.rs

62d4f13

Co-authored-by: Muharem Ismailov <ismailov.m.h@gmail.com>

tonyalaribe added 2 commits November 11, 2022 16:12

modify fungibles::Destroy public trait

b6cca71

reintroduce trait

9a18340

jsidorenko reviewed Nov 11, 2022

View reviewed changes

frame/support/src/traits/tokens/fungibles.rs Outdated Show resolved Hide resolved

jsidorenko reviewed Nov 11, 2022

View reviewed changes

frame/support/src/traits/tokens/fungibles.rs Outdated Show resolved Hide resolved

jsidorenko requested review from gilescope and joepetrowski November 11, 2022 14:40

gilescope reviewed Nov 11, 2022

View reviewed changes

gilescope approved these changes Nov 11, 2022

View reviewed changes

muharem reviewed Nov 11, 2022

View reviewed changes

update relevant trait documentation

01f3fe3

tonyalaribe added B3-apinoteworthy 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 Nov 14, 2022

jsidorenko approved these changes Nov 14, 2022

View reviewed changes

muharem reviewed Nov 14, 2022

View reviewed changes

muharem approved these changes Nov 14, 2022

View reviewed changes

update docs formatting

a408a3d

Base automatically changed from aa/safely-destroy-large-assets to master November 15, 2022 09:59

tonyalaribe mentioned this pull request Nov 15, 2022

Assets Pallet: reintroduce fungibles::Destroy trait #12708

Merged

Merge branch 'master' into aa/reintroduce-destroy-into-fungibles-trait2

c4cb5d0

tonyalaribe closed this Nov 15, 2022

		@@ -281,9 +275,41 @@ pub trait Destroy<AccountId>: Inspect<AccountId> {
		///
		/// If successful, this function will return the actual witness data from the destroyed asset.

Conversation

tonyalaribe commented Nov 11, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gilescope left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

muharem left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jsidorenko commented Nov 15, 2022

Uh oh!

paritytech-processbot bot commented Nov 15, 2022

Uh oh!

tonyalaribe commented Nov 15, 2022

Uh oh!

paritytech-processbot bot commented Nov 15, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants