refactor(blocking): replace ureq with minreq, update electrsd version #57
Conversation
notmandatory
force-pushed
the
refactor/minreq
branch
3 times, most recently
from
e099d65 to
c587389
Compare
notmandatory
force-pushed
the
refactor/minreq
branch
from
c587389 to
45c5b9d
Compare
|
Rebased on |
| @@ -18,8 +18,7 @@ path = "src/lib.rs" | |||
| [dependencies] | |||
| serde = { version = "1.0", features = ["derive"] } | |||
| bitcoin = { version = "0.30.0", features = ["serde", "std"], default-features = false } | |||
| # Temporary dependency on internals until the rust-bitcoin devs release the hex-conservative crate. | |||
| bitcoin-internals = { version = "0.1.0", features = ["alloc"] } | |||
| hex-conservative = { version = "0.1"} | |||
There was a problem hiding this comment.
I saw the following on the docs page of this crate:
Use the package key to improve import ergonomics (hex instead of hex-conservative).
hex = { package = "hex-conservative", version = "*" }
Feel free to ignore it if you think it is not important.
There was a problem hiding this comment.
Yes even though still draft, yes certainly appreciate any early feedback. And I did miss that docs suggestion, I'll update it!
| } | ||
| } | ||
|
|
||
| // Ok(deserialize(&Vec::from_hex(&resp.into_string()?)?)?), |
There was a problem hiding this comment.
Don't forget to get rid of this.
| path: &'a str, | ||
| ) -> Result<T, Error> { | ||
| let response = self.get_request(path)?.send(); | ||
| dbg!(&response); |
There was a problem hiding this comment.
I guess this is also to be removed
| message: resp.into_string()?, | ||
| }), | ||
| Err(e) => Err(Error::Ureq(e)), | ||
| let mut request = minreq::post(format!("{}/tx", self.url)).with_body( |
There was a problem hiding this comment.
I wonder if there is not a way for us to abstract the creation of this POST request the way we did it with GET
There was a problem hiding this comment.
Since we only have one post command, I think it's OK to leave this as a one-off function. If in the future a new post function is needed we can see about creating a shared function then.
|
What's the status of this? We're trying to ship binary releases with esplora-client and really don't want to ship a huge pile of HTTP dependencies that we're then taking a responsibility for ensuring we keep up to date. With minreq at least it'd be one crate and we wouldn't have to monitor as many places. |
|
@tnull will open a new PR for this, as we said during the Lib Team Call |
|
Just noticed |
Note the single author in this case is the Anyways, reported the issue, they pushed out |
|
Now superseded by #75. |
Pull Request Test Coverage Report for Build 6385992408Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
6a598b2 ci: remove MSRV pinning for jobserver dependency (Steve Myers) 973c768 chore: remove deprecated get_header() function (Steve Myers) df9c694 refactor(blocking): replace ureq with minreq (Steve Myers) Pull request description: Taking this over from #57. So far just a few minor adjustments on top of #57. Tested that syncing LDK works with the updated blocking client (in addition to the tests here). ACKs for top commit: notmandatory: ACK 6a598b2 Tree-SHA512: 19488ffbf19c59912c379ee77083ee56f7af7fd7b764f8e6c0b96f574d456a86cefdbacb6bf31dc8c1b3317e139a3ed1b54211c8ccfe55e5ad427c512a16b723
Since
ureqstill doesn't have an MSRV this PR refactors the blocking esplora client to useminreq.I also replaced the dependency
bitcoin-internalswith the newhex-conservativecrate.