[CO-347] Wallet's UTXO histogram view by paweljakubas · Pull Request #3402 · input-output-hk/cardano-sl

paweljakubas · 2018-08-14T23:14:45Z

Description

Adding Utxo statistics endpoint to API V1 with new data layer support.
Idea was to deliver solution that can in one pass calculate a number of statistics. Besides histogram I added as an example total number of stakes in a given wallet calculation - which in the last version can go out. The solution relies on Gabriel Gonzalez foldl library.
Alternatively, I was also considering the approach adopted:
http://tech.frontrowed.com/2017/09/22/aggregations/

As an example statistics look like:
data UtxoStatistics = UtxoStatistics { theHistogram :: ![HistogramBar] , theAllStakes :: !Word64 }

I added full functionality for Handler and stub for LegacyHandler as it is about to be removed soon. I added integration test in WalletSpec, and also commented one in TransactionsSpec (to be uncommented and checked when Client.hs usage of LegacyHandler will be replaced with Handler).

Moreover, I intentionally used [(Account, Utxo)] in WalletLayer. Maybe in the future installation one can extend this endpoint and filter using account info, etc.

Linked issue

https://iohk.myjetbrains.com/youtrack/issue/CO-347

Type of change

🐞 Bug fix (non-breaking change which fixes an issue)
🛠 New feature (non-breaking change which adds functionality)
⚠️ Breaking change (fix or feature that would cause existing functionality to change)
🏭 Refactoring that does not change existing functionality but does improve things like code readability, structure etc
🔨 New or improved tests for existing code
⛑ git-flow chore (backport, hotfix, etc)

Developer checklist

I have read the style guide document, and my code follows the code style of this project.
If my code deals with exceptions, it follows the guidelines.
I have updated any documentation accordingly, if needed. Documentation changes can be reflected in opening a PR on cardanodocs.com, amending the inline Haddock comments, any relevant README file or one of the document listed in the docs directory.
CHANGELOG entry has been added and is linked to the correct PR on GitHub.

Testing checklist

I have added tests to cover my changes.
All new and existing tests passed.

QA Steps

To build:
./scripts/build/cardano-sl.sh wallet-new
To test:
stack test cardano-sl-wallet-new
nix-build release.nix -A tests.walletIntegration

Screenshots (if available)

This commit adds stubs to legacy and new-wallet, histogram and bar datatypes are accomodated, one-pass many-aggregates functionality is added

Kernel level implementation of WalletLayer is added, Legacy is not supported. The implementation is used in Handlers of V1 API. Refactoring from Integer to Word64 is done

Added integration test for Wallets and Transactions (here commented until migration to Handler happens). Added ToSchema instance needed to pass swagger integration tests. Decided to use [(Account, Utxo)] type in WalletLayer as it can be useful if the method is extended for account parameters. WalletLayers quickcheck required to add several instances as a consequence

edsko

LGTM. I didn't check the logic of the histogram itself, merely how it has been integrated with the wallet layer and the kernel, which looks good. Would appreciate it though if you changed the style of the implementation of the layer ever so slightly though to bring it in line with the rest (see comment below).

edsko · 2018-08-15T05:38:51Z

+                Right accountsIxSet ->
+                    let hdRootId = HD.HdRootId . InDb $ rootAddr
+                        hdAccountId accountIndex = HD.HdAccountId hdRootId (HD.HdAccountIx accountIndex)
+                    in Right ( map (\acc -> (acc, accountUtxo db (hdAccountId $ V1.accIndex acc) ) ) $ IxSet.toList accountsIxSet )


This uses the style of the wallet layer implementation as it was before I refactored it. Take a look at what it is now, the use of ExceptT can make a bit easier to write, and then you can also use from existing combinators such as fromRootId.

KtorZ · 2018-08-16T12:40:15Z


            void $ etxn `shouldPrism` _Left
+{-- Uncomment when new Client is Handler based rather than LegacyHandler based
+        it "posted transactions gives rise to nonempty Utxo histogram" $ do


Note that there's a xit to declare pending tests. This way, we can still have GHC to verify the code as the codebase evolve, and as soon as it's ready, we can change the xit to it and have the test executed.

KtorZ · 2018-08-16T12:50:39Z

@@ -0,0 +1,51 @@
+module Cardano.Wallet.API.V1.Handlers.Internal


Rather than Cardano/Wallet/API/V1/Handlers/Internal, I'd rather have a Cardano/Wallet/Types/UtxoStatistics module with those functions and the type defined within it, then re-exported from V1/Types.hs.

What do you think?

I like this

Moved types and functions to Cardano/Wallet/Types/UtxoStatistics.hs and reexport them in V1/Types.hs

KtorZ · 2018-08-16T13:05:22Z

+
+            eresp <- getUtxoStatistics wc (walId wallet)
+            utxoStatistics <- fmap wrData eresp `shouldPrism` _Right
+            let possibleBuckets = fmap show $ ( map (\toPower -> 10^toPower :: Word64) [(1::Word64)..16] ) ++ [45 * (10^(15::Word64))]


Why not re-use the generateBounds function ?

KtorZ · 2018-08-16T13:05:58Z

+populateBuckets bounds =
+    case bounds of
+        (x:_) -> L.Fold (addCountInBuckets x) (initalizeMap bounds) (fmap (\pair -> HistogramBarCount (T.pack $ show $ fst pair, snd pair) ) . MS.toList)
+        _ -> error "populateBuckets has to be powered with nonempty bounds"


Then, use a NonEmpty Word64 instead of a [Word 64] :D !

https://hackage.haskell.org/package/base-4.11.1.0/docs/Data-List-NonEmpty.html

KtorZ · 2018-08-16T13:13:24Z

+                 utxosCoinValuesForAllAccounts pairs =
+                     concatMap (\pair -> map extractValue (M.elems $ snd pair) ) pairs
+             in do
+                 return $ single (computeUtxoStatistics $ utxosCoinValuesForAllAccounts w)


This reads better with the do at the upper-most level:

Right w -> do let extractValue :: TxOutAux -> Word64 extractValue = getCoin . txOutValue . toaOut let utxosCoinValuesForAllAccounts :: [(Account, Utxo)] -> [Word64] utxosCoinValuesForAllAccounts pairs = concatMap (\pair -> map extractValue (M.elems $ snd pair) ) pairs return $ single (computeUtxoStatistics $ utxosCoinValuesForAllAccounts w)

Also, I'd rather move these functions inside the computeUtxoStatistics function. It will also make its signature a bit clearer. Rather than [Word64] -> UtxoStatistics it becomes NonEmpty (Account, Utxo) -> UtxoStatistics which in my opinion gives already a much clearer idea of what the function is doing and how to use it.

applied within do . As underscored above I moved everything to Cardano/Wallet/Types/UtxoStatistics.hs which is reexported in V1/Types.hs. I retained lets here as there is Account which means in order to avoid cyclic dependencies one solution would be to move everything related with Account to separate Cardano/Wallet/Types/Account.hs and reexport it in V1.Types.hs. I am not sure if we want this

KtorZ · 2018-08-16T13:23:07Z

+                            case floatingOrInteger val of
+                                Left (_ :: Double)        -> HistogramBarCount ("0", 0 :: Word64)
+                                Right integer             -> HistogramBarCount (key, integer)
+                        constructHistogram _ = HistogramBarCount ("0", 0 :: Word64)


Same remark as above, if it's not an positive integer, we probably want to fail explicitly

KtorZ · 2018-08-16T13:23:39Z

+                    & type_ .~ SwaggerNumber
+                    & minimum_ .~ Just 0
+                )
+            )


KtorZ · 2018-08-16T13:25:15Z

        , _pwlCreateAddress         = pwlCreateAddress
        , _pwlGetAddresses          = pwlGetAddresses

+        , _pwlGetUtxos              = error "Method not implemented for legacy handler"


I thought it was ^.^ ?

https://github.com/input-output-hk/cardano-sl/pull/3402/files#diff-b56e363cb80a182fee20d6f6dbccefe0R193

At this level I am retrieving [(Account,Utxo)] for a given Wallet. Then ad the level of endpoint handler I will turn it into Utxo statistics. Why, maybe we are to extend endpoint and introduce some Account fields filtering, etc.

KtorZ · 2018-08-16T13:27:05Z

+    deriving Eq
+
+instance Show GetUtxosError where
+    show = formatToString build


Not sure here, but do we actually use show to display our errors :s ?
Aren't we using their Buildable instances? If not, then I'd open a ticket to change that, and wouldn't define any custom instance for Show.

I think the Show instances get used in some places (and are required when throwing these as expections). We do the same in a few places. I think the correct solution is to have Show be auto-derived and then define displayException in terms of the Buildable instances, but typically displayException is not used in top-level exception handlers. Either way, I'd say open a separate ticket about this because this is not just this PR.

@edsko Yes, that's exactly what I meant and I am not expecting such refactoring to happen in this PR :/ ... It is rather bad if we indeed rely on show to display exceptions in the top-level handlers.

