Extension for asset traits

friedger commented 2 years ago

There should be a SIP that define extensions that can be implemented by SIP-9 and SIP-10(?) assets. These extension should use the uint parameter either as id or as amount.

Trait Transferable

This trait is compatible with SIP-9, but not with SIP-10. The function transfer-memo is the corresponding function to stx-transfer-memo? defined in Stacks 2.1.

define-trait transferable
    (
        ;; Transfer from the sender to a new principal
        ;; must return `(ok true)` on success, never `(ok false)`
        ;; @param id-or-amount; identifier of NFT or amount of FTs
        ;; @param sender: owner of asset
        ;; @param recipient: new owner of asset after tx
        (transfer (uint principal principal) (response bool uint))

        ;; Transfer from the sender to a new principal
        ;; must return `(ok true)` on success, never `(ok false)`
        ;; must emit an event with `memo`
        ;; @param id-or-amount; identifier of NFT or amount of FTs
        ;; @param sender: owner of asset
        ;; @param recipient: new owner of asset after tx
        ;; @param memo: message attached to the transfer
        (transfer-memo (uint principal principal (buff 34) (response bool uint))

Example contract: https://github.com/MarvinJanssen/stx-atomic-swap/blob/master/stx/contracts/sip009-sip010-htlc.clar

Trait Operable

This trait should be implemented by transferable asset contracts. It provides functions to defined operators that are approved to transfer assets in the name of the user. Examples of operators are trusted marketplaces.

Security

In #40, some proposals have been made to improve the security of these functions

allow to specify guards for as-contract? calls: https://github.com/stacksgov/sips/issues/40#issuecomment-973939697
use an asset representing operation rights: https://github.com/stacksgov/sips/issues/40#issuecomment-973996337

Trait definition

define-trait operable
    (
        ;; set approval for an operator to handle a specified id or amount of the asset
        ;; must return `(ok true)` on success, never `(ok false)`
        ;; @param id-or-amount; identifier of NFT or amount of FTs
        ;; @param operator: principal that wants top operate the asset
        ;; @param bool: if true operator can transfer id or up to amount
        (set-approved (uint principal bool) (response bool uint))

        ;; read-only function to return the current status of given operator
        ;; if returned `(ok true)` the operator can transfer the NFT with the given id or up to the requested amount of FT
        ;; @param id-or-amount; identifier of NFT or amount of FTs
        ;; @param operator: principal that wants top operate the asset
        ;; @param bool: if true operator can transfer id or up to amount
        (is-approved (uint principal) (response bool uint))

radicleart commented 2 years ago

Thanks @friedger, aligning my traits with this..

One question.. looking at the impl for is-approved and wondering if we do need to pass in a test principle? Ie. if we just want to enforce the tx-sender/contract-caller is either the nft owner or the operator then just the token-id is enough - with an impl like this...

(define-read-only (is-approved (nftIndex uint) (address principal))
    (let
        (
            (owner (unwrap! (nft-get-owner? loopbomb nftIndex) nft-not-owned-err))
        )
        (begin 
            (if (or
                (is-eq owner tx-sender)
                (is-eq owner contract-caller)
                (default-to false (map-get? approvals {owner: owner, operator: tx-sender, nft-index: nftIndex}))
                (default-to false (map-get? approvals {owner: owner, operator: contract-caller, nft-index: nftIndex}))
            ) (ok true) nft-not-owned-err)
        )
    )
)

then the address parameter seems redundant? As opposed to testing the address instead of tx-sender and leaving this up to the caller to pass in the correct principle.

Good to know @MarvinJanssen view on this also ?

friedger commented 2 years ago

The UI might want to call this function with different addresses. Or the contract might implement different rules, like only approved contract-callers may execute an action.

With the address, the api is more open for other use cases. Implementations can ignore the parameter(s).

This implementation has one contract approved by default: https://github.com/BTC-Rocks/btc-rocks/blob/main/contracts/btc-rocks.clar#L68

MarvinJanssen commented 2 years ago

👍

@radicleart as an aside:

(define-read-only (is-approved (nftIndex uint) (address principal))
    (let
        (
            (owner (unwrap! (nft-get-owner? loopbomb nftIndex) nft-not-owned-err))
        )
        (ok (asserts!
            (or
                (is-eq owner tx-sender)
                (is-eq owner contract-caller)
                (default-to false (map-get? approvals {owner: owner, operator: tx-sender, nft-index: nftIndex}))
                (default-to false (map-get? approvals {owner: owner, operator: contract-caller, nft-index: nftIndex}))
                )
            nft-not-owned-err
            ))
        )
    )
)

friedger commented 2 years ago

Shall we add transfer-many and transfer-many-memo as suggested in #42 ?

radicleart commented 2 years ago

Could a bulk transfer be managed by another contract with the existing interface and a single user transaction?

friedger commented 2 years ago

@radicleart bulk transfer could be handled by other contracts, probably it is less efficient.

MarvinJanssen commented 2 years ago

I vote to add transfer-many and transfer-many-memo. We have seen it to be a definite need in the ecosystem. Doing it with another contract is tricky (you lose the ability to emit meaningful errors as far as I can tell) and significantly drives up the cost. The read count is hit by an amount equal to the contract size for every contract-call?. Having a bulk transfer function built-in is therefore a lot more economical.

Edit: thoughts on splitting the bulk transfer functions into a separate trait? I personally think they should be part of the base trait like in SIP013 but I can see arguments in favour of both sides.

dantrevino commented 2 years ago

I vote to add transfer-many and transfer-many-memo. We have seen it to be a definite need in the ecosystem. Doing it with another contract is tricky (you lose the ability to emit meaningful errors as far as I can tell) and significantly drives up the cost. The read count is hit by an amount equal to the contract size for every contract-call?. Having a bulk transfer function built-in is therefore a lot more economical.

Edit: thoughts on splitting the bulk transfer functions into a separate trait? I personally think they should be part of the base trait like in SIP013 but I can see arguments in favour of both sides.

@MarvinJanssen a bulk transfer option that is more generalized might be better ... something that can cover all the nfts without operable ... I assume cost would still be better than transferring individual nfts

stacksgov / sips