[CO-325] API V1 Improvements - Part 1 #3372

KtorZ · 2018-08-08T10:40:03Z

Description

This PR contains some API V1 improvements. Another one will follow in the upcoming week with another set of changes. We are shipping this first part now to avoid too big PR and because some of its content is needed by the Daedalus team to move on.

This contains:

CO-324 Per-field endpoints
CO-327 Swagger schemas' examples don't rely on the Example type-class
CO-328 --cert cURL option is missing from API documentation
CO-342 Backward-compatible CBackupPhrase parser

Linked issue

[CO-325]

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.

Testing checklist

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

QA Steps

Screenshots (if available)

adinapoli-iohk

Looks good to me, thanks @KtorZ !

I have added only some remarks to be intended as a chance to see if we can do better, but none of the comments above are serious blockers 😉

adinapoli-iohk · 2018-08-08T13:58:08Z

wallet-new/integration/AccountSpecs.hs

+accountSpecs :: WalletRef -> WalletClient IO -> Spec
+accountSpecs _ wc =
+    describe "Accounts" $ do
+        it "can retrieve only an accounts balance" $ do


Minor typo: "an account".

adinapoli-iohk · 2018-08-08T13:59:50Z

wallet-new/src/Cardano/Wallet/API/Response.hs

@@ -43,6 +41,11 @@ import           Cardano.Wallet.API.Response.Sort.IxSet as SortBackend
 import           Cardano.Wallet.API.V1.Errors
                     (WalletError (JSONValidationFailed))

+import qualified Data.Aeson.Options as Serokell
+import qualified Data.Char as Char
+import qualified Formatting.Buildable


Is there a cunning reason why these were moved here? Is there a new coding guideline which suggest we move all the qualified imports at the bottom?

Just a habit indeed, I usually do that when rebasing because it makes merge-conflicts easier to solve. Not sure we have any particular guidelines about this; I know it was suggested in the past but not clearly stated as the way to go.

adinapoli-iohk · 2018-08-08T14:00:16Z

wallet-new/src/Cardano/Wallet/API/V1/Accounts.hs

@@ -37,4 +40,14 @@ type API
          :> "certificates"
          :> ReqBody '[ValidJSON] Redemption
          :> Post '[ValidJSON] (WalletResponse Transaction)
+    :<|> "wallets" :> CaptureWalletId :> "accounts"
+          :> CaptureAccountId :> "addresses"
+          :> Summary "Retrieve only account's addressees."


addresses ? 🤔

Referring to the typo addressees here ?

adinapoli-iohk · 2018-08-08T14:01:55Z

wallet-new/src/Cardano/Wallet/API/V1/Handlers/Accounts.hs

+    -> FilterOperations WalletAddress
+    -> Handler (WalletResponse AccountAddresses)
+getAccountAddresses _layer _wId _accIdx _pagination _filters =
+    error "unimplemented"


Should we link to the ticket which is going to implement this?

Also, genuine question -- why not reuse a WalletResponse [WalletAddress] , so we can do pagination the standard way?

There's no such ticket yet 😶,about to create one.

why not reuse a WalletResponse [WalletAddress] , so we can do pagination the standard way?

Because of the way the input is returned. We made the choice of wrapping it inside a "partial" object with a single field { "addresses": [ ... ] } to basically mimic the response you'd get when querying just an account. That's debatable, but that's what we've opted for, hence the wrapper.

adinapoli-iohk · 2018-08-08T14:03:04Z

wallet-new/src/Cardano/Wallet/API/V1/Handlers/Accounts.hs

+    -> AccountIndex
+    -> Handler (WalletResponse AccountBalance)
+getAccountBalance _layer _wId _accIdx =
+    error "unimplemented"


Ditto as the above. Also, these two endpoints are more integration work for the new-wallet team (cc @edsko ).

Perhaps you folks could help with the integration work?

adinapoli-iohk · 2018-08-08T14:05:31Z

wallet-new/src/Cardano/Wallet/API/V1/Types.hs

