Fix CLI port argument bug. #407

paweljakubas · 2019-06-12T22:14:35Z

Issue Number

#401

Overview

I have added integration tests illustrating the problem
I have changed server by introducing PortOption datatype and introduced logic decided by team discussion
I have adopted exe cli

Comments

jonathanknowles

Hi @paweljakubas

Thanks for looking at this issue!

I think this PR is generally good, though I also think there are some areas that we should fix. I've highlighted these areas in comments, along with some suggestions. I'd be interested to hear your thoughts.

Cheers!

jonathanknowles · 2019-06-13T04:37:23Z

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

@@ -139,15 +140,19 @@ import qualified Data.Text as T
 import qualified Data.Text.Encoding as T
 import qualified Network.Wai.Handler.Warp as Warp

+
+data PortOption = RandomPort | ExactPort (Maybe Warp.Port)
+    deriving Show


By using the Maybe type here, I think it's not obvious what the semantics are if we specify ExactPort Nothing.

Perhaps we can make this clearer by using the following options:

-- | Specifies a port on which the application server will listen. data PortOption = DefaultPort -- ^ Listen on the default port (see 'defaultPort'). | RandomPort -- ^ Select an unused port at random on which to listen. | SpecificPort Warp.Port -- ^ Listen on a specific port. deriving Show

And then separately define:

-- | The default port on which to listen. The application server will listen -- on this port if 'start' is called with 'DefaultPort'. defaultPort :: Warp.Port defaultPort = 9080

What do you think?

This has been addressed 👍

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

jonathanknowles · 2019-06-13T04:57:23Z

lib/core/test/integration/Test/Integration/Framework/DSL.hs

@@ -762,6 +769,8 @@ createWalletViaCLI args mnemonics secondFactor passphrase = do
        err <- TIO.hGetContents stderr
        return (c, T.unpack out, err)

+
+


Here there should only be one blank line, rather than three.

https://github.com/input-output-hk/cardano-wallet/wiki/Coding-Standards#use-only-a-single-blank-line-between-top-level-definitions

jonathanknowles · 2019-06-13T04:57:49Z

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

@@ -139,15 +140,19 @@ import qualified Data.Text as T
 import qualified Data.Text.Encoding as T
 import qualified Network.Wai.Handler.Warp as Warp

+


https://github.com/input-output-hk/cardano-wallet/wiki/Coding-Standards#use-only-a-single-blank-line-between-top-level-definitions

jonathanknowles · 2019-06-13T05:04:03Z

lib/core/test/integration/Test/Integration/Scenario/CLI/Wallets.hs

-            T.unpack err `shouldContain` "Ok.\n"
-            out `shouldContain` walletName
+            checkSimpleCreateWallet
+                (Just "1337")


What is the relevance of the number 1337? Is it an arbitrarily-chosen number, or should it be the same as a number somewhere else?

Let's avoid the hard-coded number here indeed. Or at the very least, avoid repeating it through the whole file 🙏

lib/core/test/integration/Test/Integration/Scenario/CLI/Wallets.hs

lib/http-bridge/test/integration/Main.hs

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

exe/launcher/Main.hs

KtorZ · 2019-06-13T07:55:11Z

exe/launcher/Main.hs

-        ] ++ dbArg
+        ] ++ portArg ++ bridgePortArg ++ dbArg
+    portArg = maybe ["--random-port"] (\p -> ["--port", T.unpack (toText p)]) wp
+    bridgePortArg = ["--bridge-port", T.unpack (toText np)]


Suggested change

bridgePortArg = ["--bridge-port", T.unpack (toText np)]

args = mconcat

[ [ "server" ]

, [ "--network", if net == "local" then "testnet" else net ]

, maybe ["--random-port"] (\p -> ["--port", showT p]) wp

, [ "--bridge-port", showT np ]

]

showT = T.unpack . toText

KtorZ · 2019-06-13T07:55:54Z

exe/wallet/Main.hs

@@ -140,7 +140,7 @@ and can be run "offline". (e.g. 'generate mnemonic')
    ⚠️  Options are positional (--a --b is not equivalent to --b --a) ! ⚠️

 Usage:
-  cardano-wallet server [--network=STRING] [--port=INT] [--bridge-port=INT] [--database=FILE]
+  cardano-wallet server [--network=STRING] [--port=INT] [--random-port] [--bridge-port=INT] [--database=FILE]


Same here about --port=INT and --random-port being mutually exclusive.

KtorZ · 2019-06-13T07:57:53Z

lib/core/test/integration/Test/Integration/Scenario/CLI/Wallets.hs

-            T.unpack err `shouldContain` "Ok.\n"
-            out `shouldContain` walletName
+            checkSimpleCreateWallet
+                (Just "1337")


Let's avoid the hard-coded number here indeed. Or at the very least, avoid repeating it through the whole file 🙏

KtorZ · 2019-06-13T08:02:52Z

lib/core/test/integration/Test/Integration/Framework/DSL.hs

+                Just port ->
+                    [ "exec", "--", "cardano-wallet"
+                    , "wallet", "create", "--port", port
+                    ] ++ args


Suggested change

] ++ args

let portArg =

maybe [] (\p -> ["--port", p]) portM

let fullArgs =

[ "exec", "--", "cardano-wallet", "wallet", "create" ]

++ portArg ++ args

KtorZ · 2019-06-13T08:05:54Z

lib/http-bridge/test/integration/Main.hs

+            db <- MVar.newDBLayer
+            let tl = HttpBridge.newTransactionLayer
+            wallet <- newWalletLayer nullTracer block0 db nl tl
+            Server.start (putMVar port) walletListen wallet


That's a good idea and a good way to test that the random port binding works as expected 🎉

KtorZ · 2019-06-13T08:07:03Z

lib/http-bridge/test/integration/Main.hs

@@ -158,6 +168,8 @@ main = hspec $ do
        -- some places where it's less annoying.
        (UseHandle h)

+    mkBaseUrl port = "http://localhost:" <> T.pack (show port) <> "/"


You probably need toText here instead of show since we have port :: Port I believe.

KtorZ · 2019-06-13T08:08:24Z

lib/http-bridge/test/integration/Main.hs

@@ -71,13 +77,17 @@ main = hspec $ do
    describe "Cardano.WalletSpec" Wallet.spec
    describe "Cardano.Wallet.HttpBridge.NetworkSpec" HttpBridge.spec
    describe "CLI commands not requiring bridge" MnemonicsCLI.spec
-    beforeAll startCluster $ afterAll killCluster $ after tearDown $ do
+    beforeAll (startCluster ListenOnRandomPort) $ afterAll killCluster $ after tearDown $ do


This shouldn't be a problem for the API spec as long as we sort out the baseUrl in the manager, but the CLI specs also need to know about the port; So we probably gotta add it to the Context I believe.

I change it from IO () to IO Warp.Port in start Server in Cardano.Wallet.Api.Server now we need to accommodate it in integration tests. And we will be able to use it when random port mode is ON in integration tests

KtorZ · 2019-06-13T08:09:38Z

lib/http-bridge/test/integration/Main.hs

+
+    let defaultPort = 8090
+    beforeAll (startCluster (ListenOnPort defaultPort)) $ afterAll killCluster $ after tearDown $ do
+        describe "Port tests using wallets CLI" (WalletsCLI.specPorts defaultPort)


What's the rationale for making this one completely separated ? It's quite heavy to start a whole new cluster. Perhaps what we really need here is simply to start another wallet server listening on given port. That can be tested without requiring a new cluster.

+1. That'd also, sort of, test the case of two wallet's attached to a single server.

KtorZ · 2019-06-13T08:10:45Z

lib/core/test/integration/Test/Integration/Scenario/CLI/Wallets.hs

