Pallet assets: new status LiveAndNoPrivilege and new call revoke_all_privilege by gui1117 · Pull Request #4150 · paritytech/polkadot-sdk

gui1117 · 2024-04-16T08:23:06Z

Some coins don't want to have any privilege role once the distribution is done. We have seen that with $PINK coins which transfered its ownership to a killed proxy account, ensuring nobody can have this privilege.
The issue is that this is not easily discoverable. For instance light client can't get this proof easily.

So this PR introduces:

new asset status: live and no privilege. Under this status all Owner, Issuer, Freezer, Admin privilege are null.
new call: revoke_all_privilege. Can be called by owner or ForceOrigin, asset must be live. It set the status LiveAndNoPrivilege, and it also freezes the metadata.
new config associated type: DepositDestinationOnRevocation. When owner revoke its privilege, the deposit goes to this handler. Note that if ForceOrigin calls revoke_all_privilege then owner get its deposit back.

…hrough-status

franciscoaguirre · 2024-04-16T10:29:13Z

substrate/frame/assets/src/weights.rs

 			.saturating_add(T::DbWeight::get().writes(1_u64))
 	}
+	fn revoke_all_privileges() -> Weight {
+		Weight::zero()


Suggested change

Weight::zero()

Weight::MAX

Safer default

yes, I now ran benchmarks on my machine for a better default.

It should be fixed

prdoc/pr_4150.prdoc

joepetrowski · 2024-04-17T17:59:05Z

substrate/frame/assets/src/functions.rs

+
+	/// Check that owner is same as in asset details and that asset status is not
+	/// `LiveAndNoPrivileges`.
+	pub(super) fn check_owner_right(


Or verify_owner. Same for others.

Suggested change

pub(super) fn check_owner_right(

pub(super) fn check_owner(

Indeed, I changed internal API for a safer implementation, tell me what you think

…hrough-status

gui1117 · 2024-04-23T10:10:02Z

substrate/frame/assets/src/functions.rs

 		Asset::<T, I>::insert(&id, d);
+
+		Self::deposit_event(Event::TeamChanged { asset_id: id.clone(), issuer, admin, freezer });
+		Self::deposit_event(Event::OwnerChanged { asset_id: id, owner });


I added events here. This is unrelated to the PR, Let me know if this is unwanted

cumulus/parachains/runtimes/assets/asset-hub-rococo/src/weights/pallet_assets_foreign.rs

...rachains/integration-tests/emulated/tests/assets/asset-hub-rococo/src/tests/burn_at_relay.rs

...achains/integration-tests/emulated/tests/assets/asset-hub-westend/src/tests/burn_at_relay.rs

…hrough-status

paritytech-cicd-pr · 2024-08-27T04:38:27Z

The CI pipeline was cancelled due to failure one of the required jobs.
Job name: test-linux-stable-int
Logs: https://gitlab.parity.io/parity/mirrors/polkadot-sdk/-/jobs/7138716

muharem · 2024-08-22T15:22:07Z

substrate/frame/assets/Cargo.toml

 [package]
 name = "pallet-assets"
-version = "29.1.0"
+version = "30.0.0"


I think this will be done when released to avoid multiple version increments

Yes, fixed in 6594d94

muharem · 2024-08-22T15:37:27Z

substrate/frame/assets/src/lib.rs

 				let d = maybe_details.as_mut().ok_or(Error::<T, I>::Unknown)?;
-				ensure!(d.status == AssetStatus::Live, Error::<T, I>::AssetNotLive);
-				ensure!(origin == d.freezer, Error::<T, I>::NoPermission);
+				Self::ensure_live_asset(&d)?;


may be something like d.is_live(), etc .., so we could have it more consistent. it is easy to make a mistake with the statuses

I added the method on asset details: is_live. 6594d94

muharem · 2024-09-02T09:40:55Z

substrate/frame/assets/src/functions.rs

+
 	/// Reset the team for the asset with the given `id`.
 	///
+	/// If the asset status is `LiveAndNoPrivileges` then it is changed to `Live`.


Maybe worth mentioning something in ResetTeam trait doc that it is a force reset

Would you prefer to change the trait to have 2 functions: try_reset_team and force_reset_team instead?

EDIT: keeping the trait with just an additional doc could be fine considering any trait using ResetTeam would know that the owner doesn't exist when the status is LiveAndNoPrivileges

I only improved the doc: d0a8ab2

I think it is fine to keep the trait as it is, because reset_team is quite authoritative by itself, and caller should know why it can or cannot reset the team.

muharem · 2024-09-02T09:43:42Z

substrate/frame/assets/src/functions.rs

+		}
+		match d.try_set_team(&owner, &issuer, &admin, &freezer) {
+			Ok(()) => (),
+			Err(SetTeamError::AssetStatusLiveAndNoPrivileges) => log::error!(


should we still update the status on this failure? we probably wanna exit with this arm from the function

Yes ok, I added a new error variant: InternalError and abort in case of such internal error. 6594d94

muharem · 2024-09-02T09:48:57Z

substrate/frame/assets/src/lib.rs

+
+				match details.try_set_owner(&owner) {
+					Ok(()) => (),
+					Err(SetTeamError::AssetStatusLiveAndNoPrivileges) => log::error!(


should not we exit?

yes ok 6594d94

muharem · 2024-09-02T09:54:30Z

substrate/frame/assets/src/lib.rs

+				);
+				match res {
+					Ok(()) => (),
+					Err(SetTeamError::AssetStatusLiveAndNoPrivileges) => log::error!(


status will change to Live even though we failed to change the team

yes ok 6594d94

muharem · 2024-09-02T09:57:14Z

substrate/frame/assets/src/lib.rs

+				Err(origin) => Some(ensure_signed(origin)?),
+			};
+			let id: T::AssetId = id.into();
+			let mut asset = Asset::<T, I>::get(id.clone()).ok_or(Error::<T, I>::Unknown)?;


should work with a reference, no cloned needed

Great catch, fixed in 6594d94

muharem · 2024-09-02T10:15:20Z

substrate/frame/assets/src/lib.rs

+			Metadata::<T, I>::insert(&id, &new_metadata);
+
+			asset.deposit = Zero::zero();
+			asset.status = AssetStatus::LiveAndNoPrivileges;


AFAIK we do not have invalid account address for our crypto. not sure how reasonable (probably not) would be to have some valid but random address to be used as a common null address.

It is indeed a bit safer in case of error in the runtime.

We can copy the logic of pure proxy.

thinking again, the best would be a special account owned by the asset pallet (same as the treasury account), which doesn't have any action.

I will do it.

done in 5d30a4e

I copied the pallet proxy logic.

…of skipped

…hrough-status

eskimor · 2025-03-31T15:00:19Z

@gui1117 thanks for this initiative! Being able to drop privileges easily (and discoverable) is super important! Any chance we can get this over the finish line?

gui1117 · 2025-04-08T08:12:40Z

@gui1117 thanks for this initiative! Being able to drop privileges easily (and discoverable) is super important! Any chance we can get this over the finish line?

No idea, I think the PR is ready for review, I don't want to merge master if nobody reviews it.

gui1117 added 6 commits April 15, 2024 21:23

WIP

89ccc72

rename

e286945

Merge remote-tracking branch 'origin/master' into gui-assets-revoke-t…

a716e0f

…hrough-status

version and prdoc

9430c24

doc

f185216

formatting refactoring

ef36ecf

gui1117 requested a review from a team as a code owner April 16, 2024 08:23

prdoc

4f61356

franciscoaguirre reviewed Apr 16, 2024

View reviewed changes

joepetrowski reviewed Apr 17, 2024

View reviewed changes

jak-pan mentioned this pull request Apr 19, 2024

Add check for AssetHub admin keys galacticcouncil/hydration-ui#1249

Closed

gui1117 changed the title ~~Pallet assets: new status LiveAndNoPrivilege and new call revoke_all_privilege~~ WIP: Pallet assets: new status LiveAndNoPrivilege and new call revoke_all_privilege Apr 23, 2024

gui1117 added 2 commits April 23, 2024 14:36

Merge remote-tracking branch 'origin/master' into gui-assets-revoke-t…

31ebf61

…hrough-status

address comments improves prdoc

1c8185d

joepetrowski added the T2-pallets This PR/Issue is related to a particular pallet. label Apr 23, 2024

gui1117 added 2 commits April 23, 2024 18:49

safer internal API

fb0a90c

doc

602eeab

paritytech-review-bot bot requested a review from a team April 23, 2024 10:01

gui1117 commented Apr 23, 2024

View reviewed changes

gui1117 added 3 commits April 23, 2024 19:19

reorg and fix destroying assert

18358ad

fmt and typo

f7021bb

make runtimes burn when privilege is revoked

a2a3a26

gui1117 requested review from a team and athei as code owners April 23, 2024 10:43

gui1117 added 2 commits April 23, 2024 19:46

fmt

e2315c6

weights

27cf5a8

gui1117 changed the title ~~WIP: Pallet assets: new status LiveAndNoPrivilege and new call revoke_all_privilege~~ Pallet assets: new status LiveAndNoPrivilege and new call revoke_all_privilege Apr 23, 2024

gui1117 added 2 commits April 23, 2024 19:59

fake weight before benchmarking with bot

b5884a2

fix

a9f51b5

gui1117 commented Apr 23, 2024

View reviewed changes

cumulus/parachains/runtimes/assets/asset-hub-rococo/src/weights/pallet_assets_foreign.rs Show resolved Hide resolved

minor improvement

85e4fcc

joepetrowski approved these changes Jul 3, 2024

View reviewed changes

joepetrowski and others added 4 commits July 3, 2024 13:54

Apply suggestions from code review

47d4bfd

Merge branch 'master' into gui-assets-revoke-through-status

69ee8b4

add migration for penpal and rococo and do prdoc

5962bb5

fmt

36a3d7d

joepetrowski requested a review from muharem August 7, 2024 07:15

gui1117 added 2 commits August 7, 2024 16:28

Merge remote-tracking branch 'origin/master' into gui-assets-revoke-t…

a49b673

…hrough-status

Merge remote-tracking branch 'origin/master' into gui-assets-revoke-t…

905f658

…hrough-status

gui1117 force-pushed the gui-assets-revoke-through-status branch from a4baa63 to 905f658 Compare August 7, 2024 09:21

gui1117 added 3 commits August 7, 2024 21:21

Merge branch 'master' into gui-assets-revoke-through-status

35fb7e2

fix nostd

c0929cb

Merge remote-tracking branch 'origin/master' into gui-assets-revoke-t…

5be3576

…hrough-status

gui1117 added 2 commits August 27, 2024 14:02

fix tests

ec8f2d0

prdoc

50634c9

muharem reviewed Sep 2, 2024

View reviewed changes

gui1117 added 8 commits September 6, 2024 18:13

address review: refactor + error returned on internal error, instead …

6594d94

…of skipped

Merge remote-tracking branch 'origin/master' into gui-assets-revoke-t…

04bd5ae

…hrough-status

fixup

24ab012

better doc about reset team

d0a8ab2

fix test

d029b29

slightly better wording

0fbdeb3

use some random account for team with no privileges

5d30a4e

prdoc

6b99f08

pgherveou added this to [preview] release tracker Mar 18, 2026

github-project-automation bot moved this to Todo in [preview] release tracker Mar 18, 2026

pgherveou removed this from [preview] release tracker Mar 19, 2026

Conversation

gui1117 commented Apr 16, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gui1117 Apr 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paritytech-cicd-pr commented Aug 27, 2024

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!

gui1117 Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gui1117 Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

gui1117 Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

eskimor commented Mar 31, 2025

Uh oh!

gui1117 commented Apr 8, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

11 participants

gui1117 commented Apr 16, 2024 •

edited

Loading

gui1117 Apr 23, 2024 •

edited

Loading

gui1117 Sep 6, 2024 •

edited

Loading

gui1117 Sep 6, 2024 •

edited

Loading

gui1117 Sep 6, 2024 •

edited

Loading