I think it would be fine to go directly for formatToString build in those handlers (since the actual purpose of the Buildable is to construct a user-friendly readable output) yet I don't mind having that exposed as a clear displayException 👍

KtorZ · 2018-08-16T13:27:48Z

+genTxIn = oneof
+    [ TxInUtxo <$> arbitrary <*> arbitrary
+    , TxInUnknown <$> choose (1, 255) <*> scale (min 150) arbitrary
+    ]


Why have this defined here as a separated function 🤔 ?

was reused in one of my earlier versions, now only in one place -> fixed

parsonsmatt · 2018-08-16T16:00:22Z

+            eresp <- getUtxoStatistics wc (walId wallet)
+            utxoStatistics <- fmap wrData eresp `shouldPrism` _Right
+            let possibleBuckets = fmap show $ ( map (\toPower -> 10^toPower :: Word64) [(1::Word64)..16] ) ++ [45 * (10^(15::Word64))]
+            let histogram = map (\ x -> curry HistogramBarCount x 0) possibleBuckets


style nit: no space between \ and the variable it introduces

parsonsmatt · 2018-08-16T16:26:48Z

@@ -0,0 +1,51 @@
+module Cardano.Wallet.API.V1.Handlers.Internal


I like this

parsonsmatt · 2018-08-16T16:27:06Z

+    )  where
+
+
+import           Prelude


The project uses Universum as the prelude instead of the stock Prelude.

parsonsmatt · 2018-08-16T16:32:43Z

+generateBounds :: BoundType -> [Word64]
+generateBounds bType =
+    case bType of
+        Log10 -> ( map (\toPower -> 10^toPower :: Word64) [(1::Word64)..16] ) ++ [45 * (10^(15::Word64))]


When I have a bunch of type signatures I usually like to write a "specialized identity function" to make it a little more natural:

asWord64 :: Word64 -> Word64 asWord64 = id

which makes this look like:

Log10 -> (map (\toPower -> asWord64 (10^toPower)) [1 .. asWord64 16]) ++ [45 * (10 ^ asWord64 15)]

Or, as the ambiguity appears to come from the polymorphic type of (^), you could define a specialized variant that removes the need for signatures:

(^!) :: Word64 -> Word64 -> Word64 (^!) = (^) (map (\toPower -> 10 ^! toPower) [1..16]) ++ [45 * (10 ^! 15)]

This can also be expressed as:

take 16 (iterate (*10) 10) ++ [45 * (10 ^! 15)]

and since * requires both operands to be the same type, you only need the type annotation once.

suggestion applied

parsonsmatt · 2018-08-16T16:36:50Z

+import           Cardano.Wallet.API.V1.Types
+
+import qualified Control.Foldl as L
+import qualified Data.Map.Strict as MS


I thought the codebase had a convention to do it with import ... as Map but it is actually split fairly evenly between as M and as Map.

parsonsmatt · 2018-08-16T17:04:38Z

+-- | (b) avg or std of stake in a bucket
+-- | (c) topN buckets
+-- | to name a few
+newtype HistogramBar = HistogramBarCount (Text, Word64) deriving (Show, Eq, Ord, Generic)


Instead of a newtype over a tuple, I'd prefer to see a data with record fields:

data HistogramBar = HistogramBar { bucketName :: !Text , bucketUpperBound :: !Word64 }

(,) as a type is usually lazier than what you want. This also allows you to provide better information on the type for generic serialization/deserialization and schemas, and it is more robust to refactoring/changes.

parsonsmatt · 2018-08-16T17:05:12Z

+-- | (a) sum of stakes in a bucket
+-- | (b) avg or std of stake in a bucket
+-- | (c) topN buckets
+-- | to name a few


Haskell's doc comments do not use multiple | characters for each line. IIRC this won't render correctly.

removed | from everywhere in the comments I introduced

parsonsmatt · 2018-08-16T17:06:56Z

+data UtxoStatistics = UtxoStatistics
+  { theHistogram :: ![HistogramBar]
+  , theAllStakes :: !Word64
+  } deriving (Show, Generic, Ord)


4 space indent

parsonsmatt · 2018-08-16T17:09:52Z

+                  , "allStakes" .= allStakes ]
+
+instance FromJSON UtxoStatistics where
+    parseJSON (Object v) =


This should be derived generically. There's nothing about the type here that requires it to be manually written.

Once the HistogramBar type is a record with proper serde instances, this should be derivable.

According to specification of the task endpoint expects to have:

