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

Continue preparing the for the introduction of AlonzoEra #3131

Merged
merged 6 commits into from
May 14, 2021

Conversation

nfrisby
Copy link
Contributor

@nfrisby nfrisby commented May 12, 2021

This PR introduces the changes that then allow us to add AlonzoEra to the CardanoEras list.

@nfrisby nfrisby added the consensus issues related to ouroboros-consensus label May 12, 2021
@nfrisby nfrisby requested a review from jasagredo May 12, 2021 01:59
@nfrisby nfrisby force-pushed the nfrisby/alonzo-preparation-2 branch from 00c3321 to 32b8484 Compare May 12, 2021 02:03
@nfrisby
Copy link
Contributor Author

nfrisby commented May 12, 2021

This is a Draft PR because it should be rebased and then merged after we merge PR #3130.

Copy link
Contributor

@jasagredo jasagredo left a comment

Choose a reason for hiding this comment

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

Looks reasonable to me.

Copy link
Contributor

@jasagredo jasagredo left a comment

Choose a reason for hiding this comment

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

The changes since my last review are related to the other PR and to my previous comment for renaming CanMockOld, so still looks good.

@nfrisby nfrisby mentioned this pull request May 13, 2021
@nfrisby nfrisby force-pushed the nfrisby/alonzo-preparation-2 branch 3 times, most recently from 4fccf5a to 5201d9c Compare May 13, 2021 19:10
@nfrisby nfrisby marked this pull request as ready for review May 13, 2021 19:16
@nfrisby nfrisby removed request for coot and karknu May 13, 2021 19:16
@nfrisby nfrisby force-pushed the nfrisby/alonzo-preparation-2 branch from 5201d9c to 457e7f0 Compare May 13, 2021 19:24
cabal.project Show resolved Hide resolved
This is a temporary workaround; the AlonzoEra does not satisfy
CanMockPreAlonzo, so we cannot use CanMockPreAlonzo for instances that are
elsewhere required for each element of CardanoEras. Thankfully, only these uses
of CanMockPreAlonzo need the Alonzo-incompatible constraints, and these three
are also not requires of CardanoEras.
The motivation is to bring in the NoThunks AlonzoGenesis instance and the
TranslationContext (ShelleyEra c) ~ () instance.
@nfrisby nfrisby force-pushed the nfrisby/alonzo-preparation-2 branch from 457e7f0 to 2b8ed44 Compare May 14, 2021 02:31
Copy link
Contributor

@EncodePanda EncodePanda left a comment

Choose a reason for hiding this comment

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

LGTM. Few minor comments but those we can always revisit.

Alonzo is the first era with a `TranslationContext` that is not `()`.
Therefore, this commit must pipe through the `TranslationContext` through to
the calls to `translateEra`. In particular, some of this are in classes used by
the HFC in which the only era-specific input is the `LedgerConfig`. For that
reason, we're adding `TranslationContext` to `ShelleyLedgerConfig`. It's
possible we should instead alter the HFC to carry something in addition
to/instead of the `LedgerConfig`, but we're taking the direct path for now.

By adding a field to the `ShelleyLedgerConfig` we take on the burden of
providing that `TranslationContext` in every interface and/or test function
that creates a Shelley protocol info etc. This is noisy but simple, since all
eras before `AlonzoEra` just use `()` for their context.
@nfrisby nfrisby force-pushed the nfrisby/alonzo-preparation-2 branch from 98396bf to 67751c9 Compare May 14, 2021 13:49
@nfrisby nfrisby force-pushed the nfrisby/alonzo-preparation-2 branch from 67751c9 to ed24dfa Compare May 14, 2021 13:51
@nfrisby
Copy link
Contributor Author

nfrisby commented May 14, 2021

bors r+

iohk-bors bot added a commit that referenced this pull request May 14, 2021
3131: Continue preparing the for the introduction of AlonzoEra r=nfrisby a=nfrisby

This PR introduces the changes that then allow us to add `AlonzoEra` to the `CardanoEras` list.

Co-authored-by: Nicolas Frisby <nick.frisby@iohk.io>
@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 14, 2021

Build failed:

@nfrisby
Copy link
Contributor Author

nfrisby commented May 14, 2021

The Hydra build timed out on the mac-mini again b/c of Issue IntersectMBO/ouroboros-consensus#606. I restarted it 🤞

Edit: and it succeeded -- but bors didn't seem to notice. So I've reissued the command here.

@nfrisby
Copy link
Contributor Author

nfrisby commented May 14, 2021

bors r+

@iohk-bors
Copy link
Contributor

iohk-bors bot commented May 14, 2021

@iohk-bors iohk-bors bot merged commit c489b00 into master May 14, 2021
@iohk-bors iohk-bors bot deleted the nfrisby/alonzo-preparation-2 branch May 14, 2021 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
consensus issues related to ouroboros-consensus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants