Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Better error message for query utxo without oops #4788

Merged

Conversation

newhoggy
Copy link
Contributor

@newhoggy newhoggy commented Jan 16, 2023

This makes the checking of node to client versions less manual. It replaces a earlier PRs:

This PR improves on the earlier version by making it so that the monadic expressions no longer need to know what version each query requires. If the node that is connected to does not support the query being made, then the query expression returns UnsupportedNtcVersionError which can be handled in a convenient top-level location.

The PR uses the new functions onLeft and onNothing from transformers-except-0.1.3 to improve the way we structure code.

@newhoggy newhoggy changed the title Newhoggy/better error message for query utxo without oops Better error message for query utxo without oops Jan 16, 2023
@newhoggy newhoggy force-pushed the newhoggy/better-error-message-for-query-utxo-without-oops branch from f4375a9 to 7216a81 Compare January 16, 2023 09:03
@@ -45,7 +46,7 @@ import Cardano.Api.Modes
-- In order to make pipelining still possible we can explore the use of Selective Functors
-- which would allow us to straddle both worlds.
newtype LocalStateQueryExpr block point query r m a = LocalStateQueryExpr
{ runLocalStateQueryExpr :: ContT (Net.Query.ClientStAcquired block point query m r) m a
{ runLocalStateQueryExpr :: ReaderT NodeToClientVersion (ContT (Net.Query.ClientStAcquired block point query m r) m) a
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was always suspect about using ConT here to get monadic local state query expressions. Can we not write our own Monad instance for which will potentially make it more pleasant to use?

e.g result <- ExceptT . fmap (join . first ShelleyQueryCmdAcquireFailure) $ ... does not compose well.

Can we avoid the fmap (join ... by writing our own Monad instance? The type errors I experienced trying to get the expression to compile resulted in me abandoning trying to using it. Adding ReaderT here as things are now only serves to make this less usable imo.

Copy link
Contributor Author

@newhoggy newhoggy Jan 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The need for fmap (join ... doesn't come from ContT and writing our own monad instance wouldn't fix it.

It comes from the fact that we are returning multiple error types. It's one of my motivations for using oops which makes returning multiple error types seamless.

What that code is doing is converting Either ShelleyQueryCmdError (Either AcquireError a) to Either ShelleyQueryCmdError (Either ShelleyQueryCmdError a) and then the join squishes it to Either ShelleyQueryCmdError a.

Before this PR the code had to squish AcquireError into ShelleyQueryCmdError.

In this code, the PR has to squish AcquireError and UnsupportedNtcVersionError into ShelleyQueryCmdError, which means more hoops to jump.

Then when you're done jumping all the hoops to make the code compile you find out later the code is brittle because ordering of errors in stacked Eithers matters.

Either a (Either b (Either c)) is different to Either c (Either b (Either a))

Different code that stacks the Eithers differently are incompatible so can't call those functions one after the other without reordering the Either stack and adding removing error types is painful.

It's also difficult to see when looking at the code how the Eithers are stacked in each sub-expression and you have to know in order to successfully connected bits of code together.

As for ContT I originally wrote this code without ContT back in 2021 but someone commented that I should use ContT which allows me to lean on all the code that ContT provides rather than rewriting it all from scratch which would have been near duplications.

@newhoggy newhoggy force-pushed the newhoggy/better-error-message-for-query-utxo-without-oops branch 2 times, most recently from 85d122f to 0daa172 Compare January 17, 2023 05:24
@@ -1031,20 +1043,24 @@ runQueryStakePools (AnyConsensusModeParams cModeParams)
let localNodeConnInfo = LocalNodeConnectInfo cModeParams network sockPath
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How runQueryStakePools is implemented without oops.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without oops:

runQueryStakePools
  :: AnyConsensusModeParams
  -> NetworkId
  -> Maybe OutputFile
  -> ExceptT ShelleyQueryCmdError IO ()
runQueryStakePools (AnyConsensusModeParams cModeParams)
                          network mOutFile = do
  SocketPath sockPath <- firstExceptT ShelleyQueryCmdEnvVarSocketErr
                           $ newExceptT readEnvSocketPath
  let localNodeConnInfo = LocalNodeConnectInfo cModeParams network sockPath


  result <- ExceptT . fmap (join . first ShelleyQueryCmdAcquireFailure) $
    executeLocalStateQueryExpr localNodeConnInfo Nothing $ runExceptT @ShelleyQueryCmdError $ do
      anyE@(AnyCardanoEra era) <- case consensusModeOnly cModeParams of
        ByronMode -> return $ AnyCardanoEra ByronEra
        ShelleyMode -> return $ AnyCardanoEra ShelleyEra
        CardanoMode -> ExceptT $ fmap (first ShelleyQueryCmdUnsupportedNtcVersion) $ queryExpr $ QueryCurrentEra CardanoModeIsMultiEra

      let cMode = consensusModeOnly cModeParams

      case toEraInMode era cMode of
        Just eInMode -> do
          sbe <- getSbe $ cardanoEraStyle era

          r <- ExceptT $ fmap (first ShelleyQueryCmdUnsupportedNtcVersion) $ queryExpr $
            QueryInEra eInMode $ QueryInShelleyBasedEra sbe $ QueryStakePools

          case r of
            Right a -> pure a
            Left e -> throwE (ShelleyQueryCmdEraMismatch e)

        Nothing -> left $ ShelleyQueryCmdEraConsensusModeMismatch (AnyConsensusMode cMode) anyE

  writeStakePools mOutFile result

@newhoggy newhoggy marked this pull request as ready for review January 17, 2023 10:09
@newhoggy newhoggy force-pushed the newhoggy/better-error-message-for-query-utxo-without-oops branch 3 times, most recently from 899530b to c806e41 Compare January 19, 2023 03:12
@newhoggy newhoggy force-pushed the newhoggy/better-error-message-for-query-utxo-without-oops branch 3 times, most recently from a05ceb0 to a7a9ba0 Compare January 20, 2023 00:21
Just eInMode -> do
sbe <- getSbe $ cardanoEraStyle era
eInMode <- toEraInMode era cMode
& hoistMaybe (ShelleyQueryCmdEraConsensusModeMismatch (AnyConsensusMode cMode) anyE)
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can also be written as:

        eInMode <- pure (toEraInMode era cMode)
          & onNothing (throwE $ ShelleyQueryCmdEraConsensusModeMismatch (AnyConsensusMode cMode) anyE)

This shows the versatility of the on* combinators.

@newhoggy newhoggy force-pushed the newhoggy/better-error-message-for-query-utxo-without-oops branch 7 times, most recently from d20ecea to c1b2b37 Compare January 20, 2023 07:03
@newhoggy newhoggy force-pushed the newhoggy/better-error-message-for-query-utxo-without-oops branch 3 times, most recently from 2ff2e9b to 095d372 Compare January 20, 2023 08:58
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments

cardano-api/src/Cardano/Api/IPC/Monad.hs Outdated Show resolved Hide resolved
cabal.project Show resolved Hide resolved
cardano-api/src/Cardano/Api/IPC/Monad.hs Outdated Show resolved Hide resolved
@@ -210,7 +215,8 @@ runQueryProtocolParameters (AnyConsensusModeParams cModeParams) network mOutFile
let localNodeConnInfo = LocalNodeConnectInfo cModeParams network sockPath

result <- liftIO $ executeLocalStateQueryExpr localNodeConnInfo Nothing $ runExceptT $ do
anyE@(AnyCardanoEra era) <- lift $ determineEraExpr cModeParams
anyE@(AnyCardanoEra era) <- lift (determineEraExpr cModeParams)
& onLeft (throwE . ShelleyQueryCmdUnsupportedNtcVersion)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I really don't like the name throwE because it implies we are throwing an exception, which we're not. There is the function left that is exposed in Control.Monad.Trans.Except.Extra that I think is better named. What are your thoughts here? Do you feel strongly about throwE?

Copy link
Contributor Author

@newhoggy newhoggy Jan 23, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was worried about confusion between Left in Either and the Left in ExceptT. The onLeft extracts the Left of Either and the throwE or left puts the value in the Left of the ExceptT.

Writing & onLeft (left . ShelleyQueryCmdUnsupportedNtcVersion) makes it look like we are leaving the structure unchanged, but that is not the case. The structure changes from ExceptT x (Either e a) to ExceptT x.

I don't think of throwE as "throw exception". I think of it as "throw except" or "throw error".

However, I am happy to switch to left for this PR, which I've done.

cardano-cli/src/Cardano/CLI/Shelley/Run/Query.hs Outdated Show resolved Hide resolved
@newhoggy newhoggy force-pushed the newhoggy/better-error-message-for-query-utxo-without-oops branch 2 times, most recently from 6f3d2a8 to db89c35 Compare January 23, 2023 17:44
@newhoggy newhoggy dismissed Jimbo4350’s stale review January 23, 2023 17:47

Comments addressed

@newhoggy newhoggy requested review from Jimbo4350 and removed request for LudvikGalois January 23, 2023 17:48
cardano-api/src/Cardano/Api/IPC/Monad.hs Outdated Show resolved Hide resolved
@newhoggy newhoggy force-pushed the newhoggy/better-error-message-for-query-utxo-without-oops branch from db89c35 to 3c5a949 Compare January 23, 2023 19:43
@newhoggy newhoggy force-pushed the newhoggy/better-error-message-for-query-utxo-without-oops branch from 3c5a949 to 3ff371c Compare January 23, 2023 19:46
@newhoggy newhoggy merged commit feb1b0e into master Jan 23, 2023
@iohk-bors iohk-bors bot deleted the newhoggy/better-error-message-for-query-utxo-without-oops branch January 23, 2023 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants