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

Deprecate native NodeId implementation #869

Closed
2 tasks
njgheorghita opened this issue Aug 26, 2023 · 6 comments · Fixed by #1070
Closed
2 tasks

Deprecate native NodeId implementation #869

njgheorghita opened this issue Aug 26, 2023 · 6 comments · Fixed by #1070
Labels
good-first-issue Good for newcomers

Comments

@njgheorghita
Copy link
Collaborator

Remove the native type implementation, and replace all usage with the equivalent type from the discv5 crate.

  • Make sure that all of our portal_*traceRecursiveFindContent jsonrpc api endpoints correctly ser/de the response
  • Run it against portal-hive
@njgheorghita njgheorghita added the good-first-issue Good for newcomers label Aug 26, 2023
@morph-dev
Copy link
Collaborator

I looked into this issue, and here are my findings.

We can't just simply change to discv5::enr::NodeId. The reason is that deserializing NodeId from serde_json::Value fails.

I created the issue with minimal test to reproduce and a way to fix it in the original project: sigp/enr#62

We would have to wait for enr crate to fix it, push new version, for discv5 crate to upgrade deps and push new version. And than we can upgrade and remove our version.

Alternatively, maybe in our code we can avoid using serde_json::Value (and use String instead), or serialize it to String before deserializing.

@njgheorghita
Copy link
Collaborator Author

@KolbyML IIRC you were the last one tracking the progress of this issue?

@KolbyML
Copy link
Member

KolbyML commented Oct 18, 2023

Waiting for the enr crate wouldn't be a problem. Normally my goto is to just make a PR on sigp/enr with your fix, as that normally resolves the issue faster then making an issue and don't be afraid to ping the maintainers for review. Good investigating 🧐

@morph-dev
Copy link
Collaborator

The PR that updates the enr dependency in discv5 repo is merged.

I just now noticed that we actually don't use their release version. Instead we use this one: https://github.com/njgheorghita/discv5. I don't know why we depend on it and not the official release version (I assume we have some custom changes).

@njgheorghita, what would be the best way to proceed?

@KolbyML
Copy link
Member

KolbyML commented Dec 1, 2023

@morph-dev we are using the branch version right now since Nick's concurrent PR isn't merged yet on sigp/discv5. Once that is done we will switch back to using sigp/discv5 instead of nick/discv5

@njgheorghita
Copy link
Collaborator Author

@morph-dev I believe this is unblocked for you after #1067 gets merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good-first-issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants