Unify Node APIs #3882

parsonsmatt · 2018-11-22T00:15:28Z

Description

This PR unifies the two node APIs in the new Node API.

parsonsmatt · 2018-11-22T00:16:26Z

chain/src/Pos/Chain/Update/Configuration.hs

-ourAppName :: HasUpdateConfiguration => ApplicationName
-ourAppName = ccApplicationName updateConfiguration
+ourAppName :: UpdateConfiguration -> ApplicationName
+ourAppName = ccApplicationName


I got annoyed at the UpdateConfiguration and went down a bit of a rabbit hole getting that to not be an implicit param.

parsonsmatt · 2018-11-22T00:18:00Z

db/src/Pos/DB/Block/Logic/Creation.hs

@@ -74,7 +74,7 @@ import           Pos.Util.Wlog (WithLogger, logDebug)

 -- | A set of constraints necessary to create a block from mempool.
 type MonadCreateBlock ctx m
-     = ( HasUpdateConfiguration
+     = ( HasLens' ctx UpdateConfiguration


but instead of passing the UpdateConfiguration around directly, I have decided to pass it in the various Context's that are used in a MonadReader manner. That's part of why this PR is as small as it is.

Unfortunately, the Ether lensy stuff is super verbose and boilerplatey. I'll swap it over to generic-lens soon...

parsonsmatt · 2018-11-23T23:52:55Z

lib/src/Pos/Web/Server.hs

+    , HasPrimaryKey r, HasLens TxpHolderTag r (GenericTxpLocalData ext)
+    , HasLens UpdateConfiguration r UpdateConfiguration
+    , HasSscContext r
+    ) => ServerT NodeApi m


Generalized this one so I could reuse it with less context than all of WorkMode

parsonsmatt · 2018-11-23T23:53:24Z

lib/src/Pos/Web/Server.hs

@@ -273,29 +284,34 @@ nodeServantHandlers =
    -- :<|> getOurSecret
    -- :<|> getSscStage

-getLeaders :: Maybe EpochIndex -> WebMode ext SlotLeaders
+getLeaders :: MonadDBRead m => Maybe EpochIndex -> m SlotLeaders


What's the point of all these mtl classes if you just run everything in SomeMonstrousMonad anyway?

parsonsmatt · 2018-11-23T23:54:10Z

node/src/Cardano/Node/API.hs

+    = "v1"
+    :> ( Node.API
+    :<|> Legacy.NodeApi
+    )


Hooray, a v1 prefix!

As a matter of fact, I think we are now "technically" at v2 😂 ...

parsonsmatt · 2018-11-23T23:54:20Z

node/src/Cardano/Node/API.hs

@@ -8,8 +8,9 @@ module Cardano.Node.API where
 import           Universum


This module is where most of the real work is done.

Ah! Was looking for it 😛 ! Would have been nice to make two PRs I believe, but then I know.. CI.. :(

parsonsmatt · 2018-11-23T23:55:01Z

node/src/Cardano/Node/API.hs

+        :: !SscContext
+    , legacyCtxNodeDBs
+        :: !DB.NodeDBs
+    }


I define a separate LegacyCtx so that I can instantiate all the classes etc that I need.

With generic-lens, this (can) become a tuple of all the things it needs, and the entire datatype disappears. Alternatively, you have the datatype + the fields and a deriving Generic line.

parsonsmatt · 2018-11-23T23:55:50Z

node/src/Cardano/Node/API.hs

+legacyNodeApi r =
+    hoistServer
+        (Proxy :: Proxy Legacy.NodeApi)
+        (Handler . ExceptT . try . flip runReaderT r)


This trys for ServantErrors that are thrown via IO.

🤦‍♂️

parsonsmatt · 2018-11-23T23:57:30Z

node/src/Cardano/Node/API.hs

    -> NtpConfiguration
-    -> NodeResources ext
+    -> NodeResources ()


It'd be nice if NodeResources were a Functor so I could void :: Functor f => f a -> f () and retain polymorphism. However, the ext eventually bottoms out to a TVar ext in GenericTxpLocalData, and TVar isn't a Functor, so, oh well.

parsonsmatt · 2018-11-23T23:59:10Z

node/src/Cardano/Node/API.hs

@@ -264,16 +341,14 @@ instance HasLens LastKnownHeaderTag InfoCtx LastKnownHeader where
    lensOf =
        lens infoCtxLastKnownHeader (\i s -> i { infoCtxLastKnownHeader = s })

-instance


The MonadTrans instance for MonadDBRead is super frustrating. It clashes with any polymorphic implementation like this, and even an OVERLAPPING pragma doesn't shut it up. So, you need dozens of instances like this.

erikd

Seems legit!

Would be good to drop the -fno-warn-orphans if possible.

erikd · 2018-11-26T21:24:59Z

db/src/Pos/DB/Update/Poll/DBPoll.hs

@@ -1,5 +1,7 @@
 {-# LANGUAGE TypeFamilies #-}

+{-# OPTIONS_GHC -fno-warn-orphans #-}
+


Is that really needed? I don't see any typeclass instances added to this file.

It's for instance (...) MonadDBPollRead DBPoll, because DBPoll is a type alias and not a newtype.

erikd · 2018-11-26T21:26:29Z

explorer/src/Pos/Explorer/ExplorerMode.hs

+    { etcGState              :: !GS.GStateContext
+    , etcSystemStart         :: !Timestamp
+    , etcSSlottingVar        :: !SimpleSlottingStateVar
+    , etcSlotId              :: !(Maybe SlotId)


This is why I am against vertical alignment. This change is semanically null.

strong agree

erikd · 2018-11-26T21:30:16Z

node/src/Cardano/Node/API.hs

+legacyNodeApi r =
+    hoistServer
+        (Proxy :: Proxy Legacy.NodeApi)
+        (Handler . ExceptT . try . flip runReaderT r)


🤦‍♂️

parsonsmatt added the wip label Nov 22, 2018

parsonsmatt changed the title ~~Unify Node APIs~~ [WIP] Unify Node APIs Nov 22, 2018

KtorZ self-requested a review November 22, 2018 17:32

parsonsmatt commented Nov 23, 2018

View reviewed changes

parsonsmatt changed the title ~~[WIP] Unify Node APIs~~ Unify Node APIs Nov 26, 2018

parsonsmatt removed the wip label Nov 26, 2018

parsonsmatt added 11 commits November 26, 2018 14:26

Include v1 in API type

58aaa83

HasUpdateConfiguration

6e07e79

revert that rabbit hole

1e5a672

UpdateConfiguration pls

81ec5df

update configuration threading

2a52a88

style

5aa50c8

Identify minimal constraints to run the legacy server

cf67d5c

Wire up the legacy API

38f8f6c

pkgs

33ea1cd

Fix warnings, stylish:

a372993

fix warns

d393b58

parsonsmatt force-pushed the matt/fold-in-other-node-api branch from 0f397d8 to d393b58 Compare November 26, 2018 21:27

erikd approved these changes Nov 26, 2018

View reviewed changes

parsonsmatt added 4 commits November 26, 2018 14:53

fix test

74ca728

fix builds/tests

1603ba1

stylish

2672996

style

b2c9282

parsonsmatt merged commit 92a1e97 into develop Nov 27, 2018

iohk-bors bot deleted the matt/fold-in-other-node-api branch November 27, 2018 04:25

This was referenced Nov 27, 2018

Start HTTP Server & Implement API on The Node's End input-output-hk/cardano-wallet-legacy#30

Closed

Remove Update Configuration Context input-output-hk/cardano-wallet-legacy#127

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Unify Node APIs #3882

Unify Node APIs #3882

parsonsmatt commented Nov 22, 2018

parsonsmatt Nov 22, 2018

parsonsmatt Nov 22, 2018

parsonsmatt Nov 23, 2018

parsonsmatt Nov 23, 2018

KtorZ Nov 27, 2018

parsonsmatt Nov 23, 2018

KtorZ Nov 27, 2018

parsonsmatt Nov 23, 2018

KtorZ Nov 27, 2018

parsonsmatt Nov 23, 2018

parsonsmatt Nov 23, 2018

erikd Nov 26, 2018

parsonsmatt Nov 23, 2018

parsonsmatt Nov 23, 2018

erikd left a comment

erikd Nov 26, 2018

parsonsmatt Nov 26, 2018

erikd Nov 26, 2018

parsonsmatt Nov 26, 2018

erikd Nov 26, 2018

		@@ -8,8 +8,9 @@ module Cardano.Node.API where
		import Universum

		@@ -1,5 +1,7 @@
		{-# LANGUAGE TypeFamilies #-}

		{-# OPTIONS_GHC -fno-warn-orphans #-}

Unify Node APIs #3882

Unify Node APIs #3882

Conversation

parsonsmatt commented Nov 22, 2018

Description

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

erikd 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