stacksgov / sips

Community-submitted Stacks Improvement Proposals (SIPs)
131 stars 80 forks source link

Trait for Tradables/Marketplace functions #40

Closed friedger closed 2 years ago

friedger commented 2 years ago

This sips is about standardizing smart contract functions that enables digital assets to contain ability to participate in an open, decentralized market place.

Owners of NFTs (SIP9) and FTs (SIP10) should be able to list and unlist their token. Buyers should be able to buy the assets from the owner.

A simple trait would look like this:

(use-trait commission-trait .commisions.trait)
(define-trait tradable
    (
        ;; announce listing to global marketplace
        ;; must return `(ok true)` on success, never `(ok false)`
        ;; must send a list event
        ;; @param id; identifier of NFT or amount of FTs
        ;; @param price: of sale in micro STX
        ;; @param commission: action to happen after sale
        (list-asset (uint uint <commission-trait>) (response bool uint))

        ;; announce delisting to global marketplace
        ;; must return `(ok true)` on success, never `(ok false)`
        ;; must send a delist event
        ;; @param id; identifier of NFT or amount of FTs
        (unlist-asset (uint) (response bool uint))

        ;; buy and announce delisting to global marketplace
        ;; must return `(ok true)` on success, never `(ok false)`
        ;; commission must match the one set during listing
        ;; must send a delist event
        ;; @param id; identifier of NFT or amount of FTs
        ;; @param commission: action to happen after sale        
        (buy-asset (uint  <commission-trait>) (response bool uint))
    )
)

(define-trait commission
    (
        ;; additional action after a sale happened
        ;; must return `(ok true)` on success, never `(ok false)`
        ;; @param id; identifier of NFT
        ;; @param price: of sale in micro STX
        (pay (uint uint) (response bool uint))
    )
)

Marketplaces would observe the list, unlist events and update their inventory accordingly. They act mainly off-chain.

Commission allows to implement royalties for artist on NFTs or fee for marketplaces or different sales mechanims like auctions.

jcnelson commented 2 years ago

Thanks for submitting this @friedger! Is this a continuation of #35?

friedger commented 2 years ago

35 is about transfer with memo. This one is about listing and purchase. Both try to define the trait for both FTs and NFTs.

friedger commented 2 years ago

We might want to add more option to set the price like {sats: uint, ustx: uint} and add a list-ft function that has a trait as parameter.

MarvinJanssen commented 2 years ago

This seems rather limiting. Can you explain how these traits would work in practice? What if I don't want to sell for STX but another NFT? Or a SIP010? Or something else entirely? Will the future token developer need to implement a plethora of these? It makes more sense to build upon the transfer function instead of stuffing the token contracts.

friedger commented 2 years ago

@MarvinJanssen Indeed, this is only a small part of the opportunities you have to exchange your assets. You can always go to a market and swap your asset for another one. I could imagine services that enable on the spot exchange of the buyers assets to the requested ones from the seller. This SIP is only an extension to SIP-9 and SIP-10. I don't see how transfer could be used for market participation.

A possible implementation is here: https://explorer.stacks.co/txid/0x0c983e4a0138f86d4dc91e3eeb277f14fafb1a118fef1035ff06db84093697d1?chain=testnet

This SIP would explore the ability of owner to participate in an open, global market without 3rd parties involved.

Instead of covering all possible this covers the main uses case for selling an asset in STX or BTC. Sale with FTs can't use the same trait because the FT trait needs to be passed in as a parameter.

Something like this would be possible

  (list-asset (uint uint <tradable-trait> <commission-trait>) (response bool uint))
  (buy-asset (uint  <tradable-trait> <commission-trait>) (response bool uint))

Then there would be two special tradables for STX and SATS. Does that make more sense?

LNow commented 2 years ago

Do I understand it correctly that main idea behind this SIP is to eliminate middle man and embed micro marketplace in token itself, and allow token owners to put them on sale without transferring them to 3rd party (potentially faulty or malicious) contracts?

friedger commented 2 years ago

@LNow Correct, for a user owned internet. The role of marketplaces is to aggregate, filter, present,..

friedger commented 2 years ago

Here is a first implementation: https://explorer.stacks.co/txid/SP3D6PV2ACBPEKYJTCMH7HEN02KP87QSP8KTEH335.megapont-ape-club-nft?chain=mainnet

LNow commented 2 years ago

@friedger I like it.

One remark - if we want to make sure commissions and royalties are payed correctly, NTF should have list of allowed commission contracts that can be used with it. Otherwise everyone will be able to use any commission contract they like.

