Use reqwest, remove http, ureq and hyper #1632
Conversation
|
CI failed, possibly due to #1631 |
| .bytes_stream() | ||
| .map(|result| result.map_err(|e| std::io::Error::new(std::io::ErrorKind::Other, e))); | ||
| let stream_reader = StreamReader::new(stream); | ||
| let buf_reader = BufReader::new(stream_reader); |
There was a problem hiding this comment.
Why the introduction of the buf reader before the GzipDecoder? Where to place the buffers is somewhat sensitive in this area code to performance. I saw massive differences depending on where they were placed. I'll test it out on my side too before merging to see what the performance is like.
There was a problem hiding this comment.
It doesn't seem necessary. I saw this kind of writing in libsql, and I'll do some benchmarking later to see the performance differences between them.
There was a problem hiding this comment.
From the source code perspective, using it should theoretically bring performance improvements. However, when using snapshot create, we don't notice any performance differences between the two implementations.
|
|
||
| #[tokio::test] | ||
| async fn test_helper_url_test_network() { | ||
| // It might be a bit difficult to conduct client mock here. | ||
| let friendbot_url = "https://friendbot.stellar.org/secret_key"; | ||
| let addr = "GBZXN7PIRZGNMHGA7MUUUF4GWPY5AYPV6LY4UV2GL6VJGIQRXFDNMADI"; | ||
| let url = Url::from_str(&format!("{friendbot_url}?addr={addr}")).unwrap(); | ||
| assert_eq!(url.as_str(), "https://friendbot.stellar.org/secret_key?addr=GBZXN7PIRZGNMHGA7MUUUF4GWPY5AYPV6LY4UV2GL6VJGIQRXFDNMADI"); | ||
| } |
There was a problem hiding this comment.
This test doesn't appear to actually test any logic in the application.
| #[tokio::test] | |
| async fn test_helper_url_test_network() { | |
| // It might be a bit difficult to conduct client mock here. | |
| let friendbot_url = "https://friendbot.stellar.org/secret_key"; | |
| let addr = "GBZXN7PIRZGNMHGA7MUUUF4GWPY5AYPV6LY4UV2GL6VJGIQRXFDNMADI"; | |
| let url = Url::from_str(&format!("{friendbot_url}?addr={addr}")).unwrap(); | |
| assert_eq!(url.as_str(), "https://friendbot.stellar.org/secret_key?addr=GBZXN7PIRZGNMHGA7MUUUF4GWPY5AYPV6LY4UV2GL6VJGIQRXFDNMADI"); | |
| } |
There was a problem hiding this comment.
Because the mock client here is a bit complex, I only verified that let url = Url::from_str(&format!("{friendbot_url}?addr={addr}")).unwrap(); works as expected. This line of code is copied from the codebase. If you would like me to remove it, please let me know.
There was a problem hiding this comment.
Added mock testing.
cmd/soroban-cli/Cargo.toml
Outdated
| "json", | ||
| "blocking", | ||
| "stream", | ||
| "native-tls-vendored", |
There was a problem hiding this comment.
We should check which tls the https://github.com/stellar/rs-stellar-rpc-client uses, so that both ideally use the same. I thought it used rustls, but I'm not sure.
There was a problem hiding this comment.
stellar-rpc-client v21.4.0
├── jsonrpsee-http-client v0.20.3
│ ├── hyper-rustls v0.24.2
...
I think it uses hyper-rustls. Do you want to use hyper-rustls on all platforms? Reqwest defaults to using the system's TLS library. See https://github.com/seanmonstar/reqwest?tab=readme-ov-file#requirements
There was a problem hiding this comment.
We switched to rustls-tls. e128ae9
overcat
changed the title
| let _mock = server | ||
| .mock("POST", "/") | ||
| .with_body_from_request(|req| { | ||
| let body: Value = serde_json::from_slice(req.body().unwrap()).unwrap(); |
There was a problem hiding this comment.
I noticed that other place used httpmock, but it doesn't meet our needs; it can't retrieve the id from the request. So, I add mockito
What
We only use reqwest to make HTTP requests in our code, no longer directly using http, ureq, and hyper, to enhance consistency.
Why
fix #1629
Known limitations
N/A