zcash_protocol: Add constructors to LocalNetwork
#1281
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1281 +/- ##
==========================================
- Coverage 63.40% 63.35% -0.06%
==========================================
Files 121 121
Lines 13757 13768 +11
==========================================
Hits 8723 8723
- Misses 5034 5045 +11 ☔ View full report in Codecov by Sentry. |
…f NU ordering is changed
| @@ -10,6 +10,8 @@ and this library adheres to Rust's notion of | |||
| ### Added | |||
| - `zcash_protocol::memo`: | |||
| - `impl TryFrom<&MemoBytes> for Memo` | |||
| - `zcash_protocol::local_consensus`: | |||
| - `new`, `all_upgrades_active` and `canopy_active` constructors to `LocalNetwork` | |||
There was a problem hiding this comment.
PR needs a rebase because this now merge-conflicts with the 0.1.1 release (in that it will end up editing the 0.1.1 changelog).
| overwinter: u64, | ||
| sapling: u64, | ||
| blossom: u64, | ||
| heartwood: u64, | ||
| canopy: u64, | ||
| nu5: u64, |
There was a problem hiding this comment.
I just realised that all of these arguments are u64. Two questions:
- Why not
BlockHeight? - If not
BlockHeight, whyu64? The internal casts tou32will silently truncate the heights, which is undocumented and confusing. Additionally, these truncations occur after the assertion, which means that a caller can bypass the ordering requirement by setting the high bits of the heights.
The latter question is blocking; these should be u32 at a minimum, and I don't see a good reason for these to not be BlockHeight.
|
|
||
| /// Construct a new `LocalNetwork` with all network upgrades up to and including canopy | ||
| /// initally active. | ||
| pub fn canopy_active(nu5_activation_height: u64) -> Self { |
There was a problem hiding this comment.
Ditto: this should at least be u32, and probably a BlockHeight.
Adds commonly used constructors to
LocalNetworkstruct.