Finalize filtering in `listTransactions` by converting `Range UTCTime` to `Range SlotId` #595

Anviking · 2019-07-31T18:28:00Z

Issue Number

Overview

Previous PR #588 added filtering-support to DB.readTxHistory using Range SlotId. Here we do the final connecting to allow WalletLayer.listTransactions filtering by Range UTCTime.

I have added a slotAtTime mirroring slotStartTime. I've added an unfinished roundtrip test for this. ⚠️ I'm not sure this is exactly how we want it. There might be subtleties regarding to which slot the boundary belongs to etc.
TODO: Convert Range UTCTime to Range SlotId argument listTransactions (Maybe Range could be functor or something?)

Comments

I've tried to separate into confined commits
Doing the UTCTime -> SlotId conversion might not be needed in the future, depending on how we deal with proper slotting and changing epoch/slot-lengths.

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

Anviking · 2019-07-31T23:47:50Z

lib/core/src/Cardano/Wallet.hs

@@ -759,10 +759,11 @@ newWalletLayer tracer bp db nw tl = do
                ErrListTransactionsStartTimeLaterThanEndTime $
                    ErrStartTimeLaterThanEndTime start end
            _ -> pure ()
+        let slotIdRange = (error "TODO :: UTCTime -> SlotId") timeRange


Re

case (mStart, mEnd) of (Just start, Just end) -> when (start > end) $ throwE $ ErrListTransactionsStartTimeLaterThanEndTime $ ErrStartTimeLaterThanEndTime start end _ -> pure ()

I considered moving this logic to Primitive.Types as isValid :: Range a -> Bool, but then there is no nice way of constructing ErrStartTimeLaterThanEndTime with pure UTCTimes.

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

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

We represent slot number in 'SlotId' as 'Word16', so it doesn't make much sense to allow the 'EpochLength' to go far beyond that bound. This might bite us later in rounding and overflowing. So, better keep the epoch length the same size as the slot number

We cannot generate 'SlotId' and 'EpochLength' independently, as this may generate invalid data..

Make sure the guard is triggered before we actually go fetch the wallet in the database.

KtorZ · 2019-08-02T14:27:00Z

lib/core/src/Cardano/Wallet.hs

