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

wants to merge 1 commit into from

Conversation

spartucus
Copy link

This way, bitcoind-json-rpc-client can use jsonrpc with feature minreq_http for https connection.

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

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

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!

@spartucus spartucus closed this Sep 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants