Add very preliminary interface/structure for wallet getter and creation #62

paweljakubas · 2019-03-13T21:01:59Z

Issue Number

#23

Overview

I have added very preliminary wallet types, error types and createWallet/getWallet signature

Comments

rvl

Looks promising...

rvl · 2019-03-14T01:38:18Z

src/Cardano/Wallet/Kernel/Wallet.hs

+newtype WalletName = WalletName { _getWalletName :: Text }
+    deriving (Eq, Show)
+
+newtype WalletPassPhrase = WalletPassPhrase ScrubbedBytes


Could we instead use Passphrase "generation" defined in AddressDerivation?

definitely!

rvl · 2019-03-14T01:41:41Z

src/Cardano/Wallet/Kernel/Wallet.hs

+        :: !(Maybe (Mnemonic mw))
+    , _name
+        :: !WalletName
+    , _passphrase


Would this be a Passphrase "encryption"?
And the mnemonicSentencePassphrase be a Passphrase "generation"?
Also, are the passphrases optional? If so, what value would denote "no passphrase"?

Replaced module defined passphrase types with address derivation ones.

I looked into swagger.yaml description of API and it says:

110 wallet2ndFactor: &wallet2ndFactor 111 description: An optional passphrase used to encrypt the mnemonic sentence. 112 type: array 113 minItems: 9 114 maxItems: 12 115 items: 116 type: string 117 format: bip-0039-mnemonic-word{english} 118 example: ["squirrel", "material", "silly", "twice", "direct", "slush", "pistol", "razor", "become"]

rvl · 2019-03-14T01:42:35Z

src/Cardano/Wallet/Kernel/Wallet.hs

+data NewWallet =
+    forall mw . ValidMnemonicSentence mw => NewWallet
+    {
+      _mnemonicSentence


Did we settle the decision on how to generate lenses? TH?

good question

Generic through https://hackage.haskell.org/package/generic-lens 👍

Though, lenses is a topic we have to discuss in our coding standards before we start using them.

If we do...

added Generic derivation to handle generic-lens

KtorZ

On the right tracks. A few naming suggestion and, also, I'd recommend to draft and expose a WalletLayer interface in a similar fashion to what's done for the network layer (cf: https://github.com/input-output-hk/cardano-wallet/blob/master/src/Cardano/NetworkLayer.hs#L13).

This is in practice, what we'll expose and use from the "outer world"

KtorZ · 2019-03-14T08:12:49Z

src/Cardano/Wallet/Kernel/Wallet.hs

+-- | Module provides internal implementation of
+-- | wallet api
+
+module Cardano.Wallet.Kernel.Wallet where


Let's put that in Cardano.WalletLayer 👍

done, also added Cardano.WalletLayer to expose corresponding interface

KtorZ · 2019-03-14T08:14:17Z

src/Cardano/Wallet/Kernel/Wallet.hs

+    , name
+        :: !WalletName
+    , balance
+        :: !Coin


This, we compute from the wallet UTxOs directly 👍, no need to store or represent that at this level.

removed the balance field

KtorZ · 2019-03-14T08:16:37Z

src/Cardano/Wallet/Kernel/Wallet.hs

+newtype PassphraseInfo = PassphraseInfo { lastUpdated :: WalletTimestamp }
+    deriving (Eq, Show)
+
+data WalletState = WalletState {


What about calling this WalletMetadata ? Our "global" state for a wallet is actually the product (Wallet, WalletMetadata), with the Wallet being defined as in Cardano.Wallet

now calling this WalletMetadata

KtorZ · 2019-03-14T08:17:10Z

src/Cardano/Wallet/Kernel/Wallet.hs

+
+
+-- | Types
+newtype CreateWallet = CreateWallet NewWallet


What's the rational for this newtype 🤔 ?

I started with data but then got :

hlint src/Cardano/WalletLayer/Wallet.hs src/Cardano/WalletLayer/Wallet.hs:44:1: Suggestion: Use newtype instead of data Found: data CreateWallet = CreateWallet NewWallet Perhaps: newtype CreateWallet = CreateWallet NewWallet Note: decreases laziness

Haha, got it. I was intrigued by the need for an extra type though (regardless of the syntax used) ? What's wrong with using NewWallet directly? I believe it echoes a previous remark I made about getting a WalletLayer type, and I suspect that this was the premise of it 😅 ?

KtorZ · 2019-03-14T08:22:34Z

src/Cardano/Wallet/Kernel/Wallet.hs

+        <> "GetWalletError"
+
+instance Show GetWalletError where
+    show = fmt . build


tip: When you're grooming an initial interface like this, you can leave this kind of stuff for later and focus on getting the types right first.

yes, just wanted to signal that will be using fmt rather than formatting. Removed that

KtorZ · 2019-03-14T11:40:16Z

src/Cardano/WalletLayer.hs

+data WalletLayer m s = WalletLayer
+    { createWallet :: CreateWallet -> ExceptT WalletLayerError m (Wallet s)
+    , getWallet :: WalletId -> ExceptT WalletLayerError m (Wallet s)
+    }


Let's have separate error types for the different functions. I am not expecting getWallet to throw anything related to the creation of a wallet.

KtorZ · 2019-03-14T11:41:05Z

src/Cardano/WalletLayer/Wallet.hs

+-- | Errors
+data CreateWalletError
+
+data GetWalletError


KtorZ · 2019-03-14T11:41:25Z

src/Cardano/WalletLayer/Wallet.hs

+data GetWalletError
+
+-- | Types
+data CreateWallet = CreateWallet NewWallet | ImportWallet


This is the same thing in our new backend :)

expanded ImportWallet to ImportWallet (Key 'AccountK XPub)

src/Cardano/WalletLayer/Wallet.hs

KtorZ · 2019-03-14T11:44:44Z

src/Cardano/WalletLayer/Wallet.hs

+      _mnemonicSentence
+        :: !(Mnemonic 15)
+    , _mnemonicSentencePassphrase
+        :: !(Maybe (Passphrase "generation"))


The generation passphrase is represented as a short mnemonic (cf the API swagger spec). So we probably want to keep it the same at this stage and make the conversion later.

now using Mnemonic 9

src/Cardano/WalletLayer/Wallet.hs

KtorZ · 2019-03-14T11:46:52Z

src/Cardano/WalletLayer/Wallet.hs

+-- | Module provides internal implementation of
+-- | wallet api
+
+module Cardano.WalletLayer.Wallet


Let's have that in the same module as WalletLayer until we see a good reason to move them out :)

[23] Add getWallet and types in line with api spec [23] replace module define passphrase types with address derivation ones [23] Refining types and cutting superfluous code [23] Final fixes

paweljakubas requested review from Anviking and KtorZ March 13, 2019 21:01

rvl reviewed Mar 14, 2019

View reviewed changes

KtorZ assigned paweljakubas Mar 14, 2019

KtorZ reviewed Mar 14, 2019

View reviewed changes

paweljakubas force-pushed the paweljakubas/23/sketch-wallet-layer branch from f0d2662 to f3bdf42 Compare March 14, 2019 11:37

KtorZ reviewed Mar 14, 2019

View reviewed changes

paweljakubas force-pushed the paweljakubas/23/sketch-wallet-layer branch from f3bdf42 to c19da65 Compare March 14, 2019 13:24

[23] add interface/structure for wallet creation

c19da65

[23] Add getWallet and types in line with api spec [23] replace module define passphrase types with address derivation ones [23] Refining types and cutting superfluous code [23] Final fixes

KtorZ approved these changes Mar 14, 2019

View reviewed changes

KtorZ merged commit 847da46 into master Mar 14, 2019

paweljakubas mentioned this pull request Mar 14, 2019

Sketch wallet layer to fetch and create a wallet #23

Closed

2 tasks

paweljakubas deleted the paweljakubas/23/sketch-wallet-layer branch March 19, 2019 09:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add very preliminary interface/structure for wallet getter and creation #62

Add very preliminary interface/structure for wallet getter and creation #62

paweljakubas commented Mar 13, 2019

rvl left a comment

rvl Mar 14, 2019

paweljakubas Mar 14, 2019

rvl Mar 14, 2019

paweljakubas Mar 14, 2019

rvl Mar 14, 2019

paweljakubas Mar 14, 2019

KtorZ Mar 14, 2019

KtorZ Mar 14, 2019

KtorZ Mar 14, 2019

paweljakubas Mar 14, 2019

KtorZ left a comment

KtorZ Mar 14, 2019

paweljakubas Mar 14, 2019

KtorZ Mar 14, 2019

paweljakubas Mar 14, 2019

KtorZ Mar 14, 2019

paweljakubas Mar 14, 2019

KtorZ Mar 14, 2019

paweljakubas Mar 14, 2019 •

edited

Loading

KtorZ Mar 14, 2019

KtorZ Mar 14, 2019

paweljakubas Mar 14, 2019

KtorZ Mar 14, 2019

paweljakubas Mar 14, 2019

KtorZ Mar 14, 2019

KtorZ Mar 14, 2019

paweljakubas Mar 14, 2019

KtorZ Mar 14, 2019

paweljakubas Mar 14, 2019

KtorZ Mar 14, 2019

paweljakubas Mar 14, 2019

Add very preliminary interface/structure for wallet getter and creation #62

Add very preliminary interface/structure for wallet getter and creation #62

Conversation

paweljakubas commented Mar 13, 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

KtorZ 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

paweljakubas Mar 14, 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

paweljakubas Mar 14, 2019 •

edited

Loading