Add listTransactions in WalletLayer #497

Anviking · 2019-07-02T22:49:42Z

Issue Number

#465

Overview

I added listTransactions :: WalletId -> ExceptT ErrNoSuchWallet IO [(Tx t, TxMeta)] sorted descending on slotId.

Comments

In-memory sorting

lib/core/src/Cardano/Wallet.hs

Anviking · 2019-07-02T23:15:11Z

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

+        txs `shouldBe` L.sortBy (comparing $ Down . slotId . snd) txs
+        case txs of
+            (a:b:_) ->
+                ((slotId . snd $ a) >= (slotId . snd $ b)) `shouldBe` True


I'm not sure about this.

I like the >= because it's easy to comprehend, but it is just comparing the first two elements, so we need the re-implementation of the sorting on L354.

suggestion:

isSortedDesc = all (uncurry (<=)) . (\xs -> zip (drop 1 xs) xs) $ txs if isSortedDesc then return () else $ expectationFailure $ "..."

I like the txs == sort txs form of assertion.

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

KtorZ · 2019-07-03T09:23:29Z

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

+        txs `shouldBe` L.sortBy (comparing $ Down . slotId . snd) txs
+        case txs of
+            (a:b:_) ->
+                ((slotId . snd $ a) >= (slotId . snd $ b)) `shouldBe` True


suggestion:

isSortedDesc = all (uncurry (<=)) . (\xs -> zip (drop 1 xs) xs) $ txs if isSortedDesc then return () else $ expectationFailure $ "..."

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

rvl

Good stuff.

I would like to move sorting down to the DB, but the test case can stay in the Wallet layer.

rvl · 2019-07-03T09:23:35Z

lib/core/src/Cardano/Wallet.hs

+    _listTransactions wid = liftIO $
+        L.sortOn slotIdDescending
+            . map snd
+            . Map.toList <$> DB.readTxHistory db (PrimaryKey wid)


I think sorting is very much a job for the DBLayer.

Can we

Add a comment on DBLayer.readTxHistory saying that the Map short be sorted descending by slot.

Adjust the DBLayer backends accordingly.

@KtorZ (asking here instead) you said

Either we gotta change the type (which I think isn't really a good idea), or do the sorting in-memory in the handler itself.

Could you elaborate on why you didn't think it was a good idea?

I'm still familiarising myself with the Sqlite schema (+ sqlite). Could we not either:

Just make txHistoryFromEntity return a sorted [(W.Tx t, W.TxMeta)] given a sorted [W.TxMeta] instead of returning a Map? It seems like we currently only use readTxHistory in WalletLayer._listAddresses.

Use only one select (from three tables at once tx_meta, tx_in and tx_out)… somehow? (not sure if this is possible)

why you didn't think it was a good idea?

Mainly because of the rippling effect and the amount of changes it involves in the code-base. If we've to go through this, then, we'll have too :)

rvl · 2019-07-03T09:29:54Z

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

+        txs `shouldBe` L.sortBy (comparing $ Down . slotId . snd) txs
+        case txs of
+            (a:b:_) ->
+                ((slotId . snd $ a) >= (slotId . snd $ b)) `shouldBe` True


I like the txs == sort txs form of assertion.

KtorZ · 2019-07-08T10:43:55Z

@Anviking What's the status with this one?

Anviking · 2019-07-08T11:17:13Z

@KtorZ

My plan was to change the type of readTxHistory from Map to (sorted) List in this PR.

Although I'd be happy if you weighed in because I'm not sure this is strictly necessary, even though it would make sense:
https://input-output-rnd.slack.com/archives/GBT05825V/p1562575888008900?thread_ts=1562573912.007600&cid=GBT05825V

Not completely final implementation with ugly (txid, tx, meta)-triples

If you disagree this could be merged.

KtorZ · 2019-07-08T11:33:42Z

My plan was to change the type of readTxHistory from Map to (sorted) List in this PR.

Ok. What's missing from the branch you copy/pasted at the end of your message 🤔 ?

Anviking · 2019-07-08T11:48:06Z

@KtorZ
Mainly getting tests compiling. ~~Might be just one left~~. Had lots of trouble with https://github.com/input-output-hk/cardano-wallet/blob/6e45a373888854808e60bad37b5618262adb4a0d/lib/core/test/unit/Cardano/Wallet/DB/StateMachine.hs#L235-L250 but might be beginning to understand it 🤔

(Then one could ask one selves if (txid, tx, meta) is the best way, as it calls for some amount of pattern-matching and conversions. Maybe (txid, (tx, meta)) or some new record would be better.)

Anviking · 2019-07-09T06:45:17Z

I got the tests compiling in a brute-force way by converting back and forth between the Map and List-representations. Failing though 🤔

Wild guesses

Maybe using a sorted list in the property tests / state machine tests could be problematic somehow (because of sorting, non-uniqueness).
I still made some mistake adapting the SM code like here

I will try

