Remove defaultTxSortOrder

[Anviking]

Issue Number

#466

Overview

• I have removed defaultTxSortOrder I have moved it from the WalletLayer to the Api.
• I made _listTransactions actually pass its SortOrder to DB.readTxHistory

Comments

Base is #588

[Anviking]

Maybe the walletLayer should take a SortOrder instead of a Maybe SortOrder and leave the Api to decide on the default 🤷‍♂ I think this works though.

[KtorZ]

I do agree 👍

[jonathanknowles]

FWIW, I think that referring to a named constant (defined somewhere) within a function is greatly preferable to embedding a magic value. Having a named constant means that the code is more self-documenting.

I'd much prefer to see (not necessarily in the same module):

-- | The default sort order used when listing transactions, if none is specified.
defaultTxSortOrder :: SortOrder
defaultTxSortOrder = Descending

...

_listTransactions wid mStart mEnd mOrder = do
    (w, _) <- withExceptT ErrListTransactionsNoSuchWallet $ _readWallet wid
    case (mStart, mEnd) of
        (Just start, Just end) -> when (start > end) $ throwE $
            ErrListTransactionsStartTimeLaterThanEndTime $
                ErrStartTimeLaterThanEndTime start end
        _ -> pure ()
    let tip = currentTip w ^. #slotId
    let order = maybe defaultTxSortOrder id mOrder

...

than:

_listTransactions wid mStart mEnd mOrder = do
    (w, _) <- withExceptT ErrListTransactionsNoSuchWallet $ _readWallet wid
    case (mStart, mEnd) of
        (Just start, Just end) -> when (start > end) $ throwE $
            ErrListTransactionsStartTimeLaterThanEndTime $
                ErrStartTimeLaterThanEndTime start end
        _ -> pure ()
    let tip = currentTip w ^. #slotId
    let order = maybe Descending id mOrder

With the latter, I find myself thinking: "Where did this Descending value come from?" Of course, we could add a comment, but in my opinion creating a constant with a suitable name to describe the intent is much clearer.

If the default is defined by the API, then one option is for the API server to refer to this named constant, and then explicitly pass the value down to subordinate functions in the lower layers (so that they don't have to refer to it).

[rvl]

Probably the easiest way to test db sorting of the transactions is by modifying ReadTxHistory in the state machine tests. Then it will make test cases with random combinations for you.
edit: oh, I see this is based on another PR which does that.

[KtorZ]

@Anviking

I have removed defaultTxSortOrder

Why exactly 🤔 ?

[Anviking]

Why exactly

Because most use-cases didn't care about having a default sort order, they only needed an arbitrary sort order. (#588 (comment))

I'd much prefer to see (not necessarily in the same module):

@jonathanknowles 👍 I have now removed the value from WalletLayer.listTransactions and defined it in the where-clause of the Api listTransactions handle.

[Anviking]

The notion of a default sort order for listTransactions now only lives here in the Api. WalletLayer.listTransactions doesn't have a default sort-order anymore.

addressed 👍