Integrate RIO monad part 1 #1033

Jimbo4350 · 2025-01-28T13:26:56Z

Changelog

- description: |
    Integrate RIO monad in "compatible signed-transaction" command 
  type:
  - refactoring    # QoL changes

Context

How to trust this PR

Highlight important bits of the PR that will make the review faster. If there are commands the reviewer can run to observe the new behavior, describe them.

Checklist

Commit sequence broadly makes sense and commits have useful messages
New tests are added if needed and existing tests are updated. See Running tests for more details
Self-reviewed the diff

carbolymer

Can we finish ADR first?

cardano-cli/src/Cardano/CLI/Compatible/Exception.hs

cardano-cli/src/Cardano/CLI/Compatible/Run.hs

carbolymer

tactical merge block - I need to play with it

carbolymer · 2025-02-11T19:28:00Z

cardano-cli/src/Cardano/CLI/Compatible/Exception.hs

+fromEitherIOCli action = do
+  result <- liftIO action
+  case result of
+    Left e -> throwCliError $ CustomCliException e callStack
+    Right a -> return a


Suggested change

fromEitherIOCli action = do

result <- liftIO action

case result of

Left e -> throwCliError $ CustomCliException e callStack

Right a -> return a

fromEitherIOCli action = liftIO action >>= fromEitherCli

carbolymer · 2025-02-11T19:30:20Z

cardano-cli/src/Cardano/CLI/Compatible/Run.hs

+
+-- | This is a temporary function until all commands are migrated to RIO
+-- Once this happens we can remove this function and rely on `toplevelExceptionHandler`
+executeRio :: RIO () () -> IO ()


What about an alias type CIO e a = HasCallStack => RIO e a ?

carbolymer · 2025-02-11T19:31:03Z

cardano-cli/src/Cardano/CLI/Compatible/Transaction.hs

-  -> ExceptT CompatibleTransactionError IO ()
+   . HasCallStack
+  => CompatibleTransactionCmds era
+  -> RIO () ()


why concrete environment?

Suggested change

-> RIO () ()

-> RIO e ()

carbolymer · 2025-02-11T19:31:41Z

cardano-cli/src/Cardano/CLI/Compatible/Run.hs

+executeRio :: RIO () () -> IO ()
+executeRio r = do
+  runRIO () r
+    `catch` (\(e :: SomeException) -> putStrLn $ displayException e)


Suggested change

`catch` (\(e :: SomeException) -> putStrLn $ displayException e)

`catch` (\(e :: SomeException) -> hPutStr stderr $ displayException e)

you'd need some imports for that

The exitcode for cardano-cli here is 0 which is wrong.

I think we can remove this handler and just let Cardano.CLI.TopHandler.toplevelExceptionHandler do the job.
I'd just simplify Cardano.CLI.TopHandler.renderSomeException to just use displayException. I think show there is just making output more noisy without any useful info.

carbolymer · 2025-02-11T19:35:06Z

cardano-cli/src/Cardano/CLI/Compatible/Exception.hs

+throwCliError = throwIO
+
+fromEitherCli :: (HasCallStack, MonadIO m, Show e, Typeable e, Error e) => Either e a -> m a
+fromEitherCli = \case


Suggested change

fromEitherCli = \case

fromEitherCli = withFrozenCallStack $ \case

carbolymer · 2025-02-11T19:37:46Z

cardano-cli/src/Cardano/CLI/Compatible/Transaction.hs

+            ]
+
+      validatedRefInputs <-
+        fromEitherCli . first CompatibleTxCmdError . validateTxInsReference $


is this extra wrapping needed?

Suggested change

fromEitherCli . first CompatibleTxCmdError . validateTxInsReference $

fromEitherCli . validateTxInsReference $

carbolymer · 2025-02-11T19:38:47Z

cardano-cli/src/Cardano/CLI/Types/Errors/BootstrapWitnessError.hs

@@ -14,6 +14,9 @@ data BootstrapWitnessError
    MissingNetworkIdOrByronAddressError
  deriving Show

+instance Error BootstrapWitnessError where
+  prettyError = renderBootstrapWitnessError


Let's remove render* functions for Errors and just use prettyError. Could be out of scope for this PR, because it's only slightly related.

carbolymer · 2025-02-12T14:16:07Z

cardano-cli/src/Cardano/CLI/Compatible/Exception.hs

+data CustomCliException where
+  CustomCliException
+    :: (Show error, Typeable error, Error error)
+    => error -> CallStack -> CustomCliException


Why explicit CallStack argument? The way it's used it can be an implicit one.

carbolymer · 2025-02-12T14:17:32Z

cardano-cli/src/Cardano/CLI/Compatible/Exception.hs

+{-# LANGUAGE GADTs #-}
+{-# LANGUAGE LambdaCase #-}
+{-# LANGUAGE StandaloneDeriving #-}
+{-# LANGUAGE UndecidableInstances #-}


Suggested change

{-# LANGUAGE UndecidableInstances #-}

carbolymer · 2025-02-12T14:30:27Z

cardano-cli/src/Cardano/CLI/Compatible/Exception.hs

+
+fromEitherIOCli :: (HasCallStack, MonadIO m, Show e, Typeable e, Error e) => IO (Either e a) -> m a
+fromEitherIOCli action = do
+  result <- liftIO action


This is more complicated than just liftIO. We need to catch all synchronous exceptions here and wrap them, and also provide the call stack. So basically we need analogous wrapper to CustomCliException but for Exceptions this time.

This is because IOException for example does not carry the call stack with it (yet), so we should manually add it to it.

Unfortunately it'll be just a call stack up to fromEitherIOCli call.

carbolymer · 2025-02-12T14:52:20Z

cardano-cli/src/Cardano/CLI/Compatible/Exception.hs

+
+module Cardano.CLI.Compatible.Exception
+  ( CustomCliException (..)
+  , throwCliError


I don't think this should be part of the API of this module. This function isn't useful on its own - you still need to attach call stack manually.

Suggested change

, throwCliError

carbolymer

Looks promising. We're getting there. Can you address my comments?

Jimbo4350 force-pushed the jordan/integrate-RIO-monad-part-1 branch 4 times, most recently from 69744e1 to 989ba8a Compare January 28, 2025 13:35

Jimbo4350 marked this pull request as ready for review January 28, 2025 13:36

Jimbo4350 requested review from newhoggy, carbolymer, smelc, palas, kevinhammond and disassembler as code owners January 28, 2025 13:36

carbolymer requested changes Jan 28, 2025

View reviewed changes

carbolymer reviewed Jan 29, 2025

View reviewed changes

cardano-cli/src/Cardano/CLI/Compatible/Exception.hs Show resolved Hide resolved

carbolymer reviewed Jan 29, 2025

View reviewed changes

cardano-cli/src/Cardano/CLI/Compatible/Run.hs Outdated Show resolved Hide resolved

carbolymer reviewed Jan 29, 2025

View reviewed changes

cardano-cli/src/Cardano/CLI/Compatible/Run.hs Outdated Show resolved Hide resolved

Jimbo4350 force-pushed the jordan/integrate-RIO-monad-part-1 branch 6 times, most recently from 510a872 to 796aaf1 Compare February 10, 2025 19:14

Jimbo4350 requested a review from carbolymer February 10, 2025 19:17

Jimbo4350 added 4 commits February 11, 2025 11:23

Introduce CustomCliException exception type

2664463

Convert runCompatibleTransactionCmd to use RIO monad

a005d89

Temporarily capture SomeException in runCompatibleCommand

96e5e11

Add outstanding Error instances

5a36d5d

Jimbo4350 force-pushed the jordan/integrate-RIO-monad-part-1 branch from 796aaf1 to 5a36d5d Compare February 11, 2025 15:23

carbolymer requested changes Feb 11, 2025

View reviewed changes

carbolymer reviewed Feb 12, 2025

View reviewed changes

carbolymer requested changes Feb 12, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integrate RIO monad part 1 #1033

Integrate RIO monad part 1 #1033

Jimbo4350 commented Jan 28, 2025 •

edited

Loading

carbolymer left a comment

carbolymer left a comment

carbolymer Feb 11, 2025

carbolymer Feb 11, 2025

carbolymer Feb 11, 2025

carbolymer Feb 11, 2025 •

edited

Loading

carbolymer Feb 12, 2025 •

edited

Loading

carbolymer Feb 11, 2025

carbolymer Feb 11, 2025

carbolymer Feb 11, 2025

carbolymer Feb 12, 2025

carbolymer Feb 12, 2025

carbolymer Feb 12, 2025 •

edited

Loading

carbolymer Feb 12, 2025 •

edited

Loading

carbolymer left a comment

	`catch` (\(e :: SomeException) -> putStrLn $ displayException e)
	`catch` (\(e :: SomeException) -> hPutStr stderr $ displayException e)

	fromEitherCli = \case
	fromEitherCli = withFrozenCallStack $ \case

	fromEitherCli . first CompatibleTxCmdError . validateTxInsReference $
	fromEitherCli . validateTxInsReference $

Integrate RIO monad part 1 #1033

Are you sure you want to change the base?

Integrate RIO monad part 1 #1033

Conversation

Jimbo4350 commented Jan 28, 2025 • edited Loading

Changelog

Context

How to trust this PR

Checklist

carbolymer left a comment

Choose a reason for hiding this comment

carbolymer 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

carbolymer Feb 11, 2025 • edited Loading

Choose a reason for hiding this comment

carbolymer Feb 12, 2025 • edited Loading

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

carbolymer Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

carbolymer Feb 12, 2025 • edited Loading

Choose a reason for hiding this comment

carbolymer left a comment

Choose a reason for hiding this comment

Jimbo4350 commented Jan 28, 2025 •

edited

Loading

carbolymer Feb 11, 2025 •

edited

Loading

carbolymer Feb 12, 2025 •

edited

Loading

carbolymer Feb 12, 2025 •

edited

Loading

carbolymer Feb 12, 2025 •

edited

Loading