-
Notifications
You must be signed in to change notification settings - Fork 11.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Kiosk SDK] Support kiosk_lock_rule
and environments
#12287
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, left some comments. Approving to unblock.
import { | ||
KIOSK_PURCHASE_CAP, | ||
KIOSK_TYPE, | ||
TRANSFER_POLICY_CREATED_EVENT, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great if we had a function to create generic events of kind: transferPolicyCreated(type): 0x2:....::Created<${type}>
. Wonder what you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@damirka Could you elaborate a bit on that?
policy.bcs.bcsBytes, | ||
'base64', | ||
); | ||
let parsed = bcs.de(TRANSFER_POLICY_TYPE, policy.bcs.bcsBytes, 'base64'); | ||
|
||
return { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we could help provide initialSharedVersion for shared transfer policy. That could help build arguments for the purchase flow.
sdk/kiosk/src/tx/kiosk.ts
Outdated
@@ -200,20 +203,16 @@ export function purchase( | |||
tx: TransactionBlock, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we call txb to align with other places?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this sdk, we have it defined as tx
everywhere!
Does other places
refer to the @mysten/sui.js
sdk?
sdk/kiosk/src/tx/kiosk.ts
Outdated
@@ -200,20 +203,16 @@ export function purchase( | |||
tx: TransactionBlock, | |||
itemType: string, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some argument grouping / ordering would help. Eg kioskId, itemId, itemType?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to follow the same ordering we had on the other ones. (itemType was always first)
sdk/kiosk/src/tx/kiosk.ts
Outdated
// if we don't pass the listing or the listing doens't have a price, return. | ||
if (!listing || listing?.price === undefined) return null; | ||
if (!listing || listing?.price === undefined) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's just require price
. Perhaps, instead we should require Coin... the reason for it is that developers might choose a custom coin and not use gas for it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point for the price!
I like the Coin
idea, I think though that this is a nice generic solution for 3-line purchase flows.
Maybe we should support more advanced use-case variations
for ideas like this, by possibly extending extraParams
in the future to support coin
, etc.
Description
In this PR:
purchaseAndResolvePolicies
to a) support environments (e.g. mainnet, testnet) and b) acceptextraParams
(to support kiosk_lock_rule).kiosk_lock_rule
, enabling the SDK to fully supportstrong royalties enforcement
.purchaseAndResolvePolicies
to include the item +canTransfer
flag (whether it can be transferred or not, based on the ruleset)../types
directory).withdrawFromKiosk
andwithdrawFromPolicy
call amount params.Test Plan
How did you test the new or updated feature?
Right now the changes are tested on the Demo Kiosk dapp that is being developed in parallel. We'll need to start creating a testing suite for the SDK, as we start finalizing our requirements.
If your changes are not user-facing and not a breaking change, you can skip the following section. Otherwise, please indicate what changed, and then add to the Release Notes section as highlighted during the release process.
Type of Change (Check all that apply)
Release notes
Adds support for
kiosk_lock_rule
and environments (testnet, mainnet, custom) for the purchase flows. FixeswithdrawFromKiosk
andwithdrawFromPolicy
.