`Jormungandr m` with getTipId implementation #321

Anviking · 2019-05-24T15:55:37Z

Issue Number

#219

Overview

Spelling fix in Cardano.Wallet.Jormungandr.Api, and temporarily removed all endpoints from the Api but GetTipId
I added a Jörmungandr m client in Cardano.Wallet.Jormungandr.Network
I implemented getTipId, testing that it works, and that it can return Left ErrNetworkUnreachable. 404 is not a possibility .
I added waitForConnection to node at launch

Comments

Relevant API definition:

Anviking · 2019-05-24T16:34:26Z

lib/jormungandr/src/Cardano/Wallet/Network/Jormungandr.hs

+    , mkJormungandr
+
+    -- * Re-export
+    , BaseUrl (..)


Some gratuitous re-exports here to avoid the dependencies in .cabal and import-statements for these tiny things.

Anviking · 2019-05-24T16:36:56Z

lib/jormungandr/test/integration/Cardano/Wallet/Network/JormungandrSpec.hs

+
+    startNode = do
+        handle <- async $ launch $ return $ Command
+                "jormungandr"


Duplication with LauncherSpec 😕 https://github.com/input-output-hk/cardano-wallet/blob/e92b8e87a854e0d5de7daeb40b36682533194324/lib/jormungandr/test/integration/Cardano/LauncherSpec.hs#L22-L26

akegalj

good start

Anviking · 2019-05-28T12:18:49Z

lib/jormungandr/test/integration/Cardano/Wallet/Jormungandr/NetworkSpec.hs

+            length (transactions b) `shouldBe` 1
+
+    describe "Error paths" $ beforeAll newClient $ do
+        it "gets a 'ErrNetworkUnreachable' if jormungandr isn't up (1)"


For http-bridge we test this on the NetworkLayer. I used the Jormungandr-client here as NetworkLayer isn't implemented yet.

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Api.hs

Anviking · 2019-05-28T12:24:25Z

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Network.hs

+              return $ Left ErrNetworkTipNotFound
+            x -> do
+                let ctx = safeLink api (Proxy @GetTipId)
+                left ErrNetworkTipNetworkUnreachable <$> defaultHandler ctx x


I'm kind-of not comfortable yet with how the servant-client requests are written, the error handling or the tests, but I'm not sure its worth focusing on, since we already have the same thing going on for the HttpBridge.

What do you mean by "focusing on" ?

Making an attempt at refactoring.

Not in the pr now, but for instance I find the duplication of parentId (Just count) off

run (cGetBlockDescendantIds parentId (Just count)) >>= \case Left (FailureResponse e) | responseStatusCode e == status404 -> return $ Left ErrGetDescendantsParentNotFound x -> do let ctx = safeLink api (Proxy @GetBlockDescendantIds) parentId (Just count) left ErrGetDescendantsNetworkUnreachable <$> defaultHandler ctx x

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Network.hs

KtorZ · 2019-05-29T09:50:51Z

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Network.hs

+    :: Int -> IO (NetworkLayer IO)
+newNetworkLayer port = mkNetworkLayer <$> newHttpBridge port
+  where
+    newHttpBridge = undefined


If the network layer isn't implemented, let's not have it part of the PR at all 👍

I left it commented still, if you don't mind, as it is the end-goal of the module.

lib/jormungandr/test/integration/Cardano/Wallet/Jormungandr/NetworkSpec.hs

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Network.hs

Anviking · 2019-06-03T20:59:59Z

@rvl thanks, I think it makes much more sense than retrying in the client.

I added some simple helpers for (2) in 728d65a, and some simple tests in
23a28ce.

I added fa07dea and baccb9d

akegalj · 2019-06-04T06:15:42Z

lib/core/src/Cardano/Wallet/Network.hs

+waitForConnection
+    :: NetworkLayer IO
+    -> IO ()
+waitForConnection nw = loop 20


can't we reuse waitForConnection from Jourmungadr.Network module ?

Not really. They requests are different* and they are pattern-matching on different errors.

*) Which to me appears like a thing we want: embracing the difference between NetworkLayer and the potentially underlying Jormungandr client.

lib/http-bridge/test/integration/Cardano/Wallet/HttpBridge/NetworkSpec.hs

KtorZ · 2019-06-04T14:47:56Z

lib/core/src/Cardano/Wallet/Network.hs

+-- | Tries to waits 20 s, until 'networkTip networkLayer' succeeds.
+waitForConnection
+    :: NetworkLayer IO
+    -> Int -- ^ Number of retries / timeout in seconds


This is exactly what Quantity is for! To convey this kind of information 😉 Every time you see yourself putting a comment to explain a What (and not a Why), ask yourself if you couldn't instead craft a better type that answers it.

Also, the comment seems to be a lie.

Also, the comment seems to be a lie.

Fair point.

This is exactly what Quantity is for!

If we do retry it won't matter, but:

This is exactly what Quantity is for!

Hmmmmmmm.… 🤔

As long as we don't just

f (Quantity 1000000)

sure, maybe.

Replacing the stock threadDelay with a typed version — everywhere — might be an idea too.

KtorZ · 2019-06-04T14:48:51Z

lib/core/src/Cardano/Wallet/Network.hs

+                     TIO.putStrLn "[INFO] waiting for connection to the node..."
+                     threadDelay 1000000
+                     loop (retries - 1)
+        Left e -> throwIO e


I'd suggest to simply use functions from retry here, which already encapsulate nicely this logic. This would also allow to simply take a retryPolicy as an argument (instead of an untyped Int) and, to provide a default policy next to the waitForConnection function for an easy usage:

waitForConnection defaultPolicy

In testing, this would allow to pass in a different policy for which the cumulative delay can be just 500ms or so, to avoid hanging for nothing in tests.

KtorZ · 2019-06-04T14:49:16Z

lib/http-bridge/test/integration/Cardano/Wallet/HttpBridge/NetworkSpec.hs

+                ErrNetworkTipNetworkUnreachable _ -> True
+                _ -> False
+
+        it "returns when the network becomes availible" $ do


Typo available -> available

KtorZ · 2019-06-04T14:52:05Z

lib/http-bridge/test/integration/Cardano/Wallet/HttpBridge/NetworkSpec.hs

+            -- returns within (3s + 3s extra)
+            res <- race
+                (threadDelay 6000000)
+                (waitForConnection c 20)


woudl be nice to be consistent with variable names. c is called nw in the previous test and in the helper function. Try to pick a name, and stick to it 👍

KtorZ · 2019-06-04T14:52:57Z

lib/http-bridge/test/integration/Cardano/Wallet/HttpBridge/NetworkSpec.hs

+            -- Start the bridge after 3s, and make sure waitForConnection
+            -- returns within (3s + 3s extra)
+            res <- race
+                (threadDelay 6000000)


Avoid "magic numbers" and use named constant -> let sixtySeconds = 60000000 etc .. At the very least, use a numeric separator to make thousands more obvious.

lib/http-bridge/test/integration/Cardano/Wallet/HttpBridge/NetworkSpec.hs

KtorZ · 2019-06-04T14:57:36Z

lib/http-bridge/test/integration/Cardano/Wallet/HttpBridge/NetworkSpec.hs

+            handle <- async . launch . return
+                $ httpBridgeWithSetup (threadDelay 3000000)
+            -- Start the bridge after 3s, and make sure waitForConnection
+            -- returns within (3s + 3s extra)


Why do we need this extra 3s ? Can't we just start the bridge and call wait for connection.. That's how we would normally use it, isn't it ?

This was to check that waitForConnection did in-fact return within a reasonable time, not just that it returned. Overkill?

Hmmmm. I still don't get it? Also, are we sure that 3 seconds is indeed enough in every environment? I'd rather just wait for connection, and verify that it resolve. If it always return within 3 seconds, then why wait for 20s in the implementation? If we are ready to wait up-to 20s that's what we should be testing here and this should never throw.

KtorZ · 2019-06-04T14:58:32Z

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Network.hs

+                     TIO.putStrLn "[INFO] waiting for connection to the node..."
+                     threadDelay 1000000
+                     loop (retries - 1)
+               | otherwise -> throwIO e


This function already exists in Cardnao.Wallet.Network. What's the rationale for having both?

