-
Notifications
You must be signed in to change notification settings - Fork 1
fix: fixes type checking of tuples with primitive types #39
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
Conversation
Adds checks to ensure that type validation applies to classes only.
@@ -279,3 +289,8 @@ def _compare_abi_and_arc4_values( | |||
assert arc4_value.native == abi_value | |||
else: | |||
assert arc4_value.bytes == int_to_bytes(abi_value, len(arc4_value.bytes)) | |||
|
|||
|
|||
def test_tuple_return_with_primitive_type(context: AlgopyTestContext) -> None: # noqa: ARG001 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@daniel-makerx The error is unique to versions of python starting from 3.13 and higher python/cpython#101162
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a ci to run in matrix, here is a run from temporary commit reverting original code back you can see several tests failing due to new behaviour in 3.13 https://github.com/algorandfoundation/algorand-python-testing/actions/runs/15044342246/job/42283130835?pr=39
b9165c5
to
fd24e5f
Compare
src/_algopy_testing/serialize.py
Outdated
from _algopy_testing import arc4 | ||
from _algopy_testing.models import Account | ||
from _algopy_testing.primitives import BigUInt, Bytes, ImmutableArray, String | ||
from _algopy_testing.protocols import UInt64Backed | ||
|
||
if issubclass(typ, arc4._ABIEncoded): | ||
origin_typ = typing.get_origin(typ) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: I think this should be declared closer to the usage or perhaps even inlined as it is used in only one spot.
src/_algopy_testing/serialize.py
Outdated
arc4_to_native=lambda a: a.native, | ||
) | ||
|
||
if origin_typ is tuple or (inspect.isclass(typ) and issubclass(typ, tuple)): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we also add inspect.isclass(typ)
clause to the next if statement on line 117 just to be safe?
Adds checks to ensure that type validation applies to classes only.
Changed introduced in 0.5.0 started causing issues for abi methods that return tuple types with primitive types inside.
For instance a method like:
Would fail in serialize.py with
TypeError: issubclass() arg 1 must be a class
. The issue is due to issubclass checks that require first arg to be classes, but when elements inside are primitive types like bool it fails. PR simply tweaks the conditionals to ensure tuples are handled correctly.