[sui framework] add marker dynamic field to regulated coin metadata #19852

sblackshear · 2024-10-14T18:59:05Z

Add functions to determine whether whether a coin is regulated + whether it supports global pause or not by inspecting the metadata. Handy for use-cases that want to special case these coins.

vercel · 2024-10-14T18:59:12Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
sui-docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 22, 2024 9:28pm

3 Skipped Deployments

Name	Status	Preview	Updated (UTC)
multisig-toolkit	⬜️ Ignored (Inspect)	Visit Preview	Oct 22, 2024 9:28pm
sui-kiosk	⬜️ Ignored (Inspect)	Visit Preview	Oct 22, 2024 9:28pm
sui-typescript-docs	⬜️ Ignored (Inspect)	Visit Preview	Oct 22, 2024 9:28pm

damirka · 2024-10-15T10:10:15Z

crates/sui-framework/packages/sui-framework/sources/coin.move

+/// Return None if T was created at an epoch before `REGULATED_MARKER_START_EPOCH`. Before
+/// this epoch, it was not possible to distinguish between regulated and unregulated currencies by
+/// looking at the metadata.
+public fun is_regulated_currency<T>(metadata: &CoinMetadata<T>, ctx: &mut TxContext): Option<bool> {


I don't think context should be mutable here. Can't think of a single scenario where we'd find it useful.

Good point + I'm surprised there's no linter or compiler warning to yell at me. Is this because it would be too noisy in the case that folks do this for upgrade friendliness, and it's too hard to distinguish between that case and this one?

damirka · 2024-10-15T10:11:36Z

crates/sui-framework/packages/sui-framework/sources/coin.move

    });
+    if (ctx.epoch() >= REGULATED_MARKER_START_EPOCH) {
+        dynamic_field::add(&mut metadata.id, RegulatedMarker {}, allow_global_pause);
+        assert!(is_regulated_currency(&metadata, ctx).extract());


What is the reason behind this assert? May be missing something, but we make this assertion true in the line above, no?

Leftover debugging code; will remove

manolisliolios · 2024-10-15T13:11:06Z

crates/sui-framework/packages/sui-framework/sources/coin.move

        coin_metadata_object: object::id(&metadata),
        deny_cap_object: object::id(&deny_cap),
    });
+    if (ctx.epoch() >= REGULATED_MARKER_START_EPOCH) {


Why do we need to do epoch checks on creation / migration?

It's a fair question--I guess there's no harm to adding the marker right away. I will do that instead.

Reworked a lot of stuff based on this--now using an enum that gets added to every metadata (not jus the regulated one), and no more epoch checks. Thanks!

Love that way more, feels cleaner too!

Generally, this looks good to me. If we're definitely aligned on having this on the framework and not waiting for metadata V2!

manolisliolios · 2024-10-16T07:19:23Z

crates/sui-framework/packages/sui-framework/tests/coin_tests.move

+            false,
+            ctx,
+        );
+        assert!(coin::is_regulated_currency(&metadata).is_some());


Blind (untested) comment but receiver syntax should work, no? They are both in the coin module (can be applied to all tests / calls).

Suggested change

assert!(coin::is_regulated_currency(&metadata).is_some());

assert!(metadata.is_regulated_currency().is_some());

manolisliolios · 2024-10-16T07:44:27Z

@sblackshear should we also allow existing regulated coins to migrate their metadata (add the marker) by presenting the DenyCapV2?

sblackshear · 2024-10-17T00:08:02Z

@sblackshear should we also allow existing regulated coins to migrate their metadata (add the marker) by presenting the DenyCapV2?

I looked into this, but I don't think it's easy to do in a foolproof way because of the supported DenyCap -> DenyCapV2 migration. E.g., someone might add the marker with DenyCap and get allows_global_pause, then migrate DenyCap to DenyCapV2 and switch on the global pause without updating the metadata. So I'd rather leave this out and add it if someone needs it, with the semantics they'd expect.

tnowacki · 2024-10-18T21:26:42Z

crates/sui-framework/packages/sui-framework/sources/coin.move


+/// A dynamic field key added to `CoinMetadata` objects created after protocol version 63.
+/// This is bound to a `RegulatedInfo` enum
+public struct RegulatedMarker has copy, drop, store {}


nit

Suggested change

public struct RegulatedMarker has copy, drop, store {}

public struct RegulatedMarker() has copy, drop, store;

tnowacki · 2024-10-18T21:39:57Z

crates/sui-framework/packages/sui-framework/sources/coin.move

 //    b"Kill switch was not allowed at the creation of the DenyCapV2";
 const EGlobalPauseNotAllowed: u64 = 3;

+


Suggested change

tnowacki · 2024-10-18T21:46:40Z

crates/sui-framework/packages/sui-framework/sources/coin.move

+
+/// Return Some(true) if `T` is a regulated currency that allows global pause, Some(false) if it is
+/// regulated but does not allow global pause, or is an ordinary currency.
+/// Return None if T was created at an epoch before `REGULATED_MARKER_START_EPOCH`. Before


What is REGULATED_MARKER_START_EPOCH?

Good call--this is tale + I will fix

tnowacki · 2024-10-18T21:46:58Z

crates/sui-framework/packages/sui-framework/sources/coin.move

+    /// An ordinary coin that does not support a denylist or global pause
+    Ordinary,
+    /// A regulated coin that supports a denylist, and (maybe) global paus
+    Regulated { allow_global_pause: bool}