To have it for both Jormungandr, and NetworkLayer. (Ignoring HttpBridge as I haven't seen a need for it)

#321 (comment)

But what's the need for the one in Jörmungandr? Why isn't it possible to get the same "feature" using only the NetworkLayer ? What does it bring us to specialize this to Jormungandr?

We could decide to not have integration tests for Jormungandr. Then we wouldn't need another waitForConnection. I don't see why we shouldn't have though.

If we instead were to here implement and test networkTip networkLayer, that would rely on both jormungandr getTipId (implemented) and getBlock (not implemented), as getTipId doesn't return a BlockHeader.

Anviking · 2019-06-04T18:56:38Z

Following discussion with @KtorZ I've now:

removed the integration tests for Jormungandr. We'll test the corresponding NetworkLayer instead. No need for a jormungandr-specific waitForConnection then.
Named the client JormungandrLayer

And, well, we use the retry package now too.

KtorZ · 2019-06-05T06:28:28Z

lib/http-bridge/test/integration/Cardano/Wallet/HttpBridge/NetworkSpec.hs

+            handle <- async . launch . return
+                $ httpBridgeWithSetup (threadDelay 3000000)
+            -- Start the bridge after 3s, and make sure waitForConnection
+            -- returns within (3s + 3s extra)


Hmmmm. I still don't get it? Also, are we sure that 3 seconds is indeed enough in every environment? I'd rather just wait for connection, and verify that it resolve. If it always return within 3 seconds, then why wait for 20s in the implementation? If we are ready to wait up-to 20s that's what we should be testing here and this should never throw.

KtorZ · 2019-06-05T06:30:09Z

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Network.hs

+-- >>> mgr <- newManager defaultManagerSettings
+-- >>> j = mkJormungandrLayer mgr (BaseUrl Http "localhost" 8080 "")
+-- >>> runExceptT $ getTipId j
+-- Right (BlockId (Hash {getHash = "26c640a3de09b74398c14ca0a137ec78"}))


Also, are we sure that 3 seconds is indeed enough in every environment?

I am sure my machine will be exceptional :)

Anviking self-assigned this May 24, 2019

Anviking commented May 24, 2019

View reviewed changes

Anviking force-pushed the anviking/219/jörmungandr-network branch 4 times, most recently from c78975a to 5793f43 Compare May 24, 2019 17:53

KtorZ force-pushed the anviking/219/jörmungandr-network branch from 5793f43 to 586c722 Compare May 27, 2019 08:30

KtorZ mentioned this pull request May 27, 2019

Jörmungandr REST-API / Network Layer #219

Closed

12 tasks

Anviking marked this pull request as ready for review May 27, 2019 10:41

Anviking changed the base branch from anviking/219/jörmungandr-network to master May 27, 2019 10:41

Anviking force-pushed the anviking/219/jörmungandr-network-2 branch from af72a39 to 5034821 Compare May 27, 2019 10:42

akegalj reviewed May 27, 2019

View reviewed changes

Anviking force-pushed the anviking/219/jörmungandr-network-2 branch 5 times, most recently from b3eeb5a to 9b69e6f Compare May 28, 2019 12:12

Anviking commented May 28, 2019

View reviewed changes

Anviking requested a review from KtorZ May 28, 2019 12:19

KtorZ reviewed May 28, 2019

View reviewed changes

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Api.hs Outdated Show resolved Hide resolved

Anviking commented May 28, 2019

View reviewed changes

KtorZ reviewed May 29, 2019

View reviewed changes

Anviking force-pushed the anviking/219/jörmungandr-network-2 branch 4 times, most recently from 35ad859 to 80bf249 Compare May 29, 2019 13:34

Anviking commented May 29, 2019

View reviewed changes

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Network.hs Show resolved Hide resolved

Anviking commented May 29, 2019

View reviewed changes

lib/jormungandr/src/Cardano/Wallet/Jormungandr/Network.hs Show resolved Hide resolved

Anviking changed the title ~~Jormungandr m with getTipId and getBlock implementation~~ Jormungandr m with getTipId implementation May 29, 2019

Anviking force-pushed the anviking/219/jörmungandr-network-2 branch 4 times, most recently from 4b4392d to 728d65a Compare June 3, 2019 18:37

Anviking requested review from KtorZ and rvl June 3, 2019 20:52

Anviking force-pushed the anviking/219/jörmungandr-network-2 branch from 23a28ce to baccb9d Compare June 4, 2019 02:33

akegalj reviewed Jun 4, 2019

View reviewed changes

Anviking force-pushed the anviking/219/jörmungandr-network-2 branch 2 times, most recently from d02c6a6 to 0370c9a Compare June 4, 2019 09:19

Anviking commented Jun 4, 2019

View reviewed changes

lib/http-bridge/test/integration/Cardano/Wallet/HttpBridge/NetworkSpec.hs Show resolved Hide resolved

Anviking force-pushed the anviking/219/jörmungandr-network-2 branch 3 times, most recently from 7d1343b to befdada Compare June 4, 2019 12:06

KtorZ suggested changes Jun 4, 2019

View reviewed changes

Anviking force-pushed the anviking/219/jörmungandr-network-2 branch 2 times, most recently from 59bf517 to 173ab2e Compare June 4, 2019 18:43

Anviking requested a review from KtorZ June 4, 2019 18:44

Anviking force-pushed the anviking/219/jörmungandr-network-2 branch from 173ab2e to b50c21b Compare June 4, 2019 18:51

KtorZ reviewed Jun 5, 2019

View reviewed changes

Anviking force-pushed the anviking/219/jörmungandr-network-2 branch 2 times, most recently from 2e21d4e to c9958e2 Compare June 5, 2019 09:36

KtorZ approved these changes Jun 5, 2019

View reviewed changes

Anviking added 2 commits June 5, 2019 11:56

Add a jormungandr client supporting getTipId

56468d5

waitForConnection networkLayer at launch

56db060

Anviking force-pushed the anviking/219/jörmungandr-network-2 branch from c9958e2 to 56db060 Compare June 5, 2019 09:56

Anviking merged commit dca30dd into master Jun 5, 2019

Anviking deleted the anviking/219/jörmungandr-network-2 branch June 5, 2019 11:33

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`Jormungandr m` with getTipId implementation #321

`Jormungandr m` with getTipId implementation #321

Anviking commented May 24, 2019 •

edited

Loading

Anviking May 24, 2019

Anviking May 24, 2019

akegalj left a comment

Anviking May 28, 2019

Anviking May 28, 2019

KtorZ May 29, 2019

Anviking May 29, 2019

KtorZ May 29, 2019

Anviking May 29, 2019

Anviking commented Jun 3, 2019 •

edited

Loading

akegalj Jun 4, 2019

Anviking Jun 4, 2019

KtorZ Jun 4, 2019

Anviking Jun 4, 2019

KtorZ Jun 4, 2019

KtorZ Jun 4, 2019

KtorZ Jun 4, 2019

KtorZ Jun 4, 2019

KtorZ Jun 4, 2019

Anviking Jun 4, 2019

KtorZ Jun 5, 2019

KtorZ Jun 4, 2019

Anviking Jun 4, 2019

KtorZ Jun 4, 2019

Anviking Jun 4, 2019 •

edited

Loading

Anviking commented Jun 4, 2019

KtorZ Jun 5, 2019

KtorZ Jun 5, 2019

akegalj Jun 5, 2019 •

edited

Loading

Jormungandr m with getTipId implementation #321

Jormungandr m with getTipId implementation #321

Conversation

Anviking commented May 24, 2019 • edited Loading

Issue Number

Overview

Comments

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

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

Anviking commented Jun 3, 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

Choose a reason for hiding this comment

Anviking Jun 4, 2019 • edited Loading

Choose a reason for hiding this comment

Anviking commented Jun 4, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

akegalj Jun 5, 2019 • edited Loading

Choose a reason for hiding this comment

`Jormungandr m` with getTipId implementation #321

`Jormungandr m` with getTipId implementation #321

Anviking commented May 24, 2019 •

edited

Loading

Anviking commented Jun 3, 2019 •

edited

Loading

Anviking Jun 4, 2019 •

edited

Loading

akegalj Jun 5, 2019 •

edited

Loading