"allStakes": 0, "histogram": { "10000000000000000": 0, "100000000000000": 0, "1000000000000": 0, "10000000000": 0, "100000000": 0, "1000000": 0, "10000": 0, "100": 0, "1000000000": 0, "100000000000": 0, "10": 0, "45000000000000000": 0, "1000": 0, "10000000000000": 0, "1000000000000000": 0, "100000": 0, "10000000": 0 } }

rather than

"allStakes": 0, "histogram": [ { "bucketName" : "10", "bucketUpperBound" : 0 }, ..... ] }

parsonsmatt · 2018-08-16T17:14:39Z

+                                     & at "10" ?~ (Inline $ mempty
+                                                          & type_ .~ SwaggerNumber
+                                                          & minimum_ .~ Just 0
+                                                  )


The repetition of the Inline schema should be factored out. You can do this with:

declareNamedSchema _ = do wordRef <- declareSchemaRef (Proxy :: Proxy Word64) pure $ ... mempty & at "10" ?~ wordRef

This also suggests a further condensation, where the hashmap is constructed from a list consisting of the relevant powers paired with the schemaref instead of manually enumerated.

paweljakubas · 2018-08-17T13:28:22Z

Thank you all for the reviews and suggestions. I have just made respective commit

parsonsmatt · 2018-08-17T19:29:47Z

            utxoStatistics <- fmap wrData eresp `shouldPrism` _Right
-            let possibleBuckets = fmap show $ ( map (\toPower -> 10^toPower :: Word64) [(1::Word64)..16] ) ++ [45 * (10^(15::Word64))]
-            let histogram = map (\ x -> curry HistogramBarCount x 0) possibleBuckets
+            let possibleBuckets = fmap show $ (generateBounds Log10)


parens or $ are unnecessary here; could be either fmap show (generateBounds Log10) or fmap show $ generateBounds Log10 or even show <$> generateBounds Log10

parsonsmatt · 2018-08-17T19:34:40Z

+    arbitrary =
+        let possibleBuckets = fmap show (generateBounds Log10)
+            possibleBars = NL.zipWith HistogramBarCount possibleBuckets (NL.fromList [0..])
+        in elements (NL.toList possibleBars)


I don't think this does what you want -- this makes it choose a bucket name and a constant number from the [0..] list. Eg, for every HistogramBarCount x y, the x will uniquely determine the y.

I think what you want instead is something like:

arbitrary = do name <- elements $ map show (generateBounds Log10) bound <- arbitrary pure (HistogramBarCount name bound)

This will pick a random upper bound and name from the list.

parsonsmatt · 2018-08-17T19:39:48Z

+                  , "allStakes" .= allStakes ]
+
+instance FromJSON UtxoStatistics where
+    parseJSON (Object v) =


It is better to use Data.Aeson.withObject :: String -> (Object -> Parser a) -> Parser a rather than pattern matching directly on the constructors. The String label is used for error messages, which is really helpful when debugging a parse failure.

So, replace:

parseJSON (Object v) = ... parseJSON _ = empty -- with: parseJSON = withObject "UtxoStatistics" $ ...

refactored as suggested

parsonsmatt · 2018-08-17T19:40:10Z

+instance FromJSON UtxoStatistics where
+    parseJSON (Object v) =
+        let histogramListM = case HMS.lookup "histogram" v of
+                Nothing   -> empty


empty fails without an error message. Try to avoid doing that.

refactored as suggested

parsonsmatt · 2018-08-17T19:49:17Z

+
+instance FromJSON UtxoStatistics where
+    parseJSON (Object v) =
+        let histogramListM = case HMS.lookup "histogram" v of


aeson has an operator (.:) :: FromJSON a => Object -> Text -> Parser a that does the lookup and reports an error message if it isn't present. So this can be:

parseJSON = withObject "UtxoStatistics" $ \o -> do histo <- o .: "histogram" :: Parser (HashMap Text Word64) let constructHistogram = uncurry HistogramBarCount pure $ map constructHistogram $ HMS.toList histo

Here we specify the type of the parser, but it's unnecessary, because GHC would figure out what it needs to return. By requesting a HashMap Text Word64, we will get the right error messages for invalid values in that type:

λ> eitherDecode "-123" :: Either String Word64 Left "Error in $: Word64 is either floating or will cause over or underflow: -123.0" λ> eitherDecode "12345678901234567890" :: Either String Word64 Right 12345678901234567890 λ> eitherDecode "123456789012345678901234567890" :: Either String Word64 Left "Error in $: Word64 is either floating or will cause over or underflow: 1.2345678901234567890123456789e29"

