Fix: ban notify from contract developer

[Jim8y]

Closes #843

[Jim8y]

@shargon @cschuchardt88 @roman-khimov @vncoelho @superboyiii please check

[roman-khimov]

You can't write any meaningful contract with this PR merged. I'm not sure what's the problem really, just have a proper manifest (how you create it is a separate matter) for your contract and that's it.

[Jim8y]

You can't write any meaningful contract with this PR merged. I'm not sure what's the problem really, just have a proper manifest (how you create it is a separate matter) for your contract and that's it.

What problem are you specifying? user can still call Notify via event, existing contract will still work. The only thing that I banned is directly calling Notify without defining an event...... cause it will fail the execution for sure anyway.

Well, this actually only affects the C# compiler,,,, the core remains the same.

[shargon]

You can't write any meaningful contract with this PR merged. I'm not sure what's the problem really, just have a proper manifest (how you create it is a separate matter) for your contract and that's it.

What problem are you specifying? user can still call Notify via event, existing contract will still work. The only thing that I banned is directly calling Notify without defining an event...... cause it will fail the execution for sure anyway.

Well, this actually only affects the C# compiler,,,, the core remains the same.

But is not possible to call a dynamic event

[Jim8y]

But is not possible to call a dynamic event

Current core only accept events that are defined as event, what you mean dynamic event, i have never heard of this terminology in neo? can you please provide an example?

[roman-khimov]

@Jim8y, maybe I just don't know enough about C# devpack, if event wrapper allows to emit notifications without direct Runtime.Notify() calls in the code, then it's not that bad. Still somewhat limiting to me. I can have a pair of create/delete events with the same parameters and emitting them with Runtime.Notify(nameVariable, params) might be convenient.

[Jim8y]

I can have a pair of create/delete events with the same parameters and emitting them with Runtime.Notify(nameVariable, params) might be convenient.

But current Notify checking logic has banned from using arbitary nameVariable. you must defined them as

    public delegate void OnSayHelloDelegate(string message);

    [DisplayName("SayHello1")]
    public static event OnSayHelloDelegate OnSayHello;

    [DisplayName("SayHello2")]
    public static event Action<string> SayHello;

but if you already defined them this way, using Notify would be unecessary, yet keeping it makes it misunderstanding as it can take arbitary name without syntax error while actually arbitary name will fail the execution.

[shargon]

But is not possible to call a dynamic event

Current core only accept events that are defined as event, what you mean dynamic event, i have never heard of this terminology in neo? can you please provide an example?

I mean that maybe you want to emit a event with a dynamic name, who knows..

I think we should not remove notify because if you want to call it by syscall you will be able to do it, so in reality you are not banning it, you are making it difficult

[cschuchardt88]

@shargon
Makes sense, but it's very misleading when it compiles when it doesn't check contract manifest or parameters for validation. Putting any name in the input of Runtime.Notify without the name in manifest will error at execution of the invoke of the contract.

[shargon]

@shargon Makes sense, but it's very misleading when it compiles when it doesn't check contract manifest or parameters for validation. Putting any name in the input of Runtime.Notify without the name in manifest will error at execution of the invoke of the contract.

But currently the core allow to call notify with a event that is not present in the manifest....

[cschuchardt88]

And old contracts will still work fine. You'll just have to do it with opcode at low level. But at high level it should be removed. Since we don't support events when they don't exist in manifest.

[shargon]

Since we don't support events when they don't exist in manifest.

We do

[cschuchardt88]

Do you remember last update or so we removed this functionally. Because of NEP-14

Inner InvalidOperationException: Event 'SayHello3' does not exist.

[cschuchardt88]

@shargon see neo-project/neo#2810

[Jim8y]

I mean that maybe you want to emit a event with a dynamic name, who knows..

I think we should not remove notify because if you want to call it by syscall you will be able to do it, so in reality you are not banning it, you are making it difficult

It is not an issue of allowing user to call it or not, if i did not get you wrong, your so called dynamic notify is already banned by Erik in the core, just check the code. Yes, you can make it easier to use Notify, but that will not work at all, it will just fail the execution by all means.

Dynamic Notify/Event is BANNED BY THE CORE, which is proposed by roman, and implemented by erik.

[Jim8y]

Since we don't support events when they don't exist in manifest.

We do

@shargon We dont, new logic since the hardfork. Just check the issue and code please https://github.com/neo-project/neo/blob/c78ac5adbd790b1fc60d3ea9abc87cf3eee74a6e/src/Neo/SmartContract/ApplicationEngine.Runtime.cs#L352