+    -> String
+    -> (ExitCode -> IO ())
+    -> (String -> IO ())
+    -> (String -> IO ())


not found of this helper, it's really "stringly-typed" and is more cryptic than readable IMO. I'd rather inline it in all the test above as it reads better inlined.

jonathanknowles · 2019-06-13T08:19:37Z

exe/launcher/Main.hs

@@ -118,16 +123,16 @@ nodeHttpBridgeOn stateDir port net =
        ] ++ networkArg
    networkArg = maybe [] (\d -> ["--networks-dir", d]) stateDir

-walletOn :: Maybe FilePath -> Port "Wallet" -> Port "Node" -> String -> Command
+walletOn :: Maybe FilePath -> Maybe (Port "Wallet") -> Port "Node" -> String -> Command


This line is now too long (https://github.com/input-output-hk/cardano-wallet/wiki/Coding-Standards#limit-line-length-to-80-characters).

Suggest:

Suggested change

walletOn :: Maybe FilePath -> Maybe (Port "Wallet") -> Port "Node" -> String -> Command

walletOn

:: Maybe FilePath

-> Maybe (Port "Wallet")

-> Port "Node"

-> String

-> Command

jonathanknowles · 2019-06-13T08:25:05Z

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

@@ -170,17 +179,17 @@ startOnSocket settings socket wl = Warp.runSettingsSocket settings socket
    application :: Application
    application = serve (Proxy @("v2" :> Api t)) server

-- | Run an action with a TCP socket bound to a port. If no port is specified,
+-- | Run an action with a TCP socket bound to a port. If random port is chosen
 -- then an unused port will be selected at random.


This comment sounds a bit tautological. Suggest:

Suggested change

-- then an unused port will be selected at random.

-- | Run an action with a TCP socket bound to a port specified by the `Listen`

-- parameter.

jonathanknowles · 2019-06-13T08:27:50Z

lib/http-bridge/test/integration/Main.hs

        describe "Wallets API endpoint tests" Wallets.spec
        describe "Transactions API endpoint tests" Transactions.spec
        describe "Addresses API endpoint tests" Addresses.spec
        describe "Wallets CLI tests" WalletsCLI.spec
        describe "Transactions CLI tests" TransactionsCLI.spec
        describe "Addresses CLI tests" AddressesCLI.spec
+
+    let defaultPort = 8090
+    beforeAll (startCluster (ListenOnPort defaultPort)) $ afterAll killCluster $ after tearDown $ do


Let's try to keep lines to the length specified by https://github.com/input-output-hk/cardano-wallet/wiki/Coding-Standards#limit-line-length-to-80-characters

piotr-iohk · 2019-06-13T13:31:50Z

lib/core/test/integration/Test/Integration/Scenario/CLI/Wallets.hs

+    (c, out, err) <- createWalletViaCLI [walletName] mnemonics "\n" passphrase portM
+    exitCodeCheck c
+    errCheck (T.unpack err)
+    outCheck out


I understand that this check just landed here in this method, because it was there in the code in the first place :) Actually it is quite poor and I was planning to add more detailed checks (like here: https://github.com/input-output-hk/cardano-wallet/blob/master/lib/core/test/integration/Test/Integration/Scenario/CLI/Transactions.hs#L92-L97) but didn't get there yet for Wallets.sh. (ofc that's not in scope of this pr)

For that matter, I'd be rather for separating action and assertion not to be in a single method (but perhaps, that's just a personal preference)

Rationale here is to simplify a bit the requirement and busy work with the launcher that we're going to remove / merge into the wallet cli soon.

KtorZ

Changes LGTM. I've picked up the PR to resolve issues with the integration tests and reworked a bit the way integration scenario receive their corresponding port and made sure to thoroughly test every CLI commands.

comments have been addressed or are now outdated 👍

KtorZ · 2019-06-13T16:17:50Z

