Skip to content

Add NTN and Disk CDDLs #1422

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
Open
wants to merge 11 commits into
base: js/newer-ledger
Choose a base branch
from
Open

Add NTN and Disk CDDLs #1422

wants to merge 11 commits into from

Conversation

jasagredo
Copy link
Contributor

@jasagredo jasagredo commented Mar 13, 2025 •
edited
Loading

Description

Add CDDL definitions for types that Consensus emits.

@ch1bo
Copy link

ch1bo commented Mar 14, 2025

Very nice! See cardano-scaling/cardano-blueprint#25 for an issue that aims to use these to create an api reference.

@dnadales dnadales moved this to 👀 In review in Consensus Team Backlog Mar 17, 2025
@dnadales dnadales moved this from 👀 In review to 🏗 In progress in Consensus Team Backlog Mar 17, 2025
@ch1bo ch1bo mentioned this pull request Mar 20, 2025
Open
7 tasks
@ch1bo ch1bo moved this to In Progress in Cardano Blueprint Mar 20, 2025
@jasagredo jasagredo mentioned this pull request Mar 24, 2025
Open
ch1bo
ch1bo reviewed Mar 27, 2025
View reviewed changes
Copy link

@ch1bo ch1bo left a comment

Choose a reason for hiding this comment

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

Looks like a lot of work, but it's great that we finally have CDDLs for all these things. Also this module system looks quite good!

coot
coot reviewed Mar 31, 2025
View reviewed changes
ch1bo
ch1bo reviewed May 7, 2025
View reviewed changes
@jasagredo jasagredo force-pushed the js/cddls branch 3 times, most recently from aafadd5 to 690e912 Compare May 23, 2025 11:16
@jasagredo jasagredo force-pushed the js/cddls branch 2 times, most recently from 4ede539 to 0e37093 Compare May 23, 2025 12:00
@jasagredo jasagredo marked this pull request as ready for review May 23, 2025 12:00
@jasagredo
Copy link
Contributor Author

This PR is blocked on IntersectMBO/cardano-ledger#5056 so that we can integrate a newer ledger that contains:

@jasagredo jasagredo changed the title Add CDDLs Add NTN and Disk CDDLs May 23, 2025
@jasagredo jasagredo mentioned this pull request May 23, 2025
Closed
@jasagredo jasagredo moved this from 🏗 In progress to 🚫 Help needed in Consensus Team Backlog May 26, 2025
dnadales
dnadales reviewed May 26, 2025
View reviewed changes
Copy link
Member

@dnadales dnadales left a comment

Choose a reason for hiding this comment

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

Should we document somewhere where the CBOR specs for the ledger live, how we obtain them, and what is the testing strategy we now adopt to test CDDL conformance?

ouroboros-consensus-cardano/cddl/node-to-node/chainsync/header.cddl
@@ -0,0 +1,25 @@
header
Copy link
Member

Choose a reason for hiding this comment

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

This a comment that applies to each file: do we want to state where the corresponding Haskell data definition lives? And conversely, when defining a CBOR instance, would it make sense to define where the CDDL a CBOR instance conforms to is located?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do we want to state where the corresponding Haskell data definition lives?

I don't think so. Eventually these files will go to the blueprint so there is no need there for a Haskell link.

And conversely, when defining a CBOR instance, would it make sense to define where the CDDL a CBOR instance conforms to is located?

Perhaps?

@jasagredo jasagredo force-pushed the js/cddls branch 3 times, most recently from e381346 to a45003a Compare May 28, 2025 10:10
@jasagredo jasagredo moved this from 🚫 Help needed to 👀 In review in Consensus Team Backlog May 28, 2025
@jasagredo jasagredo changed the base branch from main to js/newer-ledger May 29, 2025 08:43
@jasagredo jasagredo mentioned this pull request May 29, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dnadales dnadales dnadales left review comments

@coot coot coot left review comments

@ch1bo ch1bo ch1bo requested changes

@nfrisby nfrisby Awaiting requested review from nfrisby nfrisby is a code owner

@amesgen amesgen Awaiting requested review from amesgen amesgen is a code owner

@fraser-iohk fraser-iohk Awaiting requested review from fraser-iohk fraser-iohk is a code owner

@geo2a geo2a Awaiting requested review from geo2a geo2a is a code owner

@jorisdral jorisdral Awaiting requested review from jorisdral

Assignees

@jasagredo jasagredo

Labels
no changelog
Projects
Consensus Team Backlog
Status: 👀 In review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@jasagredo @ch1bo @dnadales @coot @geo2a