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

JSON-RPC Client with FULLNODE_API_INFO config & JWT support #1100

Merged
merged 22 commits into from
May 28, 2021

Conversation

cryptoquick
Copy link
Contributor

@cryptoquick cryptoquick commented May 13, 2021

Summary of changes
Changes introduced in this pull request:

  • Remove jsonrpsee from forest CLI
  • Add surf for HTTP requests
  • Parse FULLNODE_API_INFO env var to configure HTTP JSON-RPC client
  • Pass Authorization header if JWT is provided

Reference issue to close (if applicable)

Closes #1084 and #998

Other information and links
Ran into a snag, so still WIP:
kardeiz/jsonrpc-v2#17

Once I get the RPC client working, I'll refactor other RPC methods

The complexity here came from jsonrpsee not supporting:

  • Dynamic variables in macro declarations
  • Authorization headers

@cryptoquick
Copy link
Contributor Author

cryptoquick commented May 25, 2021

Sprint carryover update

I got a lot of good work done during last sprint on implementing the majority of what's needed to fulfill this task.

Done

  • RPC request building via jsonrpc_v2 (required collaboration with upstream dev on missing serialization trait, since jsonrpc_v2 isn't traditionally used for clients)
  • Surf HTTP integration
  • Auth header passing (untested)
  • Multiaddress parsing via Regex (will update to an approach based on parity multisig as defined in this issue before I mark this as ready, though: Multiaddr::to_url() libp2p/rust-libp2p#2073 )
  • Calling RPC functions without parameters
  • Fixed serialization issues on Cids
  • Fixed CI by bumping Circle rustc to 1.52.1 due to me, a chad rust dev, using freshly stable std lib methods

Doing

  • Debugging RPC methods with parameters, which are currently broken due to a deserialization issue
  • Multiaddress parsing improvements (replaces regex approach)
  • Test JWT creation and passing
  • Test all forest CLI methods, with both forest and lotus nodes

node/rpc-client/src/client.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@connormullett connormullett left a comment

Choose a reason for hiding this comment

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

Looks great, will approve once this is R4R if no other changes

@cryptoquick cryptoquick marked this pull request as ready for review May 28, 2021 16:45
@cryptoquick cryptoquick requested a review from a user May 28, 2021 16:45
@connormullett connormullett self-requested a review May 28, 2021 16:45
Copy link
Contributor

@connormullett connormullett left a comment

Choose a reason for hiding this comment

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

LGTM

@cryptoquick
Copy link
Contributor Author

This is RFR, but two things:

Errors are not yet parsed correctly, I have an issue upstream for that:
kardeiz/jsonrpc-v2#18

I'll also add in some CLI usage examples in the README while y'all are reviewing

@cryptoquick
Copy link
Contributor Author

I was just writing this, but then I realized, we should use the auth api-info command instead. That's NYI, though.

### CLI Usage Examples

A tutorial for currently implemented `forest` client workflows.

> forest auth create-token -p admin

Take the JWT output and put it into multiaddress format: ugh n/m, let's wait till we have auth api-info

I'm down to merge this without the usage examples changes and the RPC client error parsing if everyone else agrees.

@connormullett
Copy link
Contributor

I was just writing this, but then I realized, we should use the auth api-info command instead. That's NYI, though.

### CLI Usage Examples

A tutorial for currently implemented `forest` client workflows.

> forest auth create-token -p admin

Take the JWT output and put it into multiaddress format: ugh n/m, let's wait till we have auth api-info

I'm down to merge this without the usage examples changes and the RPC client error parsing if everyone else agrees.

I can add a note to create some usage examples with my work if needed. You and I understand the CLI enough to be able to work with it. Documentation and usage examples can come once we get a good number of commands running I feel

@cryptoquick
Copy link
Contributor Author

@connormullett Yeah, makes sense. I'll make a separate issue for that work.

forest/src/cli/chain_cmd.rs Outdated Show resolved Hide resolved
Copy link
Member

@ec2 ec2 left a comment

Choose a reason for hiding this comment

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

LGTM. I think it may potentially be useful for us to have the CLI be able to also use WS instead of HTTP as an option in the future. Just a thought

node/rpc-client/src/client.rs Outdated Show resolved Hide resolved
forest/Cargo.toml Outdated Show resolved Hide resolved
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

I only did a shallow code review since this already has two approvals, LGTM

@cryptoquick
Copy link
Contributor Author

LGTM. I think it may potentially be useful for us to have the CLI be able to also use WS instead of HTTP as an option in the future. Just a thought

I'll make a separate issue for that. It's an entire different transport, so it'd require changes to Multiaddress parsing and some abstraction for a WS client, in addition to surf for HTTP.

@cryptoquick
Copy link
Contributor Author

I only did a shallow code review since this already has two approvals, LGTM

ty :)

@ec2
Copy link
Member

ec2 commented May 28, 2021

LGTM. I think it may potentially be useful for us to have the CLI be able to also use WS instead of HTTP as an option in the future. Just a thought

I'll make a separate issue for that. It's an entire different transport, so it'd require changes to Multiaddress parsing and some abstraction for a WS client, in addition to surf for HTTP.

Yes, technically, but since the HTTP and WS server are on the same port, you wont need to change the multiaddr parsing. But yeah there will be some heavy lifting done to abstract the client to handle http and ws. Just an idea i have :)

…r, but we can solve this in a separate issue.
@cryptoquick cryptoquick changed the title WIP RPC Client with FULLNODE_API_INFO config & JWT support RPC Client with FULLNODE_API_INFO config & JWT support May 28, 2021
@cryptoquick cryptoquick changed the title RPC Client with FULLNODE_API_INFO config & JWT support JSON-RPC Client with FULLNODE_API_INFO config & JWT support May 28, 2021
@cryptoquick cryptoquick merged commit 0a20e46 into main May 28, 2021
@cryptoquick cryptoquick deleted the hunter/1084-api-info-config branch May 28, 2021 21:03
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.

Forest CLI FULLNODE_API_INFO support, and --rpc_port override
3 participants