refactored as suggested

parsonsmatt · 2018-08-17T19:50:05Z

+                Just (Object bars) -> do
+                    let constructHistogram (key, Number val) =
+                            case floatingOrInteger val of
+                                Left (_ :: Double)        -> error "UtxoStatistics FromJson not integer"


In the Parser type, prefer fail to error as fail reports the error using Aeson's parsing machinery, while error bypasses all of that and blows up at runtime. It's a difference between decode :: FromJSON a => ByteString -> Maybe a returning a Nothing and blowing up.

refactored as suggested

parsonsmatt · 2018-08-17T19:52:51Z

-    arbitrary = genTxIn
+    arbitrary = oneof
+        [ TxInUtxo <$> arbitrary <*> arbitrary
+        , TxInUnknown <$> choose (1, 255) <*> scale (min 150) arbitrary


I'm not sure this is what you want to do. This will set the size parameter to QuickCheck generation to be the minimum of what it already is or 150. If you want to restrict the actual values, then you'd want suchThat:

arbitrary `suchThat` (<= 150)

Would generate valid values up to 150.

Here I trusted what Core people used in core/test/Test/Pos/Core/Arbitrary/Txp.hs . Which is of course not a proof that it is correct. I will contact Eric concerning this. Maybe it will give rise to a new ticket and I inform him to affect also this instance

paweljakubas · 2018-08-18T09:09:31Z

Correction commit done.

I had previously exactly these linker errors in a number of checks:
#3432

…Utxo statistics

KtorZ · 2018-08-18T20:50:39Z

+        histogramBinNumCond histo = (length $ HMS.keys histo) <= 0
+        validateKeys = any (\key -> notElem key $  map show (NL.toList $ generateBounds Log10) )
+        histogramKeysCond = validateKeys . HMS.keys
+        validateVals = any (< 0)


<= 0 ? Are we fine with zero values? Or are we interested to catch any non-positive value ?

we are fine with zero values as bar represent count

I was confused with the names at first (bounds vs values) from the constructor. As a matter of fact, we are totally fine with empty buckets 👍

KtorZ · 2018-08-18T20:53:18Z

+        histogramKeysCond = validateKeys . HMS.keys
+        validateVals = any (< 0)
+        histogramValsCond = validateVals . HMS.elems
+        allStakesCond = (< 0)


Interesting way of doing it. For those, I usually leverage the Monad instance of Either and the when combinator. This way, we avoid the hassle and overhead of defining extra identifiers. I believe it reads a bit better:

validateUtxoStatistics :: HashMap Text Word64 -> Word64 -> Either UtxoStatisticsError (HashMap Text Word64, Word64) validateUtxoStatistics histogram allStakes = do let (keys, elems) = (HMS.keys histogram, HMS.elems histogram) let acceptedKeys = show <$> (toList $ generateBounds Log10) when (length keys <= 0) $ Left ErrHistogramEmpty when (any (flip notElem acceptedKeys) keys) $ Left ErrHistogramNamesInvalid when (any (< 0) elems) $ Left ErrHistogramUpperBoundNegative when (allStakes < 0) $ Left ErrAllStakesNegative Right (histogram, allStakes)

KtorZ · 2018-08-18T21:13:36Z

+instance Arbitrary HistogramBar where
+    arbitrary = do
+        possiblenames <- elements $ map show (NL.toList $ generateBounds Log10)
+        bound <- arbitrary


This one isn't entirely arbitrary, it is suchThat (\n -> n <= upperBound)

Sorry :s .. I probably induced you to use suchThat here. That's wasn't my best choice of words / example ...

KtorZ · 2018-08-18T21:14:55Z

+data HistogramBar = HistogramBarCount
+    { bucketName       :: !Text
+    , bucketUpperBound :: !Word64
+    } deriving (Show, Eq, Ord, Generic)


There's a confusion in the field names here 🙃 , took me a short while to figure it out from the rest of the code.
bucketUpperBound actually refers to the value / number of stakes whereas bucketName is the actual upper bound.

Hence the following question: what is the need for the bound (i.e. bucketName here) to be a Text? Why isn't this a Word64, especially when it is generated using Word64 values and converted to Text using show. If it's about using it as a key in the JSON representation, having a Word64 here shouldn't be much of a problem!

you are right, I made a change

KtorZ · 2018-08-18T21:15:26Z

+module Cardano.Wallet.Types.UtxoStatistics
+    ( computeUtxoStatistics
+    , UtxoStatistics (..)
+    , HistogramBar (..)


Not sure we want to expose the internals to the outside world. I'd be in favor of opaque types with smart-constructors 👍

KtorZ · 2018-08-18T21:23:29Z

+instance ToJSON UtxoStatistics where
+    toJSON (UtxoStatistics bars allStakes) =
+        let histogramObject = Object . HMS.fromList . map extractBarKey
+            extractBarKey (HistogramBarCount bound stake) = bound .= stake


That's what lead to the conclusions about the actual meaning of bucketUpperBound and bucketName in the comments above.

KtorZ · 2018-08-18T21:25:20Z

+
+data UtxoStatistics = UtxoStatistics
+    { theHistogram :: ![HistogramBar]
+    , theAllStakes :: !Word64


Can't allStakes be derived from the list of histogram already? I mean, it's a nice-to-have in the API response, that's for sure, but I think here we better have a

allStake :: UtxoStatistics -> Word64

GHC-runtime is rather good at caching these kind of results anyway. And we only need it when serializing to JSON right?

Thinking about it, we can't actually compute it from the list of bars indeed, because we only have the buckets there so it'll only be an approximation of what's in the bucket. Though, I believe this is preferable whatsoever to having a allStakes :: Word64 next to the histogram.

let's say:

approxTotalStakes :: UtxoStatistics -> Word64

The argument behind this is that, having allStakes in the data-structure means that we have to maintain and control some sort of invariant since there could be a discrepancy between the list of bars and the total number of stakes. That discrepancy can't realistically come from us but could come from external UtxoStatistics we could parse through JSON, which is also unlikely.

Maybe I am being too picky here.

when computing histogram, in the same traverse I compute allStakes. I can add many more other statistics here in the future. And in one traverse it is going to be computed. Usually there is no easy (nonapproximative) way of going from one aggregation to other

Yes. I am just afraid about parsing UtxoStatistics from an external source but I guess this will never happen in the end 👍

KtorZ · 2018-08-18T21:30:47Z

+        histogramKeysCond = validateKeys . HMS.keys
+        validateVals = any (< 0)
+        histogramValsCond = validateVals . HMS.elems
+        allStakesCond = (< 0)


Interesting way of writing this ^.^
When dealing with Either like this, I usually go for the Monad approach and leverage the when combinator. It avoids all the hassle of defining extra identifiers (making sure variables don't conflicts etc..) and is, to my opinion, a bit more readable:

validateUtxoStatistics :: HashMap Text Word64 -> Word64 -> Either UtxoStatisticsError (HashMap Text Word64, Word64) validateUtxoStatistics histogram allStakes = do let (keys, elems) = (HMS.keys histogram, HMS.elems histogram) let acceptedKeys = show <$> (toList $ generateBounds Log10) when (length keys <= 0) $ Left ErrHistogramEmpty when (any (flip notElem acceptedKeys) keys) $ Left ErrHistogramNamesInvalid when (any (< 0) elems) $ Left ErrHistogramUpperBoundNegative when (allStakes < 0) $ Left ErrAllStakesNegative Right (histogram, allStakes)

Also, this function looks like a good candidate for a smart constructor and could return an actual UtxoStatistics rather than an HashMap and a Word64. You can find some inspiration here maybe:

https://github.com/input-output-hk/cardano-sl/blob/Squad1/CO-325/api-v1-improvements/wallet/src/Pos/Util/Mnemonic.hs#L126-L149

https://github.com/input-output-hk/cardano-sl/blob/Squad1/CO-325/api-v1-improvements/wallet/src/Pos/Util/Mnemonic.hs#L350-L363

(NOTE: We might want to expose this eitherToParse function somewhere else ...)

KtorZ · 2018-08-18T21:51:58Z

+        addCountInBuckets thefirst acc entry =
+            case Map.lookupGE entry acc of
+                Just (k, v) -> Map.insert k (v+1) acc
+                Nothing     -> Map.adjust (+1) thefirst acc


From foldl documentation, we find this:

Fold (x -> a -> x) x (x -> b) -- Fold step initial extract

When there's a clear structure like this, it's usually a good idea to have the implementation follows this structure as it helps understanding (same for bracket for instance).

What about something like:

foldBuckets :: NonEmpty Word64 -> L.Fold Word64 [HistogramBar] foldBuckets bounds = let step x a = case Map.lookupGE a x of Just (k, v) -> Map.insert k (v+1) x Nothing -> Map.adjust (+1) (head bounds) x initial = Map.fromList $ zip (NL.toList bounds) (repeat 0) extract = map (uncurry HistogramBar) . Map.toList in L.Fold step initial extract

KtorZ · 2018-08-18T21:54:27Z

+
+instance Arbitrary TxOut where
+    arbitrary = genericArbitrary
+    shrink = genericShrink


What does all this has to do with the PR 😮 ? Maybe an artifact from a rebase / merge conflict ?

it is unfortunately needed to support Utxo

KtorZ · 2018-08-18T22:27:08Z

+
+instance Arbitrary UtxoStatistics where
+    arbitrary = UtxoStatistics <$> arbitrary
+                               <*> arbitrary


The "invariant" I was talking about enters into play here. We could generate an arbitrary statistics here where we have 14000 in the last bucket, but a total take of 42 ADA which doesn't make sense.
Hence the difficulty of having such parameter as part of the structure.

I added some code that addresses this but it goes forever ;-/

KtorZ · 2018-08-18T22:27:56Z

+    = ErrHistogramEmpty
+    | ErrHistogramNamesInvalid
+    | ErrHistogramUpperBoundsNegative
+    | ErrAllStakesNegative


What if the total stake is lower than the minimum amount that can be represented with all the buckets 😛 ?

KtorZ · 2018-08-18T22:29:55Z

+summarizeUtxoStatistics bounds =
+    UtxoStatistics
+    <$> populateBuckets bounds
+    <*> L.sum


A bit the same as below, minor styling remark but we could avoid the extra summarizeUtxoStatistics and make the implementation a bit more straightforward:

computeUtxoStatistics :: [Word64] -> UtxoStatistics computeUtxoStatistics = L.fold $ UtxoStatistics <$> foldBuckets (generateBounds Log10) <*> L.sum

KtorZ · 2018-08-20T07:53:04Z

+        histogram <- arbitrary
+        let (minPossibleValue, maxPossibleValue) = getPossibleBounds histogram
+        let histoBars = map (uncurry HistogramBarCount) $ Map.toList histogram
+        allStakes <- arbitrary `suchThat` (\s -> s >= minPossibleValue && s <= maxPossibleValue)


Yep, the way Quickcheck works usually makes suchThat a poor choice in many case because the condition becomes harder and harder to satisfy as QuickCheck looks for values in a new range. A rule of the thumb is that it's good when you have to make sure some values get discarded, but when you want to pick values in a range, it's better to rely on choose:

do histogram <- arbitrary allStakes <- choose (getPossibleBounds histogram) ...

KtorZ · 2018-08-20T07:56:44Z

-        possiblenames <- elements $ map show (NL.toList $ generateBounds Log10)
-        bound <- arbitrary
+        possiblenames <- elements (NL.toList $ generateBounds Log10)
+        bound <- arbitrary `suchThat` (>= 0)


Word64 are unsigned-integers 😛

KtorZ · 2018-08-20T08:07:44Z

NOTE:

We're going to need this (#3432) for the arguments list build issue
I'll rebase the base as soon as we have it.

This was done using weeder and is for now necessary for the CI to kick in. We are starting to reach a critical point in terms of dependencies and this kind of solution might not work anymore in the future. There's a PR opened about doing something against these 'arguments list too long'. :finger_crossed:

- We do not expose type internals and provide one smart constructor for that purpose (computeUtxoStatistics) - Same for the 'BoundType' which should actually be part of the API if we intend to use it, making it an opaque type with exposed constructors allow for easy extension and maintainability - Reviewed a bit errors to make constructors a bit less specified in favor of constructor with arg - Added 'BoundType' as 'boundType' to the JSON representation - Made Aeson & Swagger imports explicit!

This is more semantically correct and type-safe than taking a raw list of 'Word64'. This way, we also get documentation for free simply by looking at the function signature and also makes calls for callers simpler (provided they have a list of available utxos, but why would they call the function if they hadn't? :) )

KtorZ · 2018-08-20T16:17:02Z

+
+    -- * Constructing 'BoundType'
+    , log10
+    )  where


@paweljakubas Only exposing opaque types here with smart-constructors.
Note that I have removed the few externals calls to mkUtxoStatistics for they were not really needed and could simply rely on computeUtxoStatistics. This function should be our one and single constructor for the outside world.

We now use mkUtxostatistics internally only (in the FromJSON instance).
Note also, I've removed the Haphazard constructor for the BoundType and here again, to keep the module easy to extend, we hide the implementation to the outside world, only having a "smart-constructor" log10 which does the trick.

KtorZ · 2018-08-20T16:17:57Z

+    = ErrEmptyHistogram
+    | ErrInvalidBounds !Text
+    | ErrInvalidTotalStakes !Text
+    deriving (Eq, Show, Read, Generic)


@paweljakubas I've factored out a couple of errors as they were semantically referring to the same thing, only with a small difference which can be captured in a message Text. It makes the error type a bit less bloated but still rather expressive.

KtorZ · 2018-08-20T16:18:45Z

+            object
+                [ "histogram" .= histogramObject bars
+                , "allStakes" .= allStakes
+                , "boundType" .= log10


Note that we need "boundType" in the JSON representation otherwise we simply force the bound type upon decoding a JSON object to Log10 and there's no point of keeping that flexible.

KtorZ · 2018-08-20T16:20:46Z

+{-# INLINE log10 #-}
+
+-- | Compute UtxoStatistics from a bunch of UTXOs
+computeUtxoStatistics :: BoundType -> [Utxo] -> UtxoStatistics


Also a change here we've suggested in the past and then forgot about. The API used to take [Word64] only, but that gives us poor type-guarantees and requires the caller to know about how to extract the correct value from an Utxo. Here, we take care of that burden ourselves and even get documentation for free!

KtorZ · 2018-08-20T16:21:09Z

+--
+-- INTERNALS
+--
+


Everything below is actually internal stuff and doesn't need to be exported 👍

KtorZ · 2018-08-20T16:22:29Z

+    calculatePossibleBound fromPair =
+        sum .
+        concatMap (\pair -> matching fromPair pair $ createBracketPairs acceptedKeys) $
+        Map.toList histogram


I honestly haven't verified much of this as my brain isn't much capable of it right now :(

paweljakubas added 3 commits August 10, 2018 13:45

[CO-347] Adding stubs to Handler and LegacyHandler

7e32ad6

This commit adds stubs to legacy and new-wallet, histogram and bar datatypes are accomodated, one-pass many-aggregates functionality is added

[CO-347] Adding WalletLayer implementation of getUtxos

824b69c

Kernel level implementation of WalletLayer is added, Legacy is not supported. The implementation is used in Handlers of V1 API. Refactoring from Integer to Word64 is done

paweljakubas requested review from KtorZ, edsko and parsonsmatt August 14, 2018 23:14

edsko approved these changes Aug 15, 2018

View reviewed changes

KtorZ suggested changes Aug 16, 2018

View reviewed changes

KtorZ changed the title ~~Origin/paweljakubas/co 347/utxos statistics~~ [CO-347] Wallet's UTXO histogram view Aug 16, 2018

parsonsmatt suggested changes Aug 16, 2018

View reviewed changes

[CO-347] Review improvements

6d31f9f

parsonsmatt suggested changes Aug 17, 2018

View reviewed changes

[CO-347] Further corrections and get rid of scientific

ac7681c

[CO-347] Pandering to hlint needs and improving on error handling to …

d23d8fa

…Utxo statistics

KtorZ suggested changes Aug 18, 2018

View reviewed changes

KtorZ reviewed Aug 18, 2018

View reviewed changes

[CO-347] Reading improvements, adding new error and smart constructor

b8727e1

KtorZ reviewed Aug 20, 2018

View reviewed changes

KtorZ approved these changes Aug 20, 2018

View reviewed changes

KtorZ and others added 4 commits August 20, 2018 15:32

[CO-347] Improvements on UtxoStatistics arbitrary instance

3dd925f

KtorZ force-pushed the origin/paweljakubas/CO-347/utxos-statistics branch from 2b5478b to 166665c Compare August 20, 2018 16:13

KtorZ reviewed Aug 20, 2018

View reviewed changes

KtorZ and others added 3 commits August 20, 2018 18:29

[CO-347] Re-generate pkgs/default.nix

8b23234

[CO-347] Fix test suite build

dd4b54a

[CO-347] pkgs

3e5df1e

parsonsmatt approved these changes Aug 21, 2018

View reviewed changes

parsonsmatt merged commit e65cc2f into Squad1/CO-325/api-v1-improvements Aug 21, 2018

KtorZ deleted the origin/paweljakubas/CO-347/utxos-statistics branch August 24, 2018 12:29

		@@ -0,0 +1,51 @@
		module Cardano.Wallet.API.V1.Handlers.Internal

Conversation

paweljakubas commented Aug 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Linked issue

Type of change

Developer checklist

Testing checklist

QA Steps

Screenshots (if available)

Uh oh!

edsko left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paweljakubas commented Aug 14, 2018 •

edited

Loading

paweljakubas Aug 17, 2018 •

edited

Loading