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.

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

Move implementation inside runTransactionCmd to toplevel definitions. #4673

Merged
merged 1 commit into from
Nov 28, 2022

Conversation

MarcFontaine
Copy link
Contributor

No description provided.

@MarcFontaine
Copy link
Contributor Author

#4672

Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

LGTM but one comment

@@ -447,6 +302,215 @@ runTransactionCmd cmd =
-- Building transactions
--

runTxBuildCmd
:: IsCardanoEra era
=> CardanoEra era
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM, however lets use AnyCardanoEra here and add some haddock annotations to the parameters where it's not obvious what they are e.g Maybe Word. The haddocks don't have to be overly descriptive, short succinct ones will do.

Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment

Choose a reason for hiding this comment

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

LGTM!

@MarcFontaine MarcFontaine force-pushed the mafo/4672-cleanup-case branch from f0aa006 to 8ec395f Compare November 28, 2022 16:17
@MarcFontaine MarcFontaine force-pushed the mafo/4672-cleanup-case branch from 8ec395f to 29edb03 Compare November 28, 2022 16:20
@MarcFontaine
Copy link
Contributor Author

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented Nov 28, 2022

Build succeeded:

@iohk-bors iohk-bors bot merged commit 20ac47f into master Nov 28, 2022
@iohk-bors iohk-bors bot deleted the mafo/4672-cleanup-case branch November 28, 2022 16:56
@MarcFontaine
Copy link
Contributor Author

#4672

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants