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 HTTP(S) #532

Merged
merged 3 commits into from
Nov 26, 2020
Merged

Deprecate HTTP(S) #532

merged 3 commits into from
Nov 26, 2020

Conversation

jaspervdm
Copy link
Contributor

Implementation of the Deprecate HTTP(S) Transactions RFC.

@jaspervdm jaspervdm requested review from antiochp and j01tz November 20, 2020 10:19
Copy link
Member

@j01tz j01tz left a comment

Choose a reason for hiding this comment

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

Looks good overall, thanks for taking care of this @jaspervdm.

I ran it on a test instance with Tor not installed and got following output

20201124 09:45:40.939 WARN grin_wallet_controller::controller - Starting TOR Hidden Service for API listener at address rglop53gkfop22lfka5z2juxsv4f6uciwseg7dslsay72otevsrsupyd, binding to 127.0.0.1:13415
20201124 09:45:40.944 WARN grin_wallet_controller::controller - Unable to start TOR listener; Check that TOR executable is installed and on your path
20201124 09:45:40.944 WARN grin_wallet_controller::controller - Tor Error: Tor Process Error: Process("TOR executable (`tor`) not found. Please ensure TOR is installed and on the path: Os { code: 2, kind: NotFound, message: \"No such file or directory\" }")
20201124 09:45:40.944 WARN grin_wallet_controller::controller - Listener will be available via HTTP only
20201124 09:45:40.945 WARN grin_wallet_controller::controller - Starting HTTP Foreign listener API server at 127.0.0.1:13415.
20201124 09:45:40.945 WARN grin_wallet_controller::controller - HTTP Foreign listener started.

I think it is doing what we expect but a couple of usability things that may be worth noting for a future PR:

  • do we want the first log line above to return a tor address or a slatepack address? this could add confusion to return a raw tor address (or am I missing something in how the API is done?)
  • even though http is no longer supported, the warning logs can confuse a user into thinking http is still working

.map_err(|_| invalid())?,
),
},
"keybase" => Box::new(KeybaseChannel::new(dest)?),
Copy link
Member

Choose a reason for hiding this comment

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

With this change, is there any reason to import or keep around any of the keybase logic in adapters?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Afaik we still support Keybase as a method. This particular block of code was just unused so I got rid of it entirely.

@antiochp
Copy link
Member

  • do we want the first log line above to return a tor address or a slatepack address? this could add confusion to return a raw tor address (or am I missing something in how the API is done?)

I have previously seen that tor address in the logs and attempted to use it as an address when testing sending from the wallet.

Copy link
Member

@antiochp antiochp left a comment

Choose a reason for hiding this comment

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

Looks good to me. 👍

@jaspervdm
Copy link
Contributor Author

@j01tz
(i edited your comment by accident instead of replying to it, my bad)

even though http is no longer supported, the warning logs can confuse a user into thinking http is still working

Technically we still have a HTTP listener, it just runs locally. This listener is also used by the TOR hidden service. But I do agree we could improve on the text here in a future PR.

@jaspervdm jaspervdm merged commit 429db61 into mimblewimble:master Nov 26, 2020
quentinlesceller added a commit to quentinlesceller/grin-wallet that referenced this pull request Jan 13, 2021
quentinlesceller added a commit to quentinlesceller/grin-wallet that referenced this pull request Jan 15, 2021
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.

3 participants