[input-output-hk/cardano-sl] [CBR-243] improve wallet worker start-up and exception handling (#3330)

matt-noonan commented on this pull request.

> @@ -55,13 +56,11 @@ data WalletWorkerState b = WalletWorkerState

makeLenses »WalletWorkerState

— A helper function for lifting a `WalletActionInterp` through a monad transformer.
-lifted :: (Monad m, MonadTrans t) => WalletActionInterp m b -> WalletActionInterp (t m) b
-lifted i = WalletActionInterp
— { applyBlocks = lift . applyBlocks i
— , switchToFork = \n bs -> lift (switchToFork i n bs)
— , emit = lift . emit i
— }
+instance MFunctor WalletActionInterp where
+ hoist nat i = WalletActionInterp
+ { applyBlocks = fmap nat (applyBlocks i)
+ , switchToFork = fmap (fmap nat) (switchToFork i)
+ , emit = fmap nat (emit i) }

Oh, I’ve got a bit of paint stored up for this particular bikeshed…

I’m generally in favor of `mmorph` and `hoist` and don’t see any problems introducing those in general. But in this specific case, I don’t think it is right.

First of all, I think this is not a lawful use of `MFunctor`, because `WalletActionInterp m` isn’t a `Monad`. Or if it is, it (1) isn’t obvious to me and (2) doesn’t matter for this code. More on that below.

In reality, the whole `Monad` constraint is wrong here (too strict). @edsko may recall that an earlier version of the worker just represented actions as elements of a `Monoid`, because the worker just sequences and dispatches actions. It never needs to snoop around with their results. So the real worker uses some monoid like `IO ()`. And really `lifted` should just be applying a monoid homomorphism.

Long story short, I wish I’d stuck to the weaker `Monoid` abstraction here, because all the business of monads and transformers distracts from the simple job of the worker: to sequence a bunch of opaque actions.

Добавить комментарий