-
Notifications
You must be signed in to change notification settings - Fork 412
Add cli esplora example #1040
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
Add cli esplora example #1040
Conversation
2bacc1f to
f1b7da3
Compare
f1b7da3 to
7a4231b
Compare
|
Thanks. In general this example could do with a lot more comments explaining what's happening and why. With examples it's better to be on the side of too many comments rather than too little. |
7a4231b to
6a5b769
Compare
6a5b769 to
3dd481f
Compare
|
While testing I found a bug in if last_index > last_active_index.map(|i| i + stop_gap as u32) {
break;
}Easy enough to fix: if last_index > last_active_index.map(|i| i.saturating_add(stop_gap as u32)) {
break;
} |
|
LGTM but i think we are waiting on #1065 before this one. |
3dd481f to
45cbe6b
Compare
|
I just pushed:
@notmandatory I don't understand how to reproduce this. Does it happen with every call to sync? |
LLFourn addressing his comments on bitcoindevkit#1040
LLFourn addressing his comments on bitcoindevkit#1040
|
I've made some requested changes. Rather than go back and forth too much I made a PR to show what I had in mind: evanlinjin#6 (note I accidentally push to this PR branch at some point. Ignore that. I've reset it back to how it was). |
0dc88d5 to
45cbe6b
Compare
LLFourn addressing his comments on bitcoindevkit#1040
45cbe6b to
019fa4c
Compare
|
Changes from my last push (019fa4c):
@evanlinjin I'd say this is ready to merge. Can you review the commits pushed on top, squash them, and ACK? Then I'll ACK as well :) |
LLFourn addressing his comments on bitcoindevkit#1040
ab33b90 to
12c49bf
Compare
|
Rebased after #1048 in order to fix CI |
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.
Thanks for taking this forward @danielabrozzoni. Just a really small nit. Looks good otherwise.
|
I also tested everything. Works fine. |
f8a893d to
3c1e5d4
Compare
Co-authored-by: remix <[email protected]> Co-authored-by: LLFourn <[email protected]> Co-authored-by: Daniela Brozzoni <[email protected]>
3c1e5d4 to
f41cc1c
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.
ACK f41cc1c
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 f41cc1c
I was also not able to reproduce the sync error I saw before.
|
I'm dismissing @LLFourn blocking review so this can be merged since it looks like comments are all addressed. |
2090021 refactor: rename methods in EsploraExt and EsploraExtAsync (Vladimir Fomene) Pull request description: ### Description This PR fixes #1058. Built on top of #1040 ### Checklists #### All Submissions: * [x] I've signed all my commits * [x] I followed the [contribution guidelines](https://github.com/bitcoindevkit/bdk/blob/master/CONTRIBUTING.md) * [x] I ran `cargo fmt` and `cargo clippy` before committing #### New Features: * [ ] I've added tests for the new feature * [ ] I've added docs for the new feature ACKs for top commit: realeinherjar: ACK 2090021 danielabrozzoni: ACK 2090021 - code looks good, `example_esplora` and `wallet_esplora_blocking` work just fine. Tree-SHA512: 5b5285aaa67a0c4e8174e480cceec7d934ec04a74d81740e1c82f6b8673c3e3d9c676dc43257a170320089efe2d3cb0d33123b4a395fc3e7fec63f85bdf70c79
Description
This PR builds on top of #1034 and adds a cli-example for our
esplorachain-src crate.Notes to the reviewers
Don't merge this until #1034 is merged. The only relevant commit is 5ff0412.
Changelog notice
esplora.Checklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features:
* [ ] I've added tests for the new feature