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

Remove Runtime.Notify #843

Closed
cschuchardt88 opened this issue Dec 20, 2023 · 12 comments · Fixed by #845
Closed

Remove Runtime.Notify #843

cschuchardt88 opened this issue Dec 20, 2023 · 12 comments · Fixed by #845

Comments

@cschuchardt88
Copy link
Member

cschuchardt88 commented Dec 20, 2023

You should just remove Runtime.Notify its useless now. Or have it add the name to contract manifest.

Get error Inner InvalidOperationException: Event 'SayHello3' does not exist.

Contract

public class EventContract : SmartContract
{
    public delegate void OnSayHelloDelegate(string message);

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

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

    public static void Main()
    {
        OnSayHello("Hello, alice");

        SayHello("Hello, bob");

        Runtime.Notify("SayHello3", new[] { "Hello, joe" }); // crash
    }
}
@vncoelho
Copy link
Member

Perhaps that is right to do

@Jim8y
Copy link
Contributor

Jim8y commented Dec 22, 2023

@shargon how do you think? I think this is a bug maybe, i have asked Erik for his opinion, but get no response.
maybe we should vote to make a decision? @vncoelho @roman-khimov

@cschuchardt88
Copy link
Member Author

cschuchardt88 commented Dec 22, 2023

@Jim8y Its not a bug. Remember when a patch came out it allowed contracts to ONLY emit a notify/event if it exists in the contract manifest.

@Jim8y
Copy link
Contributor

Jim8y commented Dec 23, 2023

Screenshot 2023-12-23 at 12 11 45 PM

Now updated the contract analyzer #839 to check the notify name with id NC4019

@cschuchardt88
Copy link
Member Author

cschuchardt88 commented Dec 23, 2023

What if i did or want to do:

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

Also what about the parameters? Do you check though too against manifest?

@Jim8y
Copy link
Contributor

Jim8y commented Dec 23, 2023

can be added.

@shargon
Copy link
Member

shargon commented Dec 23, 2023

I think that is a bug, seems good

@cschuchardt88
Copy link
Member Author

cschuchardt88 commented Dec 23, 2023

@Jim8y Why not have Runtime.Notify event name be added to the manifest if it doesn't exist in the contract? Because you have to compile the contract in order to deploy or update.

@roman-khimov
Copy link

Runtime.Notify event name be added to the manifest if it doesn't exist in the contract?

You can't do it for every possible Runtime.Notify() invocation, that's exactly your example with Runtime.Notify(someVar, ...). If event name is a constant and if you exactly know all types of all parameters, that can be done (like in nspcc-dev/neo-go#3008 with --guess-eventtypes), but not in general case. So requiring event specification of some kind (https://docs.neo.org/docs/en-us/develop/write/basics.html#events is C# probably) is perfectly fine and since this example is about event, it should just have one.

@Jim8y
Copy link
Contributor

Jim8y commented Dec 23, 2023

@shargon @roman-khimov , lets vote whether we ban Notify from contract developer. I think it is uselss now without allowing arbitary notification as user can use event directly.

@roman-khimov
Copy link

You can't ban Notify! How would you make a NEP-17 contract without it?

@Jim8y
Copy link
Contributor

Jim8y commented Dec 23, 2023

then any idea of solving the issue that notify become a trap now?

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 a pull request may close this issue.

5 participants