Skip to content
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

Trait for Tradables/Marketplace functions #40

Closed
friedger opened this issue Oct 13, 2021 · 29 comments
Closed

Trait for Tradables/Marketplace functions #40

friedger opened this issue Oct 13, 2021 · 29 comments

Comments

@friedger
Copy link
Contributor

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
Copy link
Contributor

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

@friedger
Copy link
Contributor Author

#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
Copy link
Contributor Author

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
Copy link
Collaborator

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
Copy link
Contributor Author

@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
Copy link

LNow commented Oct 22, 2021

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
Copy link
Contributor Author

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

@friedger
Copy link
Contributor Author

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

@LNow
Copy link

LNow commented Oct 26, 2021

@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
Copy link
Collaborator

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.)

@friedger friedger changed the title Trait for Tradables Trait for Tradables/Marketplace functions Nov 7, 2021
@dcsan
Copy link

dcsan commented Nov 13, 2021

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
Copy link
Contributor

@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
Copy link
Contributor

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

@friedger
Copy link
Contributor Author

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
Copy link
Contributor

Does the issue raised by @LNow Adding post conditions to clarity ( stacks-network/stacks-core#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
Copy link
Collaborator

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
Copy link
Contributor

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

(ok (map-set approvals {owner: tx-sender, operator: operator, token-id: token-id} {bool: false}))

@MarvinJanssen
Copy link
Collaborator

@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
Copy link
Contributor

radicleart commented Nov 19, 2021

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
Copy link
Contributor

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.. stacks-network/stacks-core#2921

@LNow
Copy link

LNow commented Nov 19, 2021

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
Copy link
Collaborator

@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
Copy link
Contributor Author

@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 stacks-network/stacks-core#1641 is available.

@friedger
Copy link
Contributor Author

Maybe there is space for both tradable-trait and operatable-trait, both should extend transferable-trait (#35)

@radicleart
Copy link
Contributor

radicleart commented Nov 20, 2021

@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
Copy link
Contributor

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
Copy link

Jamil commented Nov 22, 2021

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
Copy link
Contributor Author

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
Copy link
Contributor Author

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

I have created two new issues:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants