Rename CDDL fields to better reflect their origin and be more reusable #5339

aniketd · 2025-10-14T15:17:37Z

Description

This addresses the initial part of #5194 .

Switch from exporting everything and hiding in the new era to exporting just the reusable parts and import the whole module in the new era.
Move some more field definitions into core and reuse them.
Name every field that is new in an era prefixed by that era-name.

Follow-up PRs would be to actually address the heart of #5194 to reduce duplication to a minimum.

Checklist

neilmayhew

Apart from the potential irregularity I commented on, this refactoring seems like it should make no difference to the CDDL.

Perhaps the test failures are due to the issue I pointed out?

neilmayhew · 2025-10-16T04:33:10Z

eras/allegra/impl/cddl-files/allegra.cddl

-protocol_version = (major_protocol_version, uint)
+protocol_version = [major_protocol_version, uint]


This change from group to array seems significant. However, I don't see any change in the .hs that matches this. I wonder where it's coming from?

I saw the same difference in some of the other eras.

teodanciu

I had a first pass and left some comments.
In addition, I'm also wondering if it's beneficial to remove the distinction between the types in cddl that represent hashes vs just bytes (so the replacement of hash32 with bytes32).
Even though they boil down to the same thing, it might bring some value to distinguish them? What do you think?

teodanciu · 2025-10-20T13:07:30Z

eras/shelley/impl/test/Test/Cardano/Ledger/Shelley/Binary/CddlSpec.hs

  MultiSig,
  ProposedPPUpdates,
-  Update,
+  -- Update,


Presumably left by accident

Suggested change

-- Update,

teodanciu · 2025-10-20T14:05:12Z

libs/cardano-ledger-core/testlib/Test/Cardano/Ledger/Core/Binary/CDDL.hs

+pint :: Rule
+pint = "pint" =:= (1 :: Integer) ... max_word64


Not sure this is an improvement, to me it seems less consistent (since we still have nonnegative_interval and positive_coin. I would prefer the longer "positive_" prefix for all of the above, but would be good to know what others think about it

teodanciu · 2025-10-20T14:06:03Z

libs/cardano-ledger-core/testlib/Test/Cardano/Ledger/Core/Binary/CDDL.hs

-posWord32 :: Rule
-posWord32 = "posWord32" =:= (1 :: Integer) ... maxWord32
+pword32 :: Rule
+pword32 = "pword32" =:= (1 :: Integer) ... max_word32


Is the p or positive prefix necessary for word? I would have thought it's implied that it's positive, since Word is unsigned, no?

Word implies that it is non-negative 😉 which is different from positive.

On a different note, I believe posWord/pos_word or even positiveWord/positive_word would be better prefixes than just p

teodanciu · 2025-10-20T14:11:27Z

libs/cardano-ledger-core/testlib/Test/Cardano/Ledger/Core/Binary/CDDL.hs

+transaction_ix :: Rule
+transaction_ix = "transaction_ix" =:= VUInt `sized` (2 :: Word64)


Not sure if this is better than what we have now, transaction_index. Is it in order to be more consistent with the code? Or any other reason in particular for this abbreviation?

teodanciu · 2025-10-20T14:13:24Z

libs/cardano-ledger-core/testlib/Test/Cardano/Ledger/Core/Binary/CDDL.hs


-negInt64 :: Rule
-negInt64 = "negInt64" =:= minInt64 ... (-1 :: Integer)
+nint64 :: Rule


Same comment as before about pint - i think negative_int64 is better, less vague, easier to find, consistent with others like nonzero-int64

teodanciu · 2025-10-20T15:41:12Z

eras/shelley/impl/testlib/Test/Cardano/Ledger/Shelley/CDDL.hs

-        |]
-    $ "script_hash" =:= hash28
+shelley_epoch :: Rule
+shelley_epoch = "epoch" =:= VUInt


This is a bit strange - isn't it the same for all eras? Seems more like something for core to me - any reason to have it shelley-specific?

teodanciu · 2025-10-20T15:44:22Z

eras/shelley/impl/testlib/Test/Cardano/Ledger/Shelley/CDDL.hs

+script_pubkey :: Named Group
+script_pubkey = "script_pubkey" =:~ grp [0, a addr_keyhash]

-epoch :: Rule
-epoch = "epoch" =:= VUInt
+script_all :: Named Group
+script_all = "script_all" =:~ grp [1, a (arr [0 <+ a shelley_native_script])]

-genesis_delegate_hash :: Rule
-genesis_delegate_hash = "genesis_delegate_hash" =:= hash28
+script_any :: Named Group
+script_any = "script_any" =:~ grp [2, a (arr [0 <+ a shelley_native_script])]

