Fully remove the cli intermediate TxBody format #4713

Jimbo4350 · 2022-12-14T19:15:37Z

Resolves #4814

In this PR we fully remove support for the intermediate cli tx body format. As a result we make the following changes:

We extend the HasTextEnvelope type class as follows: textEnvelopeType :: AsType a -> TextEnvelopeType -> textEnvelopeType :: AsType a -> [TextEnvelopeType]. We do this so we can have backwards compatibility for the TextEnvelopeTypes that were introduced when we had to differentiate between the intermediate format and the CDDL format.
We delete Cardano.Api.SerialiseLedgerCddl as we do not want to maintain a separate type for the CDDL format. We should default to the CDDL format via the TextEnvelope type
We introduce the Cardano.Api.TxBodyInstances module because the "trick" we use to avoid the txbody intermediate format, is to convert the TxBody into an unsigned Tx.

newhoggy · 2022-12-14T22:50:22Z

There are some golden test failures.

Jimbo4350 · 2022-12-19T18:33:38Z

There are some golden test failures.

Fixed

newhoggy · 2023-01-12T22:32:09Z

cardano-api/src/Cardano/Api/SerialiseTextEnvelope.hs

    , teDescription   = fromMaybe (textEnvelopeDefaultDescr a) mbDescr
    , teRawCBOR = serialiseToCBOR a
    }
  where
+    teType :: [TextEnvelopeType] -> TextEnvelopeType
+    teType [] = ""
+    teType (x : _) = x


Paraphrasing, the teType is the first in the list or the empty string. If there original list had more than one, what is the significance of the ordering?

It will default to the first in the list. When reading a TextEnvelope file, it will check for all the types in the list hence preserving backwards compatibility. However when producing a TextEnvelope we simply default to the first in the list if there is more than one teType.

newhoggy · 2023-01-13T00:42:08Z

cardano-api/src/Cardano/Api/Tx.hs

+decodeKeyOrBootstrapWitness era bs =
+  case CBOR.decodeAnnotator "KeyWitness" fromCBOR (LBS.fromStrict bs) of
+    Right keyWit -> return $ ShelleyKeyWitness era keyWit
+    Left{} ->


What's the reason for ignoring Left payload?

So that we can try to deserialize a bootstrap witness

cardano-api/src/Cardano/Api/TxBodyInstances.hs

newhoggy · 2023-01-13T00:49:54Z

cardano-api/test/Test/Cardano/Api/Typed/Envelope.hs

+      ("roundtrip " <> (fromString . Text.unpack $ renderEra aEra) <> " TxBody envelope")
+      ("roundtrip " <> (fromString . Text.unpack $ renderEra aEra) <> " TxBody envelope")


Prefer:

("roundtrip " <> fromString (Text.unpack (renderEra aEra)) <> " TxBody envelope") ("roundtrip " <> fromString (Text.unpack (renderEra aEra)) <> " TxBody envelope")

or:

("roundtrip " <> fromString (Text.unpack $ renderEra aEra) <> " TxBody envelope") ("roundtrip " <> fromString (Text.unpack $ renderEra aEra) <> " TxBody envelope")

MarcFontaine · 2023-01-13T18:03:35Z

cardano-api/src/Cardano/Api/Certificate.hs

@@ -108,7 +108,7 @@ instance FromCBOR Certificate where
    fromCBOR = fromShelleyCertificate <$> fromCBOR

 instance HasTextEnvelope Certificate where
-    textEnvelopeType _ = "CertificateShelley"


"CertificateShelley" and the other textEnvelopeType names are all magic IDs and they are assumed to be unique throughout the code.
It would be better to have all those unique ID in one place.

What do you have in mind?

MarcFontaine · 2023-01-13T18:54:23Z

There should be an issue for this PR.
Do I understand correctly, that the CLI will solely use the ledger CBOR formats for all its file formats ?
The CLI and the ledger have very different requirements.
The ledger probably wants a very compact format, but for the CLI a human readable format like JSON may be preferable.

Is there any document that captures the requirements for the CLI on-disk-formats ?
What kinds of backward compatibility does the CLI provide ?
The CLI allows users to write stuff to files and the users expect to read the files in again.
Does the CLI support reading Txs etc that where created with older CLI versions ?
Fore sure, users expect that they can read in their secret keys etc.
Some users may expect to ready in unsigned to partially signed transactions of older CLI-versions.

Reading input and writing output formats is the core functionality of the CLI.
There should be some documentation on the subject.

