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

Remove Jsonrpc #88

Merged
merged 3 commits into from
Nov 14, 2023
Merged

Conversation

Angel-Petrov
Copy link
Contributor

@Angel-Petrov Angel-Petrov commented Nov 9, 2023

Fixes #39

This replaces both jsonrpc-core and jsonrpc-http-server with the newer jsonrpsee-core and jsonrpsee-types crates. The new jsonrpsee crates have been moved into dev-dependencies as jsonrpc was only used for its re-exported futures module in the avalanche-types library (which was able to be replaced).

The majority of this PR has been modifying tests in crates/avalanche-types/tests/rpc/ as that was the primary use of jsonrpc. With some uses of the re-exported futures crate being replaced as well.

This also removes additional tests which are no longer necessary but required the crate.

Copy link
Collaborator

@richardpringle richardpringle left a comment

Choose a reason for hiding this comment

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

Haha 😅 so this was a fun exercise...

Turns out we don't need to migrate to jsonrpsee, but remove the dependency entirely. We had a single test, testing code that was written inside the integration tests directory... so silly.

Anyway, I think my change requests are pretty clear. Let me know if you have any questions and thank you for the contribution!

crates/avalanche-types/src/subnet/rpc/utils/grpc.rs Outdated Show resolved Hide resolved
crates/avalanche-types/tests/rpc/common.rs Outdated Show resolved Hide resolved
crates/avalanche-types/tests/rpc/common.rs Outdated Show resolved Hide resolved
crates/avalanche-types/tests/rpc/common.rs Outdated Show resolved Hide resolved
crates/avalanche-types/Cargo.toml Outdated Show resolved Hide resolved
@Angel-Petrov Angel-Petrov changed the title Jsonrpc to Jsonrpsee Migration Remove Jsonrpc Nov 13, 2023
Copy link
Collaborator

@richardpringle richardpringle left a comment

Choose a reason for hiding this comment

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

Gotta love the number of deletions here. If CI passes, we can merge!

Copy link
Contributor

@gyuho gyuho left a comment

Choose a reason for hiding this comment

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

lgtm, awesome

@richardpringle richardpringle merged commit 87a2b4b into ava-labs:main Nov 14, 2023
6 checks passed
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

Successfully merging this pull request may close these issues.

Use jsonrpsee
3 participants