Finalize First API Types #107

KtorZ · 2019-03-21T16:23:27Z

Issue Number

#53

Overview

I have move validation logic and representation of various types back to the wallet primitives
I have added extra tests to validate error branches for a few JSON serialization

Comments

Anviking

First look. Looks good.

Mainly two points of concerns / confusion related to MeasuredIn "percent".

Anviking · 2019-03-21T19:15:53Z

src/Cardano/Wallet/Api/Types.hs

 --
 -- >>> Aeson.encode Ready
 -- {"status":"ready"}
 --
-- >>> Aeson.encode $ Restoring (Percentage 14)
+-- >>> Aeson.encode $ Restoring (MeasuredIn 14)


Auxiliary: On the value-level, the name is kind of confusing. Could we rename MeasuredIn to Quantity?

I would propose just defining a Percentage data type for this purpose. Percentages can only ever have a unit of percent. There's really no need for polymorphism. The question of how to encode a percentage in JSON in our API should not restrict how we define our internal Haskell type, in my opinion.

Alternatively, if we want to keep the units of measure scheme even for percentages, why not define the following function:

withUnit :: forall u a . a -> MeasuredIn (u :: Symbol) a withUnit = MeasuredIn

We can then write:

Aeson.encode $ Restoring (withUnit @"percent" 14)

Anviking · 2019-03-21T19:17:55Z

src/Data/MeasuredIn.hs

+-- disambiguate primitive types like 'Int' or 'String' which can be in different
+-- bases depending on the context.
+
+module Data.MeasuredIn


Think separating into its own module can make sense 👍

Anviking · 2019-03-21T19:23:15Z

src/Data/MeasuredIn.hs