@piotr-iohk May you have another quick look at the PR 🙏 , especially the integration test part. I used some generic-lens magic to avoid making numerous changes to the rest of the code. It's pretty neat in the end and enables a great coverage for all commands of the CLI:

--port CLI tests
    with default port
      PORT_01 - Can't reach server with wrong port (wallet list)
      PORT_01 - Can't reach server with wrong port (wallet create)
      PORT_01 - Can't reach server with wrong port (wallet get)
      PORT_01 - Can't reach server with wrong port (wallet delete)
      PORT_01 - Can't reach server with wrong port (wallet update)
      PORT_01 - Can't reach server with wrong port (transction create)
      PORT_01 - Can't reach server with wrong port (address list)
      PORT_02 - Can omit --port when server uses default port (wallet list)
      PORT_02 - Can omit --port when server uses default port (wallet create)
      PORT_02 - Can omit --port when server uses default port (wallet get)
      PORT_02 - Can omit --port when server uses default port (wallet delete)
      PORT_02 - Can omit --port when server uses default port (wallet update)
      PORT_02 - Can omit --port when server uses default port (transaction create)
      PORT_02 - Can omit --port when server uses default port (address list)
    with specified port
      PORT_01 - Can't reach server with wrong port (wallet list)
      PORT_01 - Can't reach server with wrong port (wallet create)
      PORT_01 - Can't reach server with wrong port (wallet get)
      PORT_01 - Can't reach server with wrong port (wallet delete)
      PORT_01 - Can't reach server with wrong port (wallet update)
      PORT_01 - Can't reach server with wrong port (transction create)
      PORT_01 - Can't reach server with wrong port (address list)
    with random port
      PORT_01 - Can't reach server with wrong port (wallet list)
      PORT_01 - Can't reach server with wrong port (wallet create)
      PORT_01 - Can't reach server with wrong port (wallet get)
      PORT_01 - Can't reach server with wrong port (wallet delete)
      PORT_01 - Can't reach server with wrong port (wallet update)
      PORT_01 - Can't reach server with wrong port (transction create)
      PORT_01 - Can't reach server with wrong port (address list)
      PORT_03 - random port != default port
      PORT_03 - Cannot omit --port when server uses random port (wallet list)
      PORT_03 - Cannot omit --port when server uses random port (wallet create)
      PORT_03 - Cannot omit --port when server uses random port (wallet get)
      PORT_03 - Cannot omit --port when server uses random port (wallet delete)
      PORT_03 - Cannot omit --port when server uses random port (wallet update)
      PORT_03 - Cannot omit --port when server uses random port (transaction create)
      PORT_03 - Cannot omit --port when server uses random port (address list)

let me know.

There's a bit of Haskell lens magic here. But that's for a greater good.

paweljakubas requested review from rvl, KtorZ and piotr-iohk June 12, 2019 22:14

jonathanknowles previously requested changes Jun 13, 2019

View reviewed changes

jonathanknowles changed the title ~~port arg bug~~ Fix CLI port argument bug. Jun 13, 2019

rvl force-pushed the paweljakubas/401/port-arg-bug branch from fb2f74a to 160a1dc Compare June 13, 2019 06:22

jonathanknowles reviewed Jun 13, 2019

View reviewed changes

lib/core/src/Cardano/Wallet/Api/Server.hs Outdated Show resolved Hide resolved

KtorZ suggested changes Jun 13, 2019

View reviewed changes

jonathanknowles reviewed Jun 13, 2019

View reviewed changes

KtorZ assigned paweljakubas Jun 13, 2019

introduce PortOption datatype and tweak logic in api server

52402b4

piotr-iohk reviewed Jun 13, 2019

View reviewed changes

KtorZ added 2 commits June 13, 2019 18:12

force launcher to use --random-port for wallet

b2b4433

Rationale here is to simplify a bit the requirement and busy work with the launcher that we're going to remove / merge into the wallet cli soon.

