Wallet Primitive Types #25

KtorZ · 2019-03-04T17:13:03Z

Issue Number

#20

Overview

I have added a Cardano.Wallet.Primitive containing primitive wallet types and operations
I have translated properties from Lemma 2.1 in the wallet spec into property tests

Comments

rvl

Looks good so far.

rvl · 2019-03-05T05:56:08Z

src/Cardano/Wallet/Primitive.hs

+        :: !BlockHeader
+    , transactions
+        :: !(Set Tx)
+    } deriving (Show, Generic)


Eq would be handy for testing.

rvl · 2019-03-05T05:58:54Z

src/Cardano/Wallet/Primitive.hs

+        -- ^ Order of outputs matter in the transaction representations. Outputs
+        -- are used as inputs for next transactions which refer to them using
+        -- their indexes. It matters also for serialization.
+    } deriving (Show, Generic)


Ord would be needed to insert transactions into a Set.

Anviking

Nice, lgtm! Two minor things (see comments)

Mention that Coin max bound is in Lovelace?
Implicit exports in spec for ghci?

then a few vaguer thoughts and questions (feel free to brush them off)

Anviking · 2019-03-05T11:04:40Z

src/Cardano/Wallet/Primitive.hs

+
+    It doesn't contain any particular business-logic code, but define a few
+    primitive operations on Wallet core types as well.
+-}


I knew you would like it 😉
That's actually a good habit we should take. Thanks for pointing that out. Maybe it should be part of our coding standard actually 🤔 ?

Maybe it should be part of our coding standard actually

I'd like that! 😍

Then make a proposal 😶

I thought you might say that 👍

test/unit/Cardano/Wallet/PrimitiveSpec.hs

Anviking · 2019-03-05T11:09:30Z

test/unit/Cardano/Wallet/PrimitiveSpec.hs

