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

NEP: Programmable NEP-5 Token Extension #64

Closed
wants to merge 29 commits into from

Conversation

deanpress
Copy link

@deanpress deanpress commented Aug 9, 2018

This NEP introduces memory-efficient methods for allowing conditional NEP-5 token transactions between users and third party smart contracts without the need for off-chain listeners.

View proposal

Edit:

The latest flow diagram of the method described in the NEP is as below:

image

@deanpress deanpress changed the title Create NEP-Programmable-Token-Extension.mediawiki NEP: Programmable Token Extension Aug 9, 2018
@deanpress deanpress changed the title NEP: Programmable Token Extension NEP: Programmable NEP-5 Token Extension Aug 9, 2018
@jeroenptrs
Copy link

jeroenptrs commented Aug 9, 2018

@deanpress have you considered using Dynamic Invocations instead of 2 separate methods? transferToContract would be able to appcall the dApp contract with the necessary operation and arguments, alongside the necessary txData (from, to, amount, tx_id).

This will prevent storage from being flooded, because there's no need to temporarily place it in Storage.

The onTokenReceive method in the dApp contract would now look like this onTokenReceive(txData)

@hal0x2328
Copy link
Contributor

It's not clear how the dApp contract authenticates the onTokenReceive callback is coming from the NEP-5 contract and not from a malicious invoker, I would expect to see

tokenHash == GetCallingScriptHash()

Obviously NEP-8 needs to be rolled out on the consensus nodes and the contract compiler before dynamic calls like these would be safe to use on the MainNet.

@deanpress
Copy link
Author

deanpress commented Aug 9, 2018

@jeroenptrs I discussed your suggestion with @brianlenz. The order of invocations in this case could also possibly be turned around by having the user call the dApp contract rather than the NEP-5 contract and utilizing transferFrom(). See:
programmable tokens 10

This would require the use of NEP5.1 approve() beforehand. The upside is it would eliminate any requirement for storage utilization, and there would be no need for a NEP standard. Also NEP-8 would have to be rolled out as @hal0x2328 says.

@jeroenptrs
Copy link

@deanpress it would also mean you need to use transferFrom(), which is also NEP5.1

@deanpress
Copy link
Author

deanpress commented Aug 9, 2018

@jeroenptrs Your suggestion in #64 (comment) would mean the AppCall originates from the NEP5 contract instead of the dApp. From my understanding the flow would be as follows:

programmable tokens 16

I agree that this may be a good potential solution as no additional storage from the NEP5 side is necessary, and it's simple. I'm curious as to what the security considerations are other than the requirement of NEP-8 and the contract compiler.

@erikzhang
Copy link
Member

... and dynamically invokes the script hash derived from the to parameter, providing from, to, amount, operation, args as parameters for the receiving smart contract.

How to provide these parameters? The entry point for the receiving contract should look like this:

object main(string operation, object[] args)

@nescrow
Copy link

nescrow commented Aug 10, 2018

It does not look like to be modification of NEP5, which is final btw. This creates unnecessary dependency and both developers (of NEP5 contract and of dApp contract) should be aware and keep this in mind.
There are already deployed NEP5 contracts and they will need to deploy new contract in order to support this, moreover if you want to create NEP5 token you will need to pay +500 GAS for Dynamic invocation in order to just support standard, but probably you don't even need it.
Rather than modify NEP5 would be better to implement deposit method in the dApp contract.
So if you want to create contract which is supposed to receive NEP5 tokens you will pay +500 GAS for Dynamic invocation and you will take care of "deposit" method.

Here is how we implemented deposit of NEP5 tokens in our Escrow contract:

public delegate object NEP5Contract(string method, object[] args);

//in deposit method
var args = new object[] { addressFrom, addressTo, amount };
var Contract = (NEP5Contract)assetId.ToDelegate();
var transferred = (bool)Contract("transfer", args);

where addressFrom is user's address and addressTo is ExecutionEngine.ExecutingScriptHash
So if you want to receive NEP5 tokens in your dApp contract the user will sign transaction and just call a deposit method of your contract.
In order to have clients(e.g. light wallets) aware of your deposit method, this deposit method can be considered as a new NEP and used together with NEP10.

