Let `DB.readTxHistory` take `SortOrder` and `Range SlotId` #588

Anviking · 2019-07-29T10:01:46Z

Issue Number

Overview

@Anviking I added SortDirection and Filter a (for imposing >=/<= requirements) to core
@Anviking I made DB.readTxHistory take and apply sortDir and filter with implementations in Sqlite, MVar and Sqlite's StateMachine tests.
@Anviking Some final polish / decisions missing. Done I think.
@jonathanknowles: Replace usage of Test.QuickCheck.Instances.Time with UniformTime from module Test.Utils.Time.
@jonathanknowles: Use arbitraryBoundedEnum to avoid repeating the definition of SortOrder.

Comments

Anviking · 2019-07-29T10:04:14Z

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

+    deriving (Eq, Show)
+
+-- Represents the range [start, end]
+data Filter a = Filter


Should add some clearer comment here

Ah! This si quite similar to what I proposed here: #586 (comment). I like it :)

lib/core/src/Cardano/Wallet/DB/MVar.hs

lib/core/src/Cardano/Wallet/DB/Sqlite.hs

KtorZ · 2019-07-29T14:59:16Z

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

+    deriving (Eq, Show)
+
+-- Represents the range [start, end]
+data Filter a = Filter


Ah! This si quite similar to what I proposed here: #586 (comment). I like it :)

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

lib/core/test/unit/Cardano/Wallet/DB/StateMachine.hs

lib/core/test/unit/Cardano/Wallet/DBSpec.hs

lib/core/test/unit/Cardano/WalletSpec.hs

lib/core/test/unit/Cardano/Wallet/DB/StateMachine.hs

Anviking · 2019-07-29T21:53:38Z

lib/core/src/Cardano/Wallet.hs

-        liftIO $ assemble tip <$> DB.readTxHistory db (PrimaryKey wid)
+        liftIO $ assemble tip
+            <$> DB.readTxHistory db (PrimaryKey wid)
+                defaultTxSortOrder noFilter


TODO: _listTransactions should take sort and filter parameters as well.

DB.readTxHistory is well-tested through the StateMachine tests. Not sure we need to do anything in particular on the WalletLayer level 🤔

lib/core/src/Cardano/Wallet/DB/Sqlite.hs

jonathanknowles · 2019-07-30T02:36:45Z

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

@@ -326,6 +330,23 @@ instance ToText SortOrder where
 instance FromText SortOrder where
    fromText = fromTextToBoundedEnum SnakeLowerCase

+defaultTxSortOrder :: SortOrder
+defaultTxSortOrder =  Descending


Just wondering, why is it better to have Descending as a default?

Hm indeed… this is currently not used from the API as a "defaultTxSortOrder".

It is currently used for:

Ordering the DB read for pending transactions when reading a wallet checkpoint. This appears pointless as it will be put in a Set (Tx, TxMeta) later. https://github.com/input-output-hk/cardano-wallet/blob/315fec4d4cb56114e1fd59eedd420ea963e3cf29/lib/core/src/Cardano/Wallet/DB/Sqlite.hs#L312

DBSpec which just needs an arbitrary sort-order.

SqliteFileModeSpec which just needs an arbitrary sort-order https://github.com/input-output-hk/cardano-wallet/blob/315fec4d4cb56114e1fd59eedd420ea963e3cf29/lib/core/test/unit/Cardano/Wallet/DB/SqliteFileModeSpec.hs#L137

listTransactions in WalletLayer, which is temporary

Maybe we should remove this from Primitive.Types.

Removed in #592 .

If this Descending value is used as a default somewhere (be it in the API or elsewhere), then I'd much rather see it defined as a named constant than as a magic value embedded in the functions that use it.

See: #592 (review)

listTransactions in WalletLayer, which is temporary

What do you mean "temporary" 😅 ?

I think the rationale is: the ordering should come from the caller of the wallet layer. So, starting from the WalletLayer, the sort order shouldn't be Maybe but a plain value (because a default at this stage would be totally arbitrary).
At the API-level however, we decide of a default ordering for which Descending makes sense (most recent results first).

What do you mean "temporary" 😅 ?

😄 listTransactions used a hard-coded Descending order instead of its argument.

With a default value in listTransactions we do need to refer to Descending, which I didn't think about.

the sort order shouldn't be Maybe but a plain value (because a default at this stage would be totally arbitrary).

Yes. Unless one argues that defaulting to Descending shouldn't be unique to the API. Since you seem positive, I think I'll go with it though.

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

lib/core/test/unit/Cardano/Wallet/DB/StateMachine.hs

lib/core/test/unit/Cardano/Wallet/DBSpec.hs

lib/core/test/unit/Cardano/WalletSpec.hs

lib/core/test/unit/Cardano/Wallet/DB/StateMachine.hs

rvl

Looking good

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

jonathanknowles · 2019-07-31T03:01:16Z

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

@@ -326,6 +330,23 @@ instance ToText SortOrder where
 instance FromText SortOrder where
    fromText = fromTextToBoundedEnum SnakeLowerCase

+defaultTxSortOrder :: SortOrder
+defaultTxSortOrder =  Descending


If this Descending value is used as a default somewhere (be it in the API or elsewhere), then I'd much rather see it defined as a named constant than as a magic value embedded in the functions that use it.

See: #592 (review)

lib/core/test/unit/Cardano/Wallet/DBSpec.hs

lib/core/test/unit/Cardano/Wallet/DB/SqliteFileModeSpec.hs

Anviking · 2019-07-31T12:05:40Z

Squashed fixups

diff --git a/lib/core/src/Cardano/Wallet/Primitive/Types.hs b/lib/core/src/Cardano/Wallet/Primitive/Types.hs
index 8a037865..b2a438ec 100644
--- a/lib/core/src/Cardano/Wallet/Primitive/Types.hs
+++ b/lib/core/src/Cardano/Wallet/Primitive/Types.hs
@@ -344,7 +344,7 @@ defaultTxSortOrder =  Descending
 --
 -- - [start, end]  Range (Just start) (Just end)   \x -> x >= start && x <= end
 -- - [start,∞)     Range (Just start) Nothing      \x -> x >= start
--- - (-∞,end)      Range Nothing (Just end)        \x -> x <= end
+-- - (-∞,end]      Range Nothing (Just end)        \x -> x <= end
 -- - (-∞,∞)        Range Nothing Nothing           \_x -> True
 data Range a = Range
     { rStart :: Maybe a
diff --git a/lib/core/test/unit/Cardano/Wallet/DB/SqliteFileModeSpec.hs b/lib/core/test/unit/Cardano/Wallet/DB/SqliteFileModeSpec.hs
index e2effbf4..dfeeca56 100644
--- a/lib/core/test/unit/Cardano/Wallet/DB/SqliteFileModeSpec.hs
+++ b/lib/core/test/unit/Cardano/Wallet/DB/SqliteFileModeSpec.hs
@@ -133,9 +133,11 @@ spec =  do
             unsafeRunExceptT $ createWallet db testWid testCp testMetadata
             unsafeRunExceptT $ putTxHistory db testWid (Map.fromList testTxs)
             destroyDBLayer ctx
-            testOpeningCleaning f
+            testOpeningCleaning
+                f
                 (\db' -> readTxHistory db' testWid defaultTxSortOrder wholeRange)
-                testTxs mempty
+                testTxs
+                mempty

         it "put and read checkpoint" $ \f -> do
             (ctx, db) <- newDBLayer' (Just f)
diff --git a/lib/core/test/unit/Cardano/Wallet/DBSpec.hs b/lib/core/test/unit/Cardano/Wallet/DBSpec.hs
index d0a02055..3d774062 100644
--- a/lib/core/test/unit/Cardano/Wallet/DBSpec.hs
+++ b/lib/core/test/unit/Cardano/Wallet/DBSpec.hs
@@ -199,7 +199,9 @@ type TxHistory = [(Hash "Tx", (Tx, TxMeta))]
 filterTxHistory :: SortOrder -> Range SlotId -> TxHistory -> TxHistory
 filterTxHistory order range =
     filter (isWithinRange range . slotId . snd . snd)
-    . (if order == Ascending then reverse else id)
+    . (case order of
+        Ascending -> reverse
+        Descending -> id)
     . sortBySlot
     . sortByTxId
   where

Anviking · 2019-07-31T19:02:06Z

Minor rebase after @KtorZ merged Remove defaultTxSortOrder #592 into this
Small compilation issue fix
I left Fixup: separate Asc and Dsc "put and read tx history" e23c506 un-squashed as it might be the remaining iff-y part. But without it I believe the tests don't pass.

Anviking · 2019-07-31T19:04:11Z

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

+    (maybe True (x <=) high)
+
+-- NOTE: We could imagine replacing 'Range (Just 3) Nothing' with something
+-- more magic like: `(Including 3) ... NoBound`


I considered this. Maybe superfluous comment…

with something more magic like

😬 .... We gotta find a use-case for this. Otherwise, it just introduces more code to maintain and more complexity without justification ^.^

jonathanknowles

A couple more comments:

genSortOrder can reuse arbitraryBoundedEnum.
The UTCTime instance from quickcheck-instances can be replaced with Test.Utils.Time.genUniformTime, as this gives a wider range of UTCTime values (rather than those with dates that are clustered around 1864).

EDIT: I've added a couple of commits to address the above comments.

jonathanknowles · 2019-08-01T07:12:37Z

lib/core/test/unit/Cardano/Wallet/DB/StateMachine.hs

@@ -526,6 +537,21 @@ generator (Model _ wids) = Just $ frequency $ fmap (fmap At) <$> concat
    genPrivKey :: Gen MPrivKey
    genPrivKey = elements ["pk1", "pk2", "pk3"]

+    genSortOrder :: Gen SortOrder
+    genSortOrder = QC.elements [Ascending, Descending]


This can be replaced with arbitraryBoundedEnum from Test.QuickCheck.Arbitrary:

arbitraryBoundedEnum :: (Bounded a, Enum a) => Gen a

jonathanknowles · 2019-08-01T07:15:30Z

lib/core/test/unit/Cardano/WalletSpec.hs

    , choose
    , elements
    , property
    , withMaxSuccess
    , (==>)
    )
+import Test.QuickCheck.Instances.Time
+    ()


I would recommend replacing this with Test.Utils.Time.genUniformTime to get a wider range of UTCTime values. The instance supplied by the package above generates times that are clustered around 1864:

See: https://github.com/input-output-hk/cardano-wallet/wiki/Blackboard#investigate-the-arbitrary-instance-for-utctime-in-testquickcheckinstancestime

jonathanknowles · 2019-08-01T07:16:35Z

lib/core/test/unit/Cardano/Wallet/DB/StateMachine.hs

@@ -512,7 +523,7 @@ generator (Model _ wids) = Just $ frequency $ fmap (fmap At) <$> concat
        , (5, PutWalletMeta <$> genId' <*> arbitrary)
        , (5, ReadWalletMeta <$> genId')
        , (5, PutTxHistory <$> genId' <*> fmap unGenTxHistory arbitrary)
-        , (5, ReadTxHistory <$> genId')
+        , (5, ReadTxHistory <$> genId' <*> genSortOrder <*> genRange)


The genSortOrder here can be replaced with arbitraryBoundedEnum.

I made DB.readTxHistory take and apply sortOrder and range filtering with implementations in Sqlite, MVar and Sqlite's StateMachine tests. Squashed from: Ensure readTxHistory tests pass with `Descending noFilter` [WIP] Make tests concider sorting and filtering Do actual Sqlite sorting and filtering Apply suggestions from code review Introduce defaultTxSortDirection Temporary overflow fix to make StateMachine tests pass Rename Filter to Range` Fixup: more polish

This is slightly akward. But I opted for "slightly awkward" instead of "the wrong solution".

…Order`.

Anviking self-assigned this Jul 29, 2019

Anviking force-pushed the anviking/466/add-filtering-to-db branch from 2f8cab3 to c93d2f1 Compare July 29, 2019 10:02

Anviking commented Jul 29, 2019

View reviewed changes

Anviking force-pushed the anviking/466/add-filtering-to-db branch from 68e0db7 to 56cd71b Compare July 29, 2019 14:46

KtorZ reviewed Jul 29, 2019

View reviewed changes

Anviking force-pushed the anviking/466/add-filtering-to-db branch 2 times, most recently from 44c8611 to b08b92a Compare July 29, 2019 18:40

Anviking commented Jul 29, 2019

View reviewed changes

jonathanknowles requested changes Jul 30, 2019

View reviewed changes

rvl approved these changes Jul 30, 2019

View reviewed changes

Anviking changed the title ~~Let DB.readTxHistory take SortDirection and Filter SlotId~~ Let DB.readTxHistory take SortOrder and Range SlotId Jul 30, 2019

Anviking force-pushed the anviking/466/add-filtering-to-db branch 3 times, most recently from 0bd9baa to 315fec4 Compare July 30, 2019 15:06

jonathanknowles reviewed Jul 31, 2019

View reviewed changes

jonathanknowles requested changes Jul 31, 2019

View reviewed changes

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

KtorZ reviewed Jul 31, 2019

View reviewed changes

lib/core/test/unit/Cardano/Wallet/DB/SqliteFileModeSpec.hs Outdated Show resolved Hide resolved

Anviking force-pushed the anviking/466/add-filtering-to-db branch from 315fec4 to 13eab2e Compare July 31, 2019 12:03

Anviking mentioned this pull request Jul 31, 2019

Remove defaultTxSortOrder #592

Merged

2 tasks

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 commented Jul 31, 2019

View reviewed changes

Anviking mentioned this pull request Jul 31, 2019

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

Merged

2 tasks

KtorZ mentioned this pull request Jul 31, 2019

Implement filtering and sorting for the listTransactions endpoint. #466

Closed

11 tasks

jonathanknowles reviewed Aug 1, 2019

View reviewed changes

jonathanknowles self-assigned this Aug 1, 2019

jonathanknowles force-pushed the anviking/466/add-filtering-to-db branch from e23c506 to 4f00014 Compare August 1, 2019 07:51

jonathanknowles approved these changes Aug 1, 2019

View reviewed changes

Anviking and others added 9 commits August 1, 2019 08:23

Remove use of defaultTxSortOrder in SqliteFileModeSpec

d3a7251

Remove use of defaultTxSortOrder in DBSpec

5c3b82f

Remove use of defaultTxSortOrder in SqliteSpec

a2efd06

Remove use of defaultTxSortOrder in Sqlite.hs and Primitive.Types

b9e144f

Move notion of defaultTxSortOrder from WalletLayer to Api

2aad420

Fixup: separate Asc and Dsc "put and read tx history"

8abb334

This is slightly akward. But I opted for "slightly awkward" instead of "the wrong solution".

Use UniformTime in module Cardano.WalletSpec.

3e9daae

Use arbitraryBoundedEnum to avoid repeating the definition of `Sort…

820b63f

…Order`.

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

KtorZ merged commit 9d56a3b into master Aug 1, 2019

KtorZ deleted the anviking/466/add-filtering-to-db branch August 2, 2019 12:38

KtorZ added this to the Finalize 'Transactions' API endpoints milestone Sep 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Let `DB.readTxHistory` take `SortOrder` and `Range SlotId` #588

Let `DB.readTxHistory` take `SortOrder` and `Range SlotId` #588

Anviking commented Jul 29, 2019 •

edited by jonathanknowles

Anviking Jul 29, 2019

KtorZ Jul 29, 2019

KtorZ Jul 29, 2019

Anviking Jul 29, 2019

jonathanknowles Jul 30, 2019

Anviking Jul 30, 2019

Anviking Jul 30, 2019 •

edited

jonathanknowles Jul 31, 2019

KtorZ Jul 31, 2019

Anviking Jul 31, 2019 •

edited

rvl left a comment

jonathanknowles Jul 31, 2019

Anviking commented Jul 31, 2019

Anviking commented Jul 31, 2019

Anviking Jul 31, 2019

KtorZ Aug 1, 2019

jonathanknowles left a comment •

edited

jonathanknowles Aug 1, 2019

jonathanknowles Aug 1, 2019

jonathanknowles Aug 1, 2019

Navigation Menu

Let DB.readTxHistory take SortOrder and Range SlotId #588

Let DB.readTxHistory take SortOrder and Range SlotId #588

Conversation

Anviking commented Jul 29, 2019 • edited by jonathanknowles

Issue Number

Overview

Comments

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anviking Jul 30, 2019 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anviking Jul 31, 2019 • edited

Choose a reason for hiding this comment

rvl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anviking commented Jul 31, 2019

Anviking commented Jul 31, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanknowles left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Let `DB.readTxHistory` take `SortOrder` and `Range SlotId` #588

Let `DB.readTxHistory` take `SortOrder` and `Range SlotId` #588

Anviking commented Jul 29, 2019 •

edited by jonathanknowles

Anviking Jul 30, 2019 •

edited

Anviking Jul 31, 2019 •

edited

jonathanknowles left a comment •

edited