Python-Cardano / pycardano

A lightweight Cardano library in Python
https://pycardano.readthedocs.io
MIT License
215 stars 67 forks source link

Fix Redeemer Deserialization Issue #374

Closed bhatt-deep closed 1 month ago

bhatt-deep commented 1 month ago

Problem

The recent changes to the redeemer structure introduced a deserialization error when processing transactions. The new format (dictionary with tuple keys) was incompatible with the existing deserialization logic, which expected a list of redeemers.

Solution

This PR updates the witness.py file to handle both the new and old redeemer formats:

nielstron commented 1 month ago

Hi @bhatt-deep thank for the submission, looks good. Out of curiosity - this is only necessary when trying to submit txs to pre-chang networks? Or does it also have some use in post-chang networks?

bhatt-deep commented 1 month ago

Hi @nielstron, thanks for your question.

This change is needed for post-chang networks. The issue we faced was a problem with how PyCardano handles redeemers during serialization and deserialization in the chang branch, though everything worked fine before. I think this line is causing the issue: https://github.com/Python-Cardano/pycardano/pull/371/files#diff-d8177155e5a3a27a67d01fdd3e88078e01bc853cd1d49ed0356aa6258de4dea7R1049

To explain further:

  1. Transactions submitted using the chang branch succeeded on the blockchain but failed when deserialized in PyCardano.
  2. The problem arises when reconstructing the transaction from CBOR. You can check it by running:
    print(Transaction.from_cbor(signed_tx.to_cbor()))
  3. In the _get_redeemers function, the redeemer structure changed. Instead of a list, it's now a dictionary with tuple keys. Here's an example:
    {(0, 2): [CBORTag(1281, []), [2763715, 604504597]]}
  4. This new structure didn't match the old deserialization logic, which expected a list:
    [Redeemer.from_primitive(redeemer) for redeemer in data]
  5. The error we got was:
    Error submitting transaction: ['list'] typed value is required for deserialization. Got <class 'tuple'>: (0, 2)
cffls commented 1 month ago

Nice! I didn't think of the deserialization when writing this. Could you please add a unit test for it? Otherwise, looks good to me.

bhatt-deep commented 1 month ago

Thanks! I'll add the unit test for deserialization and push the changes soon.

bhatt-deep commented 1 month ago

@cffls have added unit tests.

cffls commented 1 month ago

Thank you @bhatt-deep !