-genesis_hash :: Rule
-genesis_hash = "genesis_hash" =:= hash28
+script_n_of_k :: Named Group
+script_n_of_k = "script_n_of_k" =:~ grp [3, "n" ==> VUInt, a (arr [0 <+ a shelley_native_script])]


I would prefer here to be so consistent and explicit at the cost of verbosity and use native_script prefix instead of script , both in the name of the rules and in the keys - but curious to know what others think about it.

teodanciu · 2025-10-20T15:47:30Z

eras/shelley/impl/testlib/Test/Cardano/Ledger/Shelley/CDDL.hs

+shelley_nonce :: Rule
+shelley_nonce = "nonce" =:= arr [0] / arr [1, a (VBytes `sized` (32 :: Word64))]


This also seems era-generic, I think it would be better in core and called "nonce"

teodanciu · 2025-10-20T16:50:32Z

eras/allegra/impl/testlib/Test/Cardano/Ledger/Allegra/CDDL.hs

+allegra_script_all :: Named Group
+allegra_script_all = "script_all" =:~ grp [1, a (arr [0 <+ a allegra_native_script])]

-script_any :: Named Group
-script_any = "script_any" =:~ grp [2, a (arr [0 <+ a native_script])]
+allegra_script_any :: Named Group
+allegra_script_any = "script_any" =:~ grp [2, a (arr [0 <+ a allegra_native_script])]

 script_n_of_k :: Named Group
-script_n_of_k = "script_n_of_k" =:~ grp [3, "n" ==> int64, a (arr [0 <+ a native_script])]
+script_n_of_k = "script_n_of_k" =:~ grp [3, "n" ==> int64, a (arr [0 <+ a allegra_native_script])]


I wonder if we could parameterize these rules by the native_script rule, and then use them in each era with the respenctive native_script definition

I think this is a great idea!

teodanciu · 2025-10-20T16:51:16Z

eras/allegra/impl/testlib/Test/Cardano/Ledger/Allegra/CDDL.hs

+allegra_script_pubkey :: Named Group
+allegra_script_pubkey = "script_pubkey" =:~ grp [0, a addr_keyhash]


This is the same as in shelley, don't think there is a point to redefine it

lehins

I only took a quick look at this PR. I'll do a more thorough review tomorrow.

lehins · 2025-10-21T02:13:31Z

eras/allegra/impl/cddl-files/allegra.cddl



-hash32 = bytes .size 32
+bytes32 = bytes .size 32


Why was hash switched to bytes? It is really good to indicate that it is not some bytes but a hash

lehins · 2025-10-21T02:19:21Z

eras/allegra/impl/cddl-files/allegra.cddl

 auxiliary_data = 
-  metadata                                                                              
-  / [transaction_metadata : metadata, auxiliary_scripts : auxiliary_scripts]
+  shelley_auxiliary_data                                                                     


This is incorrect. It just so happens that in Shelley auxiliary_data is set to just metadata. But metadata is it own concept: Map Word64 Metadatum

Suggested change

shelley_auxiliary_data

metadata

lehins · 2025-10-21T02:20:50Z

eras/allegra/impl/testlib/Test/Cardano/Ledger/Allegra/CDDL.hs

+allegra_script_all :: Named Group
+allegra_script_all = "script_all" =:~ grp [1, a (arr [0 <+ a allegra_native_script])]

-script_any :: Named Group
-script_any = "script_any" =:~ grp [2, a (arr [0 <+ a native_script])]
+allegra_script_any :: Named Group
+allegra_script_any = "script_any" =:~ grp [2, a (arr [0 <+ a allegra_native_script])]

 script_n_of_k :: Named Group
-script_n_of_k = "script_n_of_k" =:~ grp [3, "n" ==> int64, a (arr [0 <+ a native_script])]
+script_n_of_k = "script_n_of_k" =:~ grp [3, "n" ==> int64, a (arr [0 <+ a allegra_native_script])]


I think this is a great idea!

lehins · 2025-10-21T02:21:31Z

eras/allegra/impl/testlib/Test/Cardano/Ledger/Allegra/CDDL.hs

    / sarr
