Add support for NonEmptyOrderedSet in Plutus_data #451

KINGH242 · 2025-08-02T01:28:53Z

This pull request improves handling of indefinite lists, update encoding of Plutus data, and expanded test coverage. Key changes include updates to the OrderedSet class to support indefinite lists, modifications to the transaction builder and utilities for more robust Plutus data handling, and update tests to validate these updates.

Enhancements to `OrderedSet` and Serialization:

Updated the OrderedSet class to inherit from IndefiniteList and added support for indefinite list detection and representation. This ensures compatibility with CBOR encoding for both definite and indefinite lists (pycardano/serialization.py). [1] [2] [3]
Extended NonEmptyOrderedSet to accept IndefiniteList as input, improving its flexibility (pycardano/serialization.py).

Improvements in Plutus Data Handling:

Modified TransactionWitnessSet to support NonEmptyOrderedSet for plutus_data (pycardano/witness.py). [1] [2]
Updated the build_witness_set method to use NonEmptyOrderedSet for Plutus data (pycardano/txbuilder.py). [1] [2] [3]

Enhancements to Utilities:

Refactored the script_data_hash function to handle NonEmptyOrderedSet for datums and ensure canonical CBOR encoding for cost models (pycardano/utils.py).

Test Coverage Expansion:

Added new test cases to validate script_data_hash behavior with RedeemerMap and updated expected hash values to reflect changes in encoding logic (test/pycardano/test_util.py).
Adjusted existing tests to align with the updated handling of Plutus data and indefinite lists (test/pycardano/test_txbuilder.py).

…atums

…andling

…erialization

codecov · 2025-08-02T01:29:41Z

Codecov Report

❌ Patch coverage is 93.18182% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 89.85%. Comparing base (2122c39) to head (65eedc8).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
pycardano/utils.py	50.00%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #451      +/-   ##
==========================================
- Coverage   89.98%   89.85%   -0.13%     
==========================================
  Files          33       33              
  Lines        4854     4881      +27     
  Branches      733      739       +6     
==========================================
+ Hits         4368     4386      +18     
- Misses        314      319       +5     
- Partials      172      176       +4

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

KINGH242 · 2025-08-02T01:35:30Z

This fixes #450

cffls

Thanks for the PR! I have a basic understanding of what you are trying to accomplish. My main concern was that, with this change, all plutus data will be converted to NonEmptyOrderedSet, regardless of their original format. This might break all transactions generated pre-conway. Since you've fixed the serialization part, I don't think the forced conversion is required in script_data_hash, but happy to discuss further or help with debugging if you have a concrete example of failure test cases.

cffls · 2025-08-02T17:22:02Z

pycardano/utils.py

    if not redeemers:
+        redeemer_bytes = cbor2.dumps({}, default=default_encoder)


We should just check if the redeemer is actually None, and if so, assign an empty redeemer instance to it: redeemers = Redeemers(), similar to the implementation here, and the encoding will be automatically handled.

Suggested change

redeemer_bytes = cbor2.dumps({}, default=default_encoder)

if redeemers is None:

redeemers = Redeemers()

Empty Redeemers() will be encoded as A0.

cffls · 2025-08-02T17:48:58Z

pycardano/utils.py

+        if isinstance(datums, list):
+            # If datums is a NonEmptyOrderedSet, convert it to a shallow primitive representation
+            # to ensure correct CBOR encoding
+            datums = NonEmptyOrderedSet(datums)


Not sure if it is a good idea to convert it to NonEmptyOrderedSet. Ideally, script_data_hash should never change its input, because all items, including redeemers and datums, should match exactly to their representation in the witness set. See this: https://github.com/IntersectMBO/cardano-ledger/blob/1f0c6a4eaa4fb8f937c30a4608a4fafedaca216e/eras/conway/impl/cddl-files/conway.cddl#L449-L451

cffls · 2025-08-02T17:54:15Z

test/pycardano/test_util.py

