Wallet Management Remaining Endpoints: update & update passphrase #249

KtorZ · 2019-05-09T12:31:33Z

Issue Number

#220

Overview

I have implemented API handlers & wallet layer methods for updating wallet metadata and updating wallet passphrase
I have altered the API specification to remove 'required' for passphrase info on wallets. In practice, this can only happen now with very tight concurrent calls, but, it conveys the idea that wallet could exist without passphrase (which we'll come up to soon enough when re-introducing externally owned wallets).

Comments

rvl

Sweet - questions/comments below...

rvl · 2019-05-10T08:27:54Z

lib/core/src/Cardano/Wallet/Api/Types.hs

@@ -144,8 +148,8 @@ newtype WalletPutData = WalletPutData
    } deriving (Eq, Generic, Show)

 data WalletPutPassphraseData = WalletPutPassphraseData
-    { oldPassphrase :: !(ApiT (Passphrase "encryption"))
-    , newPassphrase :: !(ApiT (Passphrase "encryption"))
+    { oldPassphrase :: !(ApiT (Passphrase "encryption-old"))


Are these -old/-new types really needed?

No, this has purely a self-documenting purpose. Like most of our phantom types actually.

rvl · 2019-05-10T08:33:13Z

lib/core/test/unit/Cardano/WalletSpec.hs

+    -> (Passphrase "encryption-old", Passphrase "encryption-new")
+    -> Property
+walletUpdatePassphraseWrong wallet (xprv, pwd) (old, new) =
+    pwd /= coerce old ==> monadicIO $ liftIO $ do


Could the precondition be changed to be an assertion instead? i.e.

attempt `shouldBe` if pwd == coerce old then Left err else Right ()

Same goes for the other properties with preconditions.

Hmmm, could be. But I'd rather have two properties with two opposite pre-conditions than a single one with a variable evaluation. It makes things clearer when a test fail, what exactly is failing.

rvl · 2019-05-10T08:36:26Z

specifications/api/swagger.yaml

@@ -461,7 +461,6 @@ definitions:
      - balance
      - delegation
      - name
-      - passphrase


Just a question. Does this mean the swagger spec says that's it ok to create a wallet without a passphrase?

It's even stronger than that. It means that ultimately, it's okay to create a wallet without a root private key. We'll come to that soonish.

akegalj · 2019-05-10T13:04:00Z

lib/core/src/Cardano/Wallet.hs

+        meta <- _readWalletMeta wid
+        DB.putWalletMeta db (PrimaryKey wid) (modify meta)
+
+    , updateWalletPassphrase = \wid (old, new) -> do


I wonder what happens if there are two requests at the same time in a rece condition event both calling updateWalletPassphrase

There's a DB lock, so ultimately, they'll be sequentialized.

akegalj · 2019-05-10T13:07:48Z

lib/core/src/Cardano/Wallet.hs

+        -> ExceptT ErrNoSuchWallet IO ()
+    _attachPrivateKey wid (xprv, pwd) = do
+        hpwd <- liftIO $ encryptPassphrase pwd
+        DB.putPrivateKey db (PrimaryKey wid) (xprv, hpwd)


could this fail due to the race condition?

If there's no wallet? Yes, it would fail with ErrNoSuchWallet. Could happen if the wallet is deleted before the key gets added. That's okay and already handled.

MVar implementation shouldn't. I hope sql version won't as well

…functions

akegalj

I have few remarks on race condition. Don't have much experience there but it might be worth a look

akegalj · 2019-05-10T13:23:37Z

lib/core/src/Cardano/Wallet/Api/Server.hs

+            return ()
+        Just (ApiT wName) ->
+            liftHandler $ W.updateWallet w wid (\meta -> meta { name = wName })
+    getWallet w (ApiT wid)


hm - could we have race condition between L178 and L179?

on line L178 wallet gets updated

another request is made which changes content of the same wid to <something_else>

on line L179 we return <something_else> which is not expected

In more details:

thread A wants to update a wallet to "Foo" but is stoped just before L179

thread B runs and updates a wallet. It changes name to "Bar"

thread A is resumed and it runs L179. As a result it returns "Bar" (but what A expects is tu return "Foo")

ps - its no big deal for wallet name - and in fact request for updating a wallet is not something that will likely be at a race condition situation a lot (I don't see a reason why multiple users would need to change wallet name)

hm - could we have race condition between L178 and L179?

Yes. Could be. Though it doesn't matter much. I mean, the response will be slightly off in the case of concurrent calls. I don't think it's really important for things like metadata as long as, the internal DB state is updated correctly (which is the case with the lock mechanism).

akegalj · 2019-05-10T13:25:17Z

lib/core/src/Cardano/Wallet/Primitive/AddressDerivation.hs

@@ -7,6 +7,7 @@
 {-# LANGUAGE InstanceSigs #-}
 {-# LANGUAGE LambdaCase #-}
 {-# LANGUAGE MultiParamTypeClasses #-}
+{-# LANGUAGE RoleAnnotations #-}


hm - never used this before

Maybe I'll bring the topic on next week but, we could make more use of coerce and Coercible from the base package. This allows for coercing between newtypes that have a same (or compatible) underlying representation. So instead of paying the cost of unwrapping and then re-wrapping a newtype, we can just call coerce which has no runtime cost as everything is done at compile-time; it's like a special primitive from GHC / base.

In order to do that, GHC needs some extra role annotation to know whether things can be coerced safely or not.

…rivateKey'

…assphrase info

KtorZ requested a review from akegalj May 9, 2019 12:31

KtorZ self-assigned this May 9, 2019

KtorZ mentioned this pull request May 9, 2019

Technical Debts - Week 18 #220

Closed

9 tasks

rvl approved these changes May 10, 2019

View reviewed changes

KtorZ force-pushed the KtorZ/220/remaining-endpoints branch from e666879 to ae7f1e1 Compare May 10, 2019 09:23

akegalj reviewed May 10, 2019

View reviewed changes

KtorZ added 6 commits May 10, 2019 15:17

Extend 'WalletLayer' with an alteration method

3af21d2

fix database lock usage in Wallet Layer

a193877

implement handler for 'PUT /v2/wallets/{walletId}'

9af62cb

yield 'ErrNoRootKey' error from 'withRootKey' when there's no root key

d0297dd

implement passphrase update API handler and in wallet layer

b136e4c

implement a few tests for update wallet and update wallet passphrase …

bb4395c

…functions

KtorZ force-pushed the KtorZ/220/remaining-endpoints branch from ae7f1e1 to b6c72b4 Compare May 10, 2019 13:17

akegalj reviewed May 10, 2019

View reviewed changes

KtorZ added 3 commits May 10, 2019 15:42

Make passphase info a 'Maybe', and correctly set them within 'attachP…

bd24b13

…rivateKey'

re-generate 'ApiWallet.json' to include both cases with and without p…

7cdfe85

…assphrase info

add extra property test to verify passphrase gets updated correctly

9b635c5

KtorZ force-pushed the KtorZ/220/remaining-endpoints branch from b6c72b4 to 9b635c5 Compare May 10, 2019 13:43

KtorZ merged commit d573bae into master May 10, 2019

KtorZ deleted the KtorZ/220/remaining-endpoints branch May 10, 2019 14:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wallet Management Remaining Endpoints: update & update passphrase #249

Wallet Management Remaining Endpoints: update & update passphrase #249

KtorZ commented May 9, 2019

rvl left a comment

rvl May 10, 2019

KtorZ May 10, 2019

rvl May 10, 2019

KtorZ May 10, 2019

rvl May 10, 2019

KtorZ May 10, 2019

akegalj May 10, 2019

KtorZ May 10, 2019

akegalj May 10, 2019

KtorZ May 10, 2019

akegalj May 10, 2019

akegalj left a comment

akegalj May 10, 2019

akegalj May 10, 2019

akegalj May 10, 2019 •

edited

Loading

KtorZ May 10, 2019

akegalj May 10, 2019

KtorZ May 10, 2019

Wallet Management Remaining Endpoints: update & update passphrase #249

Wallet Management Remaining Endpoints: update & update passphrase #249

Conversation

KtorZ commented May 9, 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

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

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

akegalj May 10, 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

akegalj May 10, 2019 •

edited

Loading