Review Slotting As Primitive

[KtorZ]

Issue Number

Overview

• I have remove the 'EpochIndex' and 'SlotNumber' wrappers (cf comments)
• I have merged the 'Slotting' module into the 'Primitive' module
• I have remove the ad-hoc functions for 'SlotId' in favor of a more idiomatic 'Enum' instance

Comments

NOTE: I suggest to review this commit by commit 👍

This is because we can't actually represent a SlotNumber at compile-time with a satisfactory type. At a given moment in time, this type has some given bounds. Yet, it can change at any moment and considering it fixed would be a mistake. Plus, we can already encapsulate most of the slotting arithmetic within the SlotId type, so we don't need the extra ones for the EpochIndex and SlotNumber and we can simply avoid manipulating them overall.

Same goes for the ad-hoc functions to increment and decrement slots. In practice, we'll probably never use them as this arithmetic will be done by the nodes and for us, slots are really just some ids / foreign keys on a block. Also, I've define 'Enum' instance despite this instance being partial on the boundaries like for many types in Haskell. This belongs to those gray areas that divide people. I belong to the school of thoughts that believe the type system should help us as much as possible; though, when it truly gets in the way, I find it more reasonable to add a guard and be conscious about the edge case. Here, the only thing that can go wrong is trying to get the successor of the very first slot. Because of this, we have to carry some Maybe type everywhere which was, anyway, poorly handled in for the Nothing branch in the mock test because in practice, handling the first slot requires extra logic ANYWAY. For the other boundary, even if we were processing blocks at a pace of 1 epoch per hour, it would still take us 38K MILLIONS of year to reach that bound. We've time to see it coming.

[Anviking]

Hm, interesting! This seems like basically the SlotId (interface), we'd need when taking slotsPerEpoch into account?

[KtorZ]

How we're going to handle this is still unclear but I see one easy option:

• We actually embed the epoch length inside the SlotId definition which allows us to make various computations on list of slots

We won't be able to safely compute the successors of a given slots when we cross the epoch boundary.. Though, in practice, the wallet never has to do that... We always look at slot that have passed, and we need the ability to compare them, that's it.

[Anviking]

Nice commits!

1. SlotId — I like it
2. Isn't there a beauty to having Slotting and Primitives separate? If we'd want to use alternate versions of them in tests or something? I guess the rest of the code might view them both as "primitives" though 👍
3. What is the purpose of the Enum instance? Backfilling? Just mocking? This won't work well with changing slotsPerEpoch later? Or will it?

Regardless, lgtm!

Edit, saw:

I have merged MockNetworkLayer with the RustHttpBridgeSpec, and the RustHttpBridgeClient with RustHttpBridge. Generally speaking, I'd like to avoid us to spread too much with modules, it makes code hard to find in the long run and eventually end up with us duplicating functionality and logic in various places. If there's no good reason to have multiple modules, I'd rather stick as much as we can inside one.

[rvl]

Looks good, thanks.

• You could have added these suggestions as a review comment on the original PR and I would easily have been able to fix it then and saved you some work.
• The arguments against multiple smaller modules are weak.
• SlotId is better without newtype wrappers, but why not type aliases?

[rvl]

Why Natural, rather than Word64?

[KtorZ]

I kept the original type-signature defined earlier. I believe that the rational was that Natural are unbounded, though in practice, a Word64 is more than fine indeed.

[KtorZ]

You could have added these suggestions as a review comment on the original PR and I would easily have been able to fix it then and saved you some work.

I kinda missed the shot and it was merged already when I started to look into this (which is fine, I mean, there's no "blessed code", so, if we later feel like re-organizing a bit some module(s) to make things a bit clearer or, in preparation of an upcoming feature, then let's not be afraid of doing so).

The arguments against multiple smaller modules are weak.

Which spot do you find weak exactly? From my experience, spreading common responsibility across many modules open the gate for duplication and inconsistency, especially on "big" team (big = more than 3 persons). It's hard to keep various parts of the code in sync when they are added by different people. Having less modules which are more complete plays better in the long run. It also helps identifying dead code. For instance on the old cardano-wallet (the part that was supposed not to have been written by Serokell), I found at some point 4 different ways of decrypting an address payload, in 4 different places, and of course, none of them was really tested. There should be one source of truth.

SlotId is better without newtype wrappers, but why not type aliases?

Type-aliases are evil 👿 ... They give you a false sense of type-safety. When you see a type-alias, you think you're safe because there's strong typing but in reality it's just a facade and you won't even notice. No type-alias makes the code more transparent and, at least, now we are being true about the fact that we use a primitive Word64 type for this. Having a newtype wrapper which exposes its constructor and derive all sort of instances automatically have almost no value either for the same reasons. It's also misleading because, anything can be coerced to it and it's like having none in the end.

[KtorZ]

@Anviking

Isn't there a beauty to having Slotting and Primitives separate? If we'd want to use alternate versions of them in tests or something? I guess the rest of the code might view them both as "primitives" though

Cf the previous answer, but truth is that, it depends on the language / technology you use. I would recommend a very different approach in JavaScript for instance, because code isn't weakly and dynamically typed. In JS, having many small modules is usually a good practice (small being, they should fit on your screen without scrolling). Having more modules with well-defined frontiers helps reducing errors when modifying code. In Haskell, GHC has our back, and our problem lies elsewhere. It's so easy to define new functions in Haskell that we'll rather re-write them than looking for them in the code base :/
If I may recommend, a very good talk from Evan Czaplicki: Life of a File

What is the purpose of the Enum instance? Backfilling? Just mocking? This won't work well with changing slotsPerEpoch later? Or will it?

It won't work well with changing the epoch length indeed. We can't define instances at runtime (though they are ways of doing that, for instance, by defining an instance on (EpochLength, SlotId), or by embedding the length within the SlotId.
The Enum doesn't solve any particular problem but, it is more idiomatic Haskell. When one can, one should program to an interface, especially when they are well-known and within the base built-in (Applicative, Functor, Enum, Ord etc ..). They make the intend clear because those interfaces usually come with some law, so by satisfying them, you also give extra semantic to your type, rather than having a ad-hoc function which will do the same job but will force the reader to look into the implementation details (because chances are that we won't bother documenting as much such functions).

[Anviking]

If I may recommend, a very good talk from Evan Czaplicki: Life of a File

Thanks for the recommendation btw, @KtorZ!