Better error message for query utxo 2 #3358

newhoggy · 2021-11-10T11:09:57Z

This PR adds better messages for query utxo. It replaces an earlier PR #2957.

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 throws QueryErrorUnsupportedVersion which can be handled in a convenient top-level location.

newhoggy · 2021-11-17T10:11:31Z

cardano-api/src/Cardano/Api/IPC/Monad.hs

@@ -42,15 +50,15 @@ import qualified Ouroboros.Network.Protocol.LocalStateQuery.Type as Net.Query
 -- 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


The ReaderT allows the monad expression to know the node to client version of the node server.

newhoggy · 2021-11-17T10:32:50Z

cardano-cli/src/Cardano/CLI/Shelley/Run/Query.hs

-          else return Nothing
-        mSystemStart <- if ntcVersion >= NodeToClientV_9
-          then Just <$> queryExpr QuerySystemStart
-          else return Nothing


This is simplified a lot. Not only is it shorter, but we no longer need to know what node version a given query requires. Instead if the query is not supported by the node then Nothing is returned.

newhoggy · 2021-11-17T10:33:47Z

cardano-cli/src/Cardano/CLI/Shelley/Run/Query.hs

@@ -846,10 +851,12 @@ determineEra cModeParams localNodeConnInfo =
    ByronMode -> return $ AnyCardanoEra ByronEra
    ShelleyMode -> return $ AnyCardanoEra ShelleyEra
    CardanoMode -> do
-      eraQ <- liftIO . queryNodeLocalState localNodeConnInfo Nothing
-                     $ QueryCurrentEra CardanoModeIsMultiEra


We no longer need queryNodeLocalState because they can all by replaced by executeLocalStateQueryExpr/queryExpr.

newhoggy · 2021-11-17T10:39:17Z

plutus-example/plutus-example/src/Cardano/PlutusExample/ScriptContextChecker.hs

@@ -327,20 +333,15 @@ readCustomRedeemerFromTx fp (AnyConsensusModeParams cModeParams) network = do
                   (ConsensusModeMismatch (AnyConsensusMode CardanoMode) (AnyCardanoEra cEra))
                   $ toEraInMode cEra CardanoMode

-      eResult <-
-        liftIO $ executeLocalStateQueryExpr localNodeConnInfo Nothing
-          $ \ntcVersion -> do


We no longer need to worry about ntcVersion lambda when executing queries because of the use of ReaderT.

Jimbo4350

Right direction, a couple of comments 👍

Jimbo4350 · 2021-11-17T14:00:59Z

cardano-cli/src/Cardano/CLI/Shelley/Run/Query.hs

@@ -298,6 +303,12 @@ runQueryTip (AnyConsensusModeParams cModeParams) network mOutFile = do
 -- via the local state query protocol.
 --

+minNtcVersionForQueryUTxOFilter :: QueryUTxOFilter -> NodeToClientVersion


Should probably add a comment like "See data NodeToClientVersion in ouroboros-network for more details". There also appears to be more version related information by data QueryVersion. We should aim to have all queries in the cli accounted for here so when we have a new query the type checker complains we have a missing pattern match.

I'm not sure what happened in the review presentation, but minNtcVersionForQueryUTxOFilter doesn't exist anymore. Maybe I had unpushed changes.

Jimbo4350 · 2021-11-17T14:07:01Z

cardano-cli/src/Cardano/CLI/Shelley/Run/Query.hs

@@ -347,7 +359,7 @@ runQueryPoolParams (AnyConsensusModeParams cModeParams) network poolid = do
    & hoistMaybe (ShelleyQueryCmdEraConsensusModeMismatch (AnyConsensusMode cMode) anyE)

  let qInMode = QueryInEra eInMode . QueryInShelleyBasedEra sbe $ QueryDebugLedgerState
-  result <- executeQuery era cModeParams localNodeConnInfo qInMode
+  result <- executeQuery era cModeParams NodeToClientV_1 localNodeConnInfo qInMode


So this is the type of situation we don't want. If we can implement a function that takes qInMode and returns a minimum required node version, that would be ideal.

Same as my previous comment. #3358 (comment)

cardano-api/src/Cardano/Api/IPC.hs

Jimbo4350 · 2021-11-17T14:45:52Z

cardano-cli/src/Cardano/CLI/Shelley/Run/Query.hs

@@ -141,6 +144,11 @@ runQueryCmd cmd =
    QueryUTxO' consensusModeParams qFilter networkId mOutFile ->
      runQueryUTxO consensusModeParams qFilter networkId mOutFile

+queryErrorToShelleyQueryCmdError :: QueryError ShelleyQueryCmdError -> ShelleyQueryCmdError


Why not add an additional constructor to ShelleyQueryCmdError? E.g ShelleyQueryRelatedCmdError QueryError

cardano-cli/src/Cardano/CLI/Shelley/Run/Transaction.hs

cardano-api/src/Cardano/Api/Query.hs