-    { startTime :: UTCTime
-    , endTime :: UTCTime
+    { errStartTime :: UTCTime
+    , errEndTime :: UTCTime


renamed to avoid name clash.

KtorZ · 2019-08-02T14:27:13Z

lib/core/src/Cardano/Wallet.hs

-        (SlotLength slotLength)
-        slotsPerEpoch
+        slotLength
+        epochLength


renamed for consistency in naming with the Primitive.Types module.

KtorZ · 2019-08-02T14:27:26Z

lib/core/src/Cardano/Wallet.hs

@@ -753,17 +756,26 @@ newWalletLayer tracer bp db nw tl = do
        -> SortOrder
        -> ExceptT ErrListTransactions IO [TransactionInfo]
    _listTransactions wid mStart mEnd order = do
+        guardRange (mStart, mEnd)


factored out for readability.

KtorZ · 2019-08-02T14:29:07Z

@paweljakubas I've picked this over @Anviking and finalized what he started. I think that is now ready to be reviewed and merged.

Will remain: adding the Content-Range header to the response to convey some metadata and.. probably some integration tests scenarios.

paweljakubas

LGTM

Anviking force-pushed the anviking/466/convert-time-to-slot branch from f2a06d7 to d45dc35 Compare July 31, 2019 18:34

Anviking commented Jul 31, 2019

View reviewed changes

lib/core/test/unit/Cardano/Wallet/Primitive/TypesSpec.hs Outdated Show resolved Hide resolved

Anviking commented Jul 31, 2019

View reviewed changes

lib/core/test/unit/Cardano/Wallet/Primitive/TypesSpec.hs Outdated Show resolved Hide resolved

Anviking force-pushed the anviking/466/add-filtering-to-db branch 2 times, most recently from 4aa664c to e23c506 Compare July 31, 2019 19:01

Anviking changed the title ~~Finalize filtering in listTransactions by converting UTCTime to SlotId~~ Finalize filtering in listTransactions by converting Range UTCTime to Range SlotId Jul 31, 2019

KtorZ mentioned this pull request Jul 31, 2019

Implement filtering and sorting for the listTransactions endpoint. #466

Closed

11 tasks

Anviking force-pushed the anviking/466/convert-time-to-slot branch from d45dc35 to b6c727b Compare July 31, 2019 23:44

Anviking commented Jul 31, 2019

View reviewed changes

KtorZ reviewed Aug 1, 2019

View reviewed changes

lib/core/test/unit/Cardano/Wallet/Primitive/TypesSpec.hs Outdated Show resolved Hide resolved

KtorZ reviewed Aug 1, 2019

View reviewed changes

lib/core/src/Cardano/Wallet/Primitive/Types.hs Outdated Show resolved Hide resolved

jonathanknowles force-pushed the anviking/466/add-filtering-to-db branch 2 times, most recently from a99a8c9 to 820b63f Compare August 1, 2019 08:24

KtorZ changed the base branch from anviking/466/add-filtering-to-db to master August 2, 2019 12:38

KtorZ force-pushed the anviking/466/convert-time-to-slot branch from b6c727b to ff73e0a Compare August 2, 2019 13:02

Anviking and others added 8 commits August 2, 2019 16:23

WIP: Add slotAtTime :: ... -> UTCTime -> SlotId

0753e8b

WIP: Use the range arg in listTransactions for filtering

dbf8d9e

review 'slotAt' function with computation that's easier to verify

4f44c1f

review arbitrary instances for SlotLength, EpochLength and StartTime

7f3cc4d

We cannot generate 'SlotId' and 'EpochLength' independently, as this may generate invalid data..

review guard for range in list transaction

a617827

Make sure the guard is triggered before we actually go fetch the wallet in the database.

implement slotId range for listTransaction using UTCTime range

23b6fe3

Re-generate nix machinery

d0b97b4

KtorZ force-pushed the anviking/466/convert-time-to-slot branch from ff73e0a to d0b97b4 Compare August 2, 2019 14:26

KtorZ added this to the Finalize 'Transactions' API endpoints milestone Aug 2, 2019

KtorZ self-assigned this Aug 2, 2019

KtorZ requested a review from paweljakubas August 2, 2019 14:26

KtorZ marked this pull request as ready for review August 2, 2019 14:26

KtorZ reviewed Aug 2, 2019

View reviewed changes

paweljakubas approved these changes Aug 2, 2019

View reviewed changes

paweljakubas merged commit 8cb070e into master Aug 2, 2019

paweljakubas deleted the anviking/466/convert-time-to-slot branch August 2, 2019 15:43

Anviking mentioned this pull request Aug 22, 2019

Test that slotStartTime . slotRange is idempotent. #652

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Finalize filtering in `listTransactions` by converting `Range UTCTime` to `Range SlotId` #595

Finalize filtering in `listTransactions` by converting `Range UTCTime` to `Range SlotId` #595

Anviking commented Jul 31, 2019 •

edited

Loading

Anviking Jul 31, 2019 •

edited

Loading

KtorZ Aug 2, 2019

KtorZ Aug 2, 2019

KtorZ Aug 2, 2019

KtorZ commented Aug 2, 2019

paweljakubas left a comment

Finalize filtering in listTransactions by converting Range UTCTime to Range SlotId #595

Finalize filtering in listTransactions by converting Range UTCTime to Range SlotId #595

Conversation

Anviking commented Jul 31, 2019 • edited Loading

Issue Number

Overview

Comments

Anviking Jul 31, 2019 • edited Loading

Choose a reason for hiding this comment

KtorZ Aug 2, 2019

Choose a reason for hiding this comment

KtorZ Aug 2, 2019

Choose a reason for hiding this comment

KtorZ Aug 2, 2019

Choose a reason for hiding this comment

KtorZ commented Aug 2, 2019

paweljakubas left a comment

Choose a reason for hiding this comment

Finalize filtering in `listTransactions` by converting `Range UTCTime` to `Range SlotId` #595

Finalize filtering in `listTransactions` by converting `Range UTCTime` to `Range SlotId` #595

Anviking commented Jul 31, 2019 •

edited

Loading

Anviking Jul 31, 2019 •

edited

Loading