Add Blockchain RPC methods #5
Open
Conversation
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
- getblockcount - getblockhash - getblockfilter - getblockheader - getrawmempool - getrawtransaction
tvpeter
force-pushed
the
feat/blockchain-rpc-methods
branch
from
d73bb7a to
6543635
Compare
- add integration tests for blockchain methods
tvpeter
force-pushed
the
feat/blockchain-rpc-methods
branch
from
6543635 to
3a16fa4
Compare
Collaborator
ValuedMammal
left a comment
ValuedMammal
left a comment
There was a problem hiding this comment.
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.
Comment on lines
+118
to
+123
| let bytes: Vec<u8> = Vec::<u8>::from_hex(&hex_string).map_err(Error::HexToBytes)?; | ||
|
|
||
| let block: Block = deserialize(&bytes) | ||
| .map_err(|e| Error::InvalidResponse(format!("failed to deserialize block: {e}")))?; | ||
|
|
||
| Ok(block) |
Collaborator
There was a problem hiding this comment.
For things that can be consensus decoded directly from a hex string, like Block, Header, and Transaction, I think it will be easier to use deserialize_hex and have a new Error::DecodeHex error that wraps the FromHexError.
Suggested change
| let bytes: Vec<u8> = Vec::<u8>::from_hex(&hex_string).map_err(Error::HexToBytes)?; | |
| let block: Block = deserialize(&bytes) | |
| .map_err(|e| Error::InvalidResponse(format!("failed to deserialize block: {e}")))?; | |
| Ok(block) | |
| bitcoin::consensus::encode::deserialize_hex(&hex_string).map_err(Error::DecodeHex) | |
Comment on lines
+127
to
+130
| 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) | ||
| } |
Collaborator
There was a problem hiding this comment.
I assume we first want to deserialize the response as a type from corepc_types (e.g. v29::GetBlockVerboseOne), and then call into_model on it. Also we'll need a test for it.
Comment on lines
+146
to
+162
| let raw: serde_json::Value = self.call("getblockhash", &[json!(height)])?; | ||
|
|
||
| let hash_str = match raw { | ||
| serde_json::Value::String(s) => s, | ||
| serde_json::Value::Object(obj) => obj | ||
| .get("hash") | ||
| .and_then(|v| v.as_str()) | ||
| .ok_or_else(|| Error::InvalidResponse("getblockhash: missing 'hash' field".into()))? | ||
| .to_string(), | ||
| _ => { | ||
| return Err(Error::InvalidResponse( | ||
| "getblockhash: unexpected response type".into(), | ||
| )); | ||
| } | ||
| }; | ||
|
|
||
| BlockHash::from_str(&hash_str).map_err(Error::HexToArray) |
Collaborator
There was a problem hiding this comment.
This seems complicated. Don't we always expect the value to be String?
Suggested change
| let raw: serde_json::Value = self.call("getblockhash", &[json!(height)])?; | |
| let hash_str = match raw { | |
| serde_json::Value::String(s) => s, | |
| serde_json::Value::Object(obj) => obj | |
| .get("hash") | |
| .and_then(|v| v.as_str()) | |
| .ok_or_else(|| Error::InvalidResponse("getblockhash: missing 'hash' field".into()))? | |
| .to_string(), | |
| _ => { | |
| return Err(Error::InvalidResponse( | |
| "getblockhash: unexpected response type".into(), | |
| )); | |
| } | |
| }; | |
| BlockHash::from_str(&hash_str).map_err(Error::HexToArray) | |
| let hex: String = self.call("getblockhash", &[json!(height)])?; | |
| Ok(hex.parse()?) |
| } | ||
|
|
||
| /// Get block filter | ||
| pub fn get_block_filter(&self, block_hash: BlockHash) -> Result<GetBlockFilter, Error> { |
Collaborator
There was a problem hiding this comment.
For consistency we should decide whether the API can take a block hash by value or by reference.
Suggested change
| pub fn get_block_filter(&self, block_hash: BlockHash) -> Result<GetBlockFilter, Error> { | |
| pub fn get_block_filter(&self, block_hash: &BlockHash) -> Result<GetBlockFilter, Error> { |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Description
This PR adds the following methods:
Fixes #4
Depends on #3
Checklists
All Submissions:
cargo fmtandcargo clippybefore committingNew Features:
CHANGELOG.md