cardano-api/src/Cardano/Api/IPC/NtcVersionOf.hs

cardano-api/src/Cardano/Api/IPC/Monad.hs

* Better error message for query utxo * Use connectToLocalNode instead of connectToLocalNodeWithVersion where possible * Introduce QueryError type * Reader monad for NtcVersion to simplify code * NtcVersionOf type class with instances * Introduce maybeQueryExpr to make it easy to handle optional queries * Delete queryNodeLocalStateWithVersion * Replace all uses of queryNodeLocalState with executeLocalStateQueryExpr

Comments addressed

Jimbo4350

Couple more comments but right direction

Jimbo4350 · 2021-11-22T14:32:17Z

cardano-api/src/Cardano/Api/IPC.hs

@@ -475,43 +486,41 @@ convLocalStateQueryClient mode =
 -- | Establish a connection to a node and execute a single query using the
 -- local state query protocol.
 --
-queryNodeLocalState :: forall mode result.
+queryNodeLocalState :: forall e mode result.


Why make this polymorphic vs IO (Either QueryError result)?

The reason is I want to be able to return errors other than those described by the QueryError constructor in monad query expressions.

For example:

https://github.com/input-output-hk/cardano-node/blob/0242e81cf452889e09e862e8ae21ae877174f7f5/cardano-cli/src/Cardano/CLI/Shelley/Run/Query.hs#L723

https://github.com/input-output-hk/cardano-node/blob/0242e81cf452889e09e862e8ae21ae877174f7f5/cardano-cli/src/Cardano/CLI/Shelley/Run/Query.hs#L725

https://github.com/input-output-hk/cardano-node/blob/0242e81cf452889e09e862e8ae21ae877174f7f5/plutus-example/plutus-example/src/Cardano/PlutusExample/ScriptContextChecker.hs#L338

https://github.com/input-output-hk/cardano-node/blob/0242e81cf452889e09e862e8ae21ae877174f7f5/cardano-cli/src/Cardano/CLI/Shelley/Run/Transaction.hs#L484

https://github.com/input-output-hk/cardano-node/blob/0242e81cf452889e09e862e8ae21ae877174f7f5/cardano-cli/src/Cardano/CLI/Shelley/Run/Transaction.hs#L468

https://github.com/input-output-hk/cardano-node/blob/0242e81cf452889e09e862e8ae21ae877174f7f5/cardano-cli/src/Cardano/CLI/Shelley/Run/Transaction.hs#L460

https://github.com/input-output-hk/cardano-node/blob/0242e81cf452889e09e862e8ae21ae877174f7f5/cardano-cli/src/Cardano/CLI/Shelley/Run/Transaction.hs#L475

And many more.

The idea is that any user code can construct a complex monadic expression and be able to throw their own errors that are not QueryErrors.

Without this, we would be required to express the error in the return value instead, in which the return value would have to have the type Either e a, but you don't get exception semantics with Either.

Using Either is error prone because it doesn't have exception semantics. The user would have to pattern match for the error to handle it at the use site and also bubble it up. In some cases the user could forget to bubble up the error so it can get forgotten and left unhandled.

Jimbo4350 · 2021-11-22T14:56:39Z

cardano-api/src/Cardano/Api/IPC.hs

                       LocalNodeConnectInfo mode
                    -> Maybe ChainPoint
+                    -> (QueryError -> e)


Ah I see. I think returning QueryError as the error type and leaving the error conversion to the consumer is a better API.

Jimbo4350 · 2021-11-22T15:03:04Z

cardano-api/src/Cardano/Api/IPC/Monad.hs

-  -> (NodeToClientVersion -> LocalStateQueryExpr (BlockInMode mode) ChainPoint (QueryInMode mode) () IO a)
-  -> IO (Either AcquireFailure a)
-executeLocalStateQueryExpr connectInfo mpoint f = do
+  -> (QueryError -> e)


Same here. We should return QueryError rather than returning e.

Jimbo4350 · 2021-11-22T15:27:46Z

cardano-api/src/Cardano/Api/IPC/Monad.hs

-      }
+-- | Get the node server's Node-to-Client version.
+getNtcVersion :: LocalStateQueryExpr block point (QueryInMode mode) r IO (Either QueryError NodeToClientVersion)
+getNtcVersion =  LocalStateQueryExpr . ReaderT $ pure . Right


Why not:

LocalStateQueryExpr $ do v <- ask pure $ Right v

In my mind it's clearer as to what's going on but I'd be interested to hear what you think.

Works for me :D

cardano-api/src/Cardano/Api/IPC/Monad.hs

cardano-api/src/Cardano/Api/IPC/NtcVersionOf.hs

Jimbo4350 · 2021-11-22T16:25:39Z

cardano-cli/src/Cardano/CLI/Shelley/Run/Query.hs

@@ -136,6 +141,10 @@ runQueryCmd cmd =
    QueryUTxO' consensusModeParams qFilter networkId mOutFile ->
      runQueryUTxO consensusModeParams qFilter networkId mOutFile

