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.

Sign up for GitHub

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

Jump to bottom

Integrate RIO monad part 1 #1033

Merged
merged 5 commits into from
Feb 19, 2025
Merged

Integrate RIO monad part 1 #1033

merged 5 commits into from
Feb 19, 2025

Conversation

Jimbo4350
Copy link
Contributor

@Jimbo4350 Jimbo4350 commented Jan 28, 2025 •
edited
Loading

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

@Jimbo4350 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 Jimbo4350 marked this pull request as ready for review January 28, 2025 13:36
carbolymer
carbolymer requested changes Jan 28, 2025
View reviewed changes
Copy link
Contributor

@carbolymer carbolymer left a comment

Choose a reason for hiding this comment

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

Can we finish ADR first?

@Jimbo4350 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 Jimbo4350 requested a review from carbolymer February 10, 2025 19:17
@Jimbo4350 Jimbo4350 force-pushed the jordan/integrate-RIO-monad-part-1 branch from 796aaf1 to 5a36d5d Compare February 11, 2025 15:23
carbolymer
carbolymer requested changes Feb 11, 2025
View reviewed changes
Copy link
Contributor

@carbolymer carbolymer left a comment

Choose a reason for hiding this comment

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

tactical merge block - I need to play with it

carbolymer
carbolymer reviewed Feb 12, 2025
View reviewed changes
cardano-cli/src/Cardano/CLI/Compatible/Exception.hs Outdated

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

@carbolymer carbolymer Feb 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We are not catching exceptions. We are converting errors to exceptions via CustomCliException. Perhaps we should change the name.

Copy link
Contributor

@carbolymer carbolymer Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Yes, you are right. I'm talking about a slightly different situation: that IO exceptions from this line sometimes won't have call stack. We can do something about it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

We could make this even more robust by making CIO a newtype, and not providing MonadIO: 3c1f6f0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you update the ADR with this first?

carbolymer
carbolymer requested changes Feb 12, 2025
View reviewed changes
Copy link
Contributor

@carbolymer carbolymer left a comment

Choose a reason for hiding this comment

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

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

@Jimbo4350 Jimbo4350 force-pushed the jordan/integrate-RIO-monad-part-1 branch from 5a36d5d to 3b56ca8 Compare February 13, 2025 13:40
carbolymer
carbolymer reviewed Feb 14, 2025
View reviewed changes
cardano-cli/src/Cardano/CLI/Compatible/Transaction.hs
shelleyBasedEraConstraints sbe $ do
sks <- mapM (fromEitherIOCli . readWitnessSigningData) witnesses

allOuts <- fromEitherIOCli . runExceptT $ mapM (toTxOutInAnyEra sbe) outs
Copy link
Contributor

@carbolymer carbolymer Feb 14, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

adding another function

fromExceptTCli = fromEitherIOCli . runExceptT

could save us some boilerplate

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm hesitant about this because we want to remove ExceptT usage.

Copy link
Contributor

Choose a reason for hiding this comment

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

But what about cardano-api? We agreed to not throw exceptions there

Copy link
Contributor Author

@Jimbo4350 Jimbo4350 Feb 19, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I don't understand what you mean. We want to remove ExceptT usage in cardano-cli. I.e not implement functions in ExceptT. We can introduce a function like you suggested but it needs to be clear that it's only for use if we are importing functions from a package that does use ExceptT.

Copy link
Contributor

Choose a reason for hiding this comment

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

We have functions returning a type ExceptT e m a in cardano-api. We still need to use them in cardano-cli.

@Jimbo4350 Jimbo4350 force-pushed the jordan/integrate-RIO-monad-part-1 branch from 3b56ca8 to 8e380af Compare February 19, 2025 18:23
@Jimbo4350 Jimbo4350 enabled auto-merge February 19, 2025 18:23
@Jimbo4350 Jimbo4350 force-pushed the jordan/integrate-RIO-monad-part-1 branch from 8e380af to c279d97 Compare February 19, 2025 18:39
@Jimbo4350 Jimbo4350 added this pull request to the merge queue Feb 19, 2025
Merged via the queue into master with commit 3b7a6ce Feb 19, 2025
25 checks passed
@Jimbo4350 Jimbo4350 deleted the jordan/integrate-RIO-monad-part-1 branch February 19, 2025 19:22
@carbolymer carbolymer mentioned this pull request Mar 4, 2025
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carbolymer carbolymer carbolymer approved these changes

@newhoggy newhoggy Awaiting requested review from newhoggy newhoggy is a code owner

@smelc smelc Awaiting requested review from smelc

@palas palas Awaiting requested review from palas palas is a code owner

@kevinhammond kevinhammond Awaiting requested review from kevinhammond kevinhammond is a code owner

@disassembler disassembler Awaiting requested review from disassembler disassembler is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Jimbo4350 @carbolymer