Add `epochCeiling` function to `Primitive.Types`. #1114

jonathanknowles · 2019-12-04T09:55:12Z

Issue Number

#1086

Overview

This PR:

Adds the functions epoch{Ceiling,Floor}
- epochCeiling calculates the earliest EpochNo whose start time is greater than or equal to the specified time. (Required by issue Create epochCeiling function. #1086.)
- epochFloor calculates the latest EpochNo whose start time is less than or equal to the specified time. (Added for completeness.)
Adds the epochStartTime and epoch{Pred,Succ} helper functions.
Adds a set of property tests for the above.

Comments

The functions added by this PR follow the same general pattern as the following existing functions:

slot{Ceiling,Floor}
slot{Pred,Succ}
slotStartTime

paweljakubas · 2019-12-04T19:47:30Z

lib/core/src/Cardano/Wallet/Primitive/Types.hs

+    | t > timeMax = Just maxBound
+    | otherwise = epochNumber <$> slotFloor sps t
+  where
+    timeMin = epochStartTime sps minBound


[suggestion]

timeMin = epochStartTime sps minBound timeMax = epochStartTime sps maxBound

is also in lines 1134-5 - maybe it is good idea to not duplicate this and put outside where once?

Well spotted!

I was thinking about doing this. However, I'm not (yet) convinced that it would actually help us much. Let's suppose we did define the following functions:

-- | Calculates the start time of the earliest representable epoch. epochStartTimeMinBound :: SlotParameters -> UTCTime epochStartTimeMinBound = flip epochStartTime minBound --- | Calculates the start time of the latest representable epoch. epochStartTimeMaxBound :: SlotParameters -> UTCTime epochStartTimeMaxBound = flip epochStartTime maxBound

We'd then be able to replace all instances of:

epochStartTime sps minBound

With:

epochStartTimeMinBound sps

Do you think this would be worth it?

(I'm trying to think of a compelling reason to convince myself. Feel free to suggest a reason if I've missed something obvious!)

paweljakubas · 2019-12-04T19:54:40Z

lib/core/test/unit/Cardano/Wallet/Primitive/TypesSpec.hs

+                    checkCoverage $
+                    cover 10      withinBounds  "within bounds" $
+                    cover 10 (not withinBounds) "out of bounds" $
+                    expectedResult === predN n (succN n $ Just epoch)


[suggestion]

Wouldn't be better to add properties in those two boundary cases where we apply n times epochPred and n times epochSucc but randomly picked (rather than one special case n times epochPred and later timesepochSucc`) and also require certain coverage of out-of-bounds cases?

[suggestion]

Wouldn't be better to add properties in those two boundary cases where we apply n times epochPred and n times epochSucc but randomly picked (rather than one special case n times epochPred and later timesepochSucc`) and also require certain coverage of out-of-bounds cases?

I'm not sure I understand what you mean here. Would you be able to give an example?

lib/core/test/unit/Cardano/Wallet/Primitive/TypesSpec.hs

lib/core/src/Cardano/Wallet/Primitive/Types.hs

lib/core/test/unit/Cardano/Wallet/Primitive/TypesSpec.hs

paweljakubas

Very nice, comprehensive testing! Left some small remarks/questions

This function computes the time at which an epoch starts.

This enables generation of arbitrary slot parameters together with an arbitrary point in time, where the point in time falls into one of three categories: 1. occurs during the lifetime of the blockchain; 2. occurs before the earliest representable slot; 3. occurs after the latest representable slot.

These functions find the predecessor and successor of an epoch, respectively.

jonathanknowles · 2019-12-05T02:46:06Z

bors r+

1114: Add `epochCeiling` function to `Primitive.Types`. r=jonathanknowles a=jonathanknowles # Issue Number #1086 # Overview This PR: - [x] Adds the functions `epoch{Ceiling,Floor}` - `epochCeiling` calculates the _earliest_ `EpochNo` whose start time is _greater than or equal to_ the specified time. (Required by issue #1086.) - `epochFloor` calculates the _latest_ `EpochNo` whose start time is _less than or equal to_ the specified time. (Added for completeness.) - [x] Adds the `epochStartTime` and `epoch{Pred,Succ}` helper functions. - [x] Adds a set of property tests for the above. # Comments The functions added by this PR follow the same general pattern as the following existing functions: * `slot{Ceiling,Floor}` * `slot{Pred,Succ}` * `slotStartTime` Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io>

iohk-bors · 2019-12-05T03:17:21Z

Build failed

buildkite/cardano-wallet

jonathanknowles · 2019-12-05T03:22:36Z

bors r+

1114: Add `epochCeiling` function to `Primitive.Types`. r=jonathanknowles a=jonathanknowles # Issue Number #1086 # Overview This PR: - [x] Adds the functions `epoch{Ceiling,Floor}` - `epochCeiling` calculates the _earliest_ `EpochNo` whose start time is _greater than or equal to_ the specified time. (Required by issue #1086.) - `epochFloor` calculates the _latest_ `EpochNo` whose start time is _less than or equal to_ the specified time. (Added for completeness.) - [x] Adds the `epochStartTime` and `epoch{Pred,Succ}` helper functions. - [x] Adds a set of property tests for the above. # Comments The functions added by this PR follow the same general pattern as the following existing functions: * `slot{Ceiling,Floor}` * `slot{Pred,Succ}` * `slotStartTime` Co-authored-by: Jonathan Knowles <jonathan.knowles@iohk.io>

iohk-bors · 2019-12-05T03:38:46Z

Build succeeded

buildkite/cardano-wallet

jonathanknowles · 2019-12-05T04:11:38Z

@paweljakubas

Thanks for your review, and very helpful feedback! I've added replies to your comments.

I have some further changes to make in response to your feedback, and will add those changes in PR #1121 (so they can be reviewed separately).

jonathanknowles requested a review from paweljakubas December 4, 2019 09:55

jonathanknowles self-assigned this Dec 4, 2019

jonathanknowles force-pushed the jonathanknowles/epoch-ceiling-floor branch from 140e9c9 to 4294df7 Compare December 4, 2019 10:05