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 event naming on Neo 3 #1646

Closed
roman-khimov opened this issue May 14, 2020 · 3 comments
Closed

NEP-5 event naming on Neo 3 #1646

roman-khimov opened this issue May 14, 2020 · 3 comments

Comments

@roman-khimov
Copy link
Contributor

Describe the bug
Native NEO and GAS contracts are not compliant with NEP-5 specification in their event (notification) name.

Details
The NEP-5 specification describes "transfer" event that is to be generated by compliant contracts under specific circumstances. It can be seen in example contract, tutorial, another example, implementation of compliant tracker plugin and compliant node. All of these work fine in Neo 2 world.

Fast-forward to Neo 3 with #717 that added native NEO and GAS contracts claiming them as supporting NEP-5. But in fact these contracts emit a bit different event named "Transfer", so technically they're not compliant with NEP-5 specification. Moreover, neo-project/neo-modules#125 changed the old standard-compliant NEP-5 tracker plugin to something working with this new "Transfer".

Expected behavior
Following existing standards in Neo 3 or creating new ones if there is a need to change something.

(Optional) Additional context
I'd suggest changing generated event name back to lower-case "transfer", because that was the name for quite some time and there is code that's expecting exactly that. Also, people trying to follow NEP-5 standard would probably be a bit surprised to find out that the new NEP-5 tracker plugin doesn't work for their contracts.

The other option of course is rolling out new specification that can define this event in any new fashion (lower case, upper case, mixed case, case-insensitive, etc...).

AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this issue May 15, 2020
Native contracts deployment creates `Transfer` notifications and adds
them into interop context. However, these notifications were not stored
for two reasons:
1. typo in `Transfer` (so these notifications were not recognised during
processing of the invocation tx in (*Blockchain).storeBlock(...) method)
2. these notifications have `from` adress setted to null, so conversion
to []byte fails.

Related C# issue: neo-project/neo#1646

For now, made both `transfer` and `Transfer` valid.
@shargon
Copy link
Member

shargon commented May 15, 2020

Vote:

uppercase (Transfer) 👍
lowercase (transfer) 👎

@erikzhang
Copy link
Member

I vote for uppercase or case-insensitive.

AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this issue May 15, 2020
Native contracts deployment creates `Transfer` notifications and adds
them into interop context. However, these notifications were not stored
for two reasons:
1. typo in `Transfer` (so these notifications were not recognised during
processing of the invocation tx in (*Blockchain).storeBlock(...) method)
2. these notifications have `from` adress setted to null, so conversion
to []byte fails. Same thing could happen with `to`.

Related C# issue: neo-project/neo#1646

For now, made both `transfer` and `Transfer` valid.
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this issue May 19, 2020
Native contracts deployment creates `Transfer` notifications and adds
them into interop context. However, these notifications were not stored
for two reasons:
1. typo in `Transfer` (so these notifications were not recognised during
processing of the invocation tx in (*Blockchain).storeBlock(...) method)
2. these notifications have `from` adress setted to null, so conversion
to []byte fails. Same thing could happen with `to`.

Related C# issue: neo-project/neo#1646

For now, made both `transfer` and `Transfer` valid.
AnnaShaleva added a commit to nspcc-dev/neo-go that referenced this issue May 20, 2020
Native contracts deployment creates `Transfer` notifications and adds
them into interop context. However, these notifications were not stored
for two reasons:
1. typo in `Transfer` (so these notifications were not recognised during
processing of the invocation tx in (*Blockchain).storeBlock(...) method)
2. these notifications have `from` adress setted to null, so conversion
to []byte fails. Same thing could happen with `to`.

Related C# issue: neo-project/neo#1646

For now, made both `transfer` and `Transfer` valid.
@roman-khimov
Copy link
Contributor Author

Solved by NEP-17.

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

3 participants