@@ -156,22 +166,39 @@ def test_script_data_hash():
    redeemers = [Redeemer(unit, ExecutionUnits(1000000, 1000000))]
    redeemers[0].tag = RedeemerTag.SPEND
    assert ScriptDataHash.from_primitive(
-        "032d812ee0731af78fe4ec67e4d30d16313c09e6fb675af28f825797e8b5621d"
+        "2ad155a692b0ddb6752df485de0a6bdb947757f9f998ff34a6f4b06ca0664fbe"


This seems to violate the purpose of unit test. The test was written to ensure a known working datum hash remains working. If it has to change, it means the code changes have broken something. If the purpose is to test the case where datums is wrapped in NonEmptyOrderedSet, please create a new test.

cffls · 2025-08-02T17:54:49Z

test/pycardano/test_util.py

 def test_script_data_hash_datum_only():
    unit = Unit()
    assert ScriptDataHash.from_primitive(
-        "2f50ea2546f8ce020ca45bfcf2abeb02ff18af2283466f888ae489184b3d2d39"
+        "264ea21d9904cd72ce5038fa60e0ddd0859383f7fbf60ecec6df22e4c4e34a1f"


Same as above, changing hash in unit test isn't a good idea.

KINGH242 · 2025-08-02T18:08:44Z

Thanks for the quick feedback. I will work on changes and revert.

… list

…t_data_hash in test_script_data_hash_redeemer_map

cffls · 2025-08-03T16:05:09Z

pycardano/utils.py

+        if not isinstance(datums, NonEmptyOrderedSet):
+            # If datums is not a NonEmptyOrderedSet, handle it as a list
+            datum_bytes = cbor2.dumps(datums, default=default_encoder)
+        else:
+            datum_bytes = cbor2.dumps(
+                datums.to_shallow_primitive(), default=default_encoder
+            )


I don't see why this change is needed. The default_encoder will be able to call to_shallow_primitive when the datum is not a list. We can keep the original implementation.

I think this was needed the way it is because NonEmptyOrderedSet also inherits from other types that might cause incorrect behavior here.

Thanks for checking. I think there might be some issue in the way the cbor encodes the OrderedSet. I will play with it a bit and see if there is a better solution.

Fixed this issue by removing list type from OrderedSet. Having list type will enable cbor to bypass to_shallow_primitve() and directly encode the object as a list.

cffls · 2025-08-03T16:06:42Z

pycardano/utils.py

+    cost_models_bytes = cbor2.dumps(
+        cost_models,
+        default=default_encoder,
+        canonical=True,  # Ensures definite length encoding and canonical map keys


This might break existing unit tests.

cffls · 2025-08-03T16:08:03Z

test/pycardano/test_util.py

 def test_script_data_hash_datum_only():
    unit = Unit()
    assert ScriptDataHash.from_primitive(
-        "2f50ea2546f8ce020ca45bfcf2abeb02ff18af2283466f888ae489184b3d2d39"
+        "244926529564c04ffdea89005076a6b6aac5e4a2f38182cd48bfbc734b3be296"


Again, I don't think we should allow the hash to change in the unit test.

cffls · 2025-08-03T16:08:18Z

test/pycardano/test_util.py

    ) == script_data_hash(redeemers=[], datums=[unit])


 def test_script_data_hash_redeemer_only():
    unit = Unit()
    redeemers = []
    assert ScriptDataHash.from_primitive(
-        "a88fe2947b8d45d1f8b798e52174202579ecf847b8f17038c7398103df2d27b0"
+        "9eb0251b2e85b082c3706a3e79b4cf2a2e96f936e912a398591e2486c757f8c1"


Same as above.

… True on cost_models_bytes

KINGH242 · 2025-08-03T19:43:55Z

I was able to fix the other issues, thought I reverted the hashes in the previous one but I maybe didn't check them properly. Now script_data_hash can handle any of the various scenarios.

cffls · 2025-08-08T15:22:55Z

Pushed a more generic fix. @KINGH242 Could you please check if this new patch works for all your use cases?

…ompatibility

KINGH242 · 2025-08-09T19:19:50Z

Yes, this still works now. I just added one more thing as I discovered a new edge case with another transaction where plutus_data is encoded as an IndefiniteList.

cffls

Thanks for the fix!

Hareem Adderley added 5 commits August 1, 2025 20:01

refactor: update script_data_hash to support NonEmptyOrderedSet for d…

9f0dc9c

…atums

fix: update plutus_data to support NonEmptyOrderedSet

5f4c7a5

fix: update plutus_data to use NonEmptyOrderedSet for improved data h…

cde7bfe

…andling

refactor: enhance OrderedSet to support IndefiniteList for improved s…

cd6a1cd

…erialization

test: update test cases for script_data_hash to reflect changes

8f23d5e

cffls reviewed Aug 2, 2025

View reviewed changes

Hareem Adderley added 4 commits August 2, 2025 13:17

fix: update redeemer handling to use RedeemerMap for empty cases

1de5302

fix: update datum handling to correctly process NonEmptyOrderedSet or…

d328f96

… list

fix: revert tests hash for lists and pass NonEmptyOrderedSet to scrip…

6c87c32

…t_data_hash in test_script_data_hash_redeemer_map

fix: update script_data_hash to pass in NonEmptyOrderedSet for datums

304375c

cffls reviewed Aug 3, 2025

View reviewed changes

Hareem Adderley added 2 commits August 3, 2025 14:30

fix: improve redeemer handling for compatibility and remove canonical…

7f6422c

… True on cost_models_bytes

fix: revert expected hash values in script_data_hash tests

fb6cdeb

Fix OrderedSet serialization

d63d31a

cffls force-pushed the main branch from dd01041 to d63d31a Compare August 8, 2025 15:11

Remove unnecessary type ignore

c2c8b39

fix: update plutus_data type to include IndefiniteList for improved c…

65eedc8

…ompatibility

cffls approved these changes Aug 9, 2025

View reviewed changes

cffls merged commit 17a0d34 into Python-Cardano:main Aug 9, 2025
12 of 13 checks passed

		if not redeemers:
		redeemer_bytes = cbor2.dumps({}, default=default_encoder)

	redeemer_bytes = cbor2.dumps({}, default=default_encoder)
	if redeemers is None:
	redeemers = Redeemers()

Uh oh!

Add support for NonEmptyOrderedSet in Plutus_data #451

Add support for NonEmptyOrderedSet in Plutus_data #451

Uh oh!

Conversation

KINGH242 commented Aug 2, 2025

Enhancements to OrderedSet and Serialization:

Improvements in Plutus Data Handling:

Enhancements to Utilities:

Test Coverage Expansion:

Uh oh!

codecov bot commented Aug 2, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

KINGH242 commented Aug 2, 2025

Uh oh!

cffls left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

KINGH242 commented Aug 2, 2025

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!

KINGH242 commented Aug 3, 2025

Uh oh!

cffls commented Aug 8, 2025

Uh oh!

KINGH242 commented Aug 9, 2025

Uh oh!

cffls left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Enhancements to `OrderedSet` and Serialization:

codecov bot commented Aug 2, 2025 •

edited

Loading

cffls left a comment •

edited

Loading