[Cli] Retrieve a waypoint value from a given url #2446
Conversation
|
This PR made the following dependency changes: Added Packages (Duplicate versions in '()'):
autocfg 1.0.0 (0.1.6)
curl 0.4.25
curl-sys 0.4.25
openssl-sys 0.9.54
|
|
|
||
| // If waypoint is given explicitly, use its value, otherwise try to retrieve it from a given | ||
| // URL. | ||
| let waypoint = args.waypoint.or_else(|| { |
There was a problem hiding this comment.
so this can still be None right? probably worth a comment
There was a problem hiding this comment.
If I specify both, should we check for the latest?
client/cli/src/main.rs
Outdated
| /// Retrieve a waypoint given the URL. | ||
| fn retrieve_waypoint(url_str: &str) -> anyhow::Result<Waypoint> { | ||
| let mut data = Vec::new(); // waypoint bytes downloaded from the given URL | ||
| let mut handle = Easy::new(); |
There was a problem hiding this comment.
@davidiw did you evaluate this client library? 😆
There was a problem hiding this comment.
I would recommend we just use ureq throughout our codebase unless there's a need for an async web client.
There was a problem hiding this comment.
There was a problem hiding this comment.
ureq doesn't support system-wide proxy , and some client needs to run on tupperware and devserver with fwdproxy configured.
There was a problem hiding this comment.
I think we could work with them to add that feature. HTTP Proxy is pretty trivial: https://en.wikipedia.org/wiki/Proxy_server#Implementations_of_proxies
There was a problem hiding this comment.
The actual proxy protocol support is relatively easy. But handle all corner cases where http_proxy https_proxy and no_proxy are defined are not. Which is also why most implementation doesn’t work out of the box . I am not saying it is impossible to do, but it is well beyond the scope of this change.
After all, reqwest has already been added to CLI dependency tree to solve this exact issue, so there are no new additional deps to be add.
There was a problem hiding this comment.
reqwest pulls in so many dependencies into the client code base. I would prefer we challenge ourselves a little bit and ask the community for help rather than bloating the dependency tree. If, when closer to launch, we this hasn't been resolved, I am happy to support changing to a different library that supports our needs.
client/cli/Cargo.toml
Outdated
| @@ -14,6 +14,7 @@ anyhow = "1.0" | |||
| chrono = "0.4.7" | |||
| tonic = "0.1" | |||
| hex = "0.3.2" | |||
| curl = "0.4.25" | |||
There was a problem hiding this comment.
can we use https://docs.rs/reqwest/0.10.1/reqwest/ instead? Mint code is already using it (due to its support on system proxies), it is best if we don't introduce new libraries.
client/cli/Cargo.toml
Outdated
| @@ -14,6 +14,7 @@ anyhow = "1.0" | |||
| chrono = "0.4.7" | |||
| tonic = "0.1" | |||
| hex = "0.3.2" | |||
| ureq = { version = "0.11.3"} | |||
There was a problem hiding this comment.
maybe we should order these alphabetically?
client/cli/src/main.rs
Outdated
| use std::num::NonZeroU16; | ||
| use std::str::FromStr; | ||
| use std::time::{Duration, UNIX_EPOCH}; |
There was a problem hiding this comment.
merge these into std::{...
?
|
|
||
| // If waypoint is given explicitly, use its value, otherwise try to retrieve it from a given | ||
| // URL. | ||
| let waypoint = args.waypoint.or_else(|| { |
There was a problem hiding this comment.
If I specify both, should we check for the latest?
client/cli/src/main.rs
Outdated
| _ => Err(anyhow::format_err!( | ||
| "URL {} returned {}", | ||
| url_str, | ||
| response.status() |
Summary: Added a waypoint-url cmd line argument to download the waypoint value from. These are still optional but we might consider making it a non-optional value with a default pointing to https://developers.libra.org/testnet_waypoint.txt. Testing: ``` ./scripts/cli/start_cli_testnet.sh -- --waypoint-url https://developers.libra.org/testnet_waypoint.txt ./scripts/cli/start_cli_testnet.sh ./scripts/cli/start_cli_testnet.sh -- --waypoint 0:997acd1b112a19eb1d2d3dff78677a0009343727926071c3858aeff2ea3499bf ``` Issues: ref #2425
|
Thanks a lot for the constructive comments! :) |
|
@bors-libra r+ |
|
📌 Commit 09b19e5 has been approved by |
|
☀️ Test successful - checks-circle_commit_workflow |
|
Who is following up on the issue of proxy support in ureq? Why was my concern dismissed without merit? @dmitri-perelman @davidiw |
Summary:
Added a waypoint-url cmd line argument to download the waypoint value from.
These are still optional but we might consider making it a non-optional value with a default pointing to https://developers.libra.org/testnet_waypoint.txt.
Testing:
Issues: ref #2425