[chore]: use wrapper structs for heights #1448
Conversation
|
Whoa, during this PR, I first started with changing all heights from u64 to my structs everywhere in code. And now I have a couple of errors saying "you try to put Stacks height but expected Bitcoin height". And this is great, this mean this aliases are really useful! |
|
whoops, |
|
Blocked until #1466 will be resolved, changing seed not saves from overflow, but do something different |
|
Well, even if changing seed to fix test is not so good, we can merge even with it, because on main branch we have same issue. And I think it is better to merge it earlier than later, because I afraid that merge conflict will be terrible if we delay this =) |
| /// Implementation of saturating_add for int wrapper type `[$type]`. Behavies same way as inner type. | ||
| pub fn saturating_add(self, rhs: impl Into<$type>) -> Self { | ||
| let rhs: $inner = rhs.into().0; | ||
| Self(self.0.saturating_add(rhs)) | ||
| } | ||
| /// Implementation of saturating_sub for int wrapper type `[$type]`. Behavies same way as inner type. | ||
| pub fn saturating_sub(self, rhs: impl Into<$type>) -> Self { | ||
| let rhs: $inner = rhs.into().0; | ||
| Self(self.0.saturating_sub(rhs)) | ||
| } | ||
| /// Implementation of wrapping_add for int wrapper type `[$type]`. Behavies same way as inner type. | ||
| pub fn wrapping_add(self, rhs: impl Into<$type>) -> Self { | ||
| let rhs: $inner = rhs.into().0; | ||
| Self(self.0.wrapping_add(rhs)) | ||
| } | ||
| /// Implementation of wrapping_sub for int wrapper type `[$type]`. Behavies same way as inner type. | ||
| pub fn wrapping_sub(self, rhs: impl Into<$type>) -> Self { | ||
| let rhs: $inner = rhs.into().0; | ||
| Self(self.0.wrapping_sub(rhs)) | ||
| } | ||
| /// Implementation of checked_add for int wrapper type `[$type]`. Behavies same way as inner type. | ||
| pub fn checked_add(self, rhs: impl Into<$type>) -> Option<Self> { | ||
| let rhs: $inner = rhs.into().0; | ||
| self.0.checked_add(rhs).map(Self) | ||
| } | ||
| /// Implementation of checked_sub for int wrapper type `[$type]`. Behavies same way as inner type. | ||
| pub fn checked_sub(self, rhs: impl Into<$type>) -> Option<Self> { | ||
| let rhs: $inner = rhs.into().0; | ||
| self.0.checked_sub(rhs).map(Self) | ||
| } | ||
| /// Implementation of overflowing_add for int wrapper type `[$type]`. Behavies same way as inner type. | ||
| pub fn overflowing_add(self, rhs: impl Into<$type>) -> (Self, bool) { | ||
| let rhs: $inner = rhs.into().0; | ||
| let (val, overflow) = self.0.overflowing_add(rhs); | ||
| (Self(val), overflow) | ||
| } | ||
| /// Implementation of overflowing_sub for int wrapper type `[$type]`. Behavies same way as inner type. |
There was a problem hiding this comment.
We do not really need all of this for now, only saturating ones, but probably it is ok to keep this impls
| @@ -339,7 +339,7 @@ pub struct SignerConfig { | |||
| /// already been run, the coordinator will attempt to re-run DKG after this | |||
| /// block height is met if `dkg_target_rounds` has not been reached. If DKG | |||
| /// has never been run, this configuration has no effect. | |||
| pub dkg_min_bitcoin_block_height: Option<NonZeroU64>, | |||
There was a problem hiding this comment.
Tbh I don't get the point of using NonZeroU64 here. We probably are not ok with height = 0 in all cases, not only here? Like, even in test environments, on height 0 there are no testBTC mined, and nobody have funds to do a deposit.
There was a problem hiding this comment.
So I think knowledge that it is a BitcoinHeight is more important that knowledge that it is non-zero. Aaand, I also don't think we want to have non-zero checks on all heights in our codebase
There was a problem hiding this comment.
And we probably can add some NonZeroBitcoinHeight only for this, but I'm not sure if it is very necessary
| } | ||
| impl $trait for $type { | ||
| type Output = $type; | ||
| fn $method(self, other: $type) -> Self::Output { |
There was a problem hiding this comment.
Maybe $int as output type will make more sense here (at least for substraction). But probably this is also okish
Description
Switch from raw
u64tostruct BlockchainNameBlockHeight(u64)insignerCloses: #1405
Changes
Scope: refactoring was only in
signerI will not promise that I changed 100% of heights to wrappers, but I think so.
Testing Information
Since it is refactoring we do not need any new tests.
Checklist: