Create an async corepc-client#505
Conversation
d5524e1 to
9a77aa2
Compare
6981d8f to
6cd8bb2
Compare
|
@apoelstra whats your take man? Would you be willing to have |
6cd8bb2 to
87dfe15
Compare
|
Holla at me if/when you would like some review mate. |
1c174a6 to
7a79b87
Compare
|
Rewrote the async client:
@tcharding thoughts? in particular the error handling of converting to the modelled type. Returning a modelled type vs the inner field e.g. |
After writing a massive post I realised this solution is not going to work (see bottom for explanation). I think we should inline the How about throwing this at the top: //! Async JSON-RPC client designed explicitly to support BDK.
//!
//! ## Project decisions
//!
//! * Support Core versions 25 to 30.
Troublesome, commented already on the code.
Another solution is to have an error type for each function. Then it can have a variant (assuming its an enum) for the exact error returned by
Agreed that a wrapper type is not useful to returen (eg
Nice.
I don't think separation by project module is not going to work because the modules will conflict with each other. And if we feature gate the features will not be additive. We could try some macro stuff but I think it defeats the purpose which is to write a simple clean client that others can fork if they need to. |
7a79b87 to
2f2ca44
Compare
Would this then tie all users of the crate to tokio? Why can't it be executor-agnostic? |
fd21983 to
8448aab
Compare
The changes to |
I was planning on doing the rename afterwards since it also requires changing the current testing client. |
|
Sweet. Whats left to undraft this then? |
20a03b0 to
9fb1295
Compare
I got it to work locally on BDK. It did require breaking changes due to being async, but I think this is the right choice. This testing resulted in the last patch to make the use in BDK easier, and probably other downstream too. Undrafted. |
9fb1295 to
9a64ce6
Compare
| RpcMethodErrorExt, ServerVersionError, UnexpectedServerVersionError, | ||
| }; | ||
| pub use crate::client_async::rpcs::RpcApi; | ||
| pub(crate) use crate::{into_json, log_response}; |
There was a problem hiding this comment.
This import is unusual, why the pub(crate)?
There was a problem hiding this comment.
Done as part of taking all your suggested changes from #664
|
|
||
| #![cfg(feature = "v30_and_below")] | ||
| #![cfg(not(feature = "v24_and_below"))] | ||
| #![allow(non_snake_case)] // Test names intentionally use double underscore. |
There was a problem hiding this comment.
I think we would be better off without this.
There was a problem hiding this comment.
I changed all the test names and removed the allow.
| #![cfg(feature = "v30_and_below")] | ||
| #![cfg(not(feature = "v24_and_below"))] |
There was a problem hiding this comment.
Perhaps add a comment stating why we only test versions 25-30
There was a problem hiding this comment.
Comment added and bumped to v31
| async fn async__get_best_block_hash__modelled() { | ||
| let node = BitcoinD::with_wallet(Wallet::None, &[]); | ||
| let client = async_client_for(&node); | ||
|
|
||
| let model: Result<bitcoin::BlockHash, GetBestBlockHashError> = | ||
| client.get_best_block_hash().await; | ||
| let model = model.unwrap(); | ||
| let expected = node.client.best_block_hash().expect("best_block_hash"); | ||
| assert_eq!(model, expected); | ||
| } |
There was a problem hiding this comment.
What if we do this for the first test and for subsequent tests the same but with the comments?
async fn get_best_block_hash() {
let node = BitcoinD::with_wallet(Wallet::None, &[]);
let client = async_client_for(&node);
// Tests that the async-client has this function.
let got = client.get_best_block_hash().await.unwrap();
// Grabs the block hash using the sync client which we know works.
let want = node.client.best_block_hash().expect("best_block_hash");
// Tests tat the async client returned the same block hash as the async client.
assert_eq!(got, want);
}The reason is that we are expecting BDK devs to look at this test file. We are just testing the functionality and how usage of the client might look. I'm undecided about the explicit error stuff, leaning towards not testing it here and just making sure we get it right. There are not that many errors anyways. Open to your thoughts though?
There was a problem hiding this comment.
I changed the tests to your example and assumed it was a typo and you meant without the comments for later tests?
|
Everything else looks good. The error stuff is pretty funky but I don't have a better solution right now. We can change it later if we want to. |
|
Hey mate, I had a bit of a play because I wanted to get rid of the |
9a64ce6 to
a51f47b
Compare
| /// remaining helpers have default implementations derived from it. | ||
| /// | ||
| /// [`as_client_error`]: RpcMethodErrorExt::as_client_error | ||
| pub trait RpcMethodErrorExt { |
There was a problem hiding this comment.
Bro the extension patter is specifically for bypassing the orphan rule. It makes no sense to use it within a single crate.
There was a problem hiding this comment.
Changed to inherent methods.
1fa0971 to
2add770
Compare
Edit the copy of the sync client to make it async. Add a set of RPCs that try the latest version of the returned type, and fall back to the previous version if that fails. This works for the RPCs here because of the change in return shape between the versions. Add integration tests for the async client. Update the README.
2add770 to
cd18940
Compare
Thanks, I included all of your cleanups and redid the error handling. |
|
Mad! Thanks for sticking with this mate. Sorry if my review the other day was a bit prickly. Nice to see this go in. Lets do the split into two crates and release this fella. |
This PR adds an async corepc-client with the RPCs used by BDK.
The patches are structured to make it easier to see what was changed from sync to async. First the sync version is copied and renamed, then in a separate patch it is changed to async.