-
Notifications
You must be signed in to change notification settings - Fork 5
Add Blockchain RPC methods #5
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 Blockchain RPC methods #5
Conversation
6543635 to
3a16fa4
Compare
ValuedMammal
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.
So far I agree with the way you've defined the client methods. I mainly focused on reviewing the implementation of the client code, and haven't spent as much time looking at the test code yet.
By convention, the commits should appear as type(module): Summary, for example
3a16fa4 test: Add integration tests
6f95a7f feat(client): Add blockchain methods
Other notes:
- Going forward try to have more descriptive API docs
- We should look into testing the client using
corepc_nodeto minimize the amount of manual testing needed and so we can run the integration tests in CI.
3632601 to
685e258
Compare
c5f1106 to
61599ad
Compare
|
I can work on testing Core Edit: tvpeter#1. |
Alright, that's fine👍. Thank you |
bb3f1b1 to
b0731c1
Compare
|
To consider this ready to merge I would want to check |
cbbedbe to
0778704
Compare
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.
cACK 6a38a71
I did a first round of review in this one, I reviewed commit by commit, so some comments might not apply as they were addressed in other commits. However, I didn't tested it yet.
Overall, I'd like to mention my comment about the verbose options, I don't those are necessary w.r.t to bdk_bitcoind_rpc usage as we don't use those AFAICT.
I think most of the methods could just be a one-line fn call, it's just decoding/deserializing it either by accessing a specific field or calling .into_model(), but that's my personal taste.
About the method's documentation, you could update it to link it to the types (e.g [Header]).
About the tests, do you plan to integrate the usage of corepc_node for it in this PR, or in a follow-up one ?
src/client.rs
Outdated
| /// Get block verboseone | ||
| pub fn get_block_verbose(&self, block_hash: &BlockHash) -> Result<GetBlockVerboseOne, Error> { | ||
| let res: GetBlockVerboseOne = self.call("getblock", &[json!(block_hash), json!(1)])?; | ||
| Ok(res) | ||
| } |
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 want to have support for getblock with the verbose flag? I don't see it being used by bdk_bitcoind_rpc.
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 think this is the same as get_block_info that is used by the Emitter.
src/client.rs
Outdated
| /// Get best block hash | ||
| pub fn get_best_block_hash(&self) -> Result<BlockHash, Error> { | ||
| let res: String = self.call("getbestblockhash", &[])?; | ||
| Ok(res.parse()?) | ||
| } |
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.
Same here, I don't think we need to support it.
src/client.rs
Outdated
| let res: String = self.call("getbestblockhash", &[])?; | ||
| Ok(res.parse()?) | ||
| let best_block_hash: String = self.call("getbestblockhash", &[])?; | ||
| Ok(best_block_hash.parse()?) |
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.
Does the .parse() uses the rust-bitcoin consensus encode/decode in the background ? 🤔
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.
No, it doesn't. Rust-bitcoin implements the FromStr trait for BlockHash making .parse() method available (as it is available on any type that implements the trait). It is preferred for handling endianness representations
yes, the corepc_node will be part of this PR. Those comments that are not already addressed will be addressed in the next push. |
c036a8d to
17266ca
Compare
|
@tvpeter Let me know when you need another round of review here. |
Alright |
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.
Made some comments regarding the error module
- Suggestions for simpler names
- Suggest to remove unused
HexToBytes - Don't implement
Error::source? - Use explicit names when doing
map_err
src/error.rs
Outdated
| Error::DecodeHex(e) => Some(e), | ||
| Error::GetBlockVerboseOneError(e) => Some(e), | ||
| Error::Overflow(e) => Some(e), | ||
| Error::GetBlockFilterError(e) => Some(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.
❓ Is it necessary to implement source here (If not, I'd say remove it). From the docs for std::error::Error:
In error types that wrap an underlying error, the underlying error should be either returned by the outer error’s Error::source(), or rendered by the outer error’s Display implementation, but not both.
|
While reviewing the code in |
Yes, I noticed how clean the TestEnv made the tests across the crates. |
ee2ebf6 to
f076f6f
Compare
Can this come in a different PR? |
ValuedMammal
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 f076f6f; tested on Bitcoin Core versions 28.1 and 29.0.
I have some remaining comments that can probably be done in a followup PR.
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.
📌 Overlooked it, but seems we'll still need an implementation of std::error::Error for Error.
impl std::error::Error for Error {}f076f6f to
fe96e78
Compare
- getblockcount - getblockhash - getblockfilter - getblockheader - getrawmempool - getrawtransaction
- update `corepc-types` to v0.11.0 to support Bitcoind v30
- refactor tests - update the error enum
fe96e78 to
6ff6641
Compare
- update json-rpc to v0.19.0 - switch from minreq_http to bitreq_http
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.
Looks like it's almost there, I'm only looking forward to bitcoindevkit/bdk#2109 being rebased on top of master so I could test it through that PR.
Also, we should use the same standard of other BDK projects and not commit the Cargo.lock file.
| [dependencies] | ||
| corepc-types = { version = "0.11.0", features = ["default"]} | ||
| jsonrpc = { version = "0.18.0", features = ["simple_http", "simple_tcp", "minreq_http", "simple_uds", "proxy"] } | ||
| jsonrpc = { version = "0.19.0", features = ["simple_http", "simple_tcp", "bitreq_http", "simple_uds", "proxy"] } |
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.
question: do we need all these features ? I guess probably only bitreq and proxy ones are sufficient.
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.
Initially, I wanted all features because the client should support all JSON-RPC transport layers. It appears that #27 will eventually remove them.
|
reACK 06526db |
f1db39a ci: Run `test_rpc_client` integration tests (valued mammal) ee52af4 test: Improve unit, integration tests (valued mammal) d77416c error: Unimplement `Error::source` (valued mammal) Pull request description: ### Description This is a follow up to #5 that introduces a `TestEnv` struct used for integration tests and fixes a number of nitpicks that were previously left out. Summary: - Add simple `TestEnv` struct with convenient interface for improved ergonomics - Avoid manually implementing RPC calls (e.g. `generate_to_address`) - Move auth tests to `client::test_auth` module - Ignore (or remove) test using potentially insecure `std::env::tempdir` - Don't call `node.stop()`, as this is handled by the `Drop` implementation of `Node` - Add missing tests for existing RPC methods `get_block_verbose`, `get_block_header_verbose` - Don't print to stdout - Have more effective test assertions. For example it's sufficient to check that the RPC method returns the expected type. Keep some basic "sanity check" assertions, but try to avoid testing functionality that is defined outside of this library (e.g. the length of a `BlockHash`, and Bitcoin Core itself). ### 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 ACKs for top commit: ValuedMammal: self-ACK f1db39a Tree-SHA512: 3634e0e00ed910b979eb998ad13b6a786f827a679495a12237c543a9a8e291eb04602cf0105753fa56dc542cdb57471d87bf630b8afee3bd1564d558cb1b016b
Description
This PR adds the following methods for versions 28, 29, and 30 of bitcoind:
getblockgetbestblockhashgetblock(verbosity = 1)getblockcountgetblockhashgetblockfiltergetblockheadergetblockheader(verbose = true)getrawmempoolgetrawtransactionAlso in this PR
corepc-typesto v0.11.0corepc-nodefor integration testscorepc-types,jsonrpccratesjsonrpcto v0.19.0 and switched fromminreq_httptobitreq_httpFixes #4
Fixes #19
Fixes #20
Checklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features:
CHANGELOG.md