Skip to content
This repository was archived by the owner on Nov 26, 2024. It is now read-only.

Add https support for minreq_http of jsonrpc #30

Closed
wants to merge 1 commit into from
Closed

Add https support for minreq_http of jsonrpc #30

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion jsonrpc/Cargo.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ default = [ "simple_http", "simple_tcp" ]
# A bare-minimum HTTP transport.
simple_http = [ "base64" ]
# A transport that uses `minreq` as the HTTP client.
minreq_http = [ "base64", "minreq" ]
minreq_http = [ "base64", "minreq", "minreq/https" ]
Copy link
Member

Choose a reason for hiding this comment

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

This change somewhat invalidates the feature name. Also I would have expected "start supporting HTTPS" to require more changes than this - did you test this works?

Copy link
Author

@spartucus spartucus Sep 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Also I would have expected "start supporting HTTPS" to require more changes than this - did you test this works?

Yes, for the minreq backend, enable https feature would work for https. I tested it, it works.

This change somewhat invalidates the feature name.

I don't quite understand, you mean "minreq_http" has "https" feature make it inappropriate?

Copy link
Member

Choose a reason for hiding this comment

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

HTTP is not HTTPS so having a feature called minreq_http that does HTTPS stuff is wrong IMO. I didn't look deeply into it but a bunch of other things are also done with the assumption that we do not support HTTPS (eg, module names, docs etc).

Copy link
Member

Choose a reason for hiding this comment

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

To clarify, I'm not opposed to the idea, and appreciate you raising a PR, but if we are going to do it we should do it properly.

Copy link
Author

@spartucus spartucus Sep 6, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Cool, my PR can be regarded as a simple starting point. If people who use this library encounter the error JsonRpc(Transport(Minreq(HttpsFeatureNotEnabled))) when connecting to https in bitcoind service, they can temporarily use my method to solve it.

Copy link
Member

Choose a reason for hiding this comment

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

Damn, I wish you put that error message and the explanation of why you raised the PR in the original message, the conversation would have been totally different. Anyways, thanks for bringing it up and thanks for using the lib!

# Basic transport over a raw TcpListener
simple_tcp = []
# Basic transport over a raw UnixStream
Expand Down