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

Replace NEP-5 with NEP-17 #126

Merged
merged 20 commits into from
Jul 29, 2021
Merged

Replace NEP-5 with NEP-17 #126

merged 20 commits into from
Jul 29, 2021

Conversation

erikzhang
Copy link
Member

@erikzhang erikzhang commented Nov 12, 2020

Differences from NEP-5:

  1. Added onPayment call if the receiver is a deployed contract.
  2. Removed payable check. It has been replaced by onPayment.
  3. Removed name method, because it should be in manifest for all the contracts, not only for the token contracts.
  4. Changed the transfer event to capitalize the first letter Transfer.

@erikzhang
Copy link
Member Author

@shargon @ixje @roman-khimov @vncoelho @EdgeDLT @igormcoelho @neo-project/ngd-shanghai

Copy link
Contributor

@ixje ixje left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First off, happy to see this new NEP 👍 I think an extension to NEP5 could have done, but completely superseding is fine with me as well.

Note that there is a downside to removing the name function for light wallets. Currently a light wallet can create a single script and use a single invokescript RPC call to fetch the name, symbol, decimals and totalSupply. Changing this requires another RPC call (getcontractstate) and fetch the name from there (I'm guessing from the extra field, of which the content aren't standardised)

nep-17.mediawiki Outdated Show resolved Hide resolved
Copy link
Member

@shargon shargon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add transferFrom and allowance?

nep-17.mediawiki Outdated Show resolved Hide resolved
@vncoelho
Copy link
Member

@shargon, what would be transferFrom and allowance?

@ixje
Copy link
Contributor

ixje commented Nov 12, 2020

@shargon, what would be transferFrom and allowance?

you allow another account to transfer from an account you own. The other account uses transferFrom

@erikzhang
Copy link
Member Author

Add transferFrom and allowance?

With approve and transferFrom, you need two steps. In Neo3, we don't need them. With transfer and onPayment, you only need one step.

nep-17.mediawiki Outdated Show resolved Hide resolved

==Abstract==

This proposal outlines a token standard for the NEO blockchain that will provide systems with a generalized interaction mechanism for tokenized Smart Contracts. This mechanic, along with the justification for each feature are defined. A template and examples are also provided to enable the development community.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be interesting to also invite previous authors of NEP-5 to join this NEP-17 for discussion, or even, authorship (if they agree too).

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P.S.: just noticed you informed that on the TODO list, requiring people to submit Pull Request. That's fine then.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ping @lllwvlvwlll to discuss with other authors of NEP-5.

@Tommo-L
Copy link

Tommo-L commented Nov 12, 2020

Removed name method, because it should be in manifest for all the contracts, not only for the token contracts.

Then we'd better add author, description, email, website in manifest file, too

@erikzhang
Copy link
Member Author

Then we'd better add author, description, email, website in manifest file, too

They can be in Extra.

nep-17.mediawiki Outdated Show resolved Hide resolved
nep-17.mediawiki Outdated Show resolved Hide resolved
nep-17.mediawiki Outdated Show resolved Hide resolved
nep-17.mediawiki Show resolved Hide resolved
nep-17.mediawiki Show resolved Hide resolved
public static BigInteger totalSupply()
</pre>

Returns the total token supply deployed in the system.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we also need to limit it for the same reasons (at least to int64), and balanceOf as well.

nep-17.mediawiki Outdated Show resolved Hide resolved
nep-17.mediawiki Show resolved Hide resolved
nep-17.mediawiki Outdated Show resolved Hide resolved
nep-17.mediawiki Outdated Show resolved Hide resolved
nep-17.mediawiki Show resolved Hide resolved
shargon
shargon previously approved these changes Nov 16, 2020
@lllwvlvwlll
Copy link
Member

Boa Reference: https://github.com/CityOfZion/neo3-boa/blob/development/boa3_test/examples/NEP17.py

@chenzhitong
Copy link
Member

Should we add the onPayment method to the standard methods of NEP-17 or C# smart contract template?

@erikzhang
Copy link
Member Author

@chenzhitong onPayment is a receiver method.

@chenzhitong
Copy link
Member

If a receiver doesn't implement the onPayment method, will the transfer execute fail?
接收者不实现onPayment ,转账是否会报错?

In addition, should the manifest of the NEP-7 method must be written

"permissions": [
  {
    "contract": "*",  
    "methods": "*"
  }
]

, because it needs to call the onPayment method of to.
是不是所有的NEP-17方法都要在manifest中添加上述的permission,因为它需要调用to的onPayment方法
@erikzhang

@erikzhang
Copy link
Member Author

{
  "contract": "*",  
  "methods": "onPayment"
}

@chenzhitong
Copy link
Member

chenzhitong commented Feb 2, 2021

那一个合约(ICO合约)想接收别人给他转账,是不是要在 manifest 中这么写
"trusts": *
这样岂不是很不安全。
@erikzhang

@erikzhang
Copy link
Member Author

erikzhang commented Feb 2, 2021

"trusts": *

For example, if the ICO contract needs to receive neo, it has to trust neo. I don't think an ICO can accept any type of tokens.

@satoshichou
Copy link

When will it be merged to master. The proposal seems is already implemented in native ocntract.

@erikzhang
Copy link
Member Author

Merge?

shargon
shargon previously approved these changes Apr 17, 2021
superboyiii
superboyiii previously approved these changes Apr 19, 2021
Copy link
Member

@superboyiii superboyiii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's OK to merge.

RichardBelsum
RichardBelsum previously approved these changes Apr 28, 2021
@erikzhang erikzhang dismissed stale reviews from RichardBelsum, superboyiii, and shargon via 2cce389 July 29, 2021 06:52
@erikzhang erikzhang merged commit 3d242f5 into master Jul 29, 2021
@erikzhang erikzhang deleted the nep-17 branch July 29, 2021 06:53
@RichardBelsum
Copy link

Celebrate🎉

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

Successfully merging this pull request may close these issues.