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

Draft NEP 25: Extended types for NEP-14 #160

Merged
merged 19 commits into from
Sep 30, 2024

Conversation

roman-khimov
Copy link
Contributor

@roman-khimov roman-khimov commented Dec 9, 2022

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)

@ixje
Copy link
Contributor

ixje commented Dec 9, 2022

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

🥇

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.

Copy link
Contributor

@devhawk devhawk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am very much in favor of this proposal, but it needs to be combined with the changes proposed in #151. In particular, I think this proposal needs a syntax-independent description of the extended type semantics. Something similar to the ContractType Type Model section of #151.

Note, I don't care if the eventual proposal uses the string encoding from #151 (Array<Hash160>) or JSON encoding from this proposal ({"type": "Array", "value": {"type": "Hash160"}}). Whatever the group at large decides to support for this proposal, debug info v2 will adopt.

@roman-khimov
Copy link
Contributor Author

Something similar to the ContractType Type Model section of #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?

@devhawk
Copy link
Contributor

devhawk commented Dec 13, 2022

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
Copy link
Contributor

devhawk commented Dec 13, 2022

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.

@roman-khimov
Copy link
Contributor Author

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
Copy link
Contributor Author

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.

@devhawk
Copy link
Contributor

devhawk commented Dec 14, 2022

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
Copy link
Contributor

devhawk commented Jan 18, 2023

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

@roman-khimov
Copy link
Contributor Author

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.

@devhawk
Copy link
Contributor

devhawk commented Jan 19, 2023

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.

@igormcoelho
Copy link

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)

@roman-khimov
Copy link
Contributor Author

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

@devhawk
Copy link
Contributor

devhawk commented Feb 6, 2023

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

@melanke
Copy link

melanke commented Mar 16, 2023

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 :)

@devhawk
Copy link
Contributor

devhawk commented Apr 4, 2023

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

@ixje
Copy link
Contributor

ixje commented May 11, 2023

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 ixje mentioned this pull request May 11, 2023
@roman-khimov
Copy link
Contributor Author

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
Copy link
Contributor Author

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?

nep-Y.mediawiki Outdated Show resolved Hide resolved
@roman-khimov roman-khimov dismissed stale reviews from Jim8y and shargon via a6d9fbc July 18, 2024 14:40
@roman-khimov
Copy link
Contributor Author

I took some time to realize that this object is impossible... as how could an Integer have an extended type? Could a better example be given here please?

As written there:

(the object below is not a valid ExtendedType specification, rather it represents all possible fields).

Any example that has all fields would be incorrect in some ways, so I doubt it can be improved.

I put it on a JSON checker and beautifier, and it is broken.

Fixed!

My beautifier did not put the fields in a particular order, but it would be easier to understand if the fields are ordered...

Well, JSON is order-agnostic for object elements, so we can have it in any way, but to make it easier to read I've reordered some things.

@cschuchardt88
Copy link
Member

name - is the name of the method, which can be any valid identifier.

Can we address this as well as others in this NEP. The above is from NEP-3 and never was addressed in NEP-14 that is any valid identifier? See neo-project/neo#2930 for more details. Now properties need to have restrictions of what they SHOULD BE or MUST BE. We need to be more strict in NEPs. They need to be written like RFC Standards, leave no wiggle room for error or guessing. Otherwise it just a guideline.

@roman-khimov
Copy link
Contributor Author

Likely what was originally meant there is https://learn.microsoft.com/en-us/dotnet/csharp/fundamentals/coding-style/identifier-names. But I don't know if there are any contracts now that go beyond this definition of identifier (which would raise compatibility question). We can fix it (to valid UTF-8 letters?), but only after mainnet/testnet check.

safe definition can also be fixed, it's very vague at the moment.

shargon
shargon previously approved these changes Jul 22, 2024
AnnaShaleva
AnnaShaleva previously approved these changes Aug 5, 2024
Copy link
Member

@cschuchardt88 cschuchardt88 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All the json should be json syntax highlighted, no sense in using legacy markdown commands.

```json
Text here
```

Signed-off-by: Roman Khimov <[email protected]>
Signed-off-by: Roman Khimov <[email protected]>
Signed-off-by: Roman Khimov <[email protected]>
Signed-off-by: Roman Khimov <[email protected]>
@Jim8y
Copy link
Contributor

Jim8y commented Sep 30, 2024

@shargon please merge if others still not review, this proposal has being extensively discussed and reviewed and updated and apporoved.

@shargon shargon merged commit 0c4395a into neo-project:master Sep 30, 2024
@roman-khimov roman-khimov deleted the extended-types branch September 30, 2024 13:47
Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good it moved forward

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.