The CLI does different task (crate TX body, sign TX, balance TX) while the ledger only deals
with complete TXs. Why use the ledger format for intermediate CLI formats ?

Jimbo4350 · 2023-01-19T15:05:03Z

There should be an issue for this PR

I created an issue created: #4814

Do I understand correctly, that the CLI will solely use the ledger CBOR formats for all its file formats ?

Yes.

The ledger probably wants a very compact format, but for the CLI a human readable format like JSON may be preferable.

We should stick to the CDDL format for tx ondisk. This is to allow compatibility for existing libraries that expect the transaction to be in the CDDL format. This whole issue arose out of this incompatibility (see #3370)

Is there any document that captures the requirements for the CLI on-disk-formats ?

No, but for now we are opting to not deviate from the CDDL format

What kinds of backward compatibility does the CLI provide ?

None as of this PR regarding transaction body formats.

Does the CLI support reading Txs etc that where created with older CLI versions ?

Not as of this PR

Some users may expect to ready in unsigned to partially signed transactions of older CLI-versions.

I highly doubt this

There should be some documentation on the subject.

This would be a great task for you to spearhead.

Why use the ledger format for intermediate CLI formats ?

Have a look at #3370

`TextEnvelopeType`s. This allows for easy backwards compatibility for the `TextEnvelopeType`s that were introduced to distinguish between the cli intermediate `TxBody` format and the CDDL format.

Implement allTxBodyEnvelopeRoundtrips which roundtrip tests the TxBody TextEnvelope format in all eras Update shelley era properties to check for the correct TextEnvelopeType

Update HasTextEnvelope (Tx era) instance to be backwards compatible with `TextEnvelopeType`s introduced to distinguish between the CDDL format and the intermediate cli txbody format Update the SerialiseAsCBOR (KeyWitness era) instance to use the ledger's CDDL format Update checkTextEnvelopeFormat to accept `[TextEnvelopeType]` as we can now specify multiple `TextEnvelopeType`s in a given `HasTextEnvelope` instance

Jimbo4350 · 2024-03-28T13:27:04Z

Resolved by: IntersectMBO/cardano-api@e739675

Jimbo4350 requested review from dcoutts, erikd, newhoggy, JaredCorduan, LudvikGalois and CarlosLopezDeLara as code owners December 14, 2022 19:15

Jimbo4350 changed the title ~~Full remove the cli intermediate TxBody format~~ Fully remove the cli intermediate TxBody format Dec 19, 2022

Jimbo4350 force-pushed the jordan/fully-remove-cli-txbody-intermediate-format branch from 2c5e96e to a52fa5e Compare December 19, 2022 18:32

Jimbo4350 force-pushed the jordan/fully-remove-cli-txbody-intermediate-format branch 3 times, most recently from 8e29465 to 053db1c Compare December 21, 2022 19:08

Jimbo4350 added the WIP Work In Progress (cannot be merged yet) label Dec 21, 2022

Jimbo4350 force-pushed the jordan/fully-remove-cli-txbody-intermediate-format branch from 053db1c to 93097ab Compare January 4, 2023 16:47

Jimbo4350 force-pushed the jordan/fully-remove-cli-txbody-intermediate-format branch from 93097ab to ec4cb25 Compare January 11, 2023 17:41

Jimbo4350 requested review from a team, deepfire, mgmeier, fmaste, jutaro and cleverca22 as code owners January 11, 2023 17:41

Jimbo4350 force-pushed the jordan/fully-remove-cli-txbody-intermediate-format branch from ec4cb25 to 34ad3dd Compare January 11, 2023 19:25

olgahryniuk approved these changes Jan 12, 2023

View reviewed changes

Jimbo4350 removed the WIP Work In Progress (cannot be merged yet) label Jan 12, 2023

Jimbo4350 force-pushed the jordan/fully-remove-cli-txbody-intermediate-format branch from 34ad3dd to 8182666 Compare January 12, 2023 19:56

newhoggy reviewed Jan 12, 2023

View reviewed changes

newhoggy reviewed Jan 13, 2023

View reviewed changes

cardano-api/src/Cardano/Api/TxBodyInstances.hs Outdated Show resolved Hide resolved

newhoggy reviewed Jan 13, 2023

View reviewed changes

cardano-api/src/Cardano/Api/TxBodyInstances.hs Outdated Show resolved Hide resolved

newhoggy reviewed Jan 13, 2023

View reviewed changes

MarcFontaine reviewed Jan 13, 2023

View reviewed changes

Jimbo4350 force-pushed the jordan/fully-remove-cli-txbody-intermediate-format branch from 8182666 to c4c8a63 Compare January 16, 2023 15:43

Jimbo4350 force-pushed the jordan/fully-remove-cli-txbody-intermediate-format branch from c4c8a63 to 5607d15 Compare January 25, 2023 12:37

Jimbo4350 force-pushed the jordan/fully-remove-cli-txbody-intermediate-format branch from 113bd3d to 0d89fdd Compare February 8, 2023 14:40

Jimbo4350 added 11 commits February 28, 2023 11:43

Extend HasTextEnvelope typeclass to allow multiple

abd7f9a

`TextEnvelopeType`s. This allows for easy backwards compatibility for the `TextEnvelopeType`s that were introduced to distinguish between the cli intermediate `TxBody` format and the CDDL format.

Propagate the HasTextEnvelope change

9ea2f9d

Remove Cardano.Api.SerialiseLedgerCddl

b75fb5c

Delete Test.Cli.CliIntermediateFormat

65ea01b

Remove anything CDDL related from Cardano.CLI.Shelley.Run.Read

e0b211c

Remove anything CDDL related from Cardano.CLI.Shelley.Run.Transaction

40d84bb

Remove roundtrip_CDDL_Tx and any tests using it

15a91e1

Implement allTxBodyEnvelopeRoundtrips which roundtrip tests the TxBody TextEnvelope format in all eras Update shelley era properties to check for the correct TextEnvelopeType

Review fixes

fb194be

Redundant module fixes

f27a937

Implement Error RequiredSignerError instance

c7f3d7a

Jimbo4350 force-pushed the jordan/fully-remove-cli-txbody-intermediate-format branch from 0d89fdd to a931959 Compare February 28, 2023 15:56

Rebase fixes

bb69f40

Jimbo4350 force-pushed the jordan/fully-remove-cli-txbody-intermediate-format branch from a931959 to bb69f40 Compare February 28, 2023 16:43

Jimbo4350 closed this Mar 28, 2024

Jimbo4350 deleted the jordan/fully-remove-cli-txbody-intermediate-format branch May 15, 2024 20:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fully remove the cli intermediate TxBody format #4713

Fully remove the cli intermediate TxBody format #4713

Jimbo4350 commented Dec 14, 2022 •

edited

Loading

newhoggy commented Dec 14, 2022

Jimbo4350 commented Dec 19, 2022

newhoggy Jan 12, 2023

Jimbo4350 Jan 16, 2023

newhoggy Jan 13, 2023

Jimbo4350 Jan 16, 2023 •

edited

Loading

newhoggy Jan 13, 2023 •

edited

Loading

Jimbo4350 Jan 16, 2023

MarcFontaine Jan 13, 2023 •

edited

Loading

Jimbo4350 Jan 16, 2023

MarcFontaine commented Jan 13, 2023

Jimbo4350 commented Jan 19, 2023 •

edited

Loading

Jimbo4350 commented Mar 28, 2024

		("roundtrip " <> (fromString . Text.unpack $ renderEra aEra) <> " TxBody envelope")
		("roundtrip " <> (fromString . Text.unpack $ renderEra aEra) <> " TxBody envelope")

Fully remove the cli intermediate TxBody format #4713

Fully remove the cli intermediate TxBody format #4713

Conversation

Jimbo4350 commented Dec 14, 2022 • edited Loading

newhoggy commented Dec 14, 2022

Jimbo4350 commented Dec 19, 2022

newhoggy Jan 12, 2023

Choose a reason for hiding this comment

Jimbo4350 Jan 16, 2023

Choose a reason for hiding this comment

newhoggy Jan 13, 2023

Choose a reason for hiding this comment

Jimbo4350 Jan 16, 2023 • edited Loading

Choose a reason for hiding this comment

newhoggy Jan 13, 2023 • edited Loading

Choose a reason for hiding this comment

Jimbo4350 Jan 16, 2023

Choose a reason for hiding this comment

MarcFontaine Jan 13, 2023 • edited Loading

Choose a reason for hiding this comment

Jimbo4350 Jan 16, 2023

Choose a reason for hiding this comment

MarcFontaine commented Jan 13, 2023

Jimbo4350 commented Jan 19, 2023 • edited Loading

Jimbo4350 commented Mar 28, 2024

Jimbo4350 commented Dec 14, 2022 •

edited

Loading

Jimbo4350 Jan 16, 2023 •

edited

Loading

newhoggy Jan 13, 2023 •

edited

Loading

MarcFontaine Jan 13, 2023 •

edited

Loading

Jimbo4350 commented Jan 19, 2023 •

edited

Loading