-
Notifications
You must be signed in to change notification settings - Fork 1
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
refactor: replace homegrown Bitcoin RPC for bitcoincore-rpc-async
crate
#239
refactor: replace homegrown Bitcoin RPC for bitcoincore-rpc-async
crate
#239
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
762766c
to
91259db
Compare
bitcoincore-rpc-async
bitcoincore-rpc-async
crate
91259db
to
e215e09
Compare
// DEBUG: What the hell is this? | ||
// FIXME: remove me | ||
#[cfg(debug_assertions)] | ||
dbg!(&e); |
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.
What are you refering to here? The Err(e) block or the cfg(debug_assertions) thing?
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 will be removed once I figure out the
Figure out how to catch the specific error -27/-25 in btcio/src/writer/broadcast.rs inside the broadcaster_task function.
in the TODO list of the PR description.
It will not be in the finished ready to review state.
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 to me.
One thing is that I am not sure if we should split the rpc interfaces to what a reader, signer and publisher might be using. For example, reader does not need to publish/sign transactions, while a signer does not need to publish transactions whereas publisher does not need to sign transactions. All are inside one trait in this PR, but I guess that's fine too. What do you think?
Yes, I agree that this might be interesting in the future. I just don't think that the added complexity is worth right now. But I might be wrong, there might be something that I am not seeing here... |
Along with a bunch of docstrings improvements
Changes to a thin tuple struct wrapper for `bitcoincore-rpc-async::Client`.
e215e09
to
4331440
Compare
a1f1908
to
3664d97
Compare
3664d97
to
d3c653a
Compare
Currently the
I am struggling to debug since I cannot force |
The rebase is too painful and effortful. Closing in favor of #245. |
Description
Replacing our homegrown solution with an off-the-shelf crate,
bitcoincore_rpc_async
(https://docs.rs/bitcoincore-rpc-async).Commits are atomic in a sense that they don't fail on CI and can be read/reviewed in a interdependent basis.
Also tried to improve all the docstrings of the functions that I had to touch.
Type of Change
Checklist
Related Issues
EXP-189
Closes #81.
TODO
-27
/-25
inbtcio/src/writer/broadcast.rs
inside thebroadcaster_task
function.