What if we did something like

Suggested change

Regulated { allow_global_pause: bool}

Regulated { deny_cap: ID }

which points to the DenyCapV2. Obviously some issues if the object is wrapped/deleted, but the flag allow_global_pause cannot be modified after creation, so it is fine to look at the last time the DenyCap existed.

My goal here is to allow existing coins (at least regulated ones) to add their RegulatedInfo after the fact.

The intention of this change is to allow Move code to special-case regulated coins (e.g., DEX pool creation, which typically already asks for CoinMetadata to get the decimals). If the Move code wants to treat global pause coins differently, it needs a boolean, not a pointer.

Would be fine adding the deny cap ID as additional metadata--more info seems better for sure

manolisliolios · 2024-10-21T21:38:37Z

crates/sui-framework/packages/sui-framework/sources/coin.move

        coin_metadata_object: object::id(&metadata),
        deny_cap_object: object::id(&deny_cap),
    });
+    dynamic_field::add(


Since we're allowing both V1 and V2 denycaps, do we really need to store the id of the deny_cap in the marker? It can get invalid the moment you migrate V1 to V2.

Yeah that's fair, the migration is giant PiTA that basically makes everything potentially inconsistent except for marking the metadata as regulated/not. Will revert to that

manolisliolios · 2024-10-21T21:39:26Z

crates/sui-framework/packages/sui-framework/sources/coin.move

+    dynamic_field::add(
+        &mut metadata.id,
+        RegulatedMarker(),
+        RegulatedInfo::Regulated { allow_global_pause: false, deny_cap_id: object::id(&deny_cap) }


allow_global_pause can end up converted to true when migrating to v2, while the metadata wouldn't reflect. Isn't this a small vulnerability for protocols who might trust this field?

Add functions to determine whether whether a coin is regulated + whether it supports global pause or not by inspecting the metadata. Handy for use-cases that want to special case these coins.

github-actions · 2024-12-22T02:06:16Z

This PR is stale because it has been open 60 days with no activity. Remove stale label or comment or this will be closed in 7 days.

manolisliolios

lgtm

sblackshear requested review from damirka, manolisliolios and tnowacki October 14, 2024 18:59

damirka reviewed Oct 15, 2024

View reviewed changes

manolisliolios reviewed Oct 15, 2024

View reviewed changes

sblackshear force-pushed the regulated_coin branch from cab8bc1 to cf92358 Compare October 16, 2024 00:26

sblackshear requested a review from ronny-mysten as a code owner October 16, 2024 00:26

vercel bot deployed to Preview – sui-docs October 16, 2024 00:28 View deployment

manolisliolios reviewed Oct 16, 2024

View reviewed changes

sblackshear force-pushed the regulated_coin branch from cf92358 to 53fcfee Compare October 17, 2024 00:42

vercel bot deployed to Preview – sui-docs October 17, 2024 00:43 View deployment

sblackshear force-pushed the regulated_coin branch from 53fcfee to f289364 Compare October 18, 2024 15:20

vercel bot deployed to Preview – sui-docs October 18, 2024 15:22 View deployment

tnowacki reviewed Oct 18, 2024

View reviewed changes

sblackshear force-pushed the regulated_coin branch from f289364 to 244b25b Compare October 21, 2024 19:00

vercel bot deployed to Preview – sui-docs October 21, 2024 19:01 View deployment

sblackshear force-pushed the regulated_coin branch from 244b25b to 3796ee3 Compare October 21, 2024 21:13

vercel bot deployed to Preview – sui-docs October 21, 2024 21:15 View deployment

manolisliolios reviewed Oct 21, 2024

View reviewed changes

[sui framework] add marker dynamic field to regulated coin metadata

b940acf

Add functions to determine whether whether a coin is regulated + whether it supports global pause or not by inspecting the metadata. Handy for use-cases that want to special case these coins.

sblackshear force-pushed the regulated_coin branch from 3796ee3 to b940acf Compare October 22, 2024 21:26

vercel bot deployed to Preview – sui-docs October 22, 2024 21:28 View deployment

github-actions bot added the Stale label Dec 22, 2024

manolisliolios approved these changes Jan 22, 2025

View reviewed changes

	assert!(coin::is_regulated_currency(&metadata).is_some());
	assert!(metadata.is_regulated_currency().is_some());

	public struct RegulatedMarker has copy, drop, store {}
	public struct RegulatedMarker() has copy, drop, store;

		// b"Kill switch was not allowed at the creation of the DenyCapV2";
		const EGlobalPauseNotAllowed: u64 = 3;

	Regulated { allow_global_pause: bool}
	Regulated { deny_cap: ID }

[sui framework] add marker dynamic field to regulated coin metadata #19852

Are you sure you want to change the base?

[sui framework] add marker dynamic field to regulated coin metadata #19852

Conversation

sblackshear commented Oct 14, 2024

Uh oh!

vercel bot commented Oct 14, 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!

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!

manolisliolios commented Oct 16, 2024

Uh oh!

sblackshear commented Oct 17, 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!

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!

manolisliolios Oct 21, 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!

github-actions bot commented Dec 22, 2024

Uh oh!

manolisliolios left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

vercel bot commented Oct 14, 2024 •

edited

Loading

sblackshear commented Oct 17, 2024 •

edited

Loading

manolisliolios Oct 21, 2024 •

edited

Loading