Based on above I would add one more function to tradable:

(get-allowed-commisions () (response (list 10 <commission-trait>) uint)
MarvinJanssen commented 2 years ago

I do not like it, at least not in this form. Diverging needs will create a situation of ever-growing and more complex token contracts. Think of sale expiry, tiered commissions, royalties, will token contract developers have to deal with all of that? I do not see a lot of upside over having an exchange contract that can call into transfer functions atomically. If you have to call into the token contract to list it for sale, you might as well call into an open exchange contract to list it for sale. Realising a "global open market without third parties involved" can be done without stuffing token contracts. It is just as easy to index open exchange contracts. (In fact, you can just check that contract's token balances to know exactly what is on sale.)

dcsan commented 2 years ago

Is this basically to enable non-custodial NFT sales @friedger ?

a way to enable delegated sales without asset transfer? I'm still a bit new around here but this seems like a critical proposal. Until we have this there will be no low-friction open marketplaces in STX ecosystem. Having an all-comer flea market like opensea is needed as much as the private curated "one collection one website" micro-sites for the initial drop/mint. Currently listing items in a marketplace other than the mint home, requires you to fully transfer the asset to the marketplace owners, and I believe that also defeats any secondary royalties (tho this maybe addressed separately)

@MarvinJanssen are you proposing to add a bunch of parameters to thetransfer function?

I would 👍 +1 keeping it simple to solve for the 95% of NFT transactions that happen out there and not try to perfect for all the possible future use cases for NFTs.

some other references:

setApprovable

@radicleart (numberOne marketplace) is working on a set-approvable trait https://github.com/radicleart/clarity-market/blob/main/contracts/nft-tradable-trait.clar https://github.com/radicleart/clarity-market/blob/main/contracts/loopbomb.clar#L114

proxies

https://docs.opensea.io/docs/1-structuring-your-smart-contract#creature-erc721-contract opensea requests contract developers to whitelist their users' proxy accounts to reduce trading friction without consigning the ownership assets to opensea.

radicleart commented 2 years ago

@dcsan have made some more progress on this and deployed a version on staging here ST1NXBK3K5YYMD6FD41MVNP3JS1GABZ8TRVX023PT.nft-tradable-trait

@friedger this is similar to yours but i've included ability to approve another (ie marketplace) address in order to delegate the transfer or listing function to the marketplace.

@MarvinJanssen I've deliberately left royalties/commissions etc out of this - do you see these as additional optional traits or off chain features - to keep the contract simple? Can you elaborate on what you think this tradable standard should look like?

radicleart commented 2 years ago

BTW the work in progress is in https://github.com/radicleart/clarity-market branch feature/tradableAndBlockHeight

friedger commented 2 years ago

A bit of history: When defining SIP9, I left out the approval functions because handling post-conditions for these would make the SIP too complicated.

Both set-approval and list-asset can bypass the post-conditions. set-approval defines an external address or contract that can later move the asset. list-asset defines a implicit condition when it is allowed to move the asset. In that sense, both proposal are not good.

I understand that defining implicit conditions to move the asset does not sound right like @MarvinJanssen pointed out.

What is a better solution? Defining the condition externally in a contract? Does it give the user enough control over their assets? Should we introduce a second native asset SP3N4AJFZZYC4BK99H53XP8KDGXFGQ2PRSQP2HGT6.loopbomb::transfer-right and transfer it to the approved principal to make the approval controllable for the user.

A post-condition for set-approval would ensure that the user is aware of the approval (ideally the post-condition contains the recipient).

On transfer, the tx-sender and/or contract-caller needs to be the owner of the asset or have the correct transfer-right tokens.

Other question: What happens with the approval/transfer-rights after a transfer?

radicleart commented 2 years ago

Does the issue raised by @LNow Adding post conditions to clarity ( https://github.com/blockstack/stacks-blockchain/issues/2921 ) change this if approved?

Other question: What happens with the approval/transfer-rights after a transfer?

There is nothing in the nft-trait or nft-transfer function that enforces rules like the nft owner to be the tx-sender - this is down to the implementing contract no? So the rules on transfer come down to the contract author. The approvals have to be reset in the transfer - in the same way the implementation of transfer ensures only the tx-sender/contract-caller is the owner (or approval).

MarvinJanssen commented 2 years ago

All very valid thoughts @friedger and @radicleart. Would a basic set-approved not be enough? The check in transfer can then be made more permissive, e.g.:


(define-constant err-unknown-token (err u3))
(define-constant err-not-authorised (err u100))

(define-non-fungible-token stacksies uint)

(define-map approvals {owner: principal, operator: principal, token-id: uint} bool)

(define-public (set-approved (operator principal) (token-id uint) (approved bool))
    (ok (map-set approvals {owner: tx-sender, operator: operator, token-id: token-id} approved))
)

(define-read-only (is-approved (token-id uint) (owner principal))
    (or
        (is-eq owner tx-sender)
        (is-eq owner contract-caller)
        (default-to false (map-get? approvals {owner: owner, operator: tx-sender, token-id: token-id}))
        (default-to false (map-get? approvals {owner: owner, operator: contract-caller, token-id: token-id}))
    )
)

(define-public (transfer (token-id uint) (sender principal) (recipient principal))
    (begin
        (asserts! (is-approved token-id (unwrap! (nft-get-owner? stacksies token-id) err-unknown-token)) err-not-authorised)
        (nft-transfer? stacksies token-id sender recipient)
    )
)

But it would be a lot nicer if as-contract calls themselves can be guarded by post conditions in Clarity. (Basically giving contracts the ability to have post conditions for themselves.)

;; Pseudo-code
;; (guard (list post-condition) expression)
;; (stx-post-condition amount sender recipient)

(guard
 (list (stx-post-condition u100 (as-contract tx-sender) tx-sender))
 (as-contract (contract-call? ...))
)

Still, there are situations where malicious contracts can still do unwanted contract calls. Another alternative would be to additionally specify which contracts are allowed to be called in a contract call by means of a post condition. For example: a contract call to nft-market may only call into nft-contract-a.

Then there was also a related discussion brought up in the Discord channels before: it would be great if we could write post conditions that assert custom print events happened (in a specific contract), so we can guard against basic events the user cares about. For example, set-approval can emit a (print {event: "approved", operator: SP..., token-id: u123}).

Excuse the segue 😁.

radicleart commented 2 years ago

Marvin, and the transfer must/should also...

(ok (map-set approvals {owner: tx-sender, operator: operator, token-id: token-id} {bool: false}))
MarvinJanssen commented 2 years ago

@radicleart rationale? Maybe the user will receive the token back at some point and wants to keep the operator approved. I would prefer not to implicitly remove approval, as it is basically making a choice on the user's behalf.

radicleart commented 2 years ago

The transfer is passing ownership to a new user who may not approve the previous owners operator.

Ah - i see the original owner is in the map - ok i get it.

radicleart commented 2 years ago

Does having

(is-eq owner tx-sender)

in is-approved expose the contract to the hack of the method being called by an intermediary contract?

as in this issue.. https://github.com/blockstack/stacks-blockchain/issues/2921

LNow commented 2 years ago

My rough idea how we could tackle approvals and transfers. This example shows how to implement approvals which are valid for one and only one transfer. But similar concept can be used for approvals valid indefinitely.

(define-non-fungible-token Token uint)
(define-non-fungible-token TokenApproval {
  owner: principal, 
  operator: principal, 
  tokenId: uint,
  transfersCount: uint,
})

;; tokenId
;; transfers count (cnt)
(define-map TransfersCount uint uint)

(define-read-only (get-transfers-count (tokenId uint))
  (default-to u0 (map-get? TransfersCount tokenId))
)

(define-public (grant-approval (tokenId uint) (operator principal))
  (let
    (
      (transfersCount (get-transfers-count tokenId))
      (approvalId {owner: tx-sender, operator: operator, tokenId: tokenId, transfersCount: transfersCount})
    )
    (try! (nft-mint? TokenApproval approvalId tx-sender))
    (try! (nft-transfer? TokenApproval approvalId tx-sender operator)) ;; require post-conditions
    (ok true)
  )  
)

(define-public (transfer (tokenId uint) (sender principal) (recipient principal))
  (let
    (
      (owner (unwrap! (nft-get-owner? Token tokenId) (err u99999)))
      (transfersCount (get-transfers-count tokenId))
      (approvalId {owner: owner, operator: tx-sender, tokenId: tokenId, transfersCount: transfersCount})
      (isApproved (is-some (nft-get-owner? TokenApproval approvalId)))
    )
    (asserts! (or (is-eq tx-sender owner) isApproved) (err u99999999))
    (if isApproved
      (try! (nft-burn? TokenApproval approvalId tx-sender))
      true
    )
    (map-set? TransfersCount tokenId (+ transfersCount u1))
    (nft-transfer? Token tokenId sender recipient)
  )
)

Maybe the user will receive the token back at some point and wants to keep the operator approved. I would prefer not to implicitly remove approval, as it is basically making a choice on the user's behalf.

I agree with you @MarvinJanssen but it needs to be defined explicitly. Eg. grant-approval and grant-infinite-approval

MarvinJanssen commented 2 years ago

@radicleart sure but contracts would (should) no longer need to do as-contract to transfer their own token or one on a user's behalf.

friedger commented 2 years ago

@MarvinJanssen Does it mean that we have one listing contract that manage all exchanges? Following the permission token concept that contract would be a huge honeypot for permission tokens, wouldn't it?

A proposal for such a solution would be below.

What should the parameters for set-approval look like? 1) Just one bool and the user must understand the contract that it does perform transfers as much as the user wants 2) Have more parameters to give users control. Did I miss any parameters for the approval conditions? It feels a bit restricted.

(use-trait commission-trait .commisions.trait)
(use-trait tradable-trait .tradable.trait)
(define-trait tradable
    (
        ;; announce listing to global marketplace
        ;; must return `(ok true)` on success, never `(ok false)`
        ;; must send a list event
        ;; @param id; identifier of NFT or amount of FTs
        ;; @param price: of sale in micro STX
        ;; @param commission: action to happen after sale
        (list-asset (uint uint <tradable-trait> <commission-trait>) (response bool uint))

        ;; announce delisting to global marketplace
        ;; must return `(ok true)` on success, never `(ok false)`
        ;; must send a delist event
        ;; @param id; identifier of NFT or amount of FTs
        (unlist-asset (uint <tradable-trait>) (response bool uint))

        ;; buy and announce delisting to global marketplace
        ;; must return `(ok true)` on success, never `(ok false)`
        ;; commission must match the one set during listing
        ;; must send a delist event
        ;; @param id; identifier of NFT or amount of FTs
        ;; @param commission: action to happen after sale        
        (buy-asset (uint <tradable-trait> <commission-trait>) (response bool uint))
    )
)

(define-trait tradable-trait
    (
        ;; approval to transfer assets
        ;; must return `(ok true)` on success, never `(ok false)`
        ;; @param id; identifier of NFT/amount of FTs
        ;; @param operator: operator of transfer function
        ;; @param approved: if true operator can call transfer
        ;; @param count: number of transfer calls the operator is allowed to call transfer
        ;; @param until: block height until when the operator is allowed to call transfer
        (set-approval (uint principal bool uint uint) (response bool uint))       
    )
   ...
  [all SIP 9 or SIP 10 functions]
)

Alternative OpenSea recommends to overwrite their approval functions. A simplified version would be to not use set-approval at all and implement transfer such that the listing contract is allowed to transfer. Marketplace UIs could do a read-only call on transfer to verify that they have the permission to do so once https://github.com/blockstack/stacks-blockchain/issues/1641 is available.

friedger commented 2 years ago

Maybe there is space for both tradable-trait and operatable-trait, both should extend transferable-trait (https://github.com/stacksgov/sips/issues/35)

radicleart commented 2 years ago

@friedger I'm gearing up to deploy a new iteration of the #1 contract for Crash Punks on testnet - is it worthwhile me refactoring to explore using the traits above or is it too early?

Edit: Just looking at it and its not realistic in the time frame I have...

radicleart commented 2 years ago

Should there be an overridden list-item that also sets the operator ?

    (list-asset (uint uint principle <commission-trait>) (response bool uint))

from a usability perspective this would equate to a t&c like checkbox where the user can decide in a single tx to list and authorise a particular operator.

Jamil commented 2 years ago

Minor point here (and sorry if I'm missing something obvious) but in the original proposal, why is specifying the commission explicitly in the buy-asset call necessary? Why not default to the one already stored in the map? Is it to make sure that the commission is what is expected and is not somehow malicious?

If the latter, it probably needs to be called out explicitly for frontends to be careful about which commission-traits are safe. Right now I can imagine an implementation which just calls the function with whatever commission-trait was specified at listing without additional checks.

friedger commented 2 years ago

The commission trait is necessary to actually call the functions on the trait.

Indeed, the frontend and the user should understand the contracts passed in. In particular, appropriate post conditions needs to be added.

friedger commented 2 years ago

@radicleart set-approved is a good solution, and the marketplace functions can go in a separate contract.

I have created two new issues: