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

Split shelley-ma into allegra and mary #3175

Merged
merged 5 commits into from
Dec 2, 2022
Merged

Conversation

lehins
Copy link
Collaborator

@lehins lehins commented Nov 30, 2022

This PR splits the package cardano-ledger-shelley-ma into two packages that implement proper eras:

  • cardano-ledger-allegra
  • cardano-ledger-mary.

It is important to point out that in order to keep this PR as small as possible the test package cardano-ledger-shelley-ma-test is still a thing. That test package will be restructured into separate libraries in the corresponding eras/allegra/ and eras/mary/ directories in a later PR. shelley-ma era also contains the spec, which I am not quite sure yet where it should eventually live, since it does combine the two eras together as one document.

There is no longer a ShelleyMAEra ma c, there are proper types that replace it:

  • AllegraEra c
  • and MaryEra c.

Conceptually the only thing that is different between the two eras is the support of multi assets by the latter era, therefore the only type that had seen a somewhat impactful change was the TxBody.

There is no longer MATxBody, but two:

  • AllegraTxBody
  • and MaryTxBody.

Care was take to reduce duplication so, both of those types are backed by the same raw type underneath. This also means that corresponding type class was split as well:

There is no longer ShelleyMAEraTxBody, but instead these two:

  • AllegraEraTxBody
  • and MaryEraTxBody.

@lehins lehins force-pushed the lehins/split-shelley-ma branch 2 times, most recently from 798edc0 to a2b70be Compare December 1, 2022 10:27
* `cardano-ledger-allegra`
* `cardano-ledger-mary`

This commit only moves files around without changes. This is done
intentionally to preserve git history. Subsequent commits will fix
the actual fnctionality so the project actually builds
@lehins lehins force-pushed the lehins/split-shelley-ma branch from 1715e8f to 01f523a Compare December 1, 2022 15:57
Copy link
Contributor

@teodanciu teodanciu left a comment

Choose a reason for hiding this comment

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

This is awesome! Looks good to me, just some minor questions and remarks.

@lehins lehins force-pushed the lehins/split-shelley-ma branch from a6ffe21 to 0ee1bf5 Compare December 1, 2022 20:12
Copy link
Contributor

@JaredCorduan JaredCorduan left a comment

Choose a reason for hiding this comment

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

Looks good, thank you @lehins !

I can't recall why shelleyToAllegraAVVMsToDelete is still around if it is not used...

Is the plan to keep eras/shelley-ma/test-suite for testing? or punt for now and move it later? (I'd love to get my tab-complete on "shel" back 😆 )

@lehins
Copy link
Collaborator Author

lehins commented Dec 1, 2022

Is the plan to keep eras/shelley-ma/test-suite for testing?

Yes, I will split cardano-ledger-shelley-ma-test in a subsequent PR. I've updated the #3171 ticket to reflect it

(I'd love to get my tab-complete on "shel" back laughing )

Unfortunately we'll have to keep cardano-ledger-shelley-ma for a little bit until downstream reflect on these changes. But afterwards we'll definitely be able to get rid of era/shelley-ma folder. One thing I am not certain about is where to put the era/shelley-ma/formal-spec folder, since it crams both Allegra and Mary in there

@JaredCorduan
Copy link
Contributor

JaredCorduan commented Dec 1, 2022

Unfortunately we'll have to keep cardano-ledger-shelley-ma for a little bit until downstream reflect on these changes. But afterwards we'll definitely be able to get rid of era/shelley-ma folder. One thing I am not certain about is where to put the era/shelley-ma/formal-spec folder, since it crams both Allegra and Mary in there

IMHO, it is the Mary spec. There is no Allegra spec, you just have to know what parts of the Mary spec to leave out... Maybe we can rename it to the Mary spec and have an appendix about the "Allegra spec".

cc @polinavino @WhatisRT

This PR splits the package `cardano-ledger-shelley-ma` into two
packages that implement proper eras:

* `cardano-ledger-allegra`
* and `cardano-ledger-mary`.

There is no longer a `ShelleyMAEra ma c`, but proper types that replace it:
* `AllegraEra c`
*  and `MaryEra c`.

Conceptually the only thing that is different between the two eras is the
support of multi assets by the latter era, therefore the only type that
had seen a somewhat impactful change was the `TxBody`.

There is no longer `MATxBody`, but two:
* `AllegraTxBody`
*  and `MaryTxBody`.

Care was take to reduce duplication so, both of those types are backed
by the same raw type underneath. This also means that corresponding
type class was split as well:

There is no longer `ShelleyMAEraTxBody`, but instead these two:
* `AllegraEraTxBody`
* and `MaryEraTxBody`.
@lehins lehins force-pushed the lehins/split-shelley-ma branch from a638e24 to aac7427 Compare December 1, 2022 22:46
@lehins lehins merged commit 064acb8 into master Dec 2, 2022
@neilmayhew neilmayhew deleted the lehins/split-shelley-ma branch March 8, 2024 21:07
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.

3 participants