Skip to content

Factor out application of transition configuration to the Ledger repository #934

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
Closed
wants to merge 7 commits into from
Closed

Factor out application of transition configuration to the Ledger repository #934

wants to merge 7 commits into from

Conversation

@dnadales
Copy link
Member

@dnadales dnadales commented Jan 31, 2024

@dnadales dnadales self-assigned this Feb 1, 2024
@dnadales
Bump index states.
5b06cea
@dnadales
Bump ledger versions
d511746
@dnadales
Change the types of GetConstitution and GetCommitteeMembersState
3545bdb 
... to match those of the corresponding ledger functions.
@dnadales
Use import orphan EpochNo Num instance from `Test.Cardano.Slottin…
74664d2 
…g.Numeric`

This instance was removed from `cardano-base`.
@dnadales
Use injectIntoTestState in protocolInfoTPraosShelleyBased function
736a144 
... which is used in the `ThreadNet` tests.

The correctness of this change relies on the ledger not registering
the initial funds in the genesis configuration file. This was recently
changed in the Ledger component.
@dnadales
Remove Ledger specific logic when injecting data into the ledger state
6d9c5d7 
We rely on the `injectIntoTestState` function, which is now defined at
the Ledger side.
@dnadales
Generalize the registerAny function.
c11286c
@dnadales dnadales force-pushed the dnadales/899-factor-out-application-of-transition-config-to-ledger-repo branch from 784f977 to c11286c Compare March 1, 2024 10:20
@dnadales dnadales changed the base branch from main to dnadales/consensus-8.10 March 1, 2024 10:21
@dnadales dnadales marked this pull request as ready for review March 1, 2024 10:21
@dnadales dnadales requested a review from a team as a code owner March 1, 2024 10:21
jasagredo
jasagredo reviewed Mar 1, 2024
View reviewed changes
ouroboros-consensus-cardano/src/ouroboros-consensus-cardano/Ouroboros/Consensus/Cardano/Node.hs
Comment on lines +1122 to +1123
type family ShelleyBlockLedgerEra blk where
ShelleyBlockLedgerEra (ShelleyBlock proto era) = era
Copy link
Contributor

@jasagredo jasagredo Mar 1, 2024

Choose a reason for hiding this comment

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

I think this should live in the shelley subdir somewhere, perhaps in Shelley/Block.hs

Copy link
Member

@amesgen amesgen Mar 1, 2024

Choose a reason for hiding this comment

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

Could even be used to share code with

ouroboros-consensus/ouroboros-consensus-cardano/src/unstable-cardano-tools/Cardano/Tools/DBAnalyser/Block/Cardano.hs

Lines 203 to 207 in af4b27e

type family ShelleyBlockEra blk where
ShelleyBlockEra (ShelleyBlock proto era) = era
class L.Era (ShelleyBlockEra blk) => IsShelleyBlock blk
instance L.Era era => IsShelleyBlock (ShelleyBlock proto era)

Copy link
Member Author

@dnadales dnadales Mar 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'd argue for leaving it here for the same reasons.

Copy link
Member Author

@dnadales dnadales Mar 1, 2024

Choose a reason for hiding this comment

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

For some reason, I missed your last comment, @amesgen. Could I use ShelleyBlockEra instead?

Copy link
Member Author

@dnadales dnadales Mar 1, 2024

Choose a reason for hiding this comment

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

Also consider replacing IsShelleyBlock with IsShelleyBlock in

ouroboros-consensus/ouroboros-consensus-cardano/src/unstable-cardano-tools/Cardano/Tools/DBAnalyser/Block/Cardano.hs

Lines 203 to 207 in af4b27e

type family ShelleyBlockEra blk where
ShelleyBlockEra (ShelleyBlock proto era) = era
class L.Era (ShelleyBlockEra blk) => IsShelleyBlock blk
instance L.Era era => IsShelleyBlock (ShelleyBlock proto era)

Copy link
Member

@amesgen amesgen Mar 1, 2024

Choose a reason for hiding this comment

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

Note that Cardano.Tools.DBAnalyser.Block.Cardano is a consumer of the module Ouroboros.Consensus.Cardano.Node this comment is attached to, so one can't use things from Cardano.Tools.DBAnalyser.Block.Cardano in Ouroboros.Consensus.Cardano.Node. So @jasagredo's suggestion of moving these utilities further up the module dependency tree (eg to Ouroboros.Consensus.Shelley.Ledger.Block) and using them in both places makes sense to me.

Copy link
Member Author

@dnadales dnadales Mar 1, 2024

Choose a reason for hiding this comment

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

I didn't realize the snippet you sent was in DBAnalyser. I could introduce a new module and reuse definitions from there 👍

@dnadales dnadales force-pushed the dnadales/consensus-8.10 branch from 74664d2 to 2552206 Compare March 1, 2024 11:51
nfrisby
nfrisby reviewed Mar 1, 2024
View reviewed changes
Copy link
Contributor

@nfrisby nfrisby left a comment

Choose a reason for hiding this comment

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

Diff looks great to me---happy to have Consensus out of the loop on this data flow 👏. (I'm not Approving only because there're ongoing conversations about where some of the type-level utilities should live.)

@dnadales dnadales mentioned this pull request Mar 1, 2024
Closed
24 tasks
@dnadales dnadales force-pushed the dnadales/consensus-8.10 branch from 23da4ad to 42356a9 Compare March 12, 2024 15:39
@dnadales dnadales force-pushed the dnadales/consensus-8.10 branch 3 times, most recently from 4d77e00 to a6bf691 Compare March 27, 2024 17:15
@amesgen amesgen mentioned this pull request Apr 3, 2024
Merged
@dnadales dnadales force-pushed the dnadales/consensus-8.10 branch from a513385 to 2b6d5f5 Compare April 3, 2024 11:42
Base automatically changed from dnadales/consensus-8.10 to main April 3, 2024 23:16
@dnadales
Copy link
Member Author

dnadales commented Apr 9, 2024

Superseded by #1032

@dnadales dnadales closed this Apr 9, 2024
@amesgen amesgen deleted the dnadales/899-factor-out-application-of-transition-config-to-ledger-repo branch April 9, 2024 09:05
@dnadales dnadales mentioned this pull request Apr 23, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Apr 24, 2024
@dnadales
Factor out class ShelleyBasedBlock and type family `ShelleyBlockLed…
f25ea0b 
…gerEra` (#1073)

Pending change from
#934 (comment)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nfrisby nfrisby nfrisby left review comments

@jasagredo jasagredo jasagredo left review comments

@amesgen amesgen amesgen left review comments

Assignees

@dnadales dnadales

Labels

None yet

Projects

Consensus Team Backlog
Status: ✅ Done

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Factor out application of transition config to the Ledger repo

5 participants

@dnadales @nfrisby @jasagredo @amesgen