+    minBound = MeasuredIn 0
+    maxBound = MeasuredIn 100
+
+instance {-# OVERLAPS #-} (Integral i, FromJSON i) => FromJSON (MeasuredIn "percent" i) where


{-# OVERLAPS #-} why is this needed?

Also seems weird to have both

Very high-tech & generlized MeasuredIn, and

Special case for percent

cf answer here: #107 (comment)

Anviking · 2019-03-21T19:42:43Z

src/Cardano/Wallet/Api/Types.hs

-    , _passphrase :: !WalletPassphraseInfo
-    , _state :: !WalletState
+    , _passphrase :: !(ApiT WalletPassphraseInfo)
+    , _state :: !(ApiT WalletState)


👍 Will be interesting to see how this plays out now when our types are much more polymorphic (and able to handle this) than in cardano-sl.

src/Cardano/Wallet.hs

Anviking · 2019-03-21T20:06:59Z

src/Cardano/Wallet.hs

    = Ready
-    | Restoring
-    deriving (Eq, Show)
+    | Restoring !(MeasuredIn "percent" Word)


Hm… You have just defined Aeson instances on a Cardano.Wallet type, haven't you? :)

Ok, not really, but I'm confused by this 🤔

Why have ApiT at all?

Why have ApiT at all?

Having ApiT allows us to avoid orphan instances, and to keep the following concerns separate:

How we define a type that we use internally.

How we represent values of that type on the wire.

For types that are only ever seen in the API, we can define FromJSON and ToJSON instances directly for those types, and the job is done.

For types that appear in the API, but are actually defined elsewhere (like AddressPoolGap), we still want to define FromJSON and ToJSON instances in the same way as the other types, but we don't want to place those instances in Primitive (to keep the separation of concerns).

Therefore, we need to wrap such types in a newtype wrapper, in order to be able to give them non-orphan instances. We could, of course, use a different newtype for each and every such type. But in the end, most of these wrappers would be the same. Hence we define ApiT as a common wrapper, and give each ApiT a a unique instance of FromJSON and ToJSON.

test/unit/Data/MeasuredInSpec.hs

Anviking · 2019-03-21T20:53:56Z

test/unit/Cardano/Wallet/Api/TypesSpec.hs

-            roundtripAndGolden $ Proxy @ WalletBalance
-            roundtripAndGolden $ Proxy @ WalletPassphraseInfo
-            roundtripAndGolden $ Proxy @ WalletState
+            roundtripAndGolden $ Proxy @ (ApiT WalletBalance)


Suggested change

roundtripAndGolden $ Proxy @ (ApiT WalletBalance)

I think I messed up, this is a duplicate

Don't have to fix here, but could

jonathanknowles · 2019-03-22T02:47:56Z

src/Cardano/Wallet.hs

    = Ready
-    | Restoring
-    deriving (Eq, Show)
+    | Restoring !(MeasuredIn "percent" Word)


| Restoring !(MeasuredIn "percent" Word)

I still think this seems very odd. Of course, the JSON encoding of a percentage is an object with a quantity and a unit of percent, but that's just the wire format.

I understand the general case where need to have a type that is polymorphic in some unit. For example, monetary amounts can be measured in different currency units, and so encoding these units in the type system makes sense, because the compiler will stop us from erroneously mixing amounts with different currency units.

But for a percentage, the units can never change, they'll always be "percent". Rather than write MeasuredIn "percent" Word, why not just have a simpler Percentage type that is unit-less, and then when we want to encode it as JSON, we can add the "unit" : "percent" property back in again?

It seems here that we're allowing the wire format to influence our choice of how to structure what could be a perfectly ordinary Haskell type.

@Anviking @jonathanknowles

The Percentage here is giving me some hard-time and there are a few points I'd like to clarify:

The MeasuredIn (or Quantity, this is probably indeed a better name) is a polymorphic type which allows us to represent numeric quantities with units. It's not only about the wiring to JSON. It happens that we do want consistency here and represent all those numeric types in the same way. It gives extra meaning to a type. Like Key or Index from the Address derivation module.

There's nothing polymorphic about MeasuredIn "percent" Word. This type has only one and one concrete representation and is strictly equivalent to a newtype Percentage = Percentage Word. I actually went for the latter initially but it felt wrong since, we already had a type to represent quantities but, we were wrapping things in a MeasuredIn during the serialization and deserialization. Felt weird.

There are actually more than one way to represent percentages. We could use a Double with a value between 0 and 1 for instance. So here, we really mean to represent percentage in a "human friendly" way. My point is really that, most of the types we are going to represent using MeasuredIn will only ever have one unit (transaction fees are going to be in lovelace/bytes and that's not going to change, transaction max size will be in bytes, and same faith). The polymorphism gives us the ability to re-use the MeasuredIn type to represent various more concrete types. Likewise, we could just create a newtype for each one of them, but we'll end-up duplicating the serialization logic (and testing). So, yes, we could define a Percentage newtype and use helper to add units to the serialization and that will do the job. But then, we do not need the MeasuredIn abstraction and we could simply use newtype all the time. This is just a fight for consistency here.

The "problem" with Percentage comes from the fact that we do not have any satisfactory primitive type to represent values between 0 and 100. I agree that the overlapping instance isn't satisfactory.

With both your feedbacks, this is the two suggestions / options I see:

We define a Percentage or Percentage100 primitive type that is basically an opaque type which can hold a non-floating value going from 0 to 100. And, we do use MeasuredIn "percent" Percentage as a type elsewhere to makes it obvious how this type is represented for us.

Or, we add extra n :: Nat parameters to the MeasuredIn types to contains actual bounds instead of an underlying type. So we have: newtype MeasuredIn (min :: Nat) (max :: Nat) (unit :: Symbol). With this, we can define bounds directly at the type-level and represent any arbitrary bounds we would need. It becomes quite unpractical to use and we probably end-up using type-alias like type Percentage = MeasuredIn 0 100 "percent" ... :/

I went for the first approach.

but we'll end-up duplicating the serialization logic (and testing)

DerivingVia?

jonathanknowles · 2019-03-22T02:51:23Z

src/Cardano/Wallet.hs

+    deriving (Generic, Eq, Show)
+
+newtype PoolId = PoolId
+    { getPoolId :: Text }


Should this be a UUID?

Not yet. It's still unclear what the PoolId format will end up being unfortunately. So for now, I'll keep that one "flexible" until we know better.

jonathanknowles · 2019-03-22T02:55:12Z

src/Cardano/Wallet/Api/Types.hs

+
+data WalletBalance = WalletBalance
+    { _available :: !(MeasuredIn "lovelace" Natural)
+    , _total :: !(MeasuredIn "lovelace" Natural)


What if we were to define a type synonym here:

type Amount x = MeasuredIn x Natural

Then we could write:

data WalletBalance = WalletBalance { _available :: !(Amount "lovelace") , _total :: !(Amount "lovelace")

jonathanknowles · 2019-03-22T03:04:47Z

src/Cardano/Wallet/Api/Types.hs

 --
 -- >>> Aeson.encode Ready
 -- {"status":"ready"}
 --
-- >>> Aeson.encode $ Restoring (Percentage 14)
+-- >>> Aeson.encode $ Restoring (MeasuredIn 14)


I would propose just defining a Percentage data type for this purpose. Percentages can only ever have a unit of percent. There's really no need for polymorphism. The question of how to encode a percentage in JSON in our API should not restrict how we define our internal Haskell type, in my opinion.

Alternatively, if we want to keep the units of measure scheme even for percentages, why not define the following function:

withUnit :: forall u a . a -> MeasuredIn (u :: Symbol) a withUnit = MeasuredIn

We can then write:

Aeson.encode $ Restoring (withUnit @"percent" 14)

jonathanknowles · 2019-03-22T03:09:26Z

src/Data/MeasuredIn.hs

+
+module Data.MeasuredIn
+    ( -- * Polymorphic MeasuredIn
+      MeasuredIn(..)


Perhaps this is just nitpicking, but some of this styling seems inconsistent.

Some brackets have spaces before them, but others don't.

Given that we already have a well-defined style for imports (== spaces before parentheses), perhaps we can copy that style for exports? (Can make a proposal if you like. )

I'd say, if we can get stylish-haskell or hlint to do it for us, definitely. Otherwise, that will rapidly become annoying to chase those :/
I do like the consistency here however.

jonathanknowles · 2019-03-22T03:12:51Z

src/Data/MeasuredIn.hs

+newtype MeasuredIn (u :: Symbol) a = MeasuredIn a
+    deriving (Generic, Show, Eq)
+
+-- | Encode to JSON delegating the


Seems like an incomplete comment? :)

Seems about right 😅

jonathanknowles · 2019-03-22T03:14:25Z

src/Data/MeasuredIn.hs

+    minBound = MeasuredIn 0
+    maxBound = MeasuredIn 100
+
+instance {-# OVERLAPS #-} (Integral i, FromJSON i) => FromJSON (MeasuredIn "percent" i) where


This line is overly long, according to our coding standards. Why not wrap this as follows:

instance {-# OVERLAPS #-} (Integral i, FromJSON i) => FromJSON (MeasuredIn "percent" i) where

jonathanknowles · 2019-03-22T03:17:38Z

test/unit/Cardano/Wallet/Api/TypesSpec.hs

-instance Arbitrary Percentage where
-    arbitrary = arbitraryBoundedEnum
+instance Arbitrary (MeasuredIn "percent" Word) where
+    arbitrary = MeasuredIn <$> choose (0, 100)


What is wrong with using arbitraryBoundedEnum? We have instances of Bounded and Enum defined, so why not use them here?

Nothing, I forgot to move the Enum instance back in the code. I intended to move it to the test code because that's the only place where it's actually used from at the moment. Enum instances are quite a pain in the *** to test, so I'd rather not give extra power to the type in our code if we don't need it. It makes it easier to define the arbitrary instance though, I totally agree.

Anviking

Still not sure about
!(MeasuredIn "percent" Word)
but happy to merge!

KtorZ · 2019-03-22T10:27:05Z

Still not sure about !(MeasuredIn "percent" Word) but happy to merge!

And you're write, I've fixed it locally (wrong merge conflict resolution from rebase), haven't yet pushed.

- re-introduce MeasuredIn to encapsulate amounts with a non-ambiguous type - re-introduce MeasuredIn to encapsulate percentage with a non-ambiguous type

…pes only about JSON serializations

KtorZ requested a review from Anviking March 21, 2019 16:23

KtorZ self-assigned this Mar 21, 2019

KtorZ closed this Mar 21, 2019

KtorZ force-pushed the KtorZ/53/finalize-first-api-types branch from 2101398 to ccd3849 Compare March 21, 2019 16:25

Anviking reopened this Mar 21, 2019

KtorZ force-pushed the KtorZ/53/finalize-first-api-types branch 2 times, most recently from f14fd67 to 081ecfe Compare March 21, 2019 19:05

KtorZ mentioned this pull request Mar 21, 2019

Servant Swagger QuickCheck Property Testing (validateEveryJSON & validateEveryPath) #108

Merged

2 tasks

jonathanknowles mentioned this pull request Mar 21, 2019

Translate the Swagger API specification into a Servant API specification. #53

Closed

3 tasks

Anviking reviewed Mar 21, 2019

View reviewed changes

jonathanknowles reviewed Mar 22, 2019

View reviewed changes

KtorZ force-pushed the KtorZ/53/finalize-first-api-types branch 2 times, most recently from 226a37b to df92987 Compare March 22, 2019 09:33

Anviking self-requested a review March 22, 2019 10:24

Anviking approved these changes Mar 22, 2019

View reviewed changes

KtorZ added 2 commits March 22, 2019 11:28

move 'MeasuredIn' in its own module and rename it to 'Quantity'

fc2d94e

- re-introduce MeasuredIn to encapsulate amounts with a non-ambiguous type - re-introduce MeasuredIn to encapsulate percentage with a non-ambiguous type

move remaining logic from Api to wallet primitive and keep the API ty…

d0ae386

…pes only about JSON serializations

KtorZ force-pushed the KtorZ/53/finalize-first-api-types branch from df92987 to d0ae386 Compare March 22, 2019 10:29

KtorZ mentioned this pull request Mar 22, 2019

Add response code verification into integration DSL #104

Merged

1 task

KtorZ merged commit f8f4df0 into master Mar 22, 2019

KtorZ deleted the KtorZ/53/finalize-first-api-types branch March 22, 2019 15:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Finalize First API Types #107

Finalize First API Types #107

KtorZ commented Mar 21, 2019

Anviking left a comment •

edited

Loading

Anviking Mar 21, 2019

jonathanknowles Mar 22, 2019

Anviking Mar 21, 2019

Anviking Mar 21, 2019

KtorZ Mar 22, 2019

Anviking Mar 21, 2019

Anviking Mar 21, 2019

Anviking Mar 21, 2019

jonathanknowles Mar 22, 2019

Anviking Mar 21, 2019

jonathanknowles Mar 22, 2019

KtorZ Mar 22, 2019 •

edited

Loading

KtorZ Mar 22, 2019

Anviking Mar 22, 2019

jonathanknowles Mar 22, 2019

KtorZ Mar 22, 2019

jonathanknowles Mar 22, 2019

jonathanknowles Mar 22, 2019

jonathanknowles Mar 22, 2019

KtorZ Mar 22, 2019

jonathanknowles Mar 22, 2019

KtorZ Mar 22, 2019

jonathanknowles Mar 22, 2019

jonathanknowles Mar 22, 2019

KtorZ Mar 22, 2019

Anviking left a comment

KtorZ commented Mar 22, 2019

Finalize First API Types #107

Finalize First API Types #107

Conversation

KtorZ commented Mar 21, 2019

Issue Number

Overview

Comments

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

KtorZ Mar 22, 2019 • 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

Anviking left a comment

Choose a reason for hiding this comment

KtorZ commented Mar 22, 2019

Anviking left a comment •

edited

Loading

KtorZ Mar 22, 2019 •

edited

Loading