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

Better edge cases and exceptions handling for NNS contract #36

Open
AnnaShaleva opened this issue Jan 23, 2024 · 2 comments
Open

Better edge cases and exceptions handling for NNS contract #36

AnnaShaleva opened this issue Jan 23, 2024 · 2 comments

Comments

@AnnaShaleva
Copy link
Member

Problem

NNS contract aborts VM execution in case if getAllRecords is requested for unexisting name:
https://github.com/neo-project/non-native-contracts/blob/8d72b92e5e5705d763232bcc24784ced0fb8fc87/src/NameService/NameService.cs#L378C42-L378C60

Tested on test contract invocations. The engine's FAULT exception itself looks like this if C# node handles invokefunction RPC call:

One of the identified items was in an invalid format.

and if Go node handles invokefunstion RPC call, then exception looks like the following:

at instruction 57 (SYSCALL): invalid conversion: Null/ByteString

The reason is in fact that if nameMap[tokenKey] doesn't contain provided tokenKey then Null stackitem will be returned from this call. And then this Null is being passed to (NameState)StdLib.Deserialize which has an offset 57 (according to native StdLib manifest):

               {
                  "offset" : 56,
                  "name" : "deserialize",
                  "returntype" : "Any",
                  "parameters" : [
                     {
                        "type" : "ByteArray",
                        "name" : "data"
                     }
                  ],
                  "safe" : true
               },

As a result, StdLib tries to convert provided Null to ByteString and fails.

Suggestion

The problem itself is in the fact that NNS doesn't properly handle this error and returns it to the user "as is". From this error it's not clear what happens on the contract side and why getAllRecords FAULTs execution. I'd suggest to return some meaningful exceptions from NNS in similar cases.

Also, for this particular case I'd suggest to return empty iterator since there are no suitable records. Not FAULT the execution.

@roman-khimov
Copy link

I'd say that semantically it's pretty close to https://github.com/neo-project/proposals/blob/master/nep-11.mediawiki#ownerof-1 (accepts token ID, returns an iterator related to it) for which we have

The parameter tokenId SHOULD be a valid NFT ID (64 or less bytes long). If not, this method SHOULD throw an exception.

specified. So exception itself is likely OK here, at least it follows NEP-11 conventions. But this exception should be meaningful of course, "invalid format" or "invalid conversion" are hard to decipher.

@AnnaShaleva
Copy link
Member Author

Just for the record: https://github.com/neo-project/non-native-contracts/pull/25/files#diff-d92adeab89009d989ac782ad3056dc68aee0e88547e4f101f95764030d2f0410R660 contains fix for this particular issue along with a set of other improvements.

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

No branches or pull requests

2 participants