Skip to content
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.

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

PlutusData parsing does not handle modern type hints #287

Open
theeldermillenial opened this issue Dec 16, 2023 · 8 comments
Open

PlutusData parsing does not handle modern type hints #287

theeldermillenial opened this issue Dec 16, 2023 · 8 comments
Labels
enhancement New feature or request

Comments

@theeldermillenial
Copy link
Contributor

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
Copy link
Collaborator

cffls commented Dec 18, 2023

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.

@theeldermillenial
Copy link
Contributor Author

theeldermillenial commented Dec 18, 2023

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
Copy link
Collaborator

cffls commented Dec 18, 2023

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.

@theeldermillenial
Copy link
Contributor Author

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
Copy link
Collaborator

cffls commented Dec 29, 2023

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?

@cffls cffls added the enhancement New feature or request label Dec 29, 2023
@theeldermillenial
Copy link
Contributor Author

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.

@theeldermillenial
Copy link
Contributor Author

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

@cffls
Copy link
Collaborator

cffls commented Jan 15, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants