-
Notifications
You must be signed in to change notification settings - Fork 85
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
Add ContentNotFoundError
#343
Conversation
}, | ||
"errors":[{ | ||
"$ref": "#/components/errors/ContentNotFoundErrorWithTrace" | ||
}] | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems like we're also going to want to add this error to simple RecursiveFindContent
requests? I'm split on whether we want to include the trace w/ the error for these endpoints... probably not i guess... and just use the simple ContentNotFoundError
type
jsonrpc/src/errors/errors.json
Outdated
@@ -0,0 +1,13 @@ | |||
{ | |||
"ContentNotFoundError": { | |||
"code": -32099, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are using 39001
for this in the code (see PR https://github.com/ethereum/portal-network-specs/pull/276/files)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have updated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still seeing the wrong error code here? Maybe the change didn't get merged? And if we're going with 39001
for ContentNotFoundError
then we should go with something like 39002
for ContentNotFoundErrorWithTrace
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ugh, sorry, the commit didn't grab the changes in this file. Should be correct now.
jsonrpc/src/errors/errors.json
Outdated
@@ -0,0 +1,13 @@ | |||
{ | |||
"ContentNotFoundError": { | |||
"code": -32099, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still seeing the wrong error code here? Maybe the change didn't get merged? And if we're going with 39001
for ContentNotFoundError
then we should go with something like 39002
for ContentNotFoundErrorWithTrace
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found one small issue everything else looks good
In the process of researching Ultralight test failures for Hive, I discovered that the JSON RPC spec currently advises that the
portal_*localContent
methods return0x
when no content is found.However, this did not solve a hive test failure for
portal_beaconLocalContent
for Ultralight when I updated our code to match this spec. After further research, I discovered thattrin
andfluffy
return an error and not0x
when content is not found. This PR updates the spec to reflect the current implemented state of the clients.Specifically:
portal_*localContent
responses to reflect that the clients return an errorContentNotFoundError
a specifies that the error code returned should be -32001 for theContentNotFoundError
and the message "content not found" following the current Trin implementation.ContentNotFoundErrorWithTrace
error type that is returned by thetraceRecursiveFindContent
method types