-
Notifications
You must be signed in to change notification settings - Fork 135
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
feat(state): implement jsonrpc for the state network #1175
Conversation
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.
⛵
find_content(network, enr, content_key).await | ||
} | ||
StateEndpoint::RecursiveFindContent(content_key) => { | ||
recursive_find_content(network, content_key, /* is_trace= */ false).await |
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.
Nice, didn't know we could inline comments this way 💡
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.
As I've mentioned in a previous pr, I don't love it. But, I guess in this case especially I see the usefulness. And creating a let is_trace=false;
variable does seem overkill. So, maybe I'll have to change my mind about this
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.
They might look weird at first, but they grow on you.
They shouldn't be overused, but they have their use cases.
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.
These feel like the kind of changes that should be accompanied with tests in ethportal-peertest
. Even if the tests aren't really testing anything other than the endpoint is supported
find_content(network, enr, content_key).await | ||
} | ||
StateEndpoint::RecursiveFindContent(content_key) => { | ||
recursive_find_content(network, content_key, /* is_trace= */ false).await |
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.
As I've mentioned in a previous pr, I don't love it. But, I guess in this case especially I see the usefulness. And creating a let is_trace=false;
variable does seem overkill. So, maybe I'll have to change my mind about this
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.
Oh, ok, just saw your comment in the pr description about the local testing. Fine to move ahead with this, though as soon as some basic state storage is supported, we should remember to also add tests for these endpoints
527bac2
to
85af987
Compare
85af987
to
2ea192a
Compare
What was wrong?
A lot of json_rpc methods weren't implemented for the state network.
How was it fixed?
Implemented missing methods.
Most of them were implemented in a similar/same way as the history network.
NOTE: Currently, none of these methods would work because storage for the state network is not implemented. However, I do have some version of the storage implemented in the local branch and methods that I tested work fine.
To-Do