+mapQueryError :: QueryError -> ShelleyQueryCmdError


These should be additional data constructors in ShelleyQueryCmdError. Generally we use firstExceptT ... to wrap the underlying error types.

Jimbo4350 · 2021-11-22T16:27:48Z

cardano-cli/src/Cardano/CLI/Shelley/Run/Transaction.hs

@@ -113,6 +113,8 @@ data ShelleyTxCmdError
  | ShelleyTxCmdTxInsDoNotExist ![TxIn]
  | ShelleyTxCmdMinimumUTxOErr !MinimumUTxOError
  | ShelleyTxCmdPParamsErr !ProtocolParametersError
+  | ShelleyTxCmdUnsupportedVersion !MinNodeToClientVersion !NodeToClientVersion


So this should be ShelleyTxCmdQueryError QueryError

I merged ShelleyQueryCmdAcquireFailure and ShelleyQueryCmdUnsupportedVersion constructors into ShelleyQueryCmdQueryError, I think this was what you meant?

Jimbo4350 · 2021-11-22T16:29:22Z

plutus-example/plutus-example/src/Cardano/PlutusExample/ScriptContextChecker.hs

@@ -243,6 +243,7 @@ data ScriptContextError = NoScriptsInByronEra
                        | QueryError ShelleyQueryCmdError
                        | ConsensusModeMismatch AnyConsensusMode AnyCardanoEra
                        | EraMismatch !Consensus.EraMismatch
+                        | ScriptContextUnsupportedVersion !MinNodeToClientVersion !MinNodeToClientVersion


And likewise here ScriptContextUnsupportedVersion QueryError

…sion constructors into ShelleyQueryCmdQueryError

Addressed some comments and explained motivation for some code.

newhoggy · 2023-01-20T07:06:47Z

Re-implemented in #4788

newhoggy force-pushed the better-error-message-for-query-utxo-2 branch 2 times, most recently from b8420d5 to a05cf06 Compare November 16, 2021 11:18

newhoggy commented Nov 17, 2021

View reviewed changes

newhoggy marked this pull request as ready for review November 17, 2021 10:34

newhoggy requested review from DavidEichmann, dcoutts, deepfire, denisshevchenko, erikd, JaredCorduan, Jimbo4350, jutaro, MarcFontaine and nc6 as code owners November 17, 2021 10:34

newhoggy commented Nov 17, 2021

View reviewed changes

Jimbo4350 previously requested changes Nov 17, 2021

View reviewed changes

Jimbo4350 reviewed Nov 17, 2021

View reviewed changes

cardano-api/src/Cardano/Api/Query.hs Show resolved Hide resolved

Jimbo4350 reviewed Nov 17, 2021

View reviewed changes

cardano-api/src/Cardano/Api/IPC/NtcVersionOf.hs Show resolved Hide resolved

Jimbo4350 reviewed Nov 17, 2021

View reviewed changes

cardano-api/src/Cardano/Api/IPC/Monad.hs Outdated Show resolved Hide resolved

newhoggy force-pushed the better-error-message-for-query-utxo-2 branch from 84d1d98 to b70ed67 Compare November 19, 2021 09:20

newhoggy requested a review from Jimbo4350 November 19, 2021 09:46

newhoggy force-pushed the better-error-message-for-query-utxo-2 branch from b70ed67 to df3209a Compare November 19, 2021 22:57

Modify the monadic query interface to not expose ExceptT

c518005

Jimbo4350 previously requested changes Nov 22, 2021

View reviewed changes

De-parameterise QueryError type

ceb47f3

newhoggy force-pushed the better-error-message-for-query-utxo-2 branch from 0242e81 to ceb47f3 Compare November 24, 2021 10:06

Merge ShelleyQueryCmdAcquireFailure and ShelleyQueryCmdUnsupportedVer…

757138a

…sion constructors into ShelleyQueryCmdQueryError

newhoggy requested a review from Jimbo4350 November 24, 2021 13:29

newhoggy requested a review from cleverca22 as a code owner February 17, 2022 01:26

newhoggy requested review from mgmeier, fmaste, LudvikGalois and CarlosLopezDeLara as code owners January 7, 2023 02:13

newhoggy closed this Jan 20, 2023

newhoggy mentioned this pull request Jan 20, 2023

Better error message for query utxo without oops #4788

Merged

Better error message for query utxo 2 #3358

Better error message for query utxo 2 #3358

Uh oh!

Conversation

newhoggy commented Nov 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Jimbo4350 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

newhoggy Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

newhoggy Nov 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Jimbo4350 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

newhoggy Nov 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

newhoggy commented Jan 20, 2023

Uh oh!

Uh oh!

newhoggy commented Nov 10, 2021 •

edited

Loading

newhoggy Nov 19, 2021 •

edited

Loading

newhoggy Nov 19, 2021 •

edited

Loading

newhoggy Nov 24, 2021 •

edited

Loading