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-5 Amendment #44

Merged
merged 5 commits into from
Jun 25, 2018
Merged

NEP-5 Amendment #44

merged 5 commits into from
Jun 25, 2018

Conversation

erikzhang
Copy link
Member

@erikzhang erikzhang commented May 17, 2018

This amendment of NEP-5 standardizes the implementation details of each method.

@erikzhang
Copy link
Member Author

@AshRolls @canesin @Dexaran @dicarlo2 @Ejhfast @lightszero @lllwvlvwlll @localhuman @luodanwg @mwherman2000 @RavenXce @shargon @tanZiWen

@shargon
Copy link
Member

shargon commented May 18, 2018

It's perfect for me

@mwherman2000
Copy link
Contributor

There are several issues related to the completeness of the NEP-5 specification from an implementation and testing/quality assurance perspective. For example, http://docs.neo.org/en-us/exchange/v2.7.3.html#dealing-with-nep-5-assets-transactions is a more complete, more definitive, more authoritative specification for NEP-5 than than the NEP-5 specification itself. This content needs to be either (a) moved/copied into NEP-5, or (b) there needs to be a new NEP-5 that defines the basic architecture of a NEO smart contract (e.g. the Main method, if-then-else dispatching, individual callable methods).

You can find a more detailed discussion and motivation for the above here:
#33
and here
#32

The specification needs to be corrected/updated. It's not strong enough to be implemented reliably on the NEO platform as it is written. It needs to be a specification ...not a high-level guide.

@mwherman2000
Copy link
Contributor

mwherman2000 commented May 18, 2018

When documenting parts of the NEO platform, I suggest we take a broad, long-term, architectural view.

For example, the different formats and lengths of an NEO Account address are applicable across the entire NEO architecture and should be contained in their own NEP specification - and referenced by NEP-5. They shouldn't be defined in NEP-5 itself. The new NEP should also define specific named constants for these lengths.

image

@erikzhang
Copy link
Member Author

@mwherman2000 NEP is Neo enhancement proposal, not the yellow paper.

NEO yellow paper is on the way.

@mwherman2000
Copy link
Contributor

@erikzhang What yellow paper? Do you have a link to a draft?

These are highly relevant topics as far as the NEP-5 discussion is concerned and the discussion can't assume anything about a document that no one has seen. Right?

@erikzhang
Copy link
Member Author

@mwherman2000 I mean, NEO's basic design will be defined in the upcoming yellow paper, not in the NEO Enhancement Proposals. NEP is for new proposals.

@mwherman2000
Copy link
Contributor

mwherman2000 commented May 21, 2018

@erikzhang when can we expect to see the yellow paper? Do you have a draft version?

@erikzhang
Copy link
Member Author

I don't have a draft yet. Maybe I should create a new repository for it and anyone can contribute to it.

shargon
shargon previously approved these changes May 22, 2018
@DaveOnBlocks
Copy link

The function SHOULD check whether the from address equals the caller contract hash. If so, the transfer SHOULD be processed; If not, the function SHOULD use the SYSCALL Neo.Runtime.CheckWitness to verify the transfer.

Is this implying that sending a transfer(address1,address1,amount) should succeed?

@DaveOnBlocks
Copy link

If the to address is a deployed contract, the function SHOULD check the payable flag of this contract to decide whether it should transfer the tokens to this contract.

How does one check the payable flag?

@erikzhang
Copy link
Member Author

The function SHOULD check whether the from address equals the caller contract hash. If so, the transfer SHOULD be processed; If not, the function SHOULD use the SYSCALL Neo.Runtime.CheckWitness to verify the transfer.

Is this implying that sending a transfer(address1,address1,amount) should succeed?

It means that if contract A calls the transfer method of contract B, and parameter from is the scripthash of contract A, then the transfer should be processed.

@erikzhang
Copy link
Member Author

How does one check the payable flag?

Use the API Neo.Contract.IsPayable

https://github.com/neo-project/neo/blob/b0b0646fb86b350e57f9010ca88ca373144d1353/neo/SmartContract/StateReader.cs#L127

@dicarlo2
Copy link

This looks fine to me since afaict this is not a breaking change.

nep-5.mediawiki Outdated

* Remarks: The "transfer" event is raised after a successful execution of the "transfer" method.
A token contract which creates new tokens SHOULD trigger a <code>transfer</code> event with the <code>from</code> address set to <code>null</code> when tokens are created.

Choose a reason for hiding this comment

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

+1000 on this!

This was not in the original NEP5, but block explorers like neotracker or neoscan depend on this event to be called when tokens are minted. So there have been cases where this notification was not emitted at the time of minting, and resulted in confusion for users who would see their tokens in their wallets, but not on the explorer. Until they make a transfer.

Hopefully this will prevent that from happening in the future.

That being said, would it be possible to update this from a SHOULD to a MUST?

Copy link

@nickfujita nickfujita Jun 6, 2018

Choose a reason for hiding this comment

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

After further consideration, I am starting to wonder if it makes more sense to remove this clause from the transfer event. The ask is to have the transfer event triggered from a method that is not a part of the NEP5 standard. Whether it be though a token sale, direct allocation method, or some dynamic token creation event, these all result in an action more akin to that of a mintToken or mint event, rather than a transfer that is inferred to be a mint when the from"address is null.

The desire for a separate event for token creation (and even token burn) have been expressed a few times in other posts.

#39

CityOfZion/neo-python#457

I would like to reaffirm the proposal stated in the latter thread, with more urgency on the mint or mintToken event over the burn or burnToken event.

mint or mintToken

public static event mint(byte[] to, BigInteger amount)
public static event mintToken(byte[] to, BigInteger amount)

MUST trigger when tokens are minted.

burn or burnToken

public static event burn(byte[] from, BigInteger amount)
public static event burnToken(byte[] from, BigInteger amount)

MUST trigger when tokens are burned.

These events will help to increase transparency into contract mint and burn events amongst users, and possibly improve confidence that tokens are not being minted unknowingly.

Copy link
Member Author

Choose a reason for hiding this comment

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

Too many events. The standard should be as simple as possible.

Choose a reason for hiding this comment

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

While I agree that the standard should be simple to understand and implement, shouldn't it also be descriptively accurate?

Rather than adding the SHOULD clause to the transfer event, it seems more accurate to create a separate event called mint. I understand that the implication that tokens are being transferred from null because they are being created, but this implication assumes this prior knowledge from a reader rather than being clear and self explanatory.

Could we please reconsider just the addition of the mint event to clean up the notifications being emitted by contracts?

Choose a reason for hiding this comment

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

I don't quite see the need for a mint/burn event myself either. Seeing funds created/destroyed by coming from/to null shows that the tokens either came from nowhere or were sent to nowhere.

+1 on changing the SHOULD to a MUST though. If tokens are being created/destroyed it is good to be able to pick this up without checking TotalSupply constantly and inferring the change. Having the change notification arise out of a certain block is a much cleaner way to show it.

Copy link

Choose a reason for hiding this comment

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

@erikzhang
Copy link
Member Author

Let's vote?

@mwherman2000
Copy link
Contributor

mwherman2000 commented Jun 9, 2018

NEP-5 now talks about methods everywhere ...and not "operationX".

Does this imply that directly invoking an NEP-5 SC's detailed methods are the only way to invoke their functionality? ...i.e. has calling Main("operationX", args) been deprecated? When should an app call Main() and when should it directly call one of the detailed methods?

@shargon
Copy link
Member

shargon commented Jun 11, 2018

In transfer method

The parameter amount MUST be greater than or equal to 0
to
The parameter amount MUST be greater than 0
?

Why we need a TX with 0 amount?

@erikzhang
Copy link
Member Author

Why we need a TX with 0 amount?

The transfer method may be invoked by another contract, and the number may be calculated in that contract, possibly equal to zero. To ensure the success of the entire contract, we should allow a transfer of 0 amount.

@shargon
Copy link
Member

shargon commented Jun 22, 2018

You only have one entry point. Is main. Then you can do whatever you want

@mwherman2000
Copy link
Contributor

mwherman2000 commented Jun 22, 2018

@shargon Agreed. But this needs to be made absolutely clear in the NEP-5 specification. The Amendment as well as the current specification are misleading. They both use C# method syntax to describe the arguments to Main() - implying to archs/devs/testers/integrators that these C# methods need to be implemented in an NEP-5 compliant SC. This makes no rational sense at all and is confusing and misleading.

For example, why does the Amendment use C# syntax to specify that an NEP-5 compliant SC has a method

public static bool transfer(byte[] from, byte[] to, BigInteger amount)

when it is more correct, simple and unambiguously stated that one of the allowed sets of arguments that can be passed to public static object Main(string operation, params object[] args) is:

operation args[0] args[1] args[2] args[3] return type
"transfer" byte[] from byte[] to BigInteger amount bool

Let's break the pattern of creating a NEO House of Cards. @erikzhang

image

@kemargrant
Copy link

kemargrant commented Jun 22, 2018

At this time there needs to be an allocation of resources to explicitly document the neo smart contract platform. It may be a bounty, stake holder driven, or a person within neo council.

From the outside looking in Solidity is much more straightforward. I am not a c# dev so I may have some bias, as well as the fact ethereum is very well documented.

Even with well documented code there are still room for mistakes, the latest being the icx contract. The more room left for interpretation is the more rope you have given to the developer.

@erikzhang
Copy link
Member Author

Would you like me to re-edit NEP-5 based on the above?

If we decide to change the wording, then all the NEP will have to be modified so that their style is consistent. But I prefer to use the existing wording.

@erikzhang
Copy link
Member Author

From the outside looking in Solidity is much more straightforward. I am not a c# dev so I may have some bias, as well as the fact ethereum is very well documented.

Neo and NeoContract are not designed for a particular language. Their design goal is to use any programming language for smart contract development. At the same time Neo and NeoVM also have a variety of different language implementations.
So their documents must be an abstract expression. But I agree that we should use as precise a language as possible to describe all the standards.

@mwherman2000
Copy link
Contributor

mwherman2000 commented Jun 23, 2018

RE: If we decide to change the wording, then all the NEP will have to be modified so that their style is consistent. But I prefer to use the existing wording.

@erikzhang, it's irrational and misleading from the perspective of any conventional software architect, developer, tester, and system integrator to the point of being ludicrous that an NEP specification would use C# method signatures to describe the NEP-5 operation and args parameters ...because it might be too much work to update 2, 3, 4, ...existing NEP specifications??

I'm finished trying to discuss this any further. Let the cards fall where they may and fall quickly.

image

@RavenXce
Copy link

RavenXce commented Jun 23, 2018

FWIW, ethereum also uses solidity to describe a large part of their specs, even though solidity is not the only language that compiles for the evm.

See the recently finalized NFT standard for example: https://github.com/ethereum/EIPs/blob/master/EIPS/eip-721.md

Note in particular: https://github.com/ethereum/EIPs/blob/master/EIPS/eip-721.md#caveats

@mwherman2000
Copy link
Contributor

mwherman2000 commented Jun 23, 2018

@RavenXce, the ETH examples are irrelevant to this discussion because Solidity/ETH SCs don't have the same SC architecture/structure that NEO smart contracts follow where a NEO SC invocation is a call to one and only one SC entrypoint method Main() - end of story, full stop.

The sole essence of the NEO smart contract procotol is to pass operation and args parameters to Main() and Main() alone. Any other functions you define in your C# contract can't be called and aren't called (unless indirectly from/through Main()). To list these methods in the NEP-5 (or any NEP) specification is misleading.

NEP specifications need to be clear and totally unambiguous about this.

@dicarlo2
Copy link

NEO does not require a Main function, that's just a convention used by the C# and python compilers. A smart contract is just a sequence of op codes that are executed in order. Ethereum works the same way. Given that, @mwherman2000, what you're suggesting is that the spec should be written in terms of the state of the stack at the beginning of the smart contract script execution.

I do not agree (and apparently, many others do not as well, see NEO and Ethereum interface specifications) that we should require smart contract authors to understand such a low level concept. Using C# syntax to describe the spec seems perfectly reasonable, for similar reasons that using Solidity to describe Ethereum smart contracts is perfectly reasonable - it's the first language supported, the syntax is relatively easy to understand even if you don't code in C#, etc.

FWIW, NEO-ONE's TypeScript compiler does not have a Main entrypoint, instead defining all public properties/methods of a subclass of SmartContract to be the external interface that is accessible by running the smart contract (e.g. https://github.com/neo-one-suite/neo-one/blob/master/packages/neo-one-smart-contract-lib/src/Token.ts).

@dicarlo2
Copy link

Pinging a few people to take a look since we should get this amendment in sooner rather than later. The only major clarification that I see is around the transfer event for minting and burning tokens.

@canesin @shargon @localhuman @RavenXce @lllwvlvwlll.

@canesin
Copy link

canesin commented Jun 24, 2018

Sounds good to me, adding SHOULD recommendations for burning and minting tokens on the transfer event is the only request I would make (since it doesn't change the standard practically I don't thing that is a big issue).

Is it possible to just use null ? So if is minting from is null and if burning to is null in the event and observers can just treat as such ?

@mwherman2000
Copy link
Contributor

null is not a valid/usable/reliable construct in C#.NEO ...if that is the specific null concept you're referring to @canesin.

Reference: https://youtu.be/Nj4-m2o94VE?t=33m32s ...listen to the next 2 slides ...until timecode 35:39 ...2 minutes.

@mwherman2000
Copy link
Contributor

mwherman2000 commented Jun 24, 2018

RE: Given that, @mwherman2000, what you're suggesting is that the spec should be written in terms of the state of the stack at the beginning of the smart contract script execution.

@dicarlo2 To be perfectly clear, I've never stated, suggested or endorsed defining NEP-5's invocation pararmeters in terms of the NEO VM stack or stack terminology, correct? never. I've only suggested (strongly) that we define the parameters to Main() as C# parameter arguments (terminology that any developer will understand) and not use the current misleading terminology of C# method syntax.

In addition, if you reread this thread, we have had two experts, @erikzhang and @shargon, attest to the fact that there is and needs to be the Main() method in a C# smart contract and all invocations of a NEO C# smart contract's internal functionality call and go through the Main() method. It's the only prescribed/supported way to invoke a SC's internal functionality. Here's the evidence:

@dicarlo2 Where/who is the source of your information? Do we have to unwind/rewind the discussion thread? Please clarify your sources of your information? If have to revisit these first principles, let start again.

@dicarlo2 Perhaps you can double check your information offline with @erikzhang and @shargon. We need a 3-way confirmation/agreement here.

@Ejhfast
Copy link

Ejhfast commented Jun 24, 2018 via email

@mwherman2000
Copy link
Contributor

@Ejhfast Exactly because of these situations. I know the answers and I'm only asking the questions to lead the discussion and get written answers that we can reference in discussions like the one we're having now.

@Ejhfast and @dicarlo2: Are @erikzhang and @shargon correct or not correct? See the 3 links quoted above.

[Note: I've edited my previous comment several times to make it as clear and precise as possible.]

@Ejhfast
Copy link

Ejhfast commented Jun 24, 2018 via email

@mwherman2000
Copy link
Contributor

mwherman2000 commented Jun 24, 2018

@Ejhfast and @dicarlo2: Maybe we're reshaping this discussion topic into: How to best represent multiple language bindings in an NEP?

I lean on the C# Main() binding as the authoritative approach because of @erikzhang and @shargon's comments and the fact that C# is the reference implementation language for the NEO platform.

If a non-C# language has a different NEO binding strategy/architecture, that's a different situation. Perhaps we need to discuss a solution for that separately.

However, the core issue is the current convention of using C# method syntax is misleading e.g.

public static bool transfer(byte[] from, byte[] to, BigInteger amount)

...and is contributing to the fragility and failure of the NEO platform.

@Ejhfast
Copy link

Ejhfast commented Jun 24, 2018 via email

@mwherman2000
Copy link
Contributor

mwherman2000 commented Jun 24, 2018

@Ejhfast I wrote NEP-10. My goal is to promote and develop a trusted ecosystem of hundreds ...maybe thousands of NEPs (and NCPs). NEP-5 is incredibly important in this regard. It's like the Block 0 NEP.

To allow the current situation of vague and misleading C# method syntax continue is untenable to the point of being ludicrous.

The NEO platform is in serious trouble because of its fragile software architecture (neo-project/neo#272 (comment)). More or better developer tutorials isn't going to fix the problem.

@Ejhfast
Copy link

Ejhfast commented Jun 24, 2018

The comment you linked to describes many important issues that should be addressed by the NEO team and platform. You can probably find community agreement on some of those issues, so I'd recommend spending time there instead of this minor point (on which most people who understand the platform disagree with you).

@lllwvlvwlll
Copy link
Member

The proposed changes are straight forward and further enhance the clarity of the standard: APPROVED

As @Ejhfast has indicated, the extent of this conversation is a poor use of resources for people who could be focusing on critical platform development (including some that @mwherman2000 has directly requested). @mwherman2000 I would also recommend auditing your method of seeding discussion. I am receiving many messages on the discord about the aggressive (and sometimes offensive) nature of these conversations.

@mwherman2000
Copy link
Contributor

#RIP

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.