NEP-17 extension discussion

Summary: I would like to open a discussion to add some kind of extension to NEP-17 standard, maybe NEP-17.1 or any name that may fit, which would allow/replicate some features from ERC-20 standard and the allowance mechanism, which i think are lacking in NEP-17 standard: allowance, transferFrom, and approve, as well as one event related to that: approval. The whole idea is to allow a spender to spend x amount of funds on behalf of an owner.

Goal: The goal is neither to replace NEP-17, nor to create a fully separate standard, but more to add some kind of extension to the current one, which project could signal their intention to support or not.

Usecase: Our own usecase is simple: this allows us to process NFT trades on behalf of users, without them having to lock their tokens in advance, what we call open offers. Without this, if someone wanted to place 10 offers on 10 different NFT for 10 NEO each, he would have to lock 100 NEO without even being sure those would be accepted. With this extension, the NEO only move from the buyer's wallet if and when a trade is made (when an offer is accepted), it is effectively way more convenient. There are of course lots more usecase than that, most of them for defi applications.

Risks: Obviously the concept of approval and allowance is complex and prone to security risks, as people giving a too high / unlimited allowance could end up with their NEP-17 drained, but that's solved with user education, learning how to revoke and check allowance etc, and I personally think that the upside out-weights this risk.

Feedback: This is a feature we already have implemented in our project and which work very well for us and our own NEP-17, and the goal is that this feature would be opt-in for any project that want to implement it on their NEP-17 token contract.

Technical details: If a project decide to implement it, it would be as easy as adding a supported_standards = ["NEP-17", "NEP-17-1"] which then any project could check on ContractManagement Manifest

                var contractDetails = ContractManagement.GetContract(contract);
                string stringToCheck = "NEP-17-1";
                bool isCompatible = false;
                foreach (var s in contractDetails.Manifest.SupportedStandards)
                {
                    if (s == stringToCheck)
                    {
                        isCompatible = true;
                        break;
                    }
                }
                return isCompatible;

Regarding the implementation itself, we currently use it like this (in python) and tried to make it very similar to ERC-20 to help with adoption. We only changed a few NEO-specific things obviously (passing spender as an arg instead of using tx.Sender, etc).

@public(safe=True)
def allowance(from_address: UInt160, spender: UInt160) -> int:
    """
    Returns the remaining number of tokens that spender will be allowed to spend on behalf
    of from_address through transferFrom. This is zero by default.

    This value changes when approve or transferFrom are called.

    :param from_address: the address to check approval for
    :type from_address: UInt160
    :param spender: the address allowed to spend on behalf of the from_address
    :type spender: UInt160

    :return: the number of tokens allowed to be spent.
    """
    expect(validateAddress(from_address), "invalid from_address address")
    expect(validateAddress(spender), "invalid spender address")
    all = get(mk_allowance_key(from_address, spender), get_read_only_context()).to_int()
    return all

@public
def approve(from_address: UInt160, spender: UInt160, amount: int) -> bool:
    """
    Sets amount as the allowance of spender over the from_address tokens.

    Returns a boolean value indicating whether the operation succeeded.

    :param from_address: the address giving approval
    :type from_address: UInt160
    :param spender: the address to allow as a spender
    :type spender: UInt160
    :param amount: the amount of tokens to allow to spend
    :type amount: int

    :return: bool value of operation success.
    """
    expect(check_witness(from_address),"Invalid witness" )
    expect(validateAddress(spender), "invalid spender address")
    expect(amount >= 0, "amount has to be >= 0")

    if amount == 0:
        remove_allowance(from_address, spender)
    else:
        set_allowance(from_address, spender, amount)

    on_approve(from_address, spender, amount)
    return True

@public
def transferFrom(spender: UInt160, from_address: UInt160, to_address: UInt160, amount: int, data: Any) -> bool:
    """
    Transfers an amount of NEP17 tokens from one account to another using the allowance mechanism.

    If the method succeeds, it must fire the `Transfer` event and must return true, even if the amount is 0,
    or from and to are the same address.

    :param spender: the address transferring
    :type spender: UInt160
    :param from_address: the address to transfer from
    :type from_address: UInt160
    :param to_address: the address to transfer to
    :type to_address: UInt160
    :param amount: the amount of NEP17 tokens to transfer
    :type amount: int
    :param data: whatever data is pertinent to the onPayment method
    :type data: Any

    :return: whether the transfer was successful
    :raise AssertionError: raised if `spender`, `from_address` or `to_address` length is not 20 or if `amount` is less than zero.
    """
    expect(validateAddress(spender), "invalid spender address")
    expect(validateAddress(from_address), "invalid from address")
    expect(validateAddress(to_address), "invalid to address")

    # the parameter amount must be greater than or equal to 0. If not, this method should throw an exception.
    expect(amount >= 0, "amount must be greater than or equal to 0")

    # The function MUST return false if the from account balance does not have enough tokens to spend.
    from_balance = get(from_address).to_int()
    if from_balance < amount:
        return False

    # The function should check whether the from address equals the caller contract hash or the spender.
    # If so, the transfer should be processed;
    # If not, the function should use the check_witness to verify the transfer.
    if not check_witness(from_address) and not check_witness(spender):
        return False

    # allowance should be > amount
    all = get(mk_allowance_key(from_address, spender), get_read_only_context()).to_int()
    expect(amount <= all, "spender allowance exceeded")

    # update new allowance
    if all == amount:
        remove_allowance(from_address, spender)
    else: 
        newAllowance = all - amount
        set_allowance(from_address, spender, newAllowance)

    # skip balance changes if transferring to yourself or transferring 0 cryptocurrency
    if from_address != to_address and amount != 0:
        if from_balance == amount:
            delete(from_address)
        else:
            put(from_address, from_balance - amount)

        to_balance = get(to_address).to_int()
        put(to_address, to_balance + amount)

    # if the method succeeds, it must fire the transfer event
    on_transfer(from_address, to_address, amount)
    # if the to_address is a smart contract, it must call the contracts onPayment
    post_transfer(from_address, to_address, amount, data)
    # and then it must return true
    return True

on_approve = CreateNewEvent(
    # trigger when an approval has been made
    [
        ('owner', UInt160),
        ('spender', UInt160),
        ('amount', int),
    ],
    'Approval'
)

as a reference, here's ERC20 implementation: https://docs.openzeppelin.com/contracts/2.x/api/token/erc20#IERC20-allowance-address-address-

Would love to hear some feedback!

Interesting proposal, and you've definitely spent enough time working on other chains and with other standards to know what works, what doesn't work, and what NEP-17 is lacking. I understand the merits for why "NEP-17.1" would include allowances, which allow users to propose trades without having to lock up their Neo. Allowances would enable more market activity derived from potential user demand.

However, we've seen time and time again BAYC and other newer NFT market entrants don't understand "Not Your Keys, Not Your Coins," how to protect their seed phrases, or to avoid malicious websites that will use allowances harmfully. I think we'd need some serious educational efforts around the subject matter. Are there any best practices that you've seen regarding education and informing users?

Are there any technical ways to resolve potential issues with allowances? Are there any methods to revoke allowances after 'x' amount of time? Or, are there any other innovative technical solutions that are being discussed in the EVM space?

I think it's a great idea, but also think we should remove as much responsibility on the end user who might not be willing to educate themselves beyond buying crypto and using it to trade for an NFT.

This topic has come up multiple times in the history of Neo and is hotly debated. For reference:

Original NEP-5 discussion: https://github.com/neo-project/proposals/pull/4
Nomenclature Discussion: https://github.com/neo-project/proposals/issues/50
Discussion on standard updates and composite concept: https://github.com/neo-project/proposals/pull/18

(There are other discussions that have arisen on this topic, but they are toxic and may derail this conversation)

Personally, I am a proponent of these methods and have not encountered a viable reason not the include them as a standard. They expand architectural options. In my opinion, a lot of the value on Neo Legacy has been cannibalized by the way we handle nep17 transfer events now, but there are still a number of usecases that have very limited architectural flexibility because these methods aren't standard.

I agree with @rdgrabowski regarding the user concerns. In my opinion, those are ultimately up to the integrator though. Many of the standards can be leveraged for dishonest means if they are implemented by someone with nefarious intent.

My personal preference would be for it to have its own integer value for the standard instead of using a hyphen since there is some implication of versioning using the other approach. I am happy with either though.

Without this, if someone wanted to place 10 offers on 10 different NFT for 10 NEO each, he would have to lock 100 NEO without even being sure those would be accepted.

Instead, I think 100% margin is the best. Otherwise, the seller accepts the order only to find that the buyer's assets are not enough to pay, which is a bad experience.

I agree with the comments from @lllwvlvwlll regarding NEP numeration. We should just pick the next one in line and avoid surfacing the "optional methods" discussion.

I expect there are use cases where these methods would be desired for NEP-11 tokens as well, right? Is it possible we can define a unified standard that accounts for both? Assuming the new standard is NEP-22, we would have supported_standards = ["NEP-11", "NEP-22"] and supported_standards = ["NEP-17", "NEP-22"].