-      [ "transaction_metadata" ==> metadata
-      , "auxiliary_scripts" ==> auxiliary_scripts
+      [ "transaction_metadata" ==> shelley_auxiliary_data


So, yeah, this should be metadata

Suggested change

[ "transaction_metadata" ==> shelley_auxiliary_data

[ "transaction_metadata" ==> metadata

lehins · 2025-10-21T02:25:32Z

eras/shelley/impl/testlib/Test/Cardano/Ledger/Shelley/CDDL.hs

+shelley_auxiliary_data :: Rule
+shelley_auxiliary_data =
+  "shelley_auxiliary_data"
+    =:= mp [0 <+ asKey shelley_transaction_metadatum_label ==> transaction_metadatum]


This is how I would write it:

Suggested change

shelley_auxiliary_data :: Rule

shelley_auxiliary_data =

"shelley_auxiliary_data"

=:= mp [0 <+ asKey shelley_transaction_metadatum_label ==> transaction_metadatum]

metadatum :: Rule

metadatum =

"metadatum"

=:= smp [0 <+ asKey metadatum ==> metadatum]

/ sarr [0 <+ a metadatum]

/ VInt

/ (VBytes `sized` (0 :: Word64, 64 :: Word64))

/ (VText `sized` (0 :: Word64, 64 :: Word64))

metadatum_label :: Rule

metadatum_label = "metadatum_label" =:= VUInt

metadata :: Rule

metadata = "metadata" =:= mp [0 <+ asKey metadatum_label ==> metadatum]

shelley_auxiliary_data :: Rule

shelley_auxiliary_data = "auxiliary_data" =:= metadata

I'd also put metadata in core and then reuse it in other eras, instead of shelley_auxiliary_data.

lehins · 2025-10-21T02:53:32Z

eras/allegra/impl/cddl-files/allegra.cddl

+nonnegative_interval = #6.30([uint, pint])

-positive_int = 1 .. maxWord64
+pint = 1 .. max_word64


positive_ is a much better prefix than p. Otherwise someone might think it is a definition for a pint of beer or something 🤣

Suggested change

pint = 1 .. max_word64

positive_int = 1 .. max_word64

lehins · 2025-10-21T02:54:49Z

eras/allegra/impl/cddl-files/allegra.cddl

 ;   "\x02" for Plutus V2 scripts
 ;   "\x03" for Plutus V3 scripts
-script_hash = hash28
+;   "\x04" for Plutus V4 scripts


There is something definitely wrong with this comment. Tags for plutus scripts should not be relevant for Allegra CDDL

aniketd changed the title ~~Rename CDDL fields to better reflect their origin and more reuse~~ Rename CDDL fields to better reflect their origin and be more reusable Oct 14, 2025

Rename CDDL fields to make reuse explicit.

e2e3e15

aniketd force-pushed the aniketd/deduplicate-cddl branch from 9594784 to e2e3e15 Compare October 15, 2025 14:58

aniketd marked this pull request as ready for review October 15, 2025 15:25

aniketd requested a review from a team as a code owner October 15, 2025 15:25

neilmayhew reviewed Oct 16, 2025

View reviewed changes

teodanciu reviewed Oct 20, 2025

View reviewed changes

lehins reviewed Oct 21, 2025

View reviewed changes

		protocol_version = (major_protocol_version, uint)
		protocol_version = [major_protocol_version, uint]

		transaction_ix :: Rule
		transaction_ix = "transaction_ix" =:= VUInt `sized` (2 :: Word64)

		shelley_nonce :: Rule
		shelley_nonce = "nonce" =:= arr [0] / arr [1, a (VBytes `sized` (32 :: Word64))]

		allegra_script_pubkey :: Named Group
		allegra_script_pubkey = "script_pubkey" =:~ grp [0, a addr_keyhash]

	[ "transaction_metadata" ==> shelley_auxiliary_data
	[ "transaction_metadata" ==> metadata

-shelley_auxiliary_data :: Rule
-shelley_auxiliary_data =
-  "shelley_auxiliary_data"
-    =:= mp [0 <+ asKey shelley_transaction_metadatum_label ==> transaction_metadatum]
+metadatum :: Rule
+metadatum =
+  "metadatum"
+    =:= smp [0 <+ asKey metadatum ==> metadatum]
+    / sarr [0 <+ a metadatum]
+    / VInt
+    / (VBytes `sized` (0 :: Word64, 64 :: Word64))
+    / (VText `sized` (0 :: Word64, 64 :: Word64))
+metadatum_label :: Rule
+metadatum_label = "metadatum_label" =:= VUInt
+metadata :: Rule
+metadata = "metadata" =:= mp [0 <+ asKey metadatum_label ==> metadatum]
+shelley_auxiliary_data :: Rule
+shelley_auxiliary_data = "auxiliary_data" =:= metadata

Rename CDDL fields to better reflect their origin and be more reusable #5339

Are you sure you want to change the base?

Rename CDDL fields to better reflect their origin and be more reusable #5339

Uh oh!

Conversation

aniketd commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Uh oh!

neilmayhew left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

teodanciu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lehins left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aniketd commented Oct 14, 2025 •

edited

Loading