@jeroenptrs
Copy link

jeroenptrs commented Aug 10, 2018

@erikzhang for the token, operation will be transferToContract, the args will include the following (I've updated the names for clarity):

  • from (user)
  • to (dApp)
  • amount
  • dappOperation (the operation the dApp contract will be called with)
  • dappArgs (optional args that will be added to the dApp contract invocation. Thus concatted after the txData)

Resulting in a call to the to contract address, with dappOperation as operation and [...txData, ...dappArgs] as args

@deanpress
Copy link
Author

deanpress commented Aug 10, 2018

I updated the NEP to include another feature. The method name is changed to transferAndInvoke:

public static bool transferAndInvoke(from, to, amount, scriptHash, operation, args)

By letting the user specify scriptHash separately from the to address, a smart contract can process transaction data sent to a different address. This allows for new use cases such as ICO smart contracts having the ability to process funds sent to a different addresses.

@erikzhang the method transferAndInvoke has its own "transferAndInvoke" operation, living in a NEP-5 contract.

User/application calls NEP-5 contract with this extension like:

transferAndInvoke [ myAddress, yourAddress, 100, dAppContractScriptHash, "processTransaction", ["This is a string to be handled by the dApp smart contract", "another string", 10, "as many as you want"] ]

With transferAndInvoke, the NEP5 contract validates that tokens can be transfered, then performs a dynamic invocation to dAppContractScriptHash with operation processTransactions and an args list, and transfers tokens:

args[0] : from
args[1] : to
args[2] : amount
args[3] : "This is a string to be handled by the dApp smart contract"
args[4] : "another string"
args[5] : 10
args[6] : "as many as you want"

Here's the latest diagram:
image

@neoescrow It's not meant as a part of NEP-5, but rather an extension applicable to NEP-5 tokens (its own NEP) which can be defined in NEP-10's supportedStandards.

@nescrow
Copy link

nescrow commented Aug 10, 2018

@neoescrow It's not meant as a modification of NEP-5, but rather an extension applicable to NEP-5 tokens (its own NEP) which can be defined in NEP-10's supportedStandards.

@deanpress okay, but you are trying to put that logic in NEP5 compatible token contract which is out of his scope of responsibility. The NEP5 contract supposed to hold tokens info, isn't?
And the user's address is basically also can be considered as a contract, so why it would be a special case of transfer.
There are already different types of NEP5 contracts, one type that can be used as described in the previous comment and another one where this "transfer" call will be failed because of new validation added silently in NEP5 regarding caller contract hash and without backward compatibility, the third one where NEP5.1 supported.
There is already mix of standards.
There are even shown examples which are supposed to be compatible with NEP5, but they are not.

And you propose to continue complicate it for the developer of NEP5 token contract, where he needs to worry about things which can be actually implemented in the dApp contract.
The less we will have such special cases the simpler support in the future, where smart contracts are independent and do only their scope of responsibility.

@hal0x2328
Copy link
Contributor

There is a mix of implementations because the NEP-5 standard implemented exactly as written leaves tokens insecure and crippled for a lot of use cases. So instead of piling on extensions, maybe it's time for a ground-up re-write of the what the official token standard should be and deprecate NEP-5 altogether.

@ediopia
Copy link

ediopia commented Aug 10, 2018

Agreed with @hal0x2328. If NEP-5 is not only for ICOs but dApps. I have been finding a way how to make a smart contract pass CheckWitness with dynamic calls so users can transfer other tokens that the smart contract owns.

Found a solution in MCT document:

"The MCT NEP-5 transfer implementation detects that the transfer is coming from an address that corresponds to a known smart contract, and instead of using CheckWitness(scripthash) to verify the owner of the funds, it verifies that the calling contract's scripthash matches the sender's."

@jeroenptrs
Copy link

jeroenptrs commented Aug 10, 2018

@neoescrow no this is not an addition, this is an extension in the form of a new NEP that's used in tokens, following the NEP-10 standard. There are many tokens that have the - now no longer in the NEP-5 standard (so I don't know how you aim to consider NEP-5 final if such edits can still be made) - optional methods, and there are many that don't.

It's important to get new ways of interaction in new standards, or like @hal0x2328 said, scrap NEP-5 all together for a new standard but that won't help either

@deanpress
Copy link
Author

deanpress commented Aug 10, 2018

@hal0x2328: ...maybe it's time for a ground-up re-write of the what the official token standard should be and deprecate NEP-5 altogether.

The new implementation of smart contract invocations (deployment becomes cheap, GAS fee for invocations) could serve to be a good moment for rewriting NEP-5. If we launch a new NEP-5 standard with all desirable "extensions" and features, it would be a smooth process, and collective deployment fees will not be excessive. Alternatively NEO 3.0 could serve as a decent starting point.

But until then, NEP-5 extensions seem to be the way to go because of NEP-10.

@nescrow
Copy link

nescrow commented Aug 10, 2018

@jeroenptrs it seems, you just read between rows :) The addition or extension doesn't matter. This is a change in NEP5 contract code and the developer should take care of all compatibility cases when he just wants to create NEP5 token, because it costs 490 GAS and you suggest even add 500 more GAS for Dynamic invocation in order to support this standard. Your extension can be implemented in the contract where this functionality is needed - in the dApp contract.
The NEP5 finality as a standard is separate topic...

@jeroenptrs
Copy link

jeroenptrs commented Aug 10, 2018

@neoescrow I feel like you are ignoring everything I stated above. This isn't NEP-5, this is a new standard that builds on top of NEP-5.

If you want to develop a NEP-5 token with the functions detailed there, that's a NEP-5 token. If you want further extensibility that this NEP and dynamic invokes provide, you create a token that uses both NEP-5 and this one, also using the supportedStandards from NEP-10.

addition 1: You, as a dev developing a new token and/or dApp in this smart economy, can just pick and choose standards from which you want to include implemention. If you pick from multiple NEPs surrounding tokens, it would be best to use NEP-10’s specification to detail what standards (including NEP-5 and NEP-10) you are extending.

As stated in an earlier comment, there’s already an inconsistency in NEP-5 tokens, because it used to include other methods that were once optional and then removed. To prevent this for future contracts, a standard like NEP-10 has been put into place, so that new functionality can be added to tokens without editing the NEP-5 specific code (as you are currently assuming?).

addition 2: this also means that you’re not obligated to pay up another 500 GAS if you’re just developing a NEP-5 token. This has no implication towards existing and future tokens that are set on solely following the NEP-5 standards. The existing tokens that do want this specific implementation in their contract, well then there’s the additional cost of 500 GAS. But this is only in place if you do so choose to use transferAndInvoke

@nescrow
Copy link

nescrow commented Aug 10, 2018

@jeroenptrs the things you described are clear, the point of the comments is that it's not a place to put it in the token contract, that's it. Just an opinion, there is no try to be rude ;)

@erikzhang
Copy link
Member

If I understand it correctly, its function is to notify the receiving contract that a transfer has occurred.

Why don't we just call the onreceived method of the receiving contract?

to("onreceived", [assetid, from, amount]);

Or we can use this way to notify the receiving contract: neo-project/neo#308

@deanpress
Copy link
Author

deanpress commented Aug 13, 2018

@erikzhang transferAndInvoke allows for sending tokens to Address A and invoking a different Scripthash B, I don't believe this is a feature with onreceived (have not heard of this one though?) or neo-project/neo#308.

@jeroenptrs
Copy link

@erikzhang where can I find the onreceived specification?

@erikzhang
Copy link
Member

You can easily combine the transfer and other operations in a single InvocationTransaction. The purpose of this proposal is to combine the two operations in order to inform the receiving contract of the correct amount and from of the transfer. I think there are two ways to better inform, which is what I said above.

And, onreceived is not a specification. I mean, why not fix the operation in transferAndInvoke to onreceived?

@deanpress
Copy link
Author

deanpress commented Aug 15, 2018

@erikzhang if you invoke the dapp contract separately, how can it verify to, from, amount is correct?

By onreceived, do you mean why not have the transferAndInvoke always dynamically invoke the same operation? That's possible, but if you want to call a specific action you would have to define sub operations under onreceived in the dApp contract. So it takes one more argument in the args list. I'm curious to hear why binding to an onreceive operation should be considered, though.

