-
Notifications
You must be signed in to change notification settings - Fork 721
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
Better error message for query utxo without oops #4788
Conversation
f4375a9
to
7216a81
Compare
@@ -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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 Either
s matters.
Either a (Either b (Either c))
is different to Either c (Either b (Either a))
Different code that stacks the Either
s 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 Either
s 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.
85d122f
to
0daa172
Compare
@@ -1031,20 +1043,24 @@ runQueryStakePools (AnyConsensusModeParams cModeParams) | |||
let localNodeConnInfo = LocalNodeConnectInfo cModeParams network sockPath |
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
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
899530b
to
c806e41
Compare
a05ceb0
to
a7a9ba0
Compare
Just eInMode -> do | ||
sbe <- getSbe $ cardanoEraStyle era | ||
eInMode <- toEraInMode era cMode | ||
& hoistMaybe (ShelleyQueryCmdEraConsensusModeMismatch (AnyConsensusMode cMode) anyE) |
There was a problem hiding this comment.
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.
d20ecea
to
c1b2b37
Compare
2ff2e9b
to
095d372
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some comments
@@ -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) |
There was a problem hiding this comment.
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
?
There was a problem hiding this comment.
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.
6f3d2a8
to
db89c35
Compare
db89c35
to
3c5a949
Compare
3c5a949
to
3ff371c
Compare
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
andonNothing
fromtransformers-except-0.1.3
to improve the way we structure code.