[shargon]

Since we don't support events when they don't exist in manifest.

We do

@shargon We dont, new logic since the hardfork. Just check the issue and code please https://github.com/neo-project/neo/blob/c78ac5adbd790b1fc60d3ea9abc87cf3eee74a6e/src/Neo/SmartContract/ApplicationEngine.Runtime.cs#L352

Yes, you are right...

[roman-khimov]

Dynamic Notify/Event is BANNED BY THE CORE,

Depends on your definition of "dynamic". If it's "I want to emit anything regardless of manifest" then yes. How is it related to Runtime.Notify though? It's just an interop call, it doesn't care. Banning an interop from devpack solves nothing to me. In most cases it shouldn't be used, but I think I still can write some code with it that will work (make event definitions for manifest, but emit via Runtime.Notify).

[cschuchardt88]

@roman-khimov
Yes you are correct. But It isn't intended to be used in this new way. It was for dynamic events only (all the logic for it in nccs). Also alot of contract developers use Runtime.Notify, and I think its do to not knowing how to program in C# or the outdated document on neo.org or even it was easier to emit events then adding an extra 3-4 lines.

Edit:
Runtime.Notify has the same name in nccs C# version as in core. So don't confuse the two as the same. We aren't talking syscall name. This is static class we are talking about in nccs.

[Jim8y]

@roman-khimov truth is even shargon and i did not know this, are you expecting developers to use it correctly? how would you feel to use an interface that accepts arbitrary input without emiting any errors during compile but actually will fail the execution without saying why? you love that? when you think it is better for neo, have you considered the developers? a group of people have no clue how to develop in c# and can not find assistence from anywhere?

[cschuchardt88]

can not find assistence from anywhere?

Maybe should we should add xml code comments to nccs. I don't why there isn't any to begin with.

[Jim8y]

can not find assistence from anywhere?

Maybe should we should add xml code comments to nccs. I don't why there isn't any to begin with.

Very general question is, how would developer know they can not do this if they don't already know they can not do this?

[roman-khimov]

@cschuchardt88, @Jim8y, I see it in a different way. Under the hood it's all Runtime.Notify anyway. There is no way to escape calling it in the resulting bytecode. It's just a lower layer of abstraction than event magic provided by devpack (that does both Runtime.Notify and manifest adjustment).

Let's try to approach it from a different angle, how can I make a Runtime.Notify invocation in my code without this wrapper and without a proper event?

[Jim8y]

@cschuchardt88, @Jim8y, I see it in a different way. Under the hood it's all Runtime.Notify anyway. There is no way to escape calling it in the resulting bytecode. It's just a lower layer of abstraction than event magic provided by devpack (that does both Runtime.Notify and manifest adjustment).

Let's try to approach it from a different angle, how can I make a Runtime.Notify invocation in my code without this wrapper and without a proper event?

Have some mercy to the neo contract developer, dont make it even harder to write neo contract. make every interface clear and easy to use please. Yes, indeed people can still use it via bytecode (only experienced can do that, i guess will only be few of us), but not via the devpack, devpack should compile contract that works, instead of compile contract without showing error but not work. If there is another Hackthon for the neo core, who is there to answer questions how to use Notify, and who is there to help them debug? Are we going to tell them, you can use this, but xxxx, you can use that, but xxxx.....

Either ban it directly such that contract developers can not use it at all, or register event to the manifest with whatever names developer pass in. Dont just leave it there like this.

[cschuchardt88]

register event to the manifest with whatever names developer pass in

This won't work at runtime, If the following is done:

public static void HelloWorld(string eventName, string name)
{
    Runtime.Notify(eventName, new[] { $"Hello, {name}" });
}

Which leaves us at the same problem to begin with.

[Jim8y]

register event to the manifest with whatever names developer pass in

This won't work at runtime, If the following is done:

public static void HelloWorld(string eventName, string name)
{
    Runtime.Notify(eventName, new[] { $"Hello, {name}" });
}

Which leaves us at the same problem to begin with.

Have you tried it in that pr? This syntax will not pass the compile as it is not a constant string. When I say whatever the name, the name must be known at compile time.

[cschuchardt88]

Have you tried it in that pr? This syntax will not pass the compile as it is not a constant string. When I say whatever the name, the name must be known at compile time.

Which PR? but i think below will give you const

Runtime.Notify($"{eventName}", new[] { $"Hello, {name}" });

[Jim8y]

#846 This one

[vncoelho]

I am fine with banning it, in fact its directly use has been a burden after events.
Banning it can be like this proposal, just remove from the devpack.