[RFC] [move] Shared Coin Standard #4680

punwai · 2022-09-16T12:25:47Z

Motivation

In many traditional smart contract platforms, it is common for protocols to be able to debit a user's account, provided that the user delegates them the permission to do so. Here are some examples of where this is useful:

Marketplace Bidding and Auctioning -- Where you want the resource owner to be able to execute the sale without you having to sign the transaction (This is a major one)
Subscriptions and any kind other kinds of automated payments -- Where you want to be automatically debited every month.

This is natively possible with the Ethereum ERC-20 standard, where a user would be able to give certain addresses the permission to transfer coins from their wallet. However, in Sui's programming model, where coins are owned objects, this is inherently not possible.

What this RFC contains

This RFCl introduces the safe module, which includes the Safe struct -- essentially a coin with shared access controls.Safe is a shared object, but only a specific owner address will be able to freely remove funds from it. Anybody is able to add funds to the safe. The owner is able to create Capability structs which allow the holder to withdraw a fixed amount of coins from the Safe. Once a Capability is created, the owner is able to revoke the access at any time.

Technical Considerations

Safe currently uses a bitmap to allow revocation of Capabilities at any time in the future. Each additional capability, on average, adds one bit to the side of the Safe. I personally don't think that the cost that this adds is of any concern, and if it is, they can always just move their funds to a new Safe.

Possible Expansions

It is also easy to create different kinds of access controls. We can also have things like SubscriptionCapability that allows withdrawals at every 10 checkpoints or something.

We are thinking of also creating a proposal for the LootBox standard -- an analogous standard for NFT delegation.

Proposers: @punwai @GrantStenger @justinwlin | kinetic.xyz

Jordan-Mysten · 2022-09-16T22:15:08Z

I think the goals here make sense, and I do think we need some solution to allow coins to be spent by other parties. In general I'd be really curious about the UI implications of the Safe proposed here. For other chains, approval itself doesn't alter coin balance, but from what I can tell here it looks like here it would actually alter the balance for users directly.

I imagine if this is something we're providing natively, we'd probably want ways to display Safes within the wallet, similar to how we display coins.

sblackshear

Like the idea, and think that this will be useful! Some details to be worked out here around revocation--see inline comments.

sblackshear · 2022-09-17T00:03:19Z

crates/sui-framework/sources/safe.move

+    /// 
+    struct Safe<phantom T> has key {
+        id: UID,
+        owner: address,


I think here, removing owner and instead adding a separate OwnerCapability { id: UID, safe_id: ID } that serves the same purpose might buy some valuable flexibility. This enables use-cases like:

Transferrable Safes (which in turn enable key rotation)

Safes protected by a smart contract wallet or managed by a DAO

KELP-like social recovery schemes for the funds in a vault

sblackshear · 2022-09-17T00:11:34Z

crates/sui-framework/sources/safe.move

+        id: UID,
+        owner: address,
+        balance: Balance<T>,
+        permissions_bitmap: vector<u64>,


https://github.com/MystenLabs/sui/blob/main/crates/sui-framework/deps/move-stdlib/sources/bit_vector.move might be useful here--I think it is desirable to separate the (rather generic) implementation of the bitvector data structure from the (rather specific) implementation of Safe.

I was looking at the BitVector implementation, and was wondering whether it could potentially be less efficient than just storing u64s? First observation is that it stores a vector of bools, which I presume is a byte each rather than a bit (you probably know best). But my biggest concern is that it has a fixed size at instantiation, which I don't think is idela, as the variance in the usages of Safe is likely huge (a few power users, many just buy an NFT here and there). I'll change to BitVector for now, but I'll keep the old commit unsquashed incase.

sblackshear · 2022-09-17T00:13:52Z

crates/sui-framework/sources/safe.move

+    }
+
+    /// Safe Owner Only
+    public fun revoke_capability<T>(safe: &mut Safe<T>, capability: &Capability<T>, ctx: &TxContext) {


I think this probably wants to take the nonce as input. Otherwise, a malicious Capability holder can prevent you from revoking their capability by making it a single-owner object, or by wrapping it.

I think this (in turn) means that either the Safe must remember the amount of each capability it issued (otherwise, it won't know what to subtract during revocation), or must only support a revoke_all API.

Yes, I've just realized that this is a problem - a fix was just pushed in.

Why does the Safe have to remember the amount of each capability? Currently revoking just completely turns off the capability.

However, another thing we might want to consider is whether it is desirable for the author to change the amounts which they allow capabilities to use. In this case, we could instead store a vector of u64 amounts in Safe that represents how much each capability could take out.

sblackshear · 2022-09-17T00:20:14Z

crates/sui-framework/sources/safe.move

+        create_capability_(safe, withdraw_amount, ctx)
+    }
+}
+


Might also be useful to have API's for:

allowing the owner (or more likely, anyone?) to deposit more funds into a Safe

allowing a Capability owner to voluntarily relinquish permissions

allowing anyone to read a Safe's balance

deleting an empty Safe

1 is just pushed in through another commit. The rest I'll add!

sblackshear · 2022-09-17T00:29:08Z

crates/sui-framework/sources/safe.move

+            id: object::new(ctx),
+            safe_id: object::uid_to_inner(&safe.id),
+            nonce: safe.next_nonce,
+            amount: withdraw_amount,


It is possible (though debatable whether it's desirable) for a Safe to have "overdraft protections"--it can remember how much it's promised via issuing Capability's and make sure not to promise more than is in the balance.

Probably not be desired for all use-cases, but interesting to think about as an option.

sblackshear · 2022-09-17T00:34:40Z

crates/sui-framework/sources/safe.move

+    ///
+    /// @ownership: Owned
+    ///
+    struct Capability<phantom T> has store, key {


If we call this something like AmountCap, that could open the door for issuing other kinds of capabilities in the future (e.g., a cap that gives you ownership of a share of the funds rather than a fixed amount). Maybe that wants to be a different standard, but could be an interesting building block.

Unrelated observation: a FlashLender does not look so different than a Safe--just a different revocability policy (you must self-revoke by end of tx + pay to do so) https://github.com/MystenLabs/sui/blob/main/sui_programmability/examples/defi/sources/flash_lender.move#L13

punwai · 2022-09-17T01:03:46Z

@Jordan-Mysten I completely agree -- the wallet should be responsible for displaying amounts in Safes. I think what would make sense is if we implement was Sam says (OwnerCapability), then we could query all Safes that the user owns along with the coin. Happy to add this into the wallet as well!

punwai · 2022-10-01T22:43:03Z

@sblackshear ready!

sblackshear

Like these improvements! A few more tweaks--the biggest conversation is around the bitmap.

crates/sui-framework/sources/safe.move

sblackshear · 2022-10-03T23:41:09Z

crates/sui-framework/sources/safe.move

+    }
+
+    /// Revoke a `TransferCapability` as an `OwnerCapability` holder
+    public fun revoke_transfer_capability<T>(safe: &mut Safe<T>, capability: &OwnerCapability<T>, capability_nonce: u64, _ctx: &TxContext) {


Suggested change

public fun revoke_transfer_capability<T>(safe: &mut Safe<T>, capability: &OwnerCapability<T>, capability_nonce: u64, _ctx: &TxContext) {

public entry fun revoke_transfer_capability<T>(safe: &mut Safe<T>, capability: &OwnerCapability<T>, capability_nonce: u64) {

crates/sui-framework/sources/safe.move

sblackshear · 2022-10-03T23:56:14Z

crates/sui-framework/sources/safe.move

+            id: object::new(ctx),
+            balance,
+            next_nonce: 0,
+            permissions_bitmap: bit_vector::new(1000),


Looking at how this is used, I'm wondering if it wouldn't be simpler (and more space-efficient, in the typical case) to just use a vector<ID> here.

A BitVector of length 1000 occupies 8 + 8 + (8 * 1000) = 8016 bytes even when there are no active capabilities, and can only grow in size over time because we never reuse "capability nonces". It also requires an extra 8 bytes in the capability to store the nonce.

By contrast, a vector<ID> will be proportional to the number of outstanding capabilities at the current point in time (we'd need to have ~286 active capabilities simultaneously before it got as big as the empty permissions_bitmap), shrinks as the number of active capabilities shrinks and we also won't need the 8 byte nonce inside each capability anymore. Checking capability membership is a a large set is less efficient than checking membership in a large bitmap, but I think this is likely to be a wash for small-to-medium capability counts (which is what I'd expect to see in practice).

The set approach would also allow you to support a revoke_all API (useful if there is e.g., a known vuln) by simply emptying out the vector.

Finally, knowing the ID's of each outstanding capability makes it easier for the owner of the Safe discover where their delegated permissions are (e.g., by using an ID lookup).

The other idea is to revert to the previous bitmap implementation that I had, it adds on a u64 every time that it needs to extend the length of the bitmap, and this means that on average, each capability adds on only 1 bit. This of course does not have the benefit of being able to clean up space when revoking an ID. We would also be able to support a revoke_all API by simply looping until next_nonce and disabling all the capabilities. But of course, this does not have the benefits of easier lookups for users.

Advantages:

More space efficient than both suggested implementations

Can support revoke_all

inclusion checks are O(1)
Disadvantages:

Harder to do lookups

@sblackshear Leaving this choice up to you!

Your other bitmap would indeed be more space-efficient than the bitvector library (sorry for suggesting you switch to that and then criticizing the choice lol, I forgot how inefficient that library is...).

I would vote for going with the vector<ID> approach mostly because it's (a) very simple and (b) not ever-growing--I think both the vector and the bitvector approach can comfortably support hundreds of simultaneous capabilities, which seems good enough. A Safe that needs thousands would probably want to use one of the forthcoming maps or something else.

crates/sui-framework/tests/safe_tests.move

punwai · 2022-10-12T07:47:49Z

Apologies for the late reply -- changes have been pushed!

punwai · 2022-10-12T07:52:26Z

Changes ready for review.

sblackshear · 2022-10-12T21:18:40Z

Thanks for this Pun! Looks good to go.

sblackshear · 2022-10-12T21:46:52Z

Looks like this needs some license headers in the new .move files

sblackshear · 2022-10-12T21:47:33Z

And updates to the snapshot tests because the additions to the framework changed genesis.

kchalkias

Great job @punwai. Good to go after resolving the failed CI tests.

change to comments add error names add comments changes f f

punwai changed the title ~~[SIP] [move] Safe (Shared Coin) Standard~~ [SIP-1] [move] Safe (Shared Coin) Standard Sep 16, 2022

punwai changed the title ~~[SIP-1] [move] Safe (Shared Coin) Standard~~ [SIP] [move] Safe (Shared Coin) Standard Sep 16, 2022

punwai requested review from sblackshear, tnowacki and oxade September 16, 2022 12:39

punwai changed the title ~~[SIP] [move] Safe (Shared Coin) Standard~~ [SIP] [move] Shared Coin Standard Sep 16, 2022

punwai requested a review from damirka September 16, 2022 13:30

sblackshear requested review from Jordan-Mysten and 666lcz September 16, 2022 21:54

sblackshear reviewed Sep 17, 2022

View reviewed changes

punwai force-pushed the safe branch from b176388 to 4938baf Compare September 17, 2022 01:30

sblackshear changed the title ~~[SIP] [move] Shared Coin Standard~~ [RFC] [move] Shared Coin Standard Oct 1, 2022

punwai force-pushed the safe branch 4 times, most recently from 295c202 to 3b87a65 Compare October 1, 2022 22:42

punwai force-pushed the safe branch 3 times, most recently from 2f353ae to c6c471d Compare October 2, 2022 02:18

sblackshear requested changes Oct 4, 2022

View reviewed changes

sblackshear mentioned this pull request Oct 11, 2022

October update to devx_roadmap #5066

Merged

punwai force-pushed the safe branch from c6c471d to 743a0d0 Compare October 12, 2022 07:16

punwai requested review from akichidis, huitseeker, bmwill, arun-koshy and asonnino as code owners October 12, 2022 07:16

punwai requested review from kchalkias, randall-Mysten and patrickkuo as code owners October 12, 2022 07:16

github-actions bot added the Type: Documentation Improvements or additions to documentation label Oct 12, 2022

punwai force-pushed the safe branch from 743a0d0 to 99b02af Compare October 12, 2022 07:18

github-actions bot removed the Type: Documentation Improvements or additions to documentation label Oct 12, 2022

sblackshear approved these changes Oct 12, 2022

View reviewed changes

sblackshear enabled auto-merge (rebase) October 12, 2022 21:18

kchalkias reviewed Oct 15, 2022

View reviewed changes

punwai force-pushed the safe branch from 71d4b3c to 64cbc17 Compare October 20, 2022 23:35

punwai and others added 6 commits October 20, 2022 22:27

add safe

616eeab

change to comments add error names add comments changes f f

edit tests

f8ddc64

upd

4e39726

fix

66f4de4

F

f3f29af

f

08ad3a0

punwai force-pushed the safe branch from eea1f56 to 08ad3a0 Compare October 21, 2022 05:41

sblackshear merged commit 02cae19 into MystenLabs:main Oct 21, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[RFC] [move] Shared Coin Standard #4680

[RFC] [move] Shared Coin Standard #4680

punwai commented Sep 16, 2022 •

edited by sblackshear

Loading

Jordan-Mysten commented Sep 16, 2022

sblackshear left a comment

sblackshear Sep 17, 2022

sblackshear Sep 17, 2022

punwai Oct 1, 2022 •

edited

Loading

sblackshear Sep 17, 2022

sblackshear Sep 17, 2022

punwai Sep 17, 2022

punwai Sep 17, 2022

punwai Sep 17, 2022 •

edited

Loading

sblackshear Sep 17, 2022

punwai Sep 17, 2022

sblackshear Sep 17, 2022

sblackshear Sep 17, 2022

punwai commented Sep 17, 2022 •

edited

Loading

punwai commented Oct 1, 2022

sblackshear left a comment

sblackshear Oct 3, 2022

sblackshear Oct 3, 2022 •

edited

Loading

punwai Oct 4, 2022 •

edited

Loading

punwai Oct 4, 2022 •

edited

Loading

sblackshear Oct 4, 2022

punwai commented Oct 12, 2022

punwai commented Oct 12, 2022

sblackshear commented Oct 12, 2022

sblackshear commented Oct 12, 2022

sblackshear commented Oct 12, 2022

kchalkias left a comment

	public fun revoke_transfer_capability<T>(safe: &mut Safe<T>, capability: &OwnerCapability<T>, capability_nonce: u64, _ctx: &TxContext) {
	public entry fun revoke_transfer_capability<T>(safe: &mut Safe<T>, capability: &OwnerCapability<T>, capability_nonce: u64) {

[RFC] [move] Shared Coin Standard #4680

[RFC] [move] Shared Coin Standard #4680

Conversation

punwai commented Sep 16, 2022 • edited by sblackshear Loading

Motivation

What this RFC contains

Technical Considerations

Possible Expansions

Jordan-Mysten commented Sep 16, 2022

sblackshear left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

punwai Oct 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

punwai Sep 17, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

punwai commented Sep 17, 2022 • edited Loading

punwai commented Oct 1, 2022

sblackshear left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sblackshear Oct 3, 2022 • edited Loading

Choose a reason for hiding this comment

punwai Oct 4, 2022 • edited Loading

Choose a reason for hiding this comment

punwai Oct 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

punwai commented Oct 12, 2022

punwai commented Oct 12, 2022

sblackshear commented Oct 12, 2022

sblackshear commented Oct 12, 2022

sblackshear commented Oct 12, 2022

kchalkias left a comment

Choose a reason for hiding this comment

punwai commented Sep 16, 2022 •

edited by sblackshear

Loading

punwai Oct 1, 2022 •

edited

Loading

punwai Sep 17, 2022 •

edited

Loading

punwai commented Sep 17, 2022 •

edited

Loading

sblackshear Oct 3, 2022 •

edited

Loading

punwai Oct 4, 2022 •

edited

Loading

punwai Oct 4, 2022 •

edited

Loading