chainweb-node
chainweb-node copied to clipboard
kadena-io
Explicit numeric conversions
This PR just adds type applications to uses of int, which is our alias to fromIntegral. Before merge, I recommend that we audit these conversions to make sure they're correct (especially correct w.r.t dimensions) and safe. An expansion to this would be using witch or something similar (perhaps vendored) to check that conversions are safe.
It's a big improvement, making all these casts explicit.
I bet there are also some readability wins to be had by scanning the int @_ @_'s for cases where a purpose-named function can take the place of a generic conversion function. E.g.
case window ver of
Just (WindowWidth w)
| int @BlockHeight @Natural (_blockHeight h) <= w -> foo
->
case window ver of
Just win | windowContainsBlock win h -> foo
@jwiegley A bad numeric conversion caused the catchup issue from 2.17.1, fixed in https://github.com/kadena-io/chainweb-node/pull/1571. A function used to produce cuts for nodes that were catching up started to take a BlockHeight instead of a CutHeight, causing the /cut GET endpoint to interpret its parameter as a BlockHeight because the numeric conversion was not annotated with its result type. As a result, despite the /cut GET endpoint code not changing, it was broken by downstream code without a type error. We have an internal issue for this, I will make a backlink here.
Would it be better to have separate newtypes for block heights and cut heights?
We do, they just... Both have Num instances. Maybe we can remove those? That could also help, we do use them a lot though.
How often do we need to transparently use them as "numbers"? What if we removed those instances and made the conversions explicit?
That is a possibility too. But it will be more verbose. For example we frequently convert though between (MaxRank, MinRank) (TreeDB types) and BlockHeight, but MaxRank and MinRank wrap Natural, so we convert MaxRank -> Natural -> Word64 -> BlockHeight, three conversions.