OpShin / opshin

A simple pythonic programming language for Smart Contracts on Cardano
https://book.opshin.dev/
MIT License
137 stars 26 forks source link

Fractions functionality issue #395 #403

Open SCMusson opened 3 weeks ago

SCMusson commented 3 weeks ago

Targetting issue #395. Dunder methods for all Fractions functions. The Fraction functions are still there, I don't know if you want to temporarily keep them for backwards compatibility or to be replaced entirely with dunder methods.

Also created Fraction.norm and Fraction.ceil class methods which could replace ceil_fraction and norm_fraction.

nielstron commented 3 weeks ago

I would remove the old methods.

One plan for this was also that it is merged after #397, such that add mul etc have support for Union[int, Fraction].

SCMusson commented 3 weeks ago

That's fine. #397 is pretty much ready to go. I'm happy to come back to this when its merged.

SCMusson commented 3 weeks ago

I had to modify a few different things so that Self in Unions was accepted.

Currently running something like:

from opshin.tests.utils import eval_uplc
from opshin.std.fractions import Fraction
a = Fraction(5,7)
b = Fraction(11,13)

source_code = """
from opshin.std.fractions import *
from typing import Dict, List, Union

def validator(a: Fraction, b: Union[Fraction, int]) -> Fraction:
    return a + b
"""
ret = eval_uplc(source_code, a,b)

Works but running eval_uplc(source_code, a, 5)

Returns the following error:

Traceback (most recent call last):
  File "/home/wppj21/Workshop/opshin/test_class.py", line 121, in <module>
    ret = eval_uplc(source_code, a, 5)
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/wppj21/Workshop/opshin/opshin/tests/utils.py", line 35, in eval_uplc
    raise ret
  File "/home/wppj21/miniconda3/envs/opshin/lib/python3.11/site-packages/uplc/machine.py", line 111, in eval
    stack.append(self.return_compute(step.context, step.value))
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/wppj21/miniconda3/envs/opshin/lib/python3.11/site-packages/uplc/machine.py", line 171, in return_compute
    return self.apply_evaluate(context.ctx, context.val, value)
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/wppj21/miniconda3/envs/opshin/lib/python3.11/site-packages/uplc/machine.py", line 210, in apply_evaluate
    res = eval_fun(*arguments)
          ^^^^^^^^^^^^^^^^^^^^
  File "/home/wppj21/miniconda3/envs/opshin/lib/python3.11/site-packages/uplc/ast.py", line 745, in wrapped_fun
    assert isinstance(
           ^^^^^^^^^^^
AssertionError: Argument 0 has invalid type, expected type <class 'uplc.ast.BuiltinInteger'> got <class 'uplc.ast.PlutusInteger'> (PlutusInteger(value=5))

Is this an bug in uplc or have I made an error somewhere?

nielstron commented 3 weeks ago

There is a bug in OpShin, likely something we overlooked in #397.

When something is a Union[x,y] it is PlutusData. However for int and other primitive types I usually assume they are unwrapped into builtin data. Here there must be some missing unwrapping of int after casting it via isinstance. Note that addition can not be done with PlutusData and requires unwrapping via UInt before.

nielstron commented 3 weeks ago

Try


def validator(x: Union[int, A]):
  if isinstance(x, int):
     print(x+2)
nielstron commented 3 weeks ago

There is likely also another edge case in #397

def validator(x: int):
   y : Union[int, A] = x
   if isinstance(y, int):
     print("foo")

will likely crash

SCMusson commented 1 week ago

Is CI not working at the moment?

nielstron commented 1 week ago

Great, thanks for the update. I think Travis is currently broken, I wanted to migrate to Github actions anyways and am preparing #406 for this. Will merge it into this branch to check that the tests are now still passing.

nielstron commented 1 week ago

It looks like Self is a 3.11 only feature of typing. In previous python versions this is instead solved using literal strings like "Fraction".

SCMusson commented 1 week ago

Ah, I'll rewrite them all as literal strings then.

coveralls commented 1 week ago

Coverage Status

coverage: 89.428% (+0.02%) from 89.406% when pulling 303821bbe914d305a4322688718f4434d9f06348 on SCMusson:feat_frac_dunder into 6f99124f990f47557a65d95c372788e5733e664e on OpShin:dev.

nielstron commented 1 week ago

The fraction code looks great now. Unfortunately I still found (and added) 2 failing cases for #397.

SCMusson commented 1 week ago

I'll get to work on those

SCMusson commented 1 week ago

Thank you for those test cases, the first one I really should have thought of myself but I wouldn't have thought of the test_Union_builtin_cast_direct. I had to modify them so that the tests were correct so It might be worth you checking to make sure I didn't break the test.

nielstron commented 1 week ago

Your modifications of if-expressions made me think... caught another bug :)

nielstron commented 1 week ago

Sorry about the never ending new crashing examples... the last ones added are out of scope though (since not even properly implemented for unions of plutus data), I created #408 dedicated for them. also its not a problem if an expression does not compile.