Translate subset of Wallet API to Servant #76

jonathanknowles · 2019-03-18T07:15:26Z

Translate a subset of the Wallet API to Servant. (See Issue #53)

Included in this PR

This PR:

translates a small number of endpoints within the wallets namespace.
translates all types associated with the above endpoints.
provides JSON encodings that (should) match the Swagger type definitions.
provides round-trip validation for all types.
provides round-trip tests for JSON serialization and deserialization.

Not included in this PR

This PR:

doesn't attempt to encode the entire API. Instead, a small subset is encoded.
doesn't attempt to automatically verify that the Servant definition matches the Swagger definition.

Design choices

This PR makes some design choices, which should be reviewed.

1. Place each type in a separate module

Reasons:

Better encapulation and type safety.
Example: WalletName cannot be constructed without a valid name.
GHC can parallelize builds of types within separate modules.

2. Favour simple generic JSON encoding

Manually-written JSON code can be tricky to debug.

This PR makes the choice of using generically-derived ToJSON and FromJSON instances wherever possible.

Whenever there's a non-trivial mapping between a Haskell type and a desired encoding, we encode the desired structure in a new Haskell type that's closer to the desired encoding, and write a pair of conversion functions that can easily be checked by the compiler.

For example: WalletState provides an idiomatic Haskell encoding of a wallet's restoration state, but we also provide UnvalidatedWalletState as a type that is more trivial to encode as JSON. We provide validate and unvalidate to convert between these two types.

src/Cardano/Wallet/Api/V2/Types/CurrencyUnit.hs

src/Cardano/Wallet/Api/V2/Lenses.hs

src/Cardano/Wallet/Api/V2/Types/WalletAddressPoolGap.hs

src/Cardano/Wallet/Api/V2/Types/WalletId.hs

cardano-wallet.cabal

test/unit/Cardano/Wallet/Api/V2/Arbitraries.hs

rvl

Looks pretty good. I have a few questions and one comment.

src/Cardano/Wallet/Api.hs

src/Cardano/Wallet/Api/Types/Percentage.hs

src/Cardano/Wallet/Api/Types/WalletDelegation.hs

src/Cardano/Wallet/Api/Types/WalletState.hs

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

src/Cardano/Wallet/Api.hs

src/Cardano/Wallet/Api/Types/Amount.hs

src/Cardano/Wallet/Api/Types/WalletAddressPoolGap.hs

src/Cardano/Wallet/Api/Types/WalletBalance.hs

src/Cardano/Wallet/Api/JSON.hs

src/Cardano/Wallet/Api/Types/WalletName.hs

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

KtorZ · 2019-03-20T17:48:24Z

@jonathanknowles I've continued the work on this PR with some of the points we raised during reviews (cf the commit history). I haven't got time to finalize aligning the primitives with the API and moving types around but it's almost there 👌

rvl

Looks pretty good.
Maybe later - some tests for parsing JSON with invalid amounts/percentages/wallet names.

src/Cardano/Api/Types.hs

rvl · 2019-03-21T05:57:01Z

I also feel (and this may be nitpicking) that Cardano.Wallet.Api is a better choice than Cardano.Api. After all, we're providing an API for the wallet, and not Cardano in general.

…lue.

…'Cardano.Api'

Didn't do in the previous commit not to mess with git and makes it easy to understand what happened to that file (shows a rename in the git history).

…let primitives

* One space before opening brackets, everywhere. * No multiple consecutive blank lines.

…or types with units of measure.

jonathanknowles · 2019-03-21T06:36:59Z

I also feel (and this may be nitpicking) that Cardano.Wallet.Api is a better choice than Cardano.Api. After all, we're providing an API for the wallet, and not Cardano in general.

I tend to agree with this. Having Cardano.Api seems to imply that we're providing an API for Cardano in general. I'd be happy to keep things in Cardano.Wallet.Api for the moment.

The API we provide is an API for the wallet, rather than for Cardano as a whole.

…r `WalletDelegation`.

Comments are addressed

KtorZ · 2019-03-21T08:53:07Z

@rvl: Comments are addressed

Well... not really :/ ---> #76 (comment)

There were still a few types to be moved from the Api.Types to the wallet primitive as mentioned. We have quite a few duplicated logic between both layers and the Api should just be about representing those internal types in Servant / JSON (like I started for WalletDelegation).

jonathanknowles changed the title ~~Jonathanknowles/wallet api servant~~ Translate subset of Wallet API to Servant Mar 18, 2019

jonathanknowles requested review from KtorZ and rvl March 18, 2019 07:16

jonathanknowles self-assigned this Mar 18, 2019

jonathanknowles commented Mar 18, 2019

View reviewed changes

src/Cardano/Wallet/Api/V2/Types/CurrencyUnit.hs Outdated Show resolved Hide resolved

rvl added this to the Wallet Layer Integration milestone Mar 18, 2019

KtorZ previously requested changes Mar 18, 2019

View reviewed changes

jonathanknowles force-pushed the jonathanknowles/wallet-api-servant branch 2 times, most recently from d6d22da to 8557eaf Compare March 19, 2019 04:20

rvl suggested changes Mar 19, 2019

View reviewed changes

KtorZ reviewed Mar 19, 2019

View reviewed changes

jonathanknowles mentioned this pull request Mar 21, 2019

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

Closed

3 tasks

rvl approved these changes Mar 21, 2019

View reviewed changes

src/Cardano/Api/Types.hs Outdated Show resolved Hide resolved

rvl requested a review from KtorZ March 21, 2019 05:58

jonathanknowles added 14 commits March 21, 2019 06:28

Add partial Servant specification of wallet API.

1af4f45

Represent Percentage with a rich JSON object rather than a naked va…

ca5d347

…lue.

Simplify Arbitrary instance for WalletName.

7fcd2db

Move uuidFromWords closer to its usage point.

ee42cf6

Add more round-trip JSON serialization tests.

b67b0d1

Add GetWallet to the API.

af6b530

Add DeleteWallet to API.

8a42e54

Remove unnecessary dependencies.

f64ca1c

Change imports to qualified.

9b21df2

Remove unnecessary Functions module.

80a285b

Remove Unknown value of CurrencyUnit.

e390779

Make the names of functions in Api.V2.JSON more self-describing.

e498ee8

Fix order of imports.

1aeae1f

Satisfy import rules in module Api.V2.Lenses.

d9d1adc

jonathanknowles and others added 18 commits March 21, 2019 06:28

Remove lens dependency.

cd8a528

Reuse existing AddressPoolGap type in wallet API.

d82e816

Remove Swagger operation IDs from Servant endpoint paths.

42b389f

Use NoContent for DeleteWallet endpoint.

c801f31

Replace Number . fromIntegral with toJSON.

ed01f71

Use snake_case instead of CamelCase for API record fields.

44c9486

regroup api types into one module and rename 'Cardano.Wallet.Api' to …

dd5dc6d

…'Cardano.Api'

make Cardano.Api compiles after renaming and grouping files

2f8db0a

Didn't do in the previous commit not to mess with git and makes it easy to understand what happened to that file (shows a rename in the git history).

review WalletState to work with generic instances

aeada77

review DelegationState to work with generic instances

f65e032

simplify percentage types to work better with generics

74e3a45

factor types with unit using a polymorphic 'MeasuredIn' type

6d09f2f

introduce 'ApiT' and start moving a few types & logic back to the wal…

22caa1c

…let primitives

Use formatting consistent with coding standards.

85b8812

* One space before opening brackets, everywhere. * No multiple consecutive blank lines.

Remove redundant Generic constraint.

f737106

Push units of measure into the JSON instances for each type.

631e36c

Add functions addUnit and removeUnit to simplify JSON instances f…

d120940

…or types with units of measure.

Improve comments for MeasuredIn type and associated functions.

9480cb3

Move Cardano.Api to Cardano.Wallet.Api.

85e386d

The API we provide is an API for the wallet, rather than for Cardano as a whole.

jonathanknowles force-pushed the jonathanknowles/wallet-api-servant branch from e9f0e03 to 85e386d Compare March 21, 2019 06:37

jonathanknowles added 5 commits March 21, 2019 06:46

Use arbitraryBoundedEnum in instance of Arbitrary for Percentage.

ee96988

Appease Haskell Stylish.

1cbb5a8

Add PoolId data type and use it to simplify Arbitrary instance fo…

32450a0

…r `WalletDelegation`.

Remove unused Arbitrary instance for MeasuredIn type.

5f96df0

Appease Stylish Haskell.

beb81bc

rvl merged commit 9ab3c90 into master Mar 21, 2019

rvl deleted the jonathanknowles/wallet-api-servant branch March 21, 2019 07:42

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Translate subset of Wallet API to Servant #76

Translate subset of Wallet API to Servant #76

jonathanknowles commented Mar 18, 2019 •

edited

Loading

rvl left a comment

KtorZ commented Mar 20, 2019

rvl left a comment

rvl commented Mar 21, 2019

jonathanknowles commented Mar 21, 2019

KtorZ commented Mar 21, 2019

Translate subset of Wallet API to Servant #76

Translate subset of Wallet API to Servant #76

Conversation

jonathanknowles commented Mar 18, 2019 • edited Loading

Included in this PR

Not included in this PR

Design choices

1. Place each type in a separate module

2. Favour simple generic JSON encoding

rvl left a comment

Choose a reason for hiding this comment

KtorZ commented Mar 20, 2019

rvl left a comment

Choose a reason for hiding this comment

rvl commented Mar 21, 2019

jonathanknowles commented Mar 21, 2019

KtorZ commented Mar 21, 2019

jonathanknowles commented Mar 18, 2019 •

edited

Loading