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

cardano-cli: simplifiy the consumption of 'InputTxBodyOrTxFile` #4647

Closed
wants to merge 2 commits into from
Closed

cardano-cli: simplifiy the consumption of 'InputTxBodyOrTxFile` #4647

wants to merge 2 commits into from

Conversation

MarcFontaine
Copy link
Contributor

@MarcFontaine MarcFontaine commented Nov 18, 2022 •
edited
Loading

This PR makes is easier to use CLI arguments of type InputTxBodyOrTxFile

  • Reorder IncompleteTx
  • Add TxOrTxBody
  • Add readInputTxBodyOrTxFile :: InputTxBodyOrTxFile -> IO (Either CddlError (InAnyCardanoEra CompleteOrIncompleteTx)

@MarcFontaine MarcFontaine added the WIP Work In Progress (cannot be merged yet) label Nov 18, 2022
@MarcFontaine MarcFontaine self-assigned this Nov 18, 2022
@MarcFontaine MarcFontaine force-pushed the mafo/IncompleteTx-type branch 2 times, most recently from a1bb2ad to 767379a Compare November 21, 2022 14:05
@MarcFontaine MarcFontaine mentioned this pull request Nov 21, 2022
Closed
@MarcFontaine MarcFontaine force-pushed the mafo/IncompleteTx-type branch 5 times, most recently from e2ed0a4 to 6b62c80 Compare November 21, 2022 16:36
@MarcFontaine
Copy link
Contributor Author

This PR implements:
#4646
and
#4576

This was referenced Nov 21, 2022
Open
Open
@MarcFontaine MarcFontaine removed the WIP Work In Progress (cannot be merged yet) label Nov 21, 2022
@MarcFontaine
cardano-cli: simplifiy the consumption of 'InputTxBodyOrTxFile`
d9ae0c7 
Also:
* extend transaction view command with new options
* extend transcation assemble command to allow adding extra witnesses.
@MarcFontaine MarcFontaine force-pushed the mafo/IncompleteTx-type branch from 6b62c80 to d9ae0c7 Compare November 21, 2022 16:45
@MarcFontaine MarcFontaine marked this pull request as ready for review November 21, 2022 16:46
newhoggy
newhoggy reviewed Nov 22, 2022
View reviewed changes
cardano-cli/src/Cardano/CLI/Shelley/Run/Transaction.hs
IncompleteCddlFormattedTx anyTx -> do
InAnyShelleyBasedEra _era unwitTx <-
anyTx <- readFileTxBody txbodyFile `rescue` ShelleyTxCmdCddlError
--TODO: in principle we should be able to support Byron era txs too
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you say what currently happens with Byron era transactions?

newhoggy
newhoggy reviewed Nov 22, 2022
View reviewed changes
cardano-cli/src/Cardano/CLI/Shelley/Run/Transaction.hs
TextFormat -> writeFileTextEnvelope oFp Nothing tx `rescue` ShelleyTxCmdWriteFileError

rescue :: Functor m => m (Either x a) -> (x -> y) -> ExceptT y m a
rescue action except = firstExceptT except $ newExceptT action
Copy link
Contributor

Choose a reason for hiding this comment

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

We should probably move this to a re-usable location, but not right now.

newhoggy
newhoggy reviewed Nov 22, 2022
View reviewed changes
cardano-cli/src/Cardano/CLI/Shelley/Run/Transaction.hs Outdated
firstExceptT ShelleyTxCmdWriteFileError . newExceptT $
writeTxFileTextEnvelopeCddl oFp tx
runTxSignWitness inputTx witnessFiles (OutputFile oFp) = do
(InAnyCardanoEra era input) <- readInputTxBodyOrTxFile inputTx `rescue` ShelleyTxCmdCddlError
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm in favour of dropping the redundant parens in this bind.

newhoggy
newhoggy reviewed Nov 22, 2022
View reviewed changes
cardano-cli/src/Cardano/CLI/Shelley/Run/Transaction.hs Outdated
CompleteTx tx -> friendlyTxBS era tx
IncompleteTx (UnwitnessedCliFormattedTxBody txBody) -> friendlyTxBodyBS era txBody
-- In the IncompleteCddlFormattedTx case, the legacy format
-- takes the body of the tx and converts the tx to a tx-body
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove double space after tx.

newhoggy
newhoggy reviewed Nov 22, 2022
View reviewed changes
cardano-cli/src/Cardano/CLI/Shelley/Run/Transaction.hs
-- and prints that using `friendlyTxBodyBS era body`.
-- Any preexisting witnesses (that may be there ?!? ) are discarded !!
-- Would make more sense:
-- IncompleteTx (IncompleteCddlFormattedTx tx) -> friendlyTxBS era tx
Copy link
Contributor

@newhoggy newhoggy Nov 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This is an interesting point. @Jimbo4350 @LudvikGalois

newhoggy
newhoggy reviewed Nov 22, 2022
View reviewed changes
Copy link
Contributor

@newhoggy newhoggy left a comment

Choose a reason for hiding this comment

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

Left some review comments

Jimbo4350
Jimbo4350 requested changes Nov 22, 2022
View reviewed changes
cardano-cli/src/Cardano/CLI/Shelley/Run/Read.hs
case eTxBody of
Left e -> fmap (IncompleteCddlFormattedTx . unCddlTx) <$> acceptTxCDDLSerialisation e
Right txBody -> return $ Right $ UnwitnessedCliFormattedTxBody txBody
data IncompleteTx era
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the benefit of adding the era parameter? This has further complicated the code without any obvious benefit.

Jimbo4350
Jimbo4350 requested changes Nov 22, 2022
View reviewed changes
Copy link
Contributor

@Jimbo4350 Jimbo4350 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I am highly skeptical of the changes to data IncompleteTx. I don't see any obvious benefit for the increase in code complexity.

@MarcFontaine
Copy link
Contributor Author

The old definition was the root-cause for the code-duplication in Cardano.CLI.Shelley.Run.Transaction.

@Jimbo4350
Copy link
Contributor

The old definition was the root-cause for the code-duplication in Cardano.CLI.Shelley.Run.Transaction.

Lets hold off on the refactor for now as:

data IncompleteTx
  = UnwitnessedCliFormattedTxBody (InAnyCardanoEra TxBody)
  | IncompleteCddlFormattedTx (InAnyCardanoEra Tx)

Will be eliminated when we remove the cli format, which I will like to do in the next cli release. Once we do that we can revisit the refactor.

Jimbo4350
Jimbo4350 requested changes Nov 28, 2022
View reviewed changes
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.

#4647 (comment)

@newhoggy newhoggy closed this Dec 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Jimbo4350 Jimbo4350 Jimbo4350 requested changes

@dcoutts dcoutts Awaiting requested review from dcoutts

@erikd erikd Awaiting requested review from erikd

@JaredCorduan JaredCorduan Awaiting requested review from JaredCorduan

@LudvikGalois LudvikGalois Awaiting requested review from LudvikGalois

@CarlosLopezDeLara CarlosLopezDeLara Awaiting requested review from CarlosLopezDeLara

@newhoggy newhoggy Awaiting requested review from newhoggy

Assignees

@MarcFontaine MarcFontaine

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MarcFontaine @Jimbo4350 @newhoggy