Python-Cardano/pycardano

PlutusData parsing does not handle modern type hints

theeldermillenial opened this issue · 8 comments

Describe the bug
PlutusData classes honor old type hints (e.g. List[bytes]) by using issubclass.

Modern type hints (e.g. list[bytes]) fail because list is not a class.

This is the guilty line of code:

if inspect.isclass(f.type) and not issubclass(f.type, valid_types):

To Reproduce
Create a PlutusData class with modern type hints and try to instantiate.

from pycardano import PlutusData

class Test(PlutusData):
    a: list[bytes]

Test(a=[b'hello', b'world'])

Environment and software version (please complete the following information):

  • PyCardano Version 0.10.0
cffls commented

What error are you referring to specifically? I tried:

    @dataclass
    class TestList(PlutusData):
        a: List[bytes]

    vals = [b'hello', b'world']
    TestList(a=vals)

and

    @dataclass
    class TestList(PlutusData):
        a: list[bytes]

    vals = [b'hello', b'world']
    TestList(a=vals)

Both examples above worked without errors.

Oops, like I had a mistake in my code blurb. I'm still getting an error though.

This is what I have:

from dataclasses import dataclass
from pycardano import PlutusData

@dataclass
class Test(PlutusData):
    a: list[bytes]

Test(a=[b"hello", b"world"])

This is the error I get:

Traceback (most recent call last):
  File "/config/workspace/test.py", line 10, in <module>
    Test(a=[b"hello", b"world"])
  File "<string>", line 4, in __init__
  File "/config/workspace/.venv/lib/python3.10/site-packages/pycardano/plutus.py", line 533, in __post_init__
    if inspect.isclass(f.type) and not issubclass(f.type, valid_types):
  File "/config/.pyenv/versions/3.10.13/lib/python3.10/abc.py", line 123, in __subclasscheck__
    return _abc_subclasscheck(cls, subclass)
TypeError: issubclass() arg 1 must be a class

More details about my environment:

pycardano==0.10.0
Python==3.10.13

Swapping out typing.List for list works.

cffls commented

Ah I see, this only worked in python 3.11. Lower python versions will have this error. However, before diving into this, should PlutusData support list? IIRC, only indefinite list should be supported.

Excellent question. I kind of like having the convenience of using list and Pythons other types. I guess I don't see a distinction between list and IndefiniteList, and using list feels more natural. I'm good for whatever though. If I had it my way, I'd try to make things as unobtrusive as possible, so I'd just support list. It is odd that typing.List works but list does not. It seems to me like if we go the route of using IndefiniteList, we should probably deprecate all Python types except int and float. Even bytes has caused us issues.

cffls commented

From a user's perspective, I guess you are right that we should natively support list.
My previous question was regarding whether a definite list should supported in Plutus data. I don't know the answer tbh, but one thing we need to consider is the divergence it brings when hashing the Plutus data. IIRC, the reason why we always use indefinite list in Plutus data was to make it compatible with datum created by PlutusTx. Before indefinite list was introduced to PyCardano, there was an issue where it couldn't derive a matching datum hash when deserializing a datum which was generated and serialized by PlutusTx. This is because indefinite list and definite list have different encoding in cbor.
With that being said, I think the solution that makes the most sense is to support native list in Plutus data and serialize it as an indefinitely list. What do you think about this proposal?

Hm...that is interesting. I did not realize that. I always wondered why there was an IndefiniteList, but that makes perfect sense.

By definition, all Python list are IndefiniteList, so I would assume those would be interchangeable. I kind of like to make things as Pythonic as possible when doing things like this, so maybe what we can do is map collections.deque to DefiniteList, and that's not a perfect match but it's much closer than mapping list to DefiniteList. The reason I like to make things Pythonic is that coding for a new protocol feels more natural, even if it might be more opaque because we do magic behind the scenes to translate.

Actually, I think the more appropriate mapping of a DefiniteList is probably a Tuple, for technical reasons.

cffls commented

We should probably test if definite list works in Plutus before supporting it. I like the idea of introducing Tuple as definite list. For now, I think we can simply support list to Plutus data and encode it as indefinite list.