Public short_id module #10
Conversation
src/short_id.rs
Outdated
| @@ -20,14 +20,21 @@ use bitcoin::{BlockHash, Txid}; | |||
|
|
|||
| #[derive(Copy, Clone, Debug, Display, PartialEq, Eq)] | |||
| #[display(Debug)] | |||
There was a problem hiding this comment.
we need here
| #[display(Debug)] | |
| #[display(doc_comments)] |
since now we have meaningful comments
There was a problem hiding this comment.
Great thanks for pointing that out 🙌
src/short_id.rs
Outdated
| pub enum Error { | ||
| /// block height invalid error |
There was a problem hiding this comment.
here and below we can remove error from the end, since it will be displayed as Error: <message>
| /// block height invalid error | |
| /// invalid block height in bitcoin ShortId |
There was a problem hiding this comment.
Makes sense, thanks for suggestion, still collecting some Rust best practices 🙂
src/short_id.rs
Outdated
| @@ -123,6 +159,8 @@ impl Default for Descriptor { | |||
| } | |||
|
|
|||
| impl Descriptor { | |||
| /// verifies if Descriptor type has valid properties otherwise returns | |||
There was a problem hiding this comment.
If possible pls uppercase the first letters of doc comments (except error variants)
There was a problem hiding this comment.
Thanks for clarifying that, should be ok now 👍
xn3cr0nx
force-pushed
the
pub-short-id
branch
from
b86697f
to
46ebd65
Compare
xn3cr0nx
force-pushed
the
pub-short-id
branch
from
46ebd65
to
32104b0
Compare
| } | ||
|
|
||
| #[test] | ||
| #[should_panic(expected = "attempt to subtract with overflow")] |
There was a problem hiding this comment.
This will test just a single case. To test multiple one needs to use assert_eq with Err - or expect_err
There was a problem hiding this comment.
Good point, it doesn't make sense to have multiple test cases here, but also cannot check the error with assert_eq or expect_error because get_block_height returns an Option, no error is raised. The method panics with such input, so the only way to test it is to capture the panic with the directive.
I refactored for a single panic case in order to check panic is properly captured.
There was a problem hiding this comment.
Hm, I think the fact that you can construct incorrect ShortId and have a method call on it which will panic is an API bug. But I assume we can address it in the separate PR.
There was a problem hiding this comment.
lol yes code panicking definitely sounds as a bug, thanks for creating the issue, makes sense as followup 👍
|
Oops, sorry, CI fails: test coverage uses rust stable, and it can't iterate over array. You have to use |
xn3cr0nx
force-pushed
the
pub-short-id
branch
from
32104b0
to
421314c
Compare
Oh I see, the issue is related to the used toolchain 1.47.0, as also shown in the CI preview. I didn't get this error cause I'm using the current stable 1.56.0. Any particular reason for keeping this toolchain for CI? Btw, I reproduced locally and refactored the tests in order to make it work with 1.47.0 |
|
Well, there should be a reason to increase MSRV, not the otherwise :) In general, rust bitcoin ecosystem tries to achieve as low MSRV as possible, to support non-mainstream rust compilers. You may see a lot of debates in rust-bitcoin on this matter - and we just follow their policy with the aim to eventually get to the same MSRV with them. |
Thanks for clarifying! Sounds good, I will stick with the proper toolchain locally 👍 |
As described in BP-WG/bp-node#4 this PR exports
short_idmodule in order to be used in bp-node.The PR also includes some basic unit tests. My initial goal was to test the code in order to understand the implementation. I would like to implement a complete test coverage of the package in order to make the implementation easy to understand. Tests can be really helpful, documenting the expected behavior of the code. Specific test cases can be defined and properly commented in order to describe the protocol representation.