proposals icon indicating copy to clipboard operation
proposals copied to clipboard

Draft NEP 25: Extended types for NEP-14

Open roman-khimov opened this issue 3 years ago • 30 comments

This is a proposal for a new ABI standard replacing NEP-14. It solves the problem of opaque Array, Map and InteropInterface types allowing to provide more specific details about them. A somewhat similar thing is also provided in #151, but it's detached from the ABI and introduces some new notations that should be parsed by clients. This NEP is based on NeoGo ExtendedType data that is used for contract-specific RPC SDK generation, but that implementation is also detached from the ABI (an additional YAML file is used).

While this problem can be solved with additional (non-ABI) data, it might add some additional barriers affecting contract interoperability (starting with the basics, where to store this data). The intention here was to create a 100% backwards-compatible extension (if new fields are ignored) that can be easily provided by compilers and can be easily consumed by any other tools, irrespective of the implementation language. Having this data in ABI means every contract will have it and it'll be easily accessible. In fact this data is a part of the ABI in its essence, that's what contracts accept/return.

Things intentionally omitted for now (I'd like to get more feedback on this before doing any of those):

  • JSON schemas would be more appropriate for specifications
  • "length" parameter for integers (8/16/32/64/128), byte arrays and strings (1-1M) can be useful for more precision if it's known exactly
  • "unsigned" for integers
  • maybe even "min" and "max" for integers
  • "nullable" to resolve "Null" handling ambiguity, can it be provided in a parameter, can it be returned from the method?
  • the problem of Any parameter in the specification (like data for transfer in NEP-17 or NEP-11) with the contract using some specific type (if it uses data, it expects some particular type of data)

roman-khimov avatar Dec 09 '22 11:12 roman-khimov

  • "nullable" to resolve "Null" handling ambiguity, can it be provided in a parameter, can it be returned from the method?

:1st_place_medal:

JSON schemas would be more appropriate for specifications

Yeah I think these are a must. I had to read the Extended Types section a couple of times because there are quite a few of "this key not allow if, but is allowed when, unless" split over paragraphs (e.g. for Array). I think a schema would help a lot with parsing that.

ixje avatar Dec 09 '22 11:12 ixje

Something similar to the ContractType Type Model section of https://github.com/neo-project/proposals/pull/151.

I only see ContractType extensions there like Struct or Address, but these can't be backwards compatible with NEP-14 and I'd like to keep that given that the additional type data here allows to differentiate between structures and arrays. Address is Hash160 to me, any Hash160 can be an address, it's just a matter of representation. Iterator is also included here as one (and only, technically the only other we have is storage context, but of course this can be changed in future) of InteropInterface subtypes. Named structured types are also possible here. Am I missing something?

roman-khimov avatar Dec 13 '22 20:12 roman-khimov

A few nitpicks:

  • I think we should use the term struct instead of namedtype
  • Why is value a JSON object with a type property? Seems needlessly complex. Can we change this to a string value?

devhawk avatar Dec 13 '22 20:12 devhawk

I only see ContractType extensions there like Struct or Address, but these can't be backwards compatible with NEP-14 and I'd like to keep that given that the additional type data here allows to differentiate between structures and arrays. Address is Hash160 to me, any Hash160 can be an address, it's just a matter of representation. Iterator is also included here as one (and only, technically the only other we have is storage context, but of course this can be changed in future) of InteropInterface subtypes. Named structured types are also possible here. Am I missing something?

It is true that the way #151 encodes Struct types would be a NEP-14 breaking change. But the semantics of #151 structs is not a breaking change. The semantics of encoding a struct param as {type: package.Structure} vs {type: Array, namedtype: package.Structure} are equivalent. The only difference is back compatibility. Arguably, the biggest issue with #151 is the breaking change to how type info is encoded. Further, I agree that the syntax approach defined in this PR preferable to #151 since it adds the additional info without breaking back compat. This has huge benefits for all language projects.

I asked for this PR to include a syntax-independent description of the extended type system to ensure that the extended type system works for both contract client code generation and better debugger experience. Once we nail down the semantics of the extended type system, we can choose a syntax that is backwards compatible.

Your point about Iterator is spot on. #151 defines semantics for Interop ContractType, but didn't originally include any specific semantics for Iterators. Given their importance, this is an oversight in #151 that I recently addressed but haven't implemented yet. Having a syntax-independent description of the type system allows us to have discussions like "What information do we need about Interop parameters for code gen + debugger purposes?" without getting bogged down on how this stuff is encoded.

BTW, Address is just an alias for Hash160 so it wouldn't break NEP-14 back compat. As you point out, "any Hash160 can be an address, it's just a matter of representation". Hash160 representation may not affect contract client code gen but it definitely affects debugger representation. The same way this proposal adds extended fields to more fully describe Array and Map, it could also add extended field to better describe Hash160 in a back compatible way such as {type: Hash160, address: true} or something like that.

devhawk avatar Dec 13 '22 20:12 devhawk

I think we should use the term struct instead of namedtype

Technically, we can have non-struct things in namedtypes, maybe they could be used for something in future. But we have only structures there now, that's true, can be renamed.

Why is value a JSON object with a type property? Seems needlessly complex. Can we change this to a string value?

A string will need to be parsed, while we can have the same JSON structures representing all of this directly (maybe some depth limit is necessary though, but I've tested things like map[int][]map[string][]interop.Hash160 (map of int to array of maps of string to array of hash160) with NeoGo ExtendedType). To me unified representation is easier to work with. #151 is more compact here, I agree, but it requires additional handling.

roman-khimov avatar Dec 13 '22 20:12 roman-khimov

Once we nail down the semantics of the extended type system, we can choose a syntax that is backwards compatible.

True, the main thing of course is to settle on the exact details we want/need to express.

{type: Hash160, address: true} or something like that.

Do we have any Hash160 that is not an address? But it can be added if this helps in any way.

roman-khimov avatar Dec 13 '22 21:12 roman-khimov

To me unified representation is easier to work with. #151 is more compact here, I agree, but it requires additional handling.

When we first built the debugger, I was concerned about debug info size. Hence we chose a compact CSV-style string representation instead of separate JSON fields. But that concern has proven to be somewhat overblown.

True, the main thing of course is to settle on the exact details we want/need to express.

If you add the semantic type system details to this document, then I will be able to leverage that for the advanced debug information. As I said, I'd be happy to update the debug info proposal to support JSON serialization for type information in order to bring it more inline with how the manifest is serialized. That should make producing debug info easier for language teams.

Do we have any Hash160 that is not an address? But it can be added if this helps in any way.

Contract Hashes are Hash160s values but are typically not treated as an address.

devhawk avatar Dec 14 '22 15:12 devhawk

@roman-khimov Any progress on including the type system info from #151 into this NEP?

devhawk avatar Jan 18 '23 22:01 devhawk

I'm waiting for more feedback on "things intentionally omitted for now" and other general topics, "Address" (as a representation hint for Hash160) can be added as well if it's OK for us to have representation details at this level (it's OK for me, but maybe there are some objections).

BTW, as I've said earlier, we have JSON-RPC SDK generator that uses similar data to create (Go) client-side code that will then work via regular Neo JSON-RPC. It works fine for backend code, but we also have something for frontend in the queue. It's a different experimental thing that automatically provides server-side contract-specific RESTful APIs using similar ABI data, this then makes it trivial for JS clients to access contract methods.

roman-khimov avatar Jan 19 '23 06:01 roman-khimov

I'm waiting for more feedback on "things intentionally omitted for now" and other general topics, "Address" (as a representation hint for Hash160) can be added as well if it's OK for us to have representation details at this level (it's OK for me, but maybe there are some objections).

Looking forward to the next revision after you get more feedback.

BTW, as I've said earlier, we have JSON-RPC SDK generator that uses similar data to create (Go) client-side code that will then work via regular Neo JSON-RPC. It works fine for backend code, but we also have something for frontend in the queue. It's a different experimental thing that automatically provides server-side contract-specific RESTful APIs using similar ABI data, this then makes it trivial for JS clients to access contract methods.

There's a similar tool in Neo.BuildTools that generates a C# interface from the current manifest file. THis interface is currently only used in C# test methods, but could also be used for generating client APIs as well. I've had similar thoughts to generating these APIs from the extended type info in the prototype v2 debug info. However, I would MUCH rather this extended type info in the manifest.

devhawk avatar Jan 19 '23 18:01 devhawk

Interesting proposal to have all types checked. Regarding syntax, do you think some shorter "template-based" version like:

Original:

         "returntype" : "InteropInterface",
         ...
         "extendedreturntype" : {
            "value" : {
               "type" : "Array",
               "namedtype" : "package.Structure"
            },
            "type" : "InteropInterface",
            "interface" : "IIterator"
         },

Possibility with template-based syntax embedded directly on the string:

         "returntype" : "InteropInterface",
         ...
         "extendedreturntype" : "InteropInterface<IIterator<Array<package.Structure>>>" 

Is this the intention, right? It's smaller, but do you think it makes it much harder to parse? (some extra code will be needed for angle brackets)

igormcoelho avatar Feb 01 '23 14:02 igormcoelho

It's more like InteropInterface<IIterator<package.Structure>> (in that we have an iterator and each value it returns is a package.Structure). But I'd keep it as is because:

  • parsing complicates things and goes a bit against the spirit of NEP-14 to me
  • we have some symmetry between extendedreturntype and Parameter type data
  • any NEP-14 type definition is also a valid extended type definition, a property that is nice to have

roman-khimov avatar Feb 01 '23 14:02 roman-khimov

Neo Debug Info v2 uses a single string like @igormcoelho asked about. But I agree with @roman-khimov that a mechanism that is easier to parse is probably easier to get adopted across the community

devhawk avatar Feb 06 '23 21:02 devhawk

This is a very interesting proposal, in fact, before knowing about this proposal, @meevee98, @luc10921 and I were working on something similar to this. We implemented a few "hints" generated by neo3-boa (PR link) that can be used by neo3-parser (PR link).

For instance, if a method is implemented like this:

@public
def Main(var: Dict[str, List[bool]]) -> List[Union[Dict[str, int], str, bool]]:
    if var:
        return []
    return []

Neo3-boa will compile the manifest to something like this:

{
    "name": "ManifestTypeHintMapsArraysUnionHint",
    "groups": [],
    "abi": {
        "methods": [
            {
                "name": "Main",
                "offset": 0,
                "parameters": [
                    {
                        "type": "Map",
                        "generickey": {
                            "type": "String"
                        },
                        "genericitem": {
                            "type": "Array",
                            "generic": {
                                "type": "Boolean"
                            }
                        },
                        "name": "var"
                    }
                ],
                "safe": false,
                "returntype": "Array",
                "returngeneric": {
                    "type": "Any",
                    "union": [
                        {
                            "type": "String"
                        },
                        {
                            "type": "Map",
                            "generickey": {
                                "type": "String"
                            },
                            "genericitem": {
                                "type": "Integer"
                            }
                        },
                        {
                            "type": "Boolean"
                        }
                    ]
                }
            }
        ],
        "events": []
    },
    "permissions": [
        {
            "contract": "*",
            "methods": "*"
        }
    ],
    "trusts": [],
    "features": {},
    "supportedstandards": [],
    "extra": null
}

There are only 5 new keys to indicate the extended types: hint, generic, genericKey, genericItem and union.

The possible "hints" are: Address, PublicKey, Scripthash, ScripthashLittleEnding, Blockhash, TransactionId, StorageContext and Iterator

Today this is saved on the manifest but when deploying to the blockchain this is lost. We have to manually copy this content from the manifest to neo3-parser as a ParseConfig.

So I am very excited by this proposal, even if it's decided a very different format than what we implemented. It would makes developer's life way easier :)

melanke avatar Mar 16 '23 19:03 melanke

I'm back from vacation. Can we start making progress on this?

devhawk avatar Apr 04 '23 15:04 devhawk

Apparenty neo-debuggers storage improvements are on hold until this NEP is approved. So let's try to move forward shall we?

I'm waiting for more feedback on "things intentionally omitted for now" and other general topics, "Address" (as a representation hint for Hash160) can be added as well if it's OK for us to have representation details at this level (it's OK for me, but maybe there are some objections).

I've seen no "new" feedback or objections since February (arguably March), I'd like to think that it has had enough time to provide input on and we should now accept it, unless @roman-khimov want's to add the "things intentionally ommitted for now" in this specific NEP in a new revision?

ixje avatar May 11 '23 11:05 ixje

As for "things intentionally ommitted for now", I don't see any specific opinions on that which to me means that there is no immediate need for any of them. They can be added in a future NEP update, so it shouldn't be a blocker.

The only relevant unsolved feedback here is the address representation from @devhawk. I don't see any objection to it, so I'll update the proposal to include this data as well and consider it to be done for now.

roman-khimov avatar May 11 '23 12:05 roman-khimov

I'll update the proposal to include this data as well

Which is easy for a regular type ({type: Hash160, address: true}), but doesn't look so well for a map key ({type: Map, key: Hash160, value: {type: Integer}, keyaddress: true}?). Adding another ParameterType is what this proposal tries to avoid.

@devhawk, do we still need it? How do you differentiate between the two when generating manifest?

roman-khimov avatar May 11 '23 14:05 roman-khimov

Need votes for going on or not @roman-khimov @shargon @Jim8y @AnnaShaleva @cschuchardt88 @vncoelho

superboyiii avatar Mar 11 '24 07:03 superboyiii

I will soon review and vote.

vncoelho avatar Mar 11 '24 11:03 vncoelho

Backwards compatibility. Valid NEP-14 manifest remains valid NEP-25 manifest which simplifies things a lot to me, you don't have to treat these manifest types separately.

roman-khimov avatar Apr 08 '24 07:04 roman-khimov

BTW, the other way around is also true. Valid NEP-25 is a valid NEP-14 if you're to ignore new fields. So all the tooling we have for NEP-14 won't break immediately.

roman-khimov avatar Apr 08 '24 07:04 roman-khimov

That'd be

"length" parameter for integers (8/16/32/64/128), byte arrays and strings (1-1M) can be useful for more precision if it's known exactly

from https://github.com/neo-project/proposals/pull/160#issue-1486551106. In general, I like it because it allows to generate more specific code from the manifest, but it complicates things at the same time (especially given the fact that VM doesn't distinguish them). Vote below, :+1: :-1:.

roman-khimov avatar Apr 08 '24 10:04 roman-khimov

but it complicates things at the same time (especially given the fact that VM doesn't distinguish them)

Vote up if we make this length parameter optional (or maybe create one more NEP standard with such additional types metadata that will extend the current NEP). Shall we? I don't have anything against this length parameter, it may be helpful for some manifest-level checks and is definitely useful for contract bindings, but I'm a little bit worried about the manifest generation part, it may be tricky to generate proper manifest with this additional info enabled.

AnnaShaleva avatar Apr 08 '24 16:04 AnnaShaleva

I really wanted to avoid additional nesting and extendedtype fields, so it's embedded (except for returntype!). There are some constraints on how different fields can be used depending on the base type and having it at the same level simplifies these checks, but then if we have it both in parameter and extendedtype it's duplication, a bit less effective (and needs to be checked as well).

But then again, we inevitably have this problem already with returntype, so your proposal adds some symmetry here. Let's see if there are any opinions on this, it can be changed if it's eaiser to implement in the end.

roman-khimov avatar May 23 '24 20:05 roman-khimov

I don't have any special preferences on this topic, Fernando's option is a bit easier to implement and follow, Roman's one is more compact. Maybe if we have this problem with return type, then it's better to have a symmetric structure for simple parameters.

AnnaShaleva avatar May 23 '24 20:05 AnnaShaleva

@Jim8y, we can't approve it yet, we firstly need to decide on https://github.com/neo-project/proposals/pull/160#pullrequestreview-2074421878.

@neo-project/core, need your opinions on this topic.

AnnaShaleva avatar Jun 01 '24 16:06 AnnaShaleva

@AnnaShaleva, I will update it and address @shargon's feedback. If it's easier to implement and we don't have any strong arguments against it (other than duplication, but you know we already have some of it) then this is the way to go.

roman-khimov avatar Jun 01 '24 16:06 roman-khimov

@Jim8y, we can't approve it yet, we firstly need to decide on #160 (review).

@AnnaShaleva This pr was marked as accepted a very long time ago, its supposed to only vote on it. But since @roman-khimov decides to update, so be it.

Jim8y avatar Jun 02 '24 01:06 Jim8y

@shargon, take a look at the latest commit, especially at the examples. I think it sucks for namedtypes, much more complex structure, it'd be harder to use. extendedreturntype on the other hand is not used often, most of the time functions return simple bool/hash things, more rarely iterators.

roman-khimov avatar Jun 06 '24 12:06 roman-khimov