Skip to content

Commit

Permalink
chore(docs): Sync wiki to docs [skip-cd]
Browse files Browse the repository at this point in the history
  • Loading branch information
cardano-node-wiki committed Jan 24, 2025
1 parent 0d61ac3 commit bcdccfa
Showing 1 changed file with 65 additions and 2 deletions.
67 changes: 65 additions & 2 deletions docs/ADR-8-ReaderT-Pattern-in-cardano‐cli.md
Original file line number Diff line number Diff line change
@@ -1,8 +1,71 @@
# Status
-[] Adopted
- [ ] Adopted YYYY/MM/DD

# Context
- In `cardano-cli` We have the (anti-)pattern `ExceptT someError IO someResult` 292 times in our codebase.
![image](https://github.com/user-attachments/assets/f405dfca-7d75-404c-b291-3d5140fc8093)

# Decision
- The vast majority of these errors are not used to recover, they are propagated and reported to the user.

This comment has been minimized.

Copy link
@carbolymer

carbolymer Jan 27, 2025

Collaborator

This code example is too big and can be further simplified to illustrate the ExceptT pain points.

This comment has been minimized.

Copy link
@Jimbo4350

Jimbo4350 Jan 27, 2025

Collaborator

Suggest something.

```
main :: IO ()
main = toplevelExceptionHandler $ do
Crypto.cryptoInit
envCli <- getEnvCli
GHC.mkTextEncoding "UTF-8" >>= GHC.setLocaleEncoding
#ifdef UNIX
_ <- setFileCreationMask (otherModes `unionFileModes` groupModes)
#endif
co <- Opt.customExecParser pref (opts envCli)
orDie (docToText . renderClientCommandError) $ runClientCommand co
...
...
...
runClientCommand :: ClientCommand -> ExceptT ClientCommandErrors IO ()
runClientCommand = \case
AnyEraCommand cmds ->
firstExceptT (CmdError (renderAnyEraCommand cmds)) $ runAnyEraCommand cmds
AddressCommand cmds ->
firstExceptT AddressCmdError $ runAddressCmds cmds
NodeCommands cmds ->
runNodeCmds cmds
& firstExceptT NodeCmdError
ByronCommand cmds ->
firstExceptT ByronClientError $ runByronClientCommand cmds
CompatibleCommands cmd ->
firstExceptT (BackwardCompatibleError (renderAnyCompatibleCommand cmd)) $
runAnyCompatibleCommand cmd
HashCmds cmds ->
firstExceptT HashCmdError $ runHashCmds cmds
KeyCommands cmds ->
firstExceptT KeyCmdError $ runKeyCmds cmds
LegacyCmds cmds ->
firstExceptT (CmdError (renderLegacyCommand cmds)) $ runLegacyCmds cmds
QueryCommands cmds ->
firstExceptT QueryCmdError $ runQueryCmds cmds
CliPingCommand cmds ->
firstExceptT PingClientError $ runPingCmd cmds
CliDebugCmds cmds ->
firstExceptT DebugCmdError $ runDebugCmds cmds
Help pprefs allParserInfo ->
runHelp pprefs allParserInfo
DisplayVersion ->
runDisplayVersion
```
- As a result we have a lot of errors wrapped in errors which makes the code unwieldly and difficult to compose with other code blocks. See image below of incidences of poor composability where we use `firstExceptT` (and sometimes `first`) to wrap errors in other errors.
![image](https://github.com/user-attachments/assets/6a50e8f2-9140-4b7e-afa5-ff778d696742)

- The ExceptT IO is a known anti-pattern for these reasons and others as per:
- https://github.com/haskell-effectful/effectful/blob/master/transformers.md
- https://tech.fpcomplete.com/blog/2016/11/exceptions-best-practices-haskell/


# Decision
- As such we are choosing to adopt the [ReaderT design pattern](https://tech.fpcomplete.com/blog/2017/06/readert-design-pattern/), treating all errors as `Exception`s and installing a single error handler at the top level to catch and report these exceptions (with a callstack included).

This comment has been minimized.

Copy link
@carbolymer

carbolymer Jan 27, 2025

Collaborator

ReaderT pattern is a different thing than ExceptT pattern. One is used for configuration and dependency injection, the other is used for error handling. If you would like to describe how we would change our top level monad, there should be two discussions in this PR:

  • change of a top level monad to ReaderT
  • change of errors handling

This ADR shouldn't be called "ReaderT Pattern in cardano‐cli" then.

This comment has been minimized.

Copy link
@carbolymer

carbolymer Jan 27, 2025

Collaborator

There's also no discussion how we will be using exceptions after removal of `ExceptT.

This comment has been minimized.

Copy link
@Jimbo4350

Jimbo4350 Jan 27, 2025

Collaborator

I should have said "install a single exception handler at the top level". We are treating everything as an exception and catching them at the top level. Although just before this I do say we will treat all errors as Exceptions.

This comment has been minimized.

Copy link
@Jimbo4350

Jimbo4350 Jan 27, 2025

Collaborator

there should be two discussions in this PR

We are going to use the RIO monad which also affects error handling. I'll include this.

# Consequences
- This should dramatically improve our code's composability and remove many unnecessary error types.
- Readability concerning what errors can be thrown will be negatively impacted. We believe the trade off is worth it.

This comment has been minimized.

Copy link
@palas

palas Jan 27, 2025

I would justify this. I understand that the reason we think the trade off is worth it is that, when using IO, we already lose this, so we don't gain much by specifying only our own errors

- Initially this will be adopted under the "compatible" group of commands so `cardano-cli` will have a design split temporarily. Once we are happy with the result we will propagate to the rest of `cardano-cli`

0 comments on commit bcdccfa

Please sign in to comment.