-
Notifications
You must be signed in to change notification settings - Fork 68
feat(client): add new submit_package api to blocking/async client
#114
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
feat(client): add new submit_package api to blocking/async client
#114
Conversation
stevenroose
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks good, let just some nits. can confirm that the calls match the endpoint added in my pr on esplora (Blockstream/electrs#119)
6a22216 to
01fb9c9
Compare
Pull Request Test Coverage Report for Build 20722592690Details
💛 - Coveralls |
01fb9c9 to
a5322c6
Compare
| pub struct SubmitPackageResult { | ||
| /// The transaction package result message. "success" indicates all transactions were accepted | ||
| /// into or are already in the mempool. | ||
| pub package_msg: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we know if the variants here are finite? Do we see a chance to parse this into an enum?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, that'd be best.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i agree that would be best, but as I can see here, there is no enum defined for that field. I'd be happy to update that part as soon as it is upgraded to one there
oleonardolima
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It could use a test with mempool/electrs implementation, while it's not available in blockstream/electrs and electrsd, as I've looked on the other PR and looks like it's following the same API.
| pub struct SubmitPackageResult { | ||
| /// The transaction package result message. "success" indicates all transactions were accepted | ||
| /// into or are already in the mempool. | ||
| pub package_msg: String, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, that'd be best.
I'm okay with adding a test, but then we need to bump electrsd to 0.30 which introduce some breaking changes on bitcoind. Might be good to first bump it in a separated PR |
a5322c6 to
6147e74
Compare
Yes, agree that it can be done in another PR. Another way that I thought was adding a test (under #[ignore]) that uses live mempool.space to test it, so we could run locally and in another step on CI while we don't have a new release on electrsd with the blockstream's esplora changes. However I think it's better to just wait until Blockstream/electrs#119 is release and just bump everything. |
|
@oleonardolima it looks like the pipeline failures are caused by something unrelated with the PR, could you help please? :) |
It's caused due to recent |
c50be44 to
7886297
Compare
|
I think we uncovered a bug in here: I suspect it's in this field: #[serde(with = "amount::serde::as_btc")]
pub base: Amount,the above should work |
cb707fb to
c0ed77e
Compare
c0ed77e to
1f1fa23
Compare
|
@acidbunny21 Could you rebase and sign the commits on this one? |
1f1fa23 to
4719808
Compare
tnull
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This needs a cargo fmt it seems
7ab7dc3 to
7992b34
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
tACK 7992b34
I've updated the example_wallet_esplora_blocking to build a zero-fee txA (v3), assert that it failed to relay due to expected: min relay fee not met, and did a CPFP with txB (v3) for the output and successfully submitted it as a package through the new API.
I've tested it for:
-
mempool.space API: I used testnet4 for syncing and submitting the package, see txA, and txB.
-
blockstream.info API: I used testnet3 (as testnet4 is not supported), though I had to use the mempool.space for syncing (due to blockstream rate limit issues), and only used blockstream.info API to try to broadcast the zero-fee and the submit package API, see txA and txB.
src/async.rs
Outdated
| pub async fn broadcast(&self, transaction: &Transaction) -> Result<Txid, Error> { | ||
| let body = serialize::<Transaction>(transaction).to_lower_hex_string(); | ||
| let response = self.post_request_bytes("/tx", body, None).await?; | ||
| let txid = Txid::from_str(&response.text().await?).map_err(|_| Error::InvalidResponse)?; | ||
| Ok(txid) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed that landing this PR has a breaking change, due to this change here and in the blocking client.
@tnull Do you need the submit_package API as a non-breaking release?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tnull Do you need the submit_package API as a non-breaking release?
No, making this a minor release would totally be fine for us!
7992b34 to
d9f3868
Compare
|
I've squashed all changes into a single patch as they touched the same files, now d9f3868 |
d9f3868 to
970ea94
Compare
|
ACK 970ea94 |
970ea94 to
f7fbaa4
Compare
luisschwab
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ACK f7fbaa4
477cf8a fix(ci): pin `proc-macro2` to MSRV supported `1.0.103` (Leonardo Lima) f857d40 fix(ci): pin `ryu` to MSRV supported `1.0.20` (Leonardo Lima) 8422454 fix(ci): pin `serde_json` to MSRV supported `1.0.145` (Leonardo Lima) f31ba50 fix(ci): pin `itoa` to MSRV supported `1.0.15` (Leonardo Lima) edac624 fix(ci): bump pinned `webpki-roots` to 1.0.5 (Leonardo Lima) Pull request description: I was about to merge #114, but there's been a new release (i.e [v1.0.5](https://crates.io/crates/webpki-roots/versions)) for `webpki-roots` which breaks our MSRV CI. This PR updates its pinned version to fix our MSRV CI jobs, so we can move forward with #114. ACKs for top commit: luisschwab: ACK 477cf8a ValuedMammal: ACK 477cf8a Tree-SHA512: c93f83c84072865cf8d14ecca98efdadfa99dc7143d9917fa4d00cbeb6fdeff329ca22d4814498be2ed9d15ab6f40b6f6f441a2c7a43d91d618a6acb875dfbc0
- adds new `submit_package` API to `AsyncClient` and `BlockingClient`, it has been added to esplora by `Blockstream/electrs#159`. - adds new `SubmitPackageResult`, `TxResult` and `MempoolFeesSubmitPackage` API structures. - changes the `post_request_hex` method to `post_request_bytes`, now handling `query_params` and having `Response` as return type. - updates the internals of `broadcast` to use new `post_request` and `post_request_bytes`, without breaking it's API.
f7fbaa4 to
cf64f97
Compare
luisschwab
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
re-ACK cf64f97
|
As @ValuedMammal reviews it, I'll merge this one. I rebased it on top of master, so it has the latest CI fixes in cf64f97. I'll add some error handling improvements I noticed in a follow-up. |
|
ACK cf64f97; used |
submit_package api to blocking/async client
Description
Add new
submit_packagemethod to bothBlockingClientandAsyncClient, as it's now supported by Esplora API, see: Blockstream/electrs#119, Blockstream/electrs#159. Also, adds newSubmitPackageResult,TxResult, andMempoolFeesSubmitPackageAPI structures.It updates the internal
post_request_hexmethod topost_request_bytes, now handlingquery_paramsand havingResponseas return type. Additionally, updates the internals ofbroadcastto utilize the newpost_requestandpost_request_byteswithout breaking its API.Notes to the reviewers
As mentioned in the PR comments, I updated the original commit by
@acidbunny21by:@ValuedMammal's suggested fixes;broadcastAPI, it's now a follow-up in feat(client)!: update broadcast API #151.Changelog notice
Checklists
All Submissions:
just pbefore pushingNew Features: