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

Finalise Cardano Legacy Block #473

Merged
merged 3 commits into from
Nov 28, 2023
Merged

Finalise Cardano Legacy Block #473

merged 3 commits into from
Nov 28, 2023

Conversation

jorisdral
Copy link
Contributor

Description

Resolves #344

@jorisdral jorisdral self-assigned this Oct 27, 2023
@jorisdral jorisdral changed the base branch from main to jdral/missing-legacy-block-instances October 27, 2023 12:51
@jorisdral jorisdral force-pushed the jdral/finalise-cardano-legacy-block branch 3 times, most recently from 31e2f1a to 3835491 Compare October 29, 2023 12:02
@jorisdral jorisdral force-pushed the jdral/finalise-cardano-legacy-block branch 2 times, most recently from 7c81830 to 199d9e3 Compare November 2, 2023 11:09
@jorisdral jorisdral marked this pull request as ready for review November 2, 2023 11:10
@jorisdral jorisdral requested a review from a team as a code owner November 2, 2023 11:10
@jorisdral jorisdral requested a review from jasagredo November 2, 2023 11:10
Base automatically changed from jdral/missing-legacy-block-instances to utxo-hd-8.2.1 November 2, 2023 15:50
jasagredo
jasagredo approved these changes Nov 22, 2023
View reviewed changes
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.

I left some comments, but nothing too serious. It looks good

ouroboros-consensus-cardano-legacy-block/src/Legacy/Byron/Node.hs Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see this file as necessarily Byron only, am I wrong?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, I suppose maybe it isn't. It's specific to Byron's protocol, not the byron block. However, this module follows the same structure as the ouroboros-consensus-cardano:byron package

ouroboros-consensus-cardano-legacy-block/src/Legacy/Cardano/Node.hs
Comment on lines +214 to +215
class (ToLegacyBlock x y, CanStowLedgerTables (LedgerState x)) => C x y
instance CanStowLedgerTables (LedgerState x) => C x (LegacyBlock 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 don't see this being used.

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's used here: https://github.com/input-output-hk/ouroboros-consensus/pull/473/files/199d9e308113dc32970549ffe533b967f6e82fe2#diff-165cfbdc83522aa96e78a011a6891f030987b36a63cea4300f1c10ae1e3ed130R253

ouroboros-consensus-cardano-legacy-block/src/Legacy/Shelley.hs Outdated
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 dubious about how useful are these gathering modules. I think it should be fine because it is only instances. If they exported stuff I would not like it.

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's similar to having a Ouroboros.Consensus.Cardano module in ouroboros-consensus-cardano. Would you not like it because of re-exports, or?

ouroboros-consensus-cardano-legacy-block/src/Legacy/Shelley/Node.hs Outdated Show resolved Hide resolved
@jorisdral jorisdral force-pushed the jdral/finalise-cardano-legacy-block branch from 199d9e3 to e6d6d3b Compare November 28, 2023 10:45
@jorisdral jorisdral merged commit 28586f9 into utxo-hd-8.2.1 Nov 28, 2023
6 of 8 checks passed
@jorisdral jorisdral deleted the jdral/finalise-cardano-legacy-block branch November 28, 2023 10:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasagredo jasagredo jasagredo approved these changes

Assignees

@jorisdral jorisdral

Labels
UTxO-HD
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Run a node with legacy blocks
2 participants
@jorisdral @jasagredo