@erikzhang
Copy link
Member

@deanpress

One way is to automatically call the onreceived method of the receiving contract in the transfer method of the token contract, and pass the from, to, and amount parameters.

Another way is to call System.Runtime.GetNotifications in the receiving contract. Since the notifications are sent from the token contract, their credibility is the same as the previous way or the transferAndInvoke way.

@deanpress
Copy link
Author

deanpress commented Aug 16, 2018

@erikzhang For your onreceived example, how does the sender supply a list of arguments with their transaction? In this case the transfer method would need fundamental changes. Therefore a different method would be more efficient so it can serve as an extension to the existing token standard.

For the second example I can see that working, but would have to try in practice. @jeroenptrs what do you think?

@jeroenptrs
Copy link

jeroenptrs commented Aug 16, 2018

@erikzhang Correct me if I'm wrong, but for the System.Runtime.GetNotifications example do you also need to invoke the receiving contract?

@deanpress we already discussed this, but a fixed operation does make sense in most cases (it would essentially help if onreceived was thus implemented in the dApp standard as well).

You could add a flattened args array to that invocation and provide your operation identifier as part of the args. This way, the choice of implementing different sub-operations within onreceived or just one singular operation is left to the dApp developer.

@deanpress
Copy link
Author

deanpress commented Aug 16, 2018

@jeroenptrs Adding onreceived to the dApp standard requires it to always support NEP-5 tokens, even though some dApps may not integrate with tokens at all. It makes more sense to update this NEP (Programmable Tokens Extension) to add onreceived in the receiving smart contract, as part of the extension.

As for your question to @erikzhang, a smart contract always has to be invoked for it to run an action. So the biggest consideration between transferAndInvoke and System.Runtime.GetNotifications is system fees. I suppose retrieving and parsing notifications is more costly than parsing args that have been passed by a dynamic invocation?

@jeroenptrs
Copy link

@deanpress I disagree that dApps that don't work with NEP-5s shouldn't implement an onreceive function. Merely for the fact that this way, if someone accidentally sends NEP-5 tokens to a contract, that contract now has standard behaviour to work with it (i.e. return it to the owner, or send it to the dapp creator because it's a donation).

This minimizes coin burn.

@erikzhang reading through the GetNotifications proposal, it appears to me this would still require a dynamic invocation because it will only show the notifications from the same invocationtransaction.

@deanpress
Copy link
Author

deanpress commented Aug 20, 2018

@erikzhang Regarding GetNotifications: If a token allows user input for Notifications, and the receiving smart contract doesn't validate Notifications properly, wouldn't this be more open to vulnerabilities than transferAndInvoke? The latter always sends data consistently.

@erikzhang
Copy link
Member

What do you mean by allowing user input? The notification is automatically sent by the token contract.
Please check the NEP-5 standard. https://github.com/neo-project/proposals/blob/master/nep-5.mediawiki#transfer-2

@jeroenptrs
Copy link

jeroenptrs commented Aug 20, 2018

@erikzhang could you confirm whether or not tranferAndInvoke is needed upon using getNotifications? The way I'm interpreting it is that you still need the notification to be called in the same invocation thus a dynamic invoke is still needed.

To expand on this, as you say here, a user would transfer first, and then call deposit. transferAndInvoke allows this to happen in the same invocation. Wouldn't this be a better method to handle actions upon transfers?

@deanpress
Copy link
Author

deanpress commented Aug 20, 2018

@erikzhang NEP-5 doesn't stop anyone from expanding the smart contract with custom Notification event triggers. Wouldn't it be possible that a different custom Notification event trigger (such as one that allows user input) adds vulnerabilities?

I agree with @jeroenptrs, it looks like GetNotifications could still benefit from a method like transferAndInvoke.

@erikzhang
Copy link
Member

@deanpress Is it still necessary to implement transferAndInvoke? Can it solve the problem that can't be solved now?

@erikzhang erikzhang closed this Jan 14, 2021
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

Successfully merging this pull request may close these issues.

None yet

6 participants