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

Extension for asset traits #52

Open
friedger opened this issue Nov 27, 2021 · 8 comments
Open

Extension for asset traits #52

friedger opened this issue Nov 27, 2021 · 8 comments

Comments

@friedger
Copy link
Contributor

friedger commented Nov 27, 2021

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

See also radicleart/clarity-market#6

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

Related Work

https://github.com/radicleart/clarity-market/blob/main/contracts/loopbomb.clar#L195

@radicleart
Copy link
Contributor

radicleart commented Nov 28, 2021

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 ?

radicleart added a commit to radicleart/clarity-market that referenced this issue Nov 28, 2021
@friedger
Copy link
Contributor Author

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

👍

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

friedger commented Dec 2, 2021

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

@radicleart
Copy link
Contributor

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

@friedger
Copy link
Contributor Author

friedger commented Dec 2, 2021

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

@MarvinJanssen
Copy link
Collaborator

MarvinJanssen commented Dec 3, 2021

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

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

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

4 participants