Implement basic API functions for Byron-style wallets. #817

jonathanknowles · 2019-10-14T15:17:44Z

Issue Number

#774 (restoreByronWallet)
#775 (listByronWallets)
#776 (getByronWallet)
#777 (deleteByronWallet)

Overview

This PR implements all API functions apart from those relating to migration.

We have:

Provided an implementation for restoreByronWallet.
Provided an implementation for listByronWallets.
Provided an implementation for getByronWallets.
Provided an implementation for deleteByronWallet.
Fixed existing integration tests for all of the above.

Not included in this PR

Further integration tests: these will be included in further sub-topic PRs.

piotr-iohk

looks good. Integration tests need to be expanded to assert also more for actual response also, not only response code. But this can be done in the next pr.

…ontext.

jonathanknowles · 2019-10-15T13:39:39Z

bors try

jonathanknowles · 2019-10-15T13:41:51Z

looks good.

Thanks for reviewing this! 😄

Integration tests need to be expanded to assert also more for actual response also, not only response code. But this can be done in the next pr.

It was indeed my intention to add more integration tests in further PRs.

paweljakubas · 2019-10-15T15:18:10Z

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

+    void
+        . liftHandler
+        . createWallet ctx wid name
+        . mkRndState rootXPrv
+        =<< liftIO (getStdRandom random)


just nitpicking - this supposed to increase readability? 🤓

I think it's fairly readable if you read it from bottom to top. (The direction is consistent.)

We can read it like this:

Take the output of getStdRandom and pass it to mkRndState.

Take the output of mkRndState and pass it to createWallet.

Take the output of createWallet and pass it to liftHandler.

Finally, take the output of liftHandler and pass it to void.

Of course, "step" 1 is slightly different from the others as getStdRandom produces its output in a monadic context, so we need to bind (=<<) rather than compose (`.').

yeah, I know how it flows ;-) And as I read from left to right, this structure forced me to do the opposite :-)

yeah, I know how it flows ;-) And as I read from left to right, this structure forced me to do the opposite :-)

Fair enough! But then surely the same applies to any composition of functions:

foo :: SomeComplicatedType foo = f . g . h

In the above example, data flows (conceptually) from right to left. If we eta-expand, we get:

foo :: SomeComplicatedType foo x = f (g (h x))

I tend to agree with @paweljakubas on that one 😬 ... I don't mind much the function composition, but the bind >>= makes the whole not very readable IMO.

paweljakubas · 2019-10-15T15:19:11Z

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

-listByronWallets _ = throwError err501
+listByronWallets ctx = do
+    wids <- liftIO $ Registry.keys re
+    fmap fst . sortOn snd <$>


don't we want sortOn (Down . snd) ?

This should be fundamentally the same as listWallets, which is defined as:

listWallets ctx = do wids <- liftIO $ Registry.keys re fmap fst . sortOn snd <$> mapM (getWalletWithCreationTime mkApiWallet ctx) (ApiT <$> wids)

paweljakubas · 2019-10-15T15:22:16Z

lib/core-integration/src/Test/Integration/Framework/DSL.hs

@@ -987,8 +987,14 @@ tearDown ctx = do
        let endpoint = "v2/wallets" </> wal ^. walletId
        d <- request @Value ctx ("DELETE", endpoint) None Empty
        expectResponseCode HTTP.status204 d
+    respByron <-


we don't want to have separate tearByronDown? And check if tearing down byron wallet do not affect wallet? Etc?

@paweljakubas wrote:

we don't want to have separate tearByronDown? And check if tearing down byron wallet do not affect wallet? Etc?

@piotr-iohk What's your opinion on this?

such tests (i.e. trying to delete byron with new endpoint and vice versa) I was planning to add in the ByronWallet.hs suite actually. They are planned in the spreadsheet for now:
https://docs.google.com/spreadsheets/d/1fadIA_j4nCd3FbylPo5J8K9Fy6tixSC2T9V3xHKuUvU/edit#gid=0

BYRON_DELETE

ok, so tearDown should be ok after that change. and there is no need for separate tearByronDown

paweljakubas

Looks fine to me!

jonathanknowles · 2019-10-15T15:30:00Z

@paweljakubas wrote:

Looks fine to me!

Thanks for looking at this!

jonathanknowles · 2019-10-15T16:09:01Z

bors r+

817: Implement basic API functions for Byron-style wallets. r=jonathanknowles a=jonathanknowles # Issue Number #774 (`restoreByronWallet`) #775 (`listByronWallets`) #776 (`getByronWallet`) #777 (`deleteByronWallet`) # Overview This PR implements all API functions _**apart from**_ those relating to _**migration**_. We have: - [x] Provided an implementation for [`restoreByronWallet`](#774). - [x] Provided an implementation for [`listByronWallets`](#775). - [x] Provided an implementation for [`getByronWallets`](#776). - [x] Provided an implementation for [`deleteByronWallet`](#777). - [x] Fixed **existing** integration tests for all of the above. # Not included in this PR Further integration tests: these will be included in _**further**_ sub-topic PRs. Co-authored-by: Jonathan Knowles <[email protected]>

iohk-bors · 2019-10-15T16:32:26Z

try

Build succeeded

iohk-bors · 2019-10-15T17:12:16Z

Build succeeded

jonathanknowles self-assigned this Oct 14, 2019

jonathanknowles force-pushed the jonathanknowles/basic-byron-wallet-functions branch 4 times, most recently from ab9372f to df6920e Compare October 15, 2019 10:04

This was referenced Oct 15, 2019

Add API endpoint to delete a legacy wallet. #777

Closed

Add API endpoint to fetch information about a legacy wallet. #776

Closed

Add API endpoint to list all legacy wallets. #775

Closed

Add API endpoint to restore a legacy wallet. #774

Closed

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

Try #817:

725ea1c

jonathanknowles force-pushed the jonathanknowles/basic-byron-wallet-functions branch from ff00b64 to 1853d83 Compare October 15, 2019 13:05

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

Try #817:

3874d82

jonathanknowles changed the title ~~WIP: Basic Byron Wallet Functions~~ Basic Byron Wallet Functions Oct 15, 2019

jonathanknowles changed the title ~~Basic Byron Wallet Functions~~ Implement basic API functions for Byron-style wallets. Oct 15, 2019

jonathanknowles requested review from piotr-iohk and KtorZ October 15, 2019 13:08

piotr-iohk approved these changes Oct 15, 2019

View reviewed changes

jonathanknowles added 5 commits October 15, 2019 13:22

[Refactoring] Move definition of pure config value out of monadic c…

95ba7a8

…ontext.

Provide implementation of getByronWallet.

9828cac

Provide implementation of listByronWallets.

0dad2b4

Provide implementation of deleteByronWallet.

61c4a72

Provide implementation of restoreByronWallet.

f4b724b

jonathanknowles force-pushed the jonathanknowles/basic-byron-wallet-functions branch from 1853d83 to f4b724b Compare October 15, 2019 13:39

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

Try #817:

a8ff639

jonathanknowles added 4 commits October 15, 2019 13:48

[Refactoring] Extract out function getWalletTip.

5b8ac3f

[Refactoring] Extract out function getWalletBalance.

3a35f94

[Reformatting] Save vertical space.

7d0a07f

[Reformatting] Obey line length limit.

9bfc3d9

cardano-foundation deleted a comment from iohk-bors bot Oct 15, 2019

[Refactoring] Generalize function getWalletWithCreationTime.

8ae6769

cardano-foundation deleted a comment from iohk-bors bot Oct 15, 2019

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

Try #817:

0ddc406

cardano-foundation deleted a comment from iohk-bors bot Oct 15, 2019

paweljakubas reviewed Oct 15, 2019

View reviewed changes

paweljakubas approved these changes Oct 15, 2019

View reviewed changes

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

Try #817:

e7147d6

jonathanknowles removed the request for review from KtorZ October 15, 2019 16:08

iohk-bors bot merged commit 8ae6769 into master Oct 15, 2019

KtorZ deleted the jonathanknowles/basic-byron-wallet-functions 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

Implement basic API functions for Byron-style wallets. #817

Implement basic API functions for Byron-style wallets. #817

jonathanknowles commented Oct 14, 2019 •

edited

Loading

piotr-iohk left a comment

jonathanknowles commented Oct 15, 2019

jonathanknowles commented Oct 15, 2019

paweljakubas Oct 15, 2019

jonathanknowles Oct 15, 2019

paweljakubas Oct 15, 2019

jonathanknowles Oct 15, 2019 •

edited

Loading

KtorZ Oct 15, 2019

paweljakubas Oct 15, 2019

jonathanknowles Oct 15, 2019

paweljakubas Oct 15, 2019

jonathanknowles Oct 15, 2019 •

edited

Loading

piotr-iohk Oct 15, 2019

paweljakubas Oct 15, 2019

paweljakubas left a comment

jonathanknowles commented Oct 15, 2019 •

edited

Loading

jonathanknowles commented Oct 15, 2019

iohk-bors bot commented Oct 15, 2019

iohk-bors bot commented Oct 15, 2019

Implement basic API functions for Byron-style wallets. #817

Implement basic API functions for Byron-style wallets. #817

Conversation

jonathanknowles commented Oct 14, 2019 • edited Loading

Issue Number

Overview

Not included in this PR

piotr-iohk left a comment

Choose a reason for hiding this comment

jonathanknowles commented Oct 15, 2019

jonathanknowles commented Oct 15, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jonathanknowles Oct 15, 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

jonathanknowles Oct 15, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paweljakubas left a comment

Choose a reason for hiding this comment

jonathanknowles commented Oct 15, 2019 • edited Loading

jonathanknowles commented Oct 15, 2019

iohk-bors bot commented Oct 15, 2019

try

Build succeeded

iohk-bors bot commented Oct 15, 2019

Build succeeded

jonathanknowles commented Oct 14, 2019 •

edited

Loading

jonathanknowles Oct 15, 2019 •

edited

Loading

jonathanknowles Oct 15, 2019 •

edited

Loading

jonathanknowles commented Oct 15, 2019 •

edited

Loading