neo-project / neo

NEO Smart Economy
MIT License
3.47k stars 1.03k forks source link

Compliance checks for manifested standards #1883

Closed roman-khimov closed 3 years ago

roman-khimov commented 4 years ago

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

Where in the software does this update applies to?

roman-khimov commented 4 years ago

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

shargon commented 4 years ago

I will implement it

shargon commented 4 years ago

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 commented 4 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

It's better to do it off-chain.

roman-khimov commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.