Fork API server context into random and sequential variants. #810

jonathanknowles · 2019-10-11T14:17:37Z

Issue Number

#808

Overview

This PR forks the API server context into two variants:

a random context that corresponds to Byron wallets.
a sequential context that corresponds to Shelley wallets.

Remaining Work

Discriminate between random and sequential wallets at the file-system level.

jonathanknowles · 2019-10-11T14:23:32Z

bors try

iohk-bors · 2019-10-11T15:16:07Z

try

Build succeeded

jonathanknowles · 2019-10-14T13:25:06Z

bors try

iohk-bors · 2019-10-14T13:55:07Z

try

Build succeeded

KtorZ · 2019-10-14T14:45:33Z

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

@@ -14,6 +14,8 @@
 {-# LANGUAGE TypeFamilies #-}
 {-# LANGUAGE TypeOperators #-}

+{-# OPTIONS_GHC -fno-warn-redundant-constraints #-}
+


Which one are they 🤔 ?

Which one are they?

I added this simply to avoid having to remove all the (currently unnecessary) constraints from Byron skeleton functions only to have to add them again when we fill in the function definitions (which will then require the constraints again).

KtorZ · 2019-10-14T14:48:45Z

lib/core/src/Cardano/Wallet/DB/Sqlite.hs

    -> FilePath
    -> IO [W.WalletId]
 findDatabases tr dir = do
    files <- listDirectory dir
    fmap catMaybes $ forM files $ \file -> do
        isFile <- doesFileExist (dir </> file)
-        let (basename:rest) = T.splitOn "." $ T.pack file
+        let (prefix : basename : rest) = T.splitOn "." $ T.pack file


This is now a partial function that may fail. We are sure that T.splitOn pattern txt will always return at least one element (the argument itself) but not necessarily two! In particular, if there's a file that isn't named with a prefix, this will crash badly.

Great observation. I had assumed that the original function was partial, and that (therefore) we didn't mind.

I'll remove the partiality.

The one was partial-safe haha. Perhaps that should be in our coding standard actually. Always add a comment to justify why a unsafe usage might actually be safe in some context. Here, that's my bad for not adding this comment. I usually do, but well, that's something we should be careful with.

@KtorZ wrote:

Always add a comment to justify why a unsafe usage might actually be safe in some context.

Agreed!

I've made another change which should have removed the partiality. Though let me know if you require any further changes.

KtorZ · 2019-10-14T14:49:58Z

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

@@ -395,6 +395,10 @@ class WalletKey (key :: Depth -> * -> *) where
        => key depth XPub
        -> Digest a

+    -- | Get a short, human-readable string descriptor that uniquely identifies
+    --   the specified key type.
+    keyTypeDescriptor :: Proxy key -> String


This instance is identical to that of the `(HttpBridge 'Mainnet) RndKey` type.

For `(Jormungandr n) RndKey` type. Note that this property is lifted directly from: lib/http-bridge/test/unit/Cardano/Wallet/HttpBridge/CompatibilitySpec.hs (present at commit: 117186d)

https://github.com/input-output-hk/cardano-wallet/wiki/Coding-Standards#limit-line-length-to-80-characters

Clarify comments and names of function arguments so that they clearly refer to either a database directory (containing database files) or a single database file.

…pes. We use the prefix "rnd" to denote database files of wallets where the address derivation style is random. We use the prefix "seq" to denote database files of wallets where the address derivation style is sequential.

jonathanknowles · 2019-10-14T15:58:16Z

bors try

KtorZ · 2019-10-14T16:05:40Z

bors r+

810: Fork API server context into random and sequential variants. r=KtorZ a=jonathanknowles # Issue Number #808 # Overview This PR forks the API server context into two variants: 1. a _**random**_ context that corresponds to _**Byron**_ wallets. 2. a _**sequential**_ context that corresponds to _**modern**_ wallets. # Remaining Work - [x] Discriminate between _**random**_ and _**sequential**_ wallets at the file-system level. 816: Network information endpoint r=KtorZ a=KtorZ # Issue Number  N/A # Overview  - [x] I have renamed 'WalletState' to 'SyncProgress', for we'll also use this in the network information endpoint and.. it truly is just about the syncing progress. - [x] I have reviewed the swagger specification network information endpoint to better reflect the information we're going to return. Basically, the `localBlockchainHeight` does not make much sense for us at this level, because there's actually a local height per wallet. So this information will better be located in each wallets directly. - [x] Added a field `pendingSince` for transactions. I shouldn't have done that in this PR but I did it as I was updating the swagger. This is not related to the networking endpoint so it might get a little confusing in the review. - [x] I've also re-generated all the golden API tests because I was getting a lot of warnings about files not being up-to-date (nothing to do with this PR but it seems that we haven't updated them along the way....) # Comments  Some screenshot of the swagger specification: ![Screenshot from 2019-10-14 12-09-15](https://user-images.githubusercontent.com/5680256/66757422-72cf2e00-ee9c-11e9-8740-8063689c0985.png) ![Screenshot from 2019-10-14 12-06-26](https://user-images.githubusercontent.com/5680256/66757428-75ca1e80-ee9c-11e9-82f4-6c90c723e9cd.png)  Co-authored-by: Jonathan Knowles <[email protected]> Co-authored-by: KtorZ <[email protected]>

iohk-bors · 2019-10-14T16:23:09Z

try

Build succeeded

iohk-bors · 2019-10-14T16:43:26Z

Build failed (retrying...)

buildkite/cardano-wallet

810: Fork API server context into random and sequential variants. r=KtorZ a=jonathanknowles # Issue Number #808 # Overview This PR forks the API server context into two variants: 1. a _**random**_ context that corresponds to _**Byron**_ wallets. 2. a _**sequential**_ context that corresponds to _**modern**_ wallets. # Remaining Work - [x] Discriminate between _**random**_ and _**sequential**_ wallets at the file-system level. Co-authored-by: Jonathan Knowles <[email protected]>

iohk-bors · 2019-10-14T17:31:23Z

Build failed

buildkite/cardano-wallet

jonathanknowles · 2019-10-15T07:31:31Z

bors r+

810: Fork API server context into random and sequential variants. r=jonathanknowles a=jonathanknowles # Issue Number #808 # Overview This PR forks the API server context into two variants: 1. a _**random**_ context that corresponds to _**Byron**_ wallets. 2. a _**sequential**_ context that corresponds to _**modern**_ wallets. # Remaining Work - [x] Discriminate between _**random**_ and _**sequential**_ wallets at the file-system level. Co-authored-by: Jonathan Knowles <[email protected]>

iohk-bors · 2019-10-15T07:54:20Z

Build succeeded

jonathanknowles self-assigned this Oct 11, 2019

jonathanknowles mentioned this pull request Oct 11, 2019

Fork API server context into random and sequential variants. #808

Closed

6 tasks

jonathanknowles force-pushed the jonathanknowles/split-context branch from 8012469 to c269a0b Compare October 11, 2019 14:23

iohk-bors bot added a commit that referenced this pull request Oct 11, 2019

Try #810:

9b4c8e7

jonathanknowles force-pushed the jonathanknowles/split-context branch 2 times, most recently from 5ecfe69 to c40e7e2 Compare October 14, 2019 13:24

jonathanknowles marked this pull request as ready for review October 14, 2019 13:24

iohk-bors bot added a commit that referenced this pull request Oct 14, 2019

Try #810:

0149685

KtorZ suggested changes Oct 14, 2019

View reviewed changes

jonathanknowles added 8 commits October 14, 2019 15:23

Provide KeyToAddress instance for (Jormungandr n) RndKey type.

57f6a8e

This instance is identical to that of the `(HttpBridge 'Mainnet) RndKey` type.

Add prop_derivedKeysAreOurs property for KeyToAddress instance.

c1bc555

For `(Jormungandr n) RndKey` type. Note that this property is lifted directly from: lib/http-bridge/test/unit/Cardano/Wallet/HttpBridge/CompatibilitySpec.hs (present at commit: 117186d)

Add comment explaining the lack of network discrimination.

a3c3568

Fork wallet API server context into random and sequential variants.

13c4d5c

Remove statement to suppress warning about redundant constraints.

534f6bf

Fix code style in module Cardano.Wallet.DB.Sqlite.

d18896f

https://github.com/input-output-hk/cardano-wallet/wiki/Coding-Standards#limit-line-length-to-80-characters

Distinguish between database directories and database files.

3f250d9

Clarify comments and names of function arguments so that they clearly refer to either a database directory (containing database files) or a single database file.

jonathanknowles force-pushed the jonathanknowles/split-context branch from c40e7e2 to 042e52b Compare October 14, 2019 15:39

Improve comment for function findDatabases.

26c5866

iohk-bors bot added a commit that referenced this pull request Oct 14, 2019

Try #810:

f37d222

KtorZ approved these changes Oct 14, 2019

View reviewed changes

iohk-bors bot merged commit 26c5866 into master Oct 15, 2019

KtorZ deleted the jonathanknowles/split-context branch October 16, 2019 12:11

KtorZ added this to the Byron Wallet Support milestone Oct 17, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fork API server context into random and sequential variants. #810

Fork API server context into random and sequential variants. #810

jonathanknowles commented Oct 11, 2019 •

edited

Loading

jonathanknowles commented Oct 11, 2019

iohk-bors bot commented Oct 11, 2019

jonathanknowles commented Oct 14, 2019

iohk-bors bot commented Oct 14, 2019

KtorZ Oct 14, 2019

jonathanknowles Oct 14, 2019

KtorZ Oct 14, 2019

jonathanknowles Oct 14, 2019

KtorZ Oct 14, 2019

jonathanknowles Oct 14, 2019

KtorZ Oct 14, 2019

jonathanknowles commented Oct 14, 2019

KtorZ commented Oct 14, 2019

iohk-bors bot commented Oct 14, 2019

iohk-bors bot commented Oct 14, 2019

iohk-bors bot commented Oct 14, 2019

jonathanknowles commented Oct 15, 2019

iohk-bors bot commented Oct 15, 2019

Fork API server context into random and sequential variants. #810

Fork API server context into random and sequential variants. #810

Conversation

jonathanknowles commented Oct 11, 2019 • edited Loading

Issue Number

Overview

Remaining Work

jonathanknowles commented Oct 11, 2019

iohk-bors bot commented Oct 11, 2019

try

Build succeeded

jonathanknowles commented Oct 14, 2019

iohk-bors bot commented Oct 14, 2019

try

Build succeeded

KtorZ Oct 14, 2019

Choose a reason for hiding this comment

jonathanknowles Oct 14, 2019

Choose a reason for hiding this comment

KtorZ Oct 14, 2019

Choose a reason for hiding this comment

jonathanknowles Oct 14, 2019

Choose a reason for hiding this comment

KtorZ Oct 14, 2019

Choose a reason for hiding this comment

jonathanknowles Oct 14, 2019

Choose a reason for hiding this comment

KtorZ Oct 14, 2019

Choose a reason for hiding this comment

jonathanknowles commented Oct 14, 2019

KtorZ commented Oct 14, 2019

iohk-bors bot commented Oct 14, 2019

try

Build succeeded

iohk-bors bot commented Oct 14, 2019

Build failed (retrying...)

iohk-bors bot commented Oct 14, 2019

Build failed

jonathanknowles commented Oct 15, 2019

iohk-bors bot commented Oct 15, 2019

Build succeeded

jonathanknowles commented Oct 11, 2019 •

edited

Loading