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

BuiltinData is Union type but fails isinstance #70

Open
juliusfrost opened this issue Mar 8, 2023 · 2 comments
Open

BuiltinData is Union type but fails isinstance #70

juliusfrost opened this issue Mar 8, 2023 · 2 comments
Labels
enhancement New feature or request wontfix This will not be worked on

Comments

@juliusfrost
Copy link
Contributor

juliusfrost commented Mar 8, 2023

Describe the bug

Anything, PlutusData, BuiltinData, Redeemer, Datum are all equivalent to pycardano.Datum

Datum = Union[PlutusData, dict, IndefiniteList, int, bytes, RawCBOR, RawPlutusData]

However, eopsin does not allow compilation with isinstance.

To Reproduce
Use eopsin compile on the following script:

from eopsin.prelude import *

def validator(datum: BuiltinData, redeemer: BuiltinData, context: BuiltinData) -> None:
    if isinstance(redeemer, int):
        assert redeemer == 42

Error message:

    if isinstance(redeemer, int):
        result = redeemer == 42
    ^
AssertionError: Can only cast instances of Union types
Note that eopsin errors may be overly restrictive as they aim to prevent code with unintended consequences.

Expected behavior
Script compiles and above types function as Union type.

Additional context
Relevant code snippet:
https://github.com/ImperatorLang/eopsin/blob/cee2aa9bb2910732f283ff9ac69e870a94024770/eopsin/type_inference.py#L222-L262

@nielstron
Copy link
Contributor

nielstron commented Mar 8, 2023

Hi @juliusfrost,
Thanks for reporting! The error message should really be changed to "can only cast instances of Unions of PlutusData"

You can not check what instance something that's not a PlutusData object is right now... though I will have to think about changing that to allow BuiltinData.

Suggestions to fix the problem at hand:

  • downcast the datum to int via redeemer_cast: int = redeemer
  • just change the type hint for redeemer to int

@nielstron nielstron added enhancement New feature or request bug Something isn't working labels Mar 8, 2023
@nielstron
Copy link
Contributor

nielstron commented Mar 8, 2023

I have updated the error message in bda9171

Currently it is also not allowed to introduce user-defined Union types with PlutusData and int/bytes/list/dict.
Since this is technically feasible with UPLC however, I think I will change a few things in the framework in a unified way

  • Union should be allowed to union bytes, int, List, Dict and PlutusData with unique constructor ids
  • it should be possible to isinstance check on these union types to differentiate between them

I have a bit of a philosophical issue with isinstance checks on Anything/BuiltinData, because they give the wrong impression of there actually being a type check performed, which is not what happens.
Eopsin will just try to squeeze whatever it got into the the type you asked for. To maybe illustrate the problem

from eopsin.prelude import *

@dataclass()
class A(PlutusData):
   x: int

@dataclass()
class B(PlutusData):
   y: int
  
def validator(z: BuiltinData) -> None:
   if isinstance(z, A):
      print("Is A with " + str(z.x))
   if isinstance(z, B):
      print("Is B with " + str(z.y))

If I give it A(3) as input the logs would be

$ eopsin eval sample.py "{\"constructor\":0, \"fields\":[{\"int\":3}]}"
Is A with 3
Is B with 3

isinstance can only reasonably check the constructor id as the actual class name is not encoded in the object itself.
This is not what the user would sensibly expect.
Maybe I need to think a bit more about how to make this safe.

To contrast this, the following will just not compile, resulting in a safer program

from eopsin.prelude import *

@dataclass
class A(PlutusData):
   x: int

@dataclass
class B(PlutusData):
  y: int
  
def validator(z: Union[A, B]) -> None:
   if isinstance(z, A):
      print("Is A with " + str(z.x))
   if isinstance(z, B):
      print("Is B with " + str(z.y))
$ eopsin compile sample.py
Traceback (most recent call last):
  File "/home/niels/git/eopsin-lang/venv/bin/eopsin", line 33, in <module>
    sys.exit(load_entry_point('eopsin-lang', 'console_scripts', 'eopsin')())
  File "/home/niels/git/eopsin-lang/eopsin/__main__.py", line 134, in main
    raise SyntaxError(
  File "sample.py", line 11
    def validator(z: Union[A, B]) -> None:
                 ^
AssertionError: Union must combine PlutusData classes with unique constructors
Note that eopsin errors may be overly restrictive as they aim to prevent code with unintended consequences.

The current way of writing it with BuiltinData is this, which makes more clear what is actually happening

from eopsin.prelude import *


@dataclass()
class A(PlutusData):
    x: int


@dataclass()
class B(PlutusData):
    y: int


def validator(z: BuiltinData) -> None:
   z_a: A = z
   print("Is A with " + str(z_a.x))
   z_b: B = z
   print("Is B with " + str(z_b.y))

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

No branches or pull requests

2 participants