[kiosk] Kiosk+ (again) #9709

damirka · 2023-03-22T17:33:31Z

Description

adds item: ID to the TransferRequest
removes metadata: Bag as redundant and rather complicated to manage
collectible nuked until a better solution is ready (requires a bit of refactoring)
removes all policies - moves them to tests for the time being
improves testing for everything kiosk-related
exposes uid and uid_mut (for owner) of the TransferPolicy so that it can contain app-specific data
Kiosk now has a custom setting "no taking" - disables take function

Test Plan

tests are in place
many cases are tested upfront

Type of Change (Check all that apply)

user-visible impact
breaking change for a client SDKs
breaking change for FNs (FN binary must upgrade)
breaking change for validators or node operators (must upgrade binaries)
breaking change for on-chain data layout
necessitate either a data wipe or data migration

Release notes

finalizes Kiosk and TransferPolicy modules

vercel · 2023-03-22T17:33:41Z

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

3 Ignored Deployments

Name	Status	Preview	Updated
explorer	⬜️ Ignored (Inspect)	Visit Preview	Mar 24, 2023 at 9:07PM (UTC)
explorer-storybook	⬜️ Ignored (Inspect)	Visit Preview	Mar 24, 2023 at 9:07PM (UTC)
wallet-adapter	⬜️ Ignored (Inspect)		Mar 24, 2023 at 9:07PM (UTC)

damirka · 2023-03-22T17:37:22Z

Also includes the changes from #9604 by @nmboavida.

amnn

I like the change overall, but have some questions (inline) on how the witnesses that kiosk actions produce are intended to be used!

crates/sui-framework/packages/sui-framework/sources/collectible/kiosk.move

amnn · 2023-03-23T22:00:24Z

crates/sui-framework/packages/sui-framework/sources/collectible/kiosk.move

        assert!(object::id(self) == cap.for, ENotOwner);
        self.item_count = self.item_count + 1;
-        dof::add(&mut self.id, Item { id: object::id(&item) }, item)
+        dof::add(&mut self.id, Item { id: object::id(&item) }, item);
+        PlacedWitness {}


Asking because it popped into my head: Is it worth including the placed item's id in the witness?

Agreed with Ashok--unless the type in question is a singleton, I think we will always need to remember the ID/check it. Otherwise, it seems hard to avoid issues where someone "reuses" a witness for a different instance of T (+ hard to reason about what the consequences might be).

ID in the witness won't be much help - we don't have the ID of the item in the TransferRequest so there's no way to compare it. TransferPolicy relies solely on type, and creating IDs everywhere can lead to "abuse of power" on the creator end.

amnn · 2023-03-23T22:10:00Z

crates/sui-framework/packages/sui-framework/sources/collectible/witness_policy.move

