NO MERGE: Review suggestions for 505 - #664
Closed
tcharding wants to merge 14 commits into
Closed
Conversation
There is a lint error: this expression creates a reference which is immediately dereferenced by the compiler. Fix it as suggested by the compiler.
In preparation for adding bitreq_http_async feature to jsonrpc move the sync version to the client-sync feature so it is not always on with jsonrpc.
These will both be used by the async client. Move them out of sync client in preparation. Edit the logging function so it doesn't panic.
Create a new folder for the upcoming async client and copy in the existing client_sync code. Code copy only to make the next patch easier to review.
Edit the copy of the sync client created in the previous commit to be async. Update the readme and cargo.toml files. Replace macros with functions. There is only one async client so the macros are not needed anymore. Create a new module for the bdk client that has the required RPCs in it that return either the rust-bitcoin type or non-version specific model types.
Explicit trumps implicit every day of the week. Remove the alias and make the error type explicit at each call site.
This trait adds no value, remove it.
The `model::` path adds useful info, it makes it easy at a glance to understand what is going on.
Import `types` and remove one layer of path.
This returns a `u64`, it should never have had an `into_model` impl. ref: rust-bitcoin#663
tcharding
requested review from
TheBlueMatt,
jamillambert,
oleonardolima and
tnull
as code owners
July 15, 2026 04:09
Collaborator
|
Cool, thanks. All of it looks good so far, but will go through more thoroughly and then add the improvements. |
As a demo of what I think the error logic should be hack up a new version of `get_block_header_verbose`. Note, for later consideration: The `Error` type is too general (it includes variants not returned by this function).
Member
Author
|
Had an insight in bed last night, check out the patch I just pushed. It allows getting rid of all the |
Member
Author
|
FTR the current error code around |
Member
Author
|
This is not needed anymore. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Hey @jamillambert I hacked these patches up while reviewing #505. Feel free to grab the ones you like. You can either use
git cherry-pickor do the changes manually yourself. I don't need attribution.