@@ -918,6 +925,15 @@ instance IxSet.Indexable (AccountIndex ': SecondaryAccountIxs)
                         (OrdByPrimKey Account) where
    indices = ixList

+-- | Datatype wrapping addresses for per-field endpoint
+newtype AccountAddresses = AccountAddresses
+    { acaAddresses :: [WalletAddress]


I might be inclined to scrap this extra type if it was added only for "aesthetic" purposes to make the API description reads nicer and not for a real need. One less type to maintain 😉

KtorZ · 2018-08-08T14:24:17Z

@adinapoli-iohk Typos fixed & ticket created (and referred to) to track the implementation work for the partial getters in Accounts.

akegalj

👍

LGTM

akegalj · 2018-08-08T14:50:30Z

wallet-new/integration/AccountSpecs.hs

+            addresses <- createAddresses wc 10 pair
+            let addr = Prelude.head addresses
+            let tests =
+                    [ PaginationTest (Just 1) (Just 5) NoFilters NoSorts


why not use paginateAll for tests like we did here https://github.com/input-output-hk/cardano-sl/pull/3372/files#diff-bff1c1799ab4f0868324f4083a4bd15aR251 (as we are not testing pagination in this specific spec)

In order to test pagination! Haha :)

then it makes sense - hope we have specialized pagination tests somewhere

No we don't. We will soon.

akegalj · 2018-08-08T14:51:57Z

wallet-new/integration/Main.hs

@@ -94,12 +96,13 @@ initialWalletState wc = do
        _transactions     = mempty
        _actionsNum       = 0
        _successActions   = mempty
-    pure $ WalletState {..}
+    return WalletState {..}


its not important - but I wonder why return instead of pure

Ah! I guess it was irritating me. I like to use return when inside a Monad and pure when inside an Applicative. I know a Monad is an Applicative but well, just developer OCD I guess.

When we get the Monad of No Return you will be forced to adapt :)

I'm with @KtorZ on this one! :)

akegalj · 2018-08-08T15:54:01Z

wallet/test/Test/Pos/Util/MnemonicSpec.hs

            Aeson.encode (CBackupPhrase mnemonic) `shouldBe` jsonV0Compat bytes

-        it "CBackupPhrase from JSON" $ forM_ testVectors $ \(bytes, _, mnemonic, _, _) ->
+        it "CBackupPhrase from JSON" $ forM_ testVectors $ \(bytes, _, _, mnemonic, _, _) ->


I am not sure I understand this properly - but shouldn't this be called "CBackupPhrase from legacy JSON" as we are using jsonV0Compat bellow?

This is new to me so I might have wrong understanding

I think the naming here is in contrast to the next one, which is called "CBackupPhrase from legacy JSON".

The difference being bytes vs legacyBytes. Might exist better ways.

akegalj · 2018-08-08T15:54:57Z

wallet/test/Test/Pos/Util/MnemonicSpec.hs

+            Aeson.decode (jsonV0Compat legacyBytes) `shouldBe` pure (CBackupPhrase mnemonic)
+
+        it "Mnemonic from legacy JSON" $ forM_ testVectors $ \(_, legacyBytes, _, _, _, _) ->
+            Aeson.decode legacyBytes `shouldSatisfy` isNothing @ (Mnemonic 12)


again, I am probably missunderstanding it - but shouldn't it be Aeson.decode (jsonV0Compat legacyBytes)...

@akegalj It should be correct, but it description lacking, my bad.

So, examples:

bytes = ["word", "word", ...] legacyBytes = ["word word word", "word", "word", ... ] -- could look like bytes too (jsonV0Compat legacyBytes) = {"bpToList": ["word word word", "word", "word", ...]} -- bytes is the format for Mnemonic -- (jsonV0Compat legacyBytes) is the format for CBackupPhrase

This test makes sure that the Mnemonic decoding doesn't work with legacyBytes.

I could

Describe these concepts in a comment at the top of the file, near TestVector

Change the it description

Aha it makes more sense now. Maybe having an example as you showed in the example above would be nice to have in comments. Nothing too serious though

DominikGuzei · 2018-08-08T17:43:38Z

I tested this together with #3367 and this solves the issues with creating and restoring wallets over api V0

Anviking · 2018-08-08T21:59:15Z

wallet/src/Pos/Wallet/Aeson/ClientTypes.hs

+--   "bpToList": ["squirrel material silly twice direct slush",
+--                "pistol razor", "becomed junk kingdom"]
+-- }
+-- as in addition to


I made a typo. "as" should be removed.

Now that client TLS verification is activated, clients are expected to provide a valid TLS client certificate. cURL command given as examples in the documentaiton should reflect that.

…rl-options-in-api-doc [CO-328] Add missing '--client' option in cURL API doc examples

Ideally, we want the /.../account/addresses to be paginated, otherwise there's no real value in isolating it as such. Also, for now, the implementation is rather heavy and requires the full account to be retrieved from the DB. On the long-term, we could imagine fetching only the addresses from the DB and streaming the response.

This is not 'ideal' yet because we are sorting and filtering on a plain list. Instead, we could have these operation at a DB-level, but this won't probably be achieved with acid-state. Perhaps if one day we switch to SQL

There was no integration tests for any accounts endpoints :s ... We should work on that in the future as I didn't take care of it in this commit / task, but solely add a few one for the newly added endpoints

Also, this rename the balance endpoint to '/api/v1/wallets/{id}/accounts/{ix}/amount' to reflect the actual name of the field in the Account representation.

1. Import and use `Example` typeclass in `../V1/Types.hs` instead of `Arbitrary` 2. Resolve circular dependencies `Errors.hs` -> `Types.hs` -> `Example.hs` -> `Response.hs` -> `Errors.hs` by moving `Example` instances from `Example.hs` to where the datatypes are defined, so that `Example.hs` does not need to import `Response.hs` or `Types.hs`.

…endpoints [CO-324] Accounts per-field endpoints

…e-typeclass [CO-327] Rely on Example type-class for Swagger API schema

As discussed here: https://github.com/input-output-hk/cardano-sl/pull/3210/files#r201989995, it doesn't make much sense to offer such capability right now as addresses are just a bunch of random characters (from the surface). Such feature could make sense with the new data-layer, but until then, we better disable it to prevent users from performing a costly operation

Weirdly, the conflict resolutions during rebase completely cut the end of some files in conflict. I was using a new tool for solving merge-conflict. Gotta be more careful

In order to maintain backwards compitability we also accept mnemonic words separated by spaces by `>>= (splitAt ' ')`

KtorZ · 2018-08-09T13:49:53Z

Merging with failing AppVeyor (timeout). We ought to do something about it, almost impossible to get a PR green and it takes an unreasonable amount of time.

KtorZ requested review from akegalj and adinapoli-iohk August 8, 2018 10:40

KtorZ mentioned this pull request Aug 8, 2018

[CBR-349] Pave the way for redemption #3373

Merged

11 tasks

adinapoli-iohk suggested changes Aug 8, 2018

View reviewed changes

adinapoli-iohk approved these changes Aug 8, 2018

View reviewed changes

akegalj approved these changes Aug 8, 2018

View reviewed changes

KtorZ force-pushed the Squad1/CO-325/api-v1-improvements-part-1 branch from bc3126a to b5e3d67 Compare August 8, 2018 15:38

akegalj reviewed Aug 8, 2018

View reviewed changes

Anviking reviewed Aug 8, 2018

View reviewed changes

KtorZ and others added 16 commits August 9, 2018 10:41

[CO-328] Add missing '--client' option in cURL API doc examples

cb623a9

Now that client TLS verification is activated, clients are expected to provide a valid TLS client certificate. cURL command given as examples in the documentaiton should reflect that.

Merge pull request #3212 from input-output-hk/KtorZ/CO-328/missing-cu…

bf70d0f

…rl-options-in-api-doc [CO-328] Add missing '--client' option in cURL API doc examples

[CO-324] Introduce pagination for getAccountAddresses

172d2e0

This is not 'ideal' yet because we are sorting and filtering on a plain list. Instead, we could have these operation at a DB-level, but this won't probably be achieved with acid-state. Perhaps if one day we switch to SQL

[CO-324] Extend integration test suites for account per-field endpoint

0800545

There was no integration tests for any accounts endpoints :s ... We should work on that in the future as I didn't take care of it in this commit / task, but solely add a few one for the newly added endpoints

[CO-324] Write about partial account representations in API doc

5820d55

Also, this rename the balance endpoint to '/api/v1/wallets/{id}/accounts/{ix}/amount' to reflect the actual name of the field in the Account representation.

Merge pull request #3210 from input-output-hk/KtorZ/CO-324/per-field-…

3e2a136

…endpoints [CO-324] Accounts per-field endpoints

Merge pull request #3215 from Anviking/anviking/CO-327/swagger-exampl…

59fc9f2

…e-typeclass [CO-327] Rely on Example type-class for Swagger API schema

[CO-325] Fix rebase issues

c6333a0

Weirdly, the conflict resolutions during rebase completely cut the end of some files in conflict. I was using a new tool for solving merge-conflict. Gotta be more careful

[CO-342] Customize FromJSON CBackupPhrase instance

009d987

In order to maintain backwards compitability we also accept mnemonic words separated by spaces by `>>= (splitAt ' ')`

[CO-342] Add comment

7b66499

[CO-342] Unit tests for legacy V0 Mnemonic JSON

916784a

[CO-342] Change documentation format

addf4f1

[CBR-325] Fix a few typos and link 'TODO's to actual tickets

e1d889d

KtorZ force-pushed the Squad1/CO-325/api-v1-improvements-part-1 branch from b5e3d67 to e1d889d Compare August 9, 2018 08:41

[CO-325] re-trigger CI

ff6716d

KtorZ merged commit 2c2c303 into develop Aug 9, 2018

KtorZ deleted the Squad1/CO-325/api-v1-improvements-part-1 branch August 16, 2018 14:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CO-325] API V1 Improvements - Part 1 #3372

[CO-325] API V1 Improvements - Part 1 #3372

KtorZ commented Aug 8, 2018

adinapoli-iohk left a comment

adinapoli-iohk Aug 8, 2018

adinapoli-iohk Aug 8, 2018

KtorZ Aug 8, 2018

adinapoli-iohk Aug 8, 2018

KtorZ Aug 8, 2018

adinapoli-iohk Aug 8, 2018

adinapoli-iohk Aug 8, 2018

KtorZ Aug 8, 2018

adinapoli-iohk Aug 8, 2018

adinapoli-iohk Aug 8, 2018

KtorZ commented Aug 8, 2018

akegalj left a comment

akegalj Aug 8, 2018

KtorZ Aug 8, 2018

akegalj Aug 8, 2018

KtorZ Aug 8, 2018

akegalj Aug 8, 2018

KtorZ Aug 8, 2018

erikd Aug 9, 2018

edsko Aug 9, 2018

akegalj Aug 8, 2018

Anviking Aug 8, 2018

akegalj Aug 8, 2018

Anviking Aug 8, 2018

akegalj Aug 9, 2018

DominikGuzei commented Aug 8, 2018

Anviking Aug 8, 2018

KtorZ commented Aug 9, 2018

[CO-325] API V1 Improvements - Part 1 #3372

[CO-325] API V1 Improvements - Part 1 #3372

Conversation

KtorZ commented Aug 8, 2018

Description

Linked issue

Type of change

Developer checklist

Testing checklist

QA Steps

Screenshots (if available)

adinapoli-iohk 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

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 commented Aug 8, 2018

akegalj 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

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

DominikGuzei commented Aug 8, 2018

Choose a reason for hiding this comment

KtorZ commented Aug 9, 2018