Skip to content

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

Merged
merged 5 commits into from
May 16, 2025

Conversation

aorumbayev
Copy link
Contributor

@aorumbayev aorumbayev commented May 14, 2025

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:

@abimethod(allow_actions=["NoOp"])
    def test_wrapper_get_position_slot(self, user_addr: Bytes, market_slot: UInt64) -> tuple[bool, UInt64]:
        """Temporary wrapper for testing _get_position_slot."""
        return self._get_position_slot(user_addr, market_slot)

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.

Adds checks to ensure that type validation applies to classes only.
@aorumbayev aorumbayev requested a review from daniel-makerx May 14, 2025 15:48
@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried reverting the fix and running this test and it still passed, so I think this isn't fully capturing the issue.

Was the issue related to tuples in storage? As support for this was only recently added to Puya. @boblat is currently working on adding support for that to this library

Copy link
Contributor Author

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

Copy link
Contributor Author

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

@aorumbayev aorumbayev force-pushed the fix/primitive-types-in-tuples branch 2 times, most recently from b9165c5 to fd24e5f Compare May 15, 2025 12:03
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)
Copy link
Contributor

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.

arc4_to_native=lambda a: a.native,
)

if origin_typ is tuple or (inspect.isclass(typ) and issubclass(typ, tuple)):
Copy link
Contributor

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?

@daniel-makerx daniel-makerx merged commit c8d9ee8 into main May 16, 2025
3 checks passed
@daniel-makerx daniel-makerx deleted the fix/primitive-types-in-tuples branch May 16, 2025 06:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants