-
Notifications
You must be signed in to change notification settings - Fork 23
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
Merge ShelleyLedgerEra
and CardanoLedgerEra
to a single type family LedgerEra
#361
Conversation
ShelleyLedgerEra MaryEra = Consensus.StandardMary | ||
ShelleyLedgerEra AlonzoEra = Consensus.StandardAlonzo | ||
ShelleyLedgerEra BabbageEra = Consensus.StandardBabbage | ||
ShelleyLedgerEra ConwayEra = Consensus.StandardConway |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete this. Use LedgerEra
instead.
LedgerEra MaryEra = L.MaryEra L.StandardCrypto | ||
LedgerEra AlonzoEra = L.AlonzoEra L.StandardCrypto | ||
LedgerEra BabbageEra = L.BabbageEra L.StandardCrypto | ||
LedgerEra ConwayEra = L.ConwayEra L.StandardCrypto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rename CardanoLedgerEra
which is never used to LedgerEra
which we use everywhere in place of ShelleyLedgerEra
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the change is pretty harmless, but can you check if anything breaks in cardano-cli
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep. Already checked.
22599ef
to
2c209ce
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not the direction we are taking cardano-api in. We do not care about all the eras, only mainnet and the upcoming era. The first step in this is to factor out the Byron era and only be concerned with shelley based eras. Consolidating the shelley based eras with cardano eras is moving in the wrong direction and prolongs this eventual separation.
It's worth noting that this PR doesn't commit us to a particular direction. On it's own it serves as a straightforward simplification. with the only impact on the rest of the code being a rename to a shorter one and removing a type we don't use anywhere. |
2c209ce
to
7f48bfa
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is an unnecessary change. It is also incorrect because it implies the existence of types in Byron that do no exist. We will be able to simply things further after the Byron related code is isolated.
What type is Byron type is being implied that doesn't exist? |
Changelog
Context
We previously defined two separate type families
ShelleyLedgerEra
andCardanoLedgerEra
but never useCardanoLedgerEra
anywhere.ShelleyLedgerEra
is basically the same asCardanoLedgerEra
except Byron is missing.There is no reason to have two type families because
CardanoLedgerEra
works everywhereShelleyLedgerEra
currently does.This PR merges the two type families into the one
LedgerEra
type family (which includes Byron) which shortens the commonly used name.Doing this opens the possibility of unifying Byron with the other eras for the minimal set of operations we care to maintain for hard forking purposes.
How to trust this PR
Highlight important bits of the PR that will make the review faster. If there are commands the reviewer can run to observe the new behaviour, describe them.
Checklist