+
+    #[test]
+    #[expected_failure(abort_code = sui::witness_policy::ERuleNotFound)]
+    fun test_wrong_proof() {


Is this policy supposed to be used with the witnesses that come from Kiosk? If so I still don't quite see how we can prevent someone from creating a witness from actions on one instance and then using it as proof on another witness.

Good question.

Given that TransferPolicy affects all items of a type T, and kiosk witnesses are also returned "typed", only an item of type T can result in KioskWitness. To cheat on this system, one has to put / list another item T. And that is okay.

While implementing I considered two main applications:

TransferPolicy has "allow_taking" set to "false". Once placed into the Kiosk, items can't be taken. The only thing that can happen to them is list-sell (simple) or PurchaseCap-grab (more complex). However, we can't lock items in the purchase function - its signature is purchase() :(T, TransferRequest<T>). Hence we need to find a way to "force" user to put it into kiosk - and the "PlacedWitness" in the Kiosk allows us to achieve that.

We issue a "key-only" type, eg Collectible has key. To sell the Collectible, we need to turn it into a transferable type "TradableCollectible" - but we need a guarantee that it was listed / placed into the Kiosk. PlacedWitness or ListedWitness again help. Once sold, we can issue a rule for a TransferPolicy which will force the TradableCollectible to be turned into non-transferable Collectible again.

sblackshear · 2023-03-24T03:04:43Z

crates/sui-framework/packages/sui-framework/sources/collectible/transfer_policy.move

+        /// performed (eg item was put back to the `Kiosk` after a sell).
+        ///
+        /// Defaults to `true`.
+        allow_taking: bool


Is there a specific witness_policy we can recommend here for someone that wants very strict enforcement?

Removed this part completely in favor of "kiosk::has_item" check. Should be better and more understandable to execute.

sblackshear · 2023-03-24T03:07:34Z

crates/sui-framework/packages/sui-framework/sources/collectible/kiosk.move

        assert!(object::id(self) == cap.for, ENotOwner);
        self.item_count = self.item_count + 1;
-        dof::add(&mut self.id, Item { id: object::id(&item) }, item)
+        dof::add(&mut self.id, Item { id: object::id(&item) }, item);
+        PlacedWitness {}


Agreed with Ashok--unless the type in question is a singleton, I think we will always need to remember the ID/check it. Otherwise, it seems hard to avoid issues where someone "reuses" a witness for a different instance of T (+ hard to reason about what the consequences might be).

amnn

LGTM! (@damirka and I chatted offline about paring this down to a minimal feature set, explaining the new red lines)

damirka requested review from amnn and sblackshear March 22, 2023 17:33

damirka requested a review from randall-Mysten as a code owner March 22, 2023 17:33

damirka self-assigned this Mar 22, 2023

damirka requested a review from ronny-mysten as a code owner March 22, 2023 17:33

vercel bot deployed to Preview – explorer March 22, 2023 17:39 View deployment

vercel bot temporarily deployed to Preview – explorer-storybook March 22, 2023 17:40 Inactive

damirka force-pushed the kiosk-forever branch from e089893 to 16c23f7 Compare March 23, 2023 07:34

vercel bot temporarily deployed to Preview – explorer-storybook March 23, 2023 07:35 Inactive

vercel bot deployed to Preview – explorer March 23, 2023 07:35 View deployment

amnn approved these changes Mar 23, 2023

View reviewed changes

sblackshear reviewed Mar 24, 2023

View reviewed changes

damirka force-pushed the kiosk-forever branch from f32f2d8 to b2343a8 Compare March 24, 2023 08:18

vercel bot deployed to Preview – explorer March 24, 2023 08:19 View deployment

vercel bot temporarily deployed to Preview – explorer-storybook March 24, 2023 08:19 Inactive

damirka changed the title ~~[kiosk] New ActionWitness pattern + proof-lock policy~~ [kiosk] Kiosk: no metadata; item ID to Request Mar 24, 2023

damirka changed the title ~~[kiosk] Kiosk: no metadata; item ID to Request~~ [kiosk] Kiosk+ (again) Mar 24, 2023

amnn approved these changes Mar 24, 2023

View reviewed changes

damirka force-pushed the kiosk-forever branch from 45830a6 to 7477b28 Compare March 24, 2023 17:41

vercel bot deployed to Preview – explorer March 24, 2023 17:42 View deployment

vercel bot temporarily deployed to Preview – explorer-storybook March 24, 2023 17:43 Inactive

damirka force-pushed the kiosk-forever branch from 91143ac to a7d3042 Compare March 24, 2023 18:25

vercel bot temporarily deployed to Preview – explorer-storybook March 24, 2023 18:26 Inactive

vercel bot deployed to Preview – explorer March 24, 2023 18:26 View deployment

damirka force-pushed the kiosk-forever branch from a7d3042 to b770541 Compare March 24, 2023 18:31

vercel bot deployed to Preview – explorer March 24, 2023 18:31 View deployment

vercel bot temporarily deployed to Preview – explorer-storybook March 24, 2023 18:32 Inactive

damirka mentioned this pull request Mar 24, 2023

Relax key + store constraint in TransferRequest and TransferRequest type parameter T #9604

Closed

damirka added 15 commits March 24, 2023 23:02

adds a policy that locks an item forever

337b541

adds has_item to kiosk

6d38427

adds royalty policy, rename: lock -> witness

54153a2

only allow placing with TransferPolicy

2157893

fix snapshots

42c50b4

more tests for kiosk

8e92bf0

framework docs

e0ab102

collectible -> kiosk subfolder

d736962

more tests, change in the kiosk functionality

a11d710

nit docs

e1bfcf2

removes collectible, removes policies

9d1a23c

adds getter tests

b9f1361

snapshots + docs

1f3a75b

policy no longer required

3f94ed6

remove collectible

2a3e7b2

damirka force-pushed the kiosk-forever branch from 4d19dcc to 2a3e7b2 Compare March 24, 2023 20:28

damirka enabled auto-merge (squash) March 24, 2023 20:29

vercel bot temporarily deployed to Preview – explorer-storybook March 24, 2023 20:29 Inactive

vercel bot deployed to Preview – explorer March 24, 2023 20:30 View deployment

fix docs!

23d8a39

damirka merged commit 9b41517 into main Mar 24, 2023

damirka deleted the kiosk-forever branch March 24, 2023 21:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[kiosk] Kiosk+ (again) #9709

[kiosk] Kiosk+ (again) #9709

damirka commented Mar 22, 2023 •

edited

Loading

vercel bot commented Mar 22, 2023 •

edited

Loading

damirka commented Mar 22, 2023

amnn left a comment

amnn Mar 23, 2023

sblackshear Mar 24, 2023

damirka Mar 24, 2023

amnn Mar 23, 2023

damirka Mar 23, 2023

sblackshear Mar 24, 2023

damirka Mar 24, 2023

sblackshear Mar 24, 2023

amnn left a comment

[kiosk] Kiosk+ (again) #9709

[kiosk] Kiosk+ (again) #9709

Conversation

damirka commented Mar 22, 2023 • edited Loading

Description

Test Plan

Type of Change (Check all that apply)

Release notes

vercel bot commented Mar 22, 2023 • edited Loading

damirka commented Mar 22, 2023

amnn left a 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

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

amnn left a comment

Choose a reason for hiding this comment

damirka commented Mar 22, 2023 •

edited

Loading

vercel bot commented Mar 22, 2023 •

edited

Loading