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.

Sign up for GitHub

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

Applying type_spec_abi_is_assignable_to to ABI computed type #567

Closed
wants to merge 6 commits into from
Closed

Applying type_spec_abi_is_assignable_to to ABI computed type #567

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
dac3abe
minor, applying abi_assignable_to in store_into and set_with_computed
ahangsu Oct 17, 2022
ea8fab6
full pass through PR, and some minor thoughts on type system or speci…
ahangsu Oct 17, 2022
d6f1de2
Merge branch 'master' into abi-type-check-assigning
ahangsu Oct 20, 2022
6da7ab4
update address not assignable to byte[32]
ahangsu Oct 20, 2022
3e776f8
per review comment
ahangsu Oct 21, 2022
885f38c
CHANGELOG
ahangsu Oct 21, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions pyteal/ast/abi/address.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,7 @@ def set(

match value:
case ComputedValue():
# TODO need to see again if we need to change on type system or not, _set_with_computed_type?
pts = value.produced_type_spec()
if pts == AddressTypeSpec() or pts == StaticArrayTypeSpec(
ByteTypeSpec(), AddressLength.Bytes
Copy link
Contributor Author

Choose a reason for hiding this comment

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

question on this part: we let type_spec_is_assignable_to to be false when we want to check if byte[32] is assignable to address, while in this case, we do want to assign byte[32] to address.

What should be changed:

  • the type system?
  • or the implementation, say we leave a note here for special corner case?

Copy link
Contributor

Choose a reason for hiding this comment

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

It's a good question.

My 2c and happy to discuss live: I favor changing the type system because it feels analogous to the bi-directional treatment of NameTuple and Tuple.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree the type system to be changed to not allow Address().set(StaticArrary[Bytes, 32]()) (and the ComputedValue variant)

Copy link
Contributor

Choose a reason for hiding this comment

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

Summarizing a verbal with @ahangsu and @jasonpaulos with these action items that @ahangsu will follow up on:

  • Realized there's an inconsistency in type_spec_is_assignable_to. Agreed to disallow assignment of Tuple to NamedTuple.
  • Address.set should follow type_spec_is_assignable_to rules. Implies:
    • Cannot assign StaticBytes[32] to Address. Remove it from method signature.
    • Offer a util method like unsafe_cast to enable conversion when the user knows conversion is safe.
  • Revert the following assignability change: 6da7ab4#diff-61b0d50c921418a3bcbad74d9887b354c015606277515b24d0094e7e5e8ae3c0L526. Address can be assigned to StaticBytes[32].

Copy link
Contributor Author

Choose a reason for hiding this comment

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

change reflected in 3e776f8

Copy link
Contributor Author

Choose a reason for hiding this comment

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

defer unsafe_cast implementation to a PR after this one get merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

@ahangsu Thanks for the changes + unsafe_cast in follow on PR - I'm good to resolve.

I have a new open question, but can move discussion out of this thread on my side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just a reminder, #580 carries the unsafe_cast implementation, we can always rebase to this PR and get all the code together.

The final question is on another thread, i.e., on the type signature of Address.set, and the behavior related with mypy.

Expand All @@ -103,6 +104,7 @@ def set(
f"Got ComputedValue with type spec {pts}, expected AddressTypeSpec or StaticArray[Byte, Literal[AddressLength.Bytes]]"
)
case BaseType():
# TODO need to see again if we need to change on type system or not
if (
value.type_spec() == AddressTypeSpec()
or value.type_spec()
Expand Down
12 changes: 10 additions & 2 deletions pyteal/ast/abi/array_base.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -125,8 +125,12 @@ def set(self, values: Sequence[T]) -> Expr:
A PyTeal expression that stores encoded sequence of ABI values in its internal
ScratchVar.
"""
from pyteal.ast.abi.util import type_spec_is_assignable_to

for index, value in enumerate(values):
if self.type_spec().value_type_spec() != value.type_spec():
if not type_spec_is_assignable_to(
value.type_spec(), self.type_spec().value_type_spec()
):
raise TealInputError(
"Cannot assign type {} at index {} to {}".format(
value.type_spec(),
Expand Down Expand Up @@ -220,7 +224,11 @@ def store_into(self, output: T) -> Expr:
Returns:
An expression that stores the byte string of the array element into value `output`.
"""
if output.type_spec() != self.produced_type_spec():
from pyteal.ast.abi.util import type_spec_is_assignable_to

if not type_spec_is_assignable_to(
self.produced_type_spec(), output.type_spec()
):
raise TealInputError("Output type does not match value type")

encodedArray = self.array.encode()
Expand Down
3 changes: 2 additions & 1 deletion pyteal/ast/abi/array_dynamic.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,11 +71,12 @@ def set(
Returns:
An expression which stores the given value into this DynamicArray.
"""
from pyteal.ast.abi.util import type_spec_is_assignable_to

if isinstance(values, ComputedValue):
return self._set_with_computed_type(values)
elif isinstance(values, BaseType):
if self.type_spec() != values.type_spec():
if not type_spec_is_assignable_to(values.type_spec(), self.type_spec()):
raise TealInputError(
f"Cannot assign type {values.type_spec()} to {self.type_spec()}"
)
Expand Down
4 changes: 3 additions & 1 deletion pyteal/ast/abi/array_static.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,10 +102,12 @@ def set(
Returns:
An expression which stores the given value into this StaticArray.
"""
from pyteal.ast.abi.util import type_spec_is_assignable_to

if isinstance(values, ComputedValue):
return self._set_with_computed_type(values)
elif isinstance(values, BaseType):
if self.type_spec() != values.type_spec():
if not type_spec_is_assignable_to(values.type_spec(), self.type_spec()):
raise TealInputError(
f"Cannot assign type {values.type_spec()} to {self.type_spec()}"
)
Expand Down
4 changes: 3 additions & 1 deletion pyteal/ast/abi/bool.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,8 @@ def set(self, value: Union[bool, Expr, "Bool", ComputedValue["Bool"]]) -> Expr:
Returns:
An expression which stores the given value into this Bool.
"""
from pyteal.ast.abi.util import type_spec_is_assignable_to

if isinstance(value, ComputedValue):
return self._set_with_computed_type(value)

Expand All @@ -77,7 +79,7 @@ def set(self, value: Union[bool, Expr, "Bool", ComputedValue["Bool"]]) -> Expr:
checked = True

if isinstance(value, BaseType):
if value.type_spec() != self.type_spec():
if not type_spec_is_assignable_to(value.type_spec(), self.type_spec()):
raise TealInputError(
"Cannot set type bool to {}".format(value.type_spec())
)
Expand Down
6 changes: 3 additions & 3 deletions pyteal/ast/abi/string.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,13 +103,13 @@ def set(
An expression which stores the given value into this String.
"""

from pyteal.ast.abi.util import type_spec_is_assignable_to

match value:
case ComputedValue():
return self._set_with_computed_type(value)
case BaseType():
if value.type_spec() == StringTypeSpec() or (
value.type_spec() == DynamicArrayTypeSpec(ByteTypeSpec())
):
if type_spec_is_assignable_to(value.type_spec(), StringTypeSpec()):
return self.stored_value.store(value.stored_value.load())

raise TealInputError(
Expand Down
4 changes: 3 additions & 1 deletion pyteal/ast/abi/tuple.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,8 @@ def _encode_tuple(values: Sequence[BaseType]) -> Expr:
def _index_tuple(
value_types: Sequence[TypeSpec], encoded: Expr, index: int, output: BaseType
) -> Expr:
from pyteal.ast.abi.util import type_spec_is_assignable_to

if not (0 <= index < len(value_types)):
raise ValueError("Index outside of range")

Expand All @@ -146,7 +148,7 @@ def _index_tuple(
offset += typeBefore.byte_length_static()

valueType = value_types[index]
if output.type_spec() != valueType:
if not type_spec_is_assignable_to(valueType, output.type_spec()):
raise TypeError("Output type does not match value type")

if type(output) is Bool:
Expand Down
14 changes: 10 additions & 4 deletions pyteal/ast/abi/type.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,10 +135,12 @@ def decode(
pass

def _set_with_computed_type(self, value: "ComputedValue[BaseType]") -> Expr:
target_type_spec = value.produced_type_spec()
if self.type_spec() != target_type_spec:
from pyteal.ast.abi.util import type_spec_is_assignable_to

value_type_spec = value.produced_type_spec()
if not type_spec_is_assignable_to(value_type_spec, self.type_spec()):
raise TealInputError(
f"Cannot set {self.type_spec()} with ComputedType of {target_type_spec}"
f"Cannot set {self.type_spec()} with ComputedType of {value_type_spec}"
)
return value.store_into(self)

Expand Down Expand Up @@ -215,7 +217,11 @@ def produced_type_spec(self) -> TypeSpec:
return self.type_spec

def store_into(self, output: BaseType) -> Expr:
if output.type_spec() != self.produced_type_spec():
from pyteal.ast.abi.util import type_spec_is_assignable_to

if not type_spec_is_assignable_to(
self.produced_type_spec(), output.type_spec()
):
raise TealInputError(
f"expected type_spec {self.produced_type_spec()} but get {output.type_spec()}"
)
Expand Down