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

Check notifications for manifest compliance #1879

Closed
roman-khimov opened this issue Aug 26, 2020 · 3 comments · Fixed by #2810
Closed

Check notifications for manifest compliance #1879

roman-khimov opened this issue Aug 26, 2020 · 3 comments · Fixed by #2810
Labels
Discussion Initial issue state - proposed but not yet accepted

Comments

@roman-khimov
Copy link
Contributor

Summary or problem description
We have events definition as a part of NEP-14 ABI included in the manifest that is deployed along with the contract. It's an important part of contract's interface to the outside world and contract's users may expect that all events emitted would comply with this definition.

But we never check emitted notifications for ABI compliance. One can use an empty list of events in the ABI and then successfully emit whatever he wants to. Or specify some event and then not comply with parameters types specified there. Both can be confusing to any receivers and users of these events.

At the same time lack of any control for this part of ABI provokes developers to not pay any attention to it or even not use it at all which is bad for the network (because it makes manifests less useful).

Do you have any solution you want to propose?
Add a check to RuntimeNotify() that will reject any notifications not complying with contract's ABI. Failing the execution with some throw new ArgumentException() should be perfectly fine for this.

Neo Version

  • Neo 3

Where in the software does this update applies to?

  • Other: contract's ABI and syscalls
@roman-khimov roman-khimov added the Discussion Initial issue state - proposed but not yet accepted label Aug 26, 2020
@ixje
Copy link
Contributor

ixje commented Sep 9, 2020

Totally agree with this and (and #1891, #1883 for that matter). No point in having manifest descriptions that are not used to validate the real data.

@erikzhang
Copy link
Member

The events emitted by the contract that not included in the abi could be the private events.

@roman-khimov
Copy link
Contributor Author

Contracts emit them quite publicly, they can't emit an event that is only visible to the contract itself (or a limited group of other contracts).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Discussion Initial issue state - proposed but not yet accepted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants