Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Impl Add, Sub for block::Height #1055

Closed
hdevalence opened this issue Sep 12, 2020 · 2 comments · Fixed by #1057
Closed

Impl Add, Sub for block::Height #1055

hdevalence opened this issue Sep 12, 2020 · 2 comments · Fixed by #1057
Assignees
Labels
C-enhancement Category: This is an improvement

Comments

@hdevalence
Copy link
Contributor

Is your feature request related to a problem? Please describe.

It would be nice to be able to add and subtract block heights. However, block heights themselves are heights, so adding and subtracting heights doesn't give a new height, it gives a difference between heights: block heights are a torsor. The ops impls we add should reflect this.

Describe the solution you'd like

We should add the following impls for block::Height:

impl Sub<Height> for Height {
    type Output = i32;
    /* ... */
}

impl Add<i32> for Height {
    type Output = Option<Height> // None if over/under max/min height
    /* ... */
}

impl Sub<i32> for Height {
    type Output = Option<Height> // None if over/under max/min height
    /* ... */
}

Describe alternatives you've considered

  • Implementing addition/subtraction for heights themselves -- this isn't a quite correct modeling of the problem, and trying to do this probably indicates a logic error;

  • Adding a custom newtype for height differences -- this provides little type safety compared to using an i32, at the cost of adding and maintaining another new type, meaning additional API surface, additional impls to write, code to maintain, etc.

@hdevalence hdevalence added C-enhancement Category: This is an improvement S-needs-triage Status: A bug report needs triage labels Sep 12, 2020
@oxarbitrage oxarbitrage self-assigned this Sep 12, 2020
@oxarbitrage
Copy link
Contributor

Will be working on this one.

@yaahc
Copy link
Contributor

yaahc commented Sep 14, 2020

We should add the following impls for block::Height:

I'm assuming all the i32's in these signatures are meant to represent the diffs, my preference would be to create a HeightDiff newtype to make this more clear.

@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label Feb 18, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: This is an improvement
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants