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

Compliance checks for manifested standards #1883

Closed
roman-khimov opened this issue Aug 27, 2020 · 11 comments
Closed

Compliance checks for manifested standards #1883

roman-khimov opened this issue Aug 27, 2020 · 11 comments
Assignees
Labels
discussion Initial issue state - proposed but not yet accepted

Comments

@roman-khimov
Copy link
Contributor

Summary or problem description
We've got SupportedStandards field in contract manifest and at the moment it's just a set of strings with no obligations attached. Anyone can write "NEP-5" or "RFC 1149" or whatever else there. I think it's a nice feature in general, but at the same time we have well-established and well-known standards like NEP-5 and if some contract declares that it implements NEP-5 I think it would be very beneficial for the network to check if it really implements NEP-5 methods/events at least at the manifest level.

Do you have any solution you want to propose?
Check manifested methods and events for compliance with well-known NEO-wide standards. This shouldn't affect "RFC 1149" case (and contract should be able to specify that), but for standards we know well like NEP-5 we can check that all methods and events listed in the standard are also listed in the manifest.

This would then also allow to reliably use this field in other contexts like Nep5Tracker plugin, before trying to parse any events it could first look into SupportedStandards and not bother with events if there is no "NEP-5" specified there. At the same time if there is "NEP-5" there then we can be sure that any "transfer" event is exactly what we're interested in (and it's well-formed if we're to implement #1879 as well).

Neo Version

  • Neo 3

Where in the software does this update applies to?

  • Other: Contract's ABI and standards
@roman-khimov roman-khimov added the discussion Initial issue state - proposed but not yet accepted label Aug 27, 2020
@roman-khimov
Copy link
Contributor Author

There is a draft implementation in Go: nspcc-dev/neo-go#1373.

@shargon shargon self-assigned this Aug 28, 2020
@shargon
Copy link
Member

shargon commented Aug 28, 2020

I will implement it

@shargon
Copy link
Member

shargon commented Sep 1, 2020

I am not very sure if we should do it, maybe the projects create his own standards and they can use it as they want... what do you think @neo-project/core @neo-project/ngd-shanghai @neo-project/ngd-seattle ?

@roman-khimov
Copy link
Contributor Author

the projects create his own standards and they can use it as they want

That's the "RFC 1149" case, it absolutely should be possible to specify any external standard that contract supports. But there are Neo standards and these are special, allowing contract to specify "NEP-5" when it clearly is not compatible harms the network. I think stricter runtime gives us more predictability that then can be leveraged in various ways. And it adds some value to the data manifest provides, lacking compliance checks for well-established standards I can't trust manifest, so I won't even look there which renders this data useless.

@Tommo-L
Copy link
Contributor

Tommo-L commented Oct 28, 2020

I don't think it's a good idea. NEP5 is for contact, not for neo-core.
If we apply NEPx checks in neo-core and want to amend some NEPs, what will happen? And the more NEPx created, we also need to modify neo-core by hard-fork.

@roman-khimov
Copy link
Contributor Author

want to amend some NEPs, what will happen?

You don't want to amend existing NEPs, you just create a new one if needed, changing old standards is bad.

And the more NEPx created, we also need to modify neo-core by hard-fork.

I don't see any problem here. Unknown "NEP-X" specifications should specifically be forbidden and you "unlock" them when new standard is accepted. It should be tied to versioned execution environment, like "version X which is the default since height Y allows to specify NEP-Z in manifests with the following constraints".

Think of it the other way around --- if this data is not checked (and thus can't be trusted), what's use of it? Why should I specify it as a contract developer? Why should I be looking into it if I'm a contract user?

@Qiao-Jin
Copy link
Contributor

I don't see any mere need for this check. reasons:

(1) This check doesn't prevent any possible harm (tell me why such contracts will be harmful) so only is a waste of time (It will prolong Persist time).

(2) Corresponding transactions will still be onchain.

(3) Still if a contract manifest doesn't contain SupportedStandards at all then this check will be meaningless.

@Tommo-L
Copy link
Contributor

Tommo-L commented Oct 30, 2020

It's better to do it off-chain.

@roman-khimov
Copy link
Contributor Author

We can't really do it off-chain. I mean, of course technically it could be done, but this check means nothing for the ledger. It's like doing some form-checking with Javascript and not bothering with it on backend, even though it seems to be working for regular people the end result can be quite surprising.

And as I've said above, "stricter runtime gives us more predictability that then can be leveraged in various ways", we didn't have to wait long for #2012 with an example of how exactly this could be leveraged. Proven compliance allows you to distinguish NEP-5 from NEP-11 from NEP-33 from NEP-42 and then plugins or other logic can rely on this.

Take RpcNep5Tracker as an example, in theory it should only track NEP-5 contracts. But at the moment it tracks anything that looks like NEP-5 transfer. Now suppose that NEP-11 (or NEP-42) is to have some transfer event with the following parameters: byte[] to, byte[] from, BigInteger amount. RpcNep5Tracker as it is now would interpret such event in wrong way, just because it doesn't really know which standard the contract is compliant with.

@lock9
Copy link
Contributor

lock9 commented Dec 7, 2020

I strongly support this idea. If you say you implement an interface, you have to implement it correctly, using identical method signatures. #2125

@roman-khimov
Copy link
Contributor Author

Just as a side note, nspcc-dev/neo-go#1373 was reworked to be a compile-time check and neo-go currently checks for NEP-17 compliance if you declare it in supported standards. But this of course doesn't make manifests from the ledger trustworthy which was the point of this issue.

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

No branches or pull requests

6 participants