Stop banging my head on this for now
Otherwise I'd try using Map (Hash "Tx") (Tx, TxMeta) in the StateMachine's DBLayer model data Model = M, but [(Hash "Tx", Tx, TxMeta)] in the actual DBLayer, although it would be only a wild guess.

KtorZ · 2019-07-09T08:51:55Z

@Anviking May you push somewhere whatever you have available for me to have a look 🤔 ?

Anviking · 2019-07-09T08:55:05Z

@KtorZ https://github.com/input-output-hk/cardano-wallet/compare/anviking/465/change-txHistory-type

It is on this branch. Maybe it should be a separate pr to this after all?

KtorZ · 2019-07-09T09:00:03Z

Ah. Will look at this branch then.

(PS: you shouldn't put ?expand=1 in your link. This tells github to open a PR instead of displaying the diff...)

rvl

The test case looks good.

rvl · 2019-07-11T03:48:35Z

lib/core/src/Cardano/Wallet.hs

+    _listTransactions
+        :: WalletId
+        -> ExceptT ErrNoSuchWallet IO [(Tx t, TxMeta)]
+    _listTransactions wid = liftIO $


We can use #524 to handle sorting, then change this to:

Suggested change

_listTransactions wid = liftIO $

_listTransactions wid = liftIO $ map snd <$> DB.readTxHistory db (PrimaryKey wid)

Anviking · 2019-07-11T12:25:25Z

I'm thinking hooking this up with the api can go in a separate pr with some integration tests. merging

Anviking self-assigned this Jul 2, 2019

KtorZ mentioned this pull request Jul 2, 2019

Implement listTransactions endpoint (no filtering) #465

Closed

7 tasks

Anviking force-pushed the anviking/465/walletlayer-list-txs branch from f13bc57 to 50ba757 Compare July 2, 2019 22:50

Anviking commented Jul 2, 2019

View reviewed changes

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

Anviking force-pushed the anviking/465/walletlayer-list-txs branch from 50ba757 to 0215d82 Compare July 2, 2019 23:11

Anviking commented Jul 2, 2019

View reviewed changes

Anviking force-pushed the anviking/465/walletlayer-list-txs branch from 0215d82 to ba08044 Compare July 3, 2019 07:50

KtorZ reviewed Jul 3, 2019

View reviewed changes

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

rvl reviewed Jul 3, 2019

View reviewed changes

Anviking force-pushed the anviking/465/walletlayer-list-txs branch 3 times, most recently from bd241e1 to 79bbf29 Compare July 8, 2019 06:10

rvl approved these changes Jul 11, 2019

View reviewed changes

Anviking force-pushed the anviking/465/walletlayer-list-txs branch 2 times, most recently from 5779f93 to 87b5def Compare July 11, 2019 11:45

Add listTransactions in Cardano.Wallet

10f4381

Anviking force-pushed the anviking/465/walletlayer-list-txs branch from 87b5def to 10f4381 Compare July 11, 2019 11:56

Anviking merged commit f683a6d into master Jul 11, 2019

Anviking deleted the anviking/465/walletlayer-list-txs branch July 11, 2019 12:26

KtorZ added this to the Finalize 'Transactions' API endpoints milestone Jul 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add listTransactions in WalletLayer #497

Add listTransactions in WalletLayer #497

Anviking commented Jul 2, 2019 •

edited

Loading

Anviking Jul 2, 2019

KtorZ Jul 3, 2019

rvl Jul 3, 2019

KtorZ Jul 3, 2019

rvl left a comment

rvl Jul 3, 2019

Anviking Jul 4, 2019

KtorZ Jul 4, 2019

rvl Jul 3, 2019

KtorZ commented Jul 8, 2019

Anviking commented Jul 8, 2019

KtorZ commented Jul 8, 2019

Anviking commented Jul 8, 2019 •

edited

Loading

Anviking commented Jul 9, 2019

KtorZ commented Jul 9, 2019

Anviking commented Jul 9, 2019 •

edited

Loading

KtorZ commented Jul 9, 2019

rvl left a comment

rvl Jul 11, 2019

Anviking commented Jul 11, 2019

	_listTransactions wid = liftIO $
	_listTransactions wid = liftIO $ map snd <$> DB.readTxHistory db (PrimaryKey wid)

Add listTransactions in WalletLayer #497

Add listTransactions in WalletLayer #497

Conversation

Anviking commented Jul 2, 2019 • edited Loading

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

rvl left a 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

KtorZ commented Jul 8, 2019

Anviking commented Jul 8, 2019

KtorZ commented Jul 8, 2019

Anviking commented Jul 8, 2019 • edited Loading

Anviking commented Jul 9, 2019

KtorZ commented Jul 9, 2019

Anviking commented Jul 9, 2019 • edited Loading

KtorZ commented Jul 9, 2019

rvl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Anviking commented Jul 11, 2019

Anviking commented Jul 2, 2019 •

edited

Loading

Anviking commented Jul 8, 2019 •

edited

Loading

Anviking commented Jul 9, 2019 •

edited

Loading