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

Add new types to list of accepted types #892

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open

Add new types to list of accepted types #892

wants to merge 7 commits into from
+91 −26

Conversation

palas
Copy link
Contributor

@palas palas commented Sep 11, 2024 •
edited
Loading

DO NOT MERGE until the source-repository-package stanza is removed

Changelog

- description: |
    Added support for new types
# uncomment types applicable to the change:
  type:
  - bugfix         # fixes a defect

Context

There was an issue with transactions created using the non-deprecated function serialiseToTextEnvelope as alternative to serialiseTxLedgerCddl. Because the implementation in cardano-cli still doesn't accept it.

This PR ensures the new types are accepted.

There is a corresponding PR that does a change required by this PR in cardano-api: IntersectMBO/cardano-api#634

How to trust this PR

Ensure this solves the problem that was reported.

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

@palas palas added the bug Something isn't working label Sep 11, 2024
@palas palas self-assigned this Sep 11, 2024
@palas palas mentioned this pull request Sep 11, 2024
Merged
3 tasks
@palas palas requested a review from ch1bo September 11, 2024 23:00
carbolymer
carbolymer reviewed Sep 20, 2024
View reviewed changes
cardano-cli/test/cardano-cli-test/Test/Cli/Parser.hs
AnyShelleyBasedEra era <- H.noteShowM . forAll $ Gen.element [minBound .. maxBound]
x <- forAll $ genTx era
shelleyBasedEraConstraints era $ do
let TextEnvelopeType d = textEnvelopeType (proxyToAsType (getProxy x))
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 not sure what this test is testing. Expected txTextEnvelopeTypes is generated using textEnvelopeType so this will always succeed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It ensures that the list has all the TextEnvelopeTypes, see this comment: #892 (comment).

Now, the implementation gets updated automatically and it is pretty similar to the test so it doesn't make that much sense anymore.

Initially it wasn't, the list was a literal, so I didn't know to update it and that caused a bug.

Also the list could contain more things (like it did before), the test checks it at least contains the TextEnvelopeTypes.

But we can remove the tests if they don't make sense anymore

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 for removing them. The expected values txTextEnvelopeTypes are generated using HasTextEnvelope. The envelopes for Tx are also coming from HasTextEnvelope. Does not make much sense to me.

cardano-cli/test/cardano-cli-test/Test/Cli/Parser.hs
hprop_test_cddl_tx_witness_envelope_completeness = property $ do
AnyShelleyBasedEra era <- H.noteShowM . forAll $ Gen.element [minBound .. maxBound]
x <- forAll $ genCardanoKeyWitness era
shelleyBasedEraConstraints era $ do
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as in the test above.

cardano-cli/src/Cardano/CLI/Read.hs

txTextEnvelopeTypes :: [Text]
txTextEnvelopeTypes =
[ let TextEnvelopeType d = shelleyBasedEraConstraints sbe $ textEnvelopeType (proxyToAsType (makeTxProxy sbe))
Copy link
Contributor

Choose a reason for hiding this comment

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

Where are those HasTextEnvelope and HasTypeProxy instances defined? How this mapping works? Previously we had both Witnessed and Unwitnessed for each era, so this looks like it would support only one for each era.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where are those HasTextEnvelope and HasTypeProxy instances defined?

HasTypeProxy is here: [cardano-api] Cadano.Api.Tx.Sign#L184
HasTextEnvelope is here: [cardano-api] Cadano.Api.Tx.Sign#L281

How this mapping works? Previously we had both Witnessed and Unwitnessed for each era, so this looks like it would support only one for each era.

We will still support both, but we will use the same type annotation for both since we cannot tell which is which from the type when serialising. We have already been producing them like that, is just we will stop supporting the old format as input. See discussion here: Slack discussion

Serialisation of TextEnvelopes is implemented here: [cardano-api] Cardano.Api.SerialiseTextEnvelope#L100

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the links. Makes sense now.

carbolymer
carbolymer reviewed Sep 23, 2024
View reviewed changes
cardano-cli/src/Cardano/CLI/Read.hs
where
teTypes =
[ FromCDDLTx "Witnessed Tx ShelleyEra" CddlTx
Copy link
Contributor

Choose a reason for hiding this comment

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

So now the types become:

["TxSignedShelley"
,"Tx AllegraEra"
,"Tx MaryEra"
,"Tx AlonzoEra"
,"Tx BabbageEra"
,"Tx ConwayEra"]

If I get this right, we won't be able to read transactions created in the previous version of cardano-cli.
I understand that it is expected?

cardano-cli/src/Cardano/CLI/Read.hs

txTextEnvelopeTypes :: [Text]
txTextEnvelopeTypes =
[ let TextEnvelopeType d = shelleyBasedEraConstraints sbe $ textEnvelopeType (proxyToAsType (makeTxProxy sbe))
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the links. Makes sense now.

cardano-cli/test/cardano-cli-test/Test/Cli/Parser.hs
AnyShelleyBasedEra era <- H.noteShowM . forAll $ Gen.element [minBound .. maxBound]
x <- forAll $ genTx era
shelleyBasedEraConstraints era $ do
let TextEnvelopeType d = textEnvelopeType (proxyToAsType (getProxy x))
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 for removing them. The expected values txTextEnvelopeTypes are generated using HasTextEnvelope. The envelopes for Tx are also coming from HasTextEnvelope. Does not make much sense to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carbolymer carbolymer carbolymer left review comments

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

@smelc smelc Awaiting requested review from smelc smelc 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

@ch1bo ch1bo Awaiting requested review from ch1bo

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

At least 1 approving review is required to merge this pull request.

Assignees

@palas palas

Labels
bug Something isn't working DO NOT MERGE
Projects
None yet
Milestone
No milestone
Development

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