fix docopt port & random-port declaration

2c4ea5b

KtorZ force-pushed the paweljakubas/401/port-arg-bug branch from d1c9f90 to eb32f30 Compare June 13, 2019 16:13

KtorZ approved these changes Jun 13, 2019

View reviewed changes

jonathanknowles mentioned this pull request Jun 13, 2019

CLI port argument marked as optional but actually required when creating wallet. #401

Closed

KtorZ added 3 commits June 13, 2019 18:42

Review tests for --port & fixes how the port get passed through tests

15aee06

There's a bit of Haskell lens magic here. But that's for a greater good.

add more tests about the --port to cover every command of the CLI

0aa67a7

re-generate nix packages

264c31f

KtorZ force-pushed the paweljakubas/401/port-arg-bug branch from eb32f30 to 264c31f Compare June 13, 2019 16:42

KtorZ merged commit cb7744e into master Jun 13, 2019

KtorZ deleted the paweljakubas/401/port-arg-bug branch June 13, 2019 17:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix CLI port argument bug. #407

Fix CLI port argument bug. #407

paweljakubas commented Jun 12, 2019 •

edited by rvl

Loading

jonathanknowles left a comment •

edited

Loading

jonathanknowles Jun 13, 2019

KtorZ Jun 13, 2019

jonathanknowles Jun 13, 2019

jonathanknowles Jun 13, 2019

jonathanknowles Jun 13, 2019

KtorZ Jun 13, 2019

KtorZ Jun 13, 2019

KtorZ Jun 13, 2019

KtorZ Jun 13, 2019

KtorZ Jun 13, 2019

KtorZ Jun 13, 2019

KtorZ Jun 13, 2019

KtorZ Jun 13, 2019

paweljakubas Jun 13, 2019 •

edited

Loading

KtorZ Jun 13, 2019

piotr-iohk Jun 13, 2019

KtorZ Jun 13, 2019

jonathanknowles Jun 13, 2019

jonathanknowles Jun 13, 2019

jonathanknowles Jun 13, 2019

piotr-iohk Jun 13, 2019

KtorZ left a comment

KtorZ commented Jun 13, 2019

		@@ -762,6 +769,8 @@ createWalletViaCLI args mnemonics secondFactor passphrase = do
		err <- TIO.hGetContents stderr
		return (c, T.unpack out, err)

		@@ -139,15 +140,19 @@ import qualified Data.Text as T
		import qualified Data.Text.Encoding as T
		import qualified Network.Wai.Handler.Warp as Warp

-    bridgePortArg = ["--bridge-port", T.unpack (toText np)]
+  args = mconcat
+      [ [ "server" ]
+      , [ "--network", if net == "local" then "testnet" else net ]
+      , maybe ["--random-port"] (\p -> ["--port", showT p]) wp
+      , [ "--bridge-port", showT np ]
+      ]
+  showT = T.unpack . toText

-                    ] ++ args
+let portArg =
+        maybe [] (\p -> ["--port", p]) portM
+let fullArgs =
+        [ "exec", "--", "cardano-wallet", "wallet", "create" ]
+        ++ portArg ++ args

	-- then an unused port will be selected at random.
	-- \| Run an action with a TCP socket bound to a port specified by the `Listen`
	-- parameter.

Fix CLI port argument bug. #407

Fix CLI port argument bug. #407

Conversation

paweljakubas commented Jun 12, 2019 • edited by rvl Loading

Issue Number

Overview

Comments

jonathanknowles left a comment • 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

Choose a reason for hiding this comment

paweljakubas Jun 13, 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

KtorZ left a comment

Choose a reason for hiding this comment

KtorZ commented Jun 13, 2019

paweljakubas commented Jun 12, 2019 •

edited by rvl

Loading

jonathanknowles left a comment •

edited

Loading

paweljakubas Jun 13, 2019 •

edited

Loading