Instead, I think 100% margin is the best. Otherwise, the seller accepts the order only to find that the buyer's assets are not enough to pay, which is a bad experience.

I share your opinion from the user perspective. Personally I'd prefer to lock tokens that I am allocating for a specific use, rather than risk letting my balance go too low and miss out on an opportunity. But I don't think that's good reason to discount the proposal.

This flow is very familiar to users on EVM-based chains, which makes the transition to N3 easier, and many (possibly most?) users prefer not having to lock up their tokens (particularly high volume traders). The users and applications will decide for themselves which approach they want to use, we should provide a standard to follow.

I agree with the comments from @lllwvlvwlll regarding NEP numeration. We should just pick the next one in line and avoid surfacing the "optional methods" discussion.

:+1: from me. Personally, I don't like these methods, but at the same time I don't see any reason for them not to be a part of some NEP-XX. NEO and GAS may not implement it, but other tokens can and this NEP will just ensure proper interoperability in case token wants/needs this. Otherwise some tokens will still provide these methods (nothing prevents them from doing so), but we're likely to get a number of incompatible/buggy versions of the same thing.

I agree with the comments from @lllwvlvwlll regarding NEP numeration. We should just pick the next one in line and avoid surfacing the "optional methods" discussion.

I expect there are use cases where these methods would be desired for NEP-11 tokens as well, right? Is it possible we can define a unified standard that accounts for both? Assuming the new standard is NEP-22, we would have supported_standards = ["NEP-11", "NEP-22"] and supported_standards = ["NEP-17", "NEP-22"].

Instead, I think 100% margin is the best. Otherwise, the seller accepts the order only to find that the buyer's assets are not enough to pay, which is a bad experience.

I share your opinion from the user perspective. Personally I'd prefer to lock tokens that I am allocating for a specific use, rather than risk letting my balance go too low and miss out on an opportunity. But I don't think that's good reason to discount the proposal.

This flow is very familiar to users on EVM-based chains, which makes the transition to N3 easier, and many (possibly most?) users prefer not having to lock up their tokens (particularly high volume traders). The users and applications will decide for themselves which approach they want to use, we should provide a standard to follow.

Yes that's also something which could be used for NEP-11, same concept of spender allowed to move NFT on behalf of an owner. I guess if this can coexist on one single standard sure, but those are quite different methods etc, not sure if it would fit in one?

Regarding @erikzhang comment re having all on margin, of course in a perfect world this would be the only way to ensure no stale orders are left. But personally i think this is something that needs to be implemented by the dapps themselves (ex enforcing balance check before placing offers, hiding stale offers, enforcing no news offers if you have some pending, etc etc). Also as @EdgeDLT and @roman-khimov suggested, it would probably help onboard more traditional users from other ecosystem as they are used to these methods. It doesn't have to be enforced, but if people want to use them, it's there.

I am for having this integrated into a new or "updated" standard as @vincentgeneste suggest. In my opinion, the tokens that support it will be more powerful, but as we all know; with great power comes great responsibility.

On one hand we are able to build better user experiences by allowing features like bids without margin, automated DeFi products, cross chain allowance for PolyNetwork contracts, and more. On the other hand we might create bad user experiences by having "out-of-money"/stale bids or the like. But in the latter case I agree with @vincentgeneste that it is up to the dApps to remedy such issues.

Then there is the issue of possible exploits, hacks or leaving the user more vulnerable. In my opinion, a lot of responsibility will be put on the wallet providers to provide a clean interface that makes it easy for users to understand what they are accepting/invoking/allowing. My suggestion is that the wallet providers makes it easy to access and view what they have currently allowed and revoke permissions. There is also a lot of responsibility on dApp developers to use this feature cautiously and not ask for unnecessarily high allowance on behalf of users. But dApp developers have to be very cautions anyways, so I am not so concerned about that aspect.

All in all I am eager about @vincentgeneste's proposal as I know I can deliver smarter products by having allowance.

Following up on this discussion .. anything else required to try and move forward? what's the general idea behind making those proposals do we need a consensus or how does it work in general, who get to decide if it's good / bad / if we can proceed or not ?

Any input appreciated!

I want to follow this proposal. The approve similar mechanism is not used in Neo ecosystem as in Neo core design it is not necessary. However, in the NFT trading, if influence the liquidity a lot. I would suggest if this can be an extended attribute for Nep-11 standard.

neo-project / proposals

NEP-17 extension discussion #152