@@ -0,0 +1,179 @@
+{-# LANGUAGE DataKinds #-}
+{-# LANGUAGE FlexibleInstances #-}
+{-# OPTIONS_GHC -fno-warn-orphans #-}


Is this "for now"/"for this" or are you suggesting most Test modules define their own Arbitrary instances?

Pretty-much yes. Ideally, we probably want something a bit more like Hedgehog were we define generators explicitly, and not via type-class. Because generators are kinda tailored to one or more tests. For instance, for those primitive, we do not care much about the shape of an address or a block header hash, that wouldn't be true of course if we were to test how addresses are serialized / unserialized!

Anviking · 2019-03-05T11:13:53Z

src/Cardano/Wallet/Primitive.hs

+    dom (UTxO utxo) = Map.keysSet utxo
+
+-- ins⋪ u
+excluding :: UTxO -> Set TxIn ->  UTxO


Shoot me, but have we considered using the unicode symbols for matching the spec?

Haha. I thought of it yes. And, that's code we will probably not use much so, that could be "nice and pretty". In practice, we probably don't want that because it's a pain in the ass to write such symbols 😂 ...

Anviking · 2019-03-05T13:12:08Z

src/Cardano/Wallet/Primitive.hs

+
+instance Bounded Coin where
+    minBound = Coin 0
+    maxBound = Coin 45000000000000000


If this is in Lovelace it would be nice to mention. Either here or for the Coin type.

Anviking · 2019-03-05T13:37:38Z

test/unit/Cardano/Wallet/PrimitiveSpec.hs

+
+prop_2_1_1 :: (Set TxIn, UTxO) -> Property
+prop_2_1_1 (ins, u) =
+    cover 50 cond "dom u ⋂ ins ≠ ∅" (property prop)


Properties look really nice to me!
At first I found it a bit hard to parse with the important logic in the prop where clause.

I see that it's inconvenient to factor out the condition from each property though.

Mayyyybe:

coverIntersection :: Ord a => String -> Set a -> Set a -> Bool -> Property coverIntersection label a b prop = cover 50 (not $ Set.null $ a `Set.intersection` b) label (property prop) prop_2_1_1 :: (Set TxIn, UTxO) -> Property prop_2_1_1 (ins, u) = coverIntersection "dom u ⋂ ins ≠ ∅" (dom u) ins $ (u `restrictedBy` ins) `isSubsetOf` u

although a bit iffy, so prob not

Though the condition and coverage threshold isn't the same, albeit identical for several. We could go and have a couple of general helpers but I think it would just mess up readability in the end, and props are already rather self contained :/ ...

Anviking · 2019-03-05T14:18:45Z

test/unit/Cardano/Wallet/PrimitiveSpec.hs

+            ===
+        (u `restrictedBy` ins) <> (v `restrictedBy` ins)
+
+prop_2_1_5 :: (Set TxIn, UTxO, UTxO) -> Property


Curios: why a tuple and not two args? Just style?

No. QuickCheck is better at shrinking and generating arbitrary tuples than it is at generating multiple args. Hence in general, you probably always want to have a single arg that is a tuple.

[3] Adding ticking function test and downloading block logic [3] Block syncer working ok plus additional tests [3] Cleaning the code [3] cabal fix [3] Complete refactoring to the review [3] Final refactoring [3] cabal fix [3] hlint and weeding [3] add killing thread at the end of test [3] Replace IORef with MVar refactoring #1 | inline intermediate functions to see more clearly + remove debug console prints refactoring #2 | review indentation of 'where' clause refactoring #3 | inline loop and remove test intrumentation from test logic refactoring #4 | define generator for ticking args in a declarative manner refactoring #5 | purify tickingFunctionTest and make it a monadic property refactoring #6 | review naming in Arbitrary TickingArgs refactoring #7 | use guards in mkConsecutiveTestBlocks refactoring #8 | define single block generator from previous block refactoring #9 | use 'fromPreviousBlock' and start loop with an already initialize list refactoring #10 | purify mkConsecutiveTestBlocks by defining a test hash function refactoring #11 | switch argument positions in mkConsecutiveTestBlocks refactoring #12 | replace loop with built-in list 'iterate' refactoring #13 | define Arbitrary instance for creating consecutive blocks refactoring #14 | replace mkConsecutiveBlocks with a property parameter refactoring #15 | use 'newMVar' instead or 'newEmptyMVar' + 'putMVar' refactoring #16 | remove unecessary IO in 'writeToIORefAction' refactoring #17 | replace takeMVar + putMVar with modifyMVar refactoring #18 | review naming for 'writeToIORefAction' --> 'reader' refactoring #19 | use a 'Map.lookup' instead of 'List.filter' + pattern-match refactoring #20 | remove 'BlocksConsumed' wrapper refactoring #21 | generalize reader with polymorphic parametrism refactoring #22 | review pushNextBlocks indentation refactoring #23 | group case pattern matches using tuple refactoring #24 | remove 'Hash BlockHeader' from the block to inject refactoring #25 | use synchronization lock instead of computed times refactoring #26 | Move generation of duplicated blocks onto 'Arbitrary Blocks' refactoring #27 | remove 'chunkSizes' in a favor of inline random selection refactoring #28 | remove 'DeliveryMode' in favor of the most general case refactoring #29 | cleanup wrapper types refactoring #30 | generalize pushNextBlocks with parametric polymorphism refactoring #31 | rename pushNextBlocks to 'writer' refactoring #32 | define reader on Block instead of BlockHeaderHash refactoring #33 | replace old reader with reader' refactoring #34 | move creation of writer MVar inside writer action refactoring #35 | remove header hash from 'Blocks' refactoring #36 | rename 'consecutiveBlocks' into 'blocks' refactoring #37 | re-organize module to separate effectful logic from declarations refactoring #38 | move waiting logic into dedicated function refactoring #39 | move 'done' and 'readerChan' initialization into reader and writer refactoring #40 | use Millisecond instead of Second for shorter tests refactoring #41 | Move creation of blocks from writer to 'Arbitrary Blocks' [3] fix line width [3] aligning the code with other code changes

[3] Adding ticking function test and downloading block logic [3] Block syncer working ok plus additional tests [3] Cleaning the code [3] cabal fix [3] Complete refactoring to the review [3] Final refactoring [3] cabal fix [3] hlint and weeding [3] add killing thread at the end of test [3] Replace IORef with MVar refactoring #1 | inline intermediate functions to see more clearly + remove debug console prints refactoring #2 | review indentation of 'where' clause refactoring #3 | inline loop and remove test intrumentation from test logic refactoring #4 | define generator for ticking args in a declarative manner refactoring #5 | purify tickingFunctionTest and make it a monadic property refactoring #6 | review naming in Arbitrary TickingArgs refactoring #7 | use guards in mkConsecutiveTestBlocks refactoring #8 | define single block generator from previous block refactoring #9 | use 'fromPreviousBlock' and start loop with an already initialize list refactoring #10 | purify mkConsecutiveTestBlocks by defining a test hash function refactoring #11 | switch argument positions in mkConsecutiveTestBlocks refactoring #12 | replace loop with built-in list 'iterate' refactoring #13 | define Arbitrary instance for creating consecutive blocks refactoring #14 | replace mkConsecutiveBlocks with a property parameter refactoring #15 | use 'newMVar' instead or 'newEmptyMVar' + 'putMVar' refactoring #16 | remove unecessary IO in 'writeToIORefAction' refactoring #17 | replace takeMVar + putMVar with modifyMVar refactoring #18 | review naming for 'writeToIORefAction' --> 'reader' refactoring #19 | use a 'Map.lookup' instead of 'List.filter' + pattern-match refactoring #20 | remove 'BlocksConsumed' wrapper refactoring #21 | generalize reader with polymorphic parametrism refactoring #22 | review pushNextBlocks indentation refactoring #23 | group case pattern matches using tuple refactoring #24 | remove 'Hash BlockHeader' from the block to inject refactoring #25 | use synchronization lock instead of computed times refactoring #26 | Move generation of duplicated blocks onto 'Arbitrary Blocks' refactoring #27 | remove 'chunkSizes' in a favor of inline random selection refactoring #28 | remove 'DeliveryMode' in favor of the most general case refactoring #29 | cleanup wrapper types refactoring #30 | generalize pushNextBlocks with parametric polymorphism refactoring #31 | rename pushNextBlocks to 'writer' refactoring #32 | define reader on Block instead of BlockHeaderHash refactoring #33 | replace old reader with reader' refactoring #34 | move creation of writer MVar inside writer action refactoring #35 | remove header hash from 'Blocks' refactoring #36 | rename 'consecutiveBlocks' into 'blocks' refactoring #37 | re-organize module to separate effectful logic from declarations refactoring #38 | move waiting logic into dedicated function refactoring #39 | move 'done' and 'readerChan' initialization into reader and writer refactoring #40 | use Millisecond instead of Second for shorter tests refactoring #41 | Move creation of blocks from writer to 'Arbitrary Blocks' [3] fix line width [3] aligning the code with other code changes [3] hlint suggestion

[3] Adding ticking function test and downloading block logic [3] Block syncer working ok plus additional tests [3] Cleaning the code [3] cabal fix [3] Complete refactoring to the review [3] Final refactoring [3] cabal fix [3] hlint and weeding [3] add killing thread at the end of test [3] Replace IORef with MVar refactoring #1 | inline intermediate functions to see more clearly + remove debug console prints refactoring #2 | review indentation of 'where' clause refactoring #3 | inline loop and remove test intrumentation from test logic refactoring #4 | define generator for ticking args in a declarative manner refactoring #5 | purify tickingFunctionTest and make it a monadic property refactoring #6 | review naming in Arbitrary TickingArgs refactoring #7 | use guards in mkConsecutiveTestBlocks refactoring #8 | define single block generator from previous block refactoring #9 | use 'fromPreviousBlock' and start loop with an already initialize list refactoring #10 | purify mkConsecutiveTestBlocks by defining a test hash function refactoring #11 | switch argument positions in mkConsecutiveTestBlocks refactoring #12 | replace loop with built-in list 'iterate' refactoring #13 | define Arbitrary instance for creating consecutive blocks refactoring #14 | replace mkConsecutiveBlocks with a property parameter refactoring #15 | use 'newMVar' instead or 'newEmptyMVar' + 'putMVar' refactoring #16 | remove unecessary IO in 'writeToIORefAction' refactoring #17 | replace takeMVar + putMVar with modifyMVar refactoring #18 | review naming for 'writeToIORefAction' --> 'reader' refactoring #19 | use a 'Map.lookup' instead of 'List.filter' + pattern-match refactoring #20 | remove 'BlocksConsumed' wrapper refactoring #21 | generalize reader with polymorphic parametrism refactoring #22 | review pushNextBlocks indentation refactoring #23 | group case pattern matches using tuple refactoring #24 | remove 'Hash BlockHeader' from the block to inject refactoring #25 | use synchronization lock instead of computed times refactoring #26 | Move generation of duplicated blocks onto 'Arbitrary Blocks' refactoring #27 | remove 'chunkSizes' in a favor of inline random selection refactoring #28 | remove 'DeliveryMode' in favor of the most general case refactoring #29 | cleanup wrapper types refactoring #30 | generalize pushNextBlocks with parametric polymorphism refactoring #31 | rename pushNextBlocks to 'writer' refactoring #32 | define reader on Block instead of BlockHeaderHash refactoring #33 | replace old reader with reader' refactoring #34 | move creation of writer MVar inside writer action refactoring #35 | remove header hash from 'Blocks' refactoring #36 | rename 'consecutiveBlocks' into 'blocks' refactoring #37 | re-organize module to separate effectful logic from declarations refactoring #38 | move waiting logic into dedicated function refactoring #39 | move 'done' and 'readerChan' initialization into reader and writer refactoring #40 | use Millisecond instead of Second for shorter tests refactoring #41 | Move creation of blocks from writer to 'Arbitrary Blocks' [3] fix line width [3] aligning the code with other code changes [3] hlint suggestion [3] add .weeder.yaml to omit duplicateMaybes and groups to be announce during weeder execution [3] undo weeder ignore file plus remove pragma and unnecessary exports

[3] Adding ticking function test and downloading block logic [3] Block syncer working ok plus additional tests [3] Cleaning the code [3] cabal fix [3] Complete refactoring to the review [3] Final refactoring [3] cabal fix [3] hlint and weeding [3] add killing thread at the end of test [3] Replace IORef with MVar refactoring #1 | inline intermediate functions to see more clearly + remove debug console prints refactoring #2 | review indentation of 'where' clause refactoring #3 | inline loop and remove test intrumentation from test logic refactoring #4 | define generator for ticking args in a declarative manner refactoring #5 | purify tickingFunctionTest and make it a monadic property refactoring #6 | review naming in Arbitrary TickingArgs refactoring #7 | use guards in mkConsecutiveTestBlocks refactoring #8 | define single block generator from previous block refactoring #9 | use 'fromPreviousBlock' and start loop with an already initialize list refactoring #10 | purify mkConsecutiveTestBlocks by defining a test hash function refactoring #11 | switch argument positions in mkConsecutiveTestBlocks refactoring #12 | replace loop with built-in list 'iterate' refactoring #13 | define Arbitrary instance for creating consecutive blocks refactoring #14 | replace mkConsecutiveBlocks with a property parameter refactoring #15 | use 'newMVar' instead or 'newEmptyMVar' + 'putMVar' refactoring #16 | remove unecessary IO in 'writeToIORefAction' refactoring #17 | replace takeMVar + putMVar with modifyMVar refactoring #18 | review naming for 'writeToIORefAction' --> 'reader' refactoring #19 | use a 'Map.lookup' instead of 'List.filter' + pattern-match refactoring #20 | remove 'BlocksConsumed' wrapper refactoring #21 | generalize reader with polymorphic parametrism refactoring #22 | review pushNextBlocks indentation refactoring #23 | group case pattern matches using tuple refactoring #24 | remove 'Hash BlockHeader' from the block to inject refactoring #25 | use synchronization lock instead of computed times refactoring #26 | Move generation of duplicated blocks onto 'Arbitrary Blocks' refactoring #27 | remove 'chunkSizes' in a favor of inline random selection refactoring #28 | remove 'DeliveryMode' in favor of the most general case refactoring #29 | cleanup wrapper types refactoring #30 | generalize pushNextBlocks with parametric polymorphism refactoring #31 | rename pushNextBlocks to 'writer' refactoring #32 | define reader on Block instead of BlockHeaderHash refactoring #33 | replace old reader with reader' refactoring #34 | move creation of writer MVar inside writer action refactoring #35 | remove header hash from 'Blocks' refactoring #36 | rename 'consecutiveBlocks' into 'blocks' refactoring #37 | re-organize module to separate effectful logic from declarations refactoring #38 | move waiting logic into dedicated function refactoring #39 | move 'done' and 'readerChan' initialization into reader and writer refactoring #40 | use Millisecond instead of Second for shorter tests refactoring #41 | Move creation of blocks from writer to 'Arbitrary Blocks' [3] fix line width [3] aligning the code with other code changes [3] hlint suggestion [3] add .weeder.yaml to omit duplicateMaybes and groups to be announce during weeder execution [3] undo weeder ignore file plus remove pragma and unnecessary exports [3] remove not needed imports

KtorZ self-assigned this Mar 4, 2019

KtorZ requested a review from Anviking March 4, 2019 17:13

KtorZ force-pushed the KtorZ/#20/wallet-primitive-types branch from be17200 to 81427c4 Compare March 4, 2019 17:25

rvl mentioned this pull request Mar 5, 2019

Add block decoder #28

Merged

4 tasks

KtorZ mentioned this pull request Mar 5, 2019

Define type primitives needed to represent a wallet #20

Closed

rvl reviewed Mar 5, 2019

View reviewed changes

KtorZ force-pushed the KtorZ/#20/wallet-primitive-types branch from 81427c4 to fa6fbd6 Compare March 5, 2019 08:24

Anviking approved these changes Mar 5, 2019

View reviewed changes

KtorZ added 3 commits March 5, 2019 16:25

add primitive types for a wallet

99fc7a5

remove temporary 'Cardano.Wallet' placeholder file

77a7f58

add property tests from Lemma 2.1 about UTxO operations

0026c45

KtorZ force-pushed the KtorZ/#20/wallet-primitive-types branch from fa6fbd6 to 0026c45 Compare March 5, 2019 15:26

KtorZ merged commit 4d1ef01 into master Mar 5, 2019

KtorZ deleted the KtorZ/#20/wallet-primitive-types branch March 5, 2019 15:33

rvl mentioned this pull request Mar 6, 2019

Pack file decoder #32

Merged

KtorZ added a commit that referenced this pull request Mar 11, 2019

refactoring #25 | use synchronization lock instead of computed times

033898f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wallet Primitive Types #25

Wallet Primitive Types #25

KtorZ commented Mar 4, 2019

rvl left a comment

rvl Mar 5, 2019

rvl Mar 5, 2019

Anviking left a comment •

edited

Loading

Anviking Mar 5, 2019

KtorZ Mar 5, 2019

Anviking Mar 5, 2019

KtorZ Mar 5, 2019

Anviking Mar 5, 2019

Anviking Mar 5, 2019

KtorZ Mar 5, 2019

Anviking Mar 5, 2019

KtorZ Mar 5, 2019

Anviking Mar 5, 2019

KtorZ Mar 5, 2019

Anviking Mar 5, 2019

KtorZ Mar 5, 2019

Anviking Mar 5, 2019

KtorZ Mar 5, 2019

Wallet Primitive Types #25

Wallet Primitive Types #25

Conversation

KtorZ commented Mar 4, 2019

Issue Number

Overview

Comments

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

Anviking left a comment • edited Loading

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

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

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 left a comment •

edited

Loading