Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

#

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Tuple Index Into Minor Fix #663

Merged
merged 9 commits into from
Feb 10, 2023
Merged

Tuple Index Into Minor Fix #663

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
d421660
minor
ahangsu Feb 9, 2023
cae8721
changelog and unit test
ahangsu Feb 9, 2023
f1b6ead
more testcase
ahangsu Feb 9, 2023
b4b55f3
integration testcase
ahangsu Feb 9, 2023
6c55c4a
combine tuple index decode logic
ahangsu Feb 9, 2023
eaf8c26
optimize for all static case
ahangsu Feb 9, 2023
dda9b0a
f string
ahangsu Feb 10, 2023
7bc748d
reasonable testcase of using static bytes that makes a difference fro…
ahangsu Feb 10, 2023
8fb2e33
update testcase and some logic
ahangsu Feb 10, 2023
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 CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,8 @@

## Fixed

* Fixed wrong encoding result in tuple get last item. ([#663](https://github.com/algorand/pyteal/pull/663))

## Changed

# v0.22.0
Expand Down
3 changes: 0 additions & 3 deletions pyteal/ast/abi/tuple.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -198,9 +198,6 @@ def _index_tuple(
if offset == 0:
# This is the first and only value in the tuple, so decode all of encoded
return output.decode(encoded)
# This is the last value in the tuple, so decode the substring from start_index to the end of
# encoded
return output.decode(encoded, start_index=start_index)

if offset == 0:
# This is the first value in the tuple, so decode the substring from 0 with length length
Expand Down
13 changes: 12 additions & 1 deletion pyteal/ast/abi/tuple_test.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -330,7 +330,9 @@ class IndexTest(NamedTuple):
IndexTest(
types=[byte_t, tuple_t],
typeIndex=1,
expected=lambda output: output.decode(encoded, start_index=pt.Int(1)),
expected=lambda output: output.decode(
encoded, start_index=pt.Int(1), length=pt.Int(1)
),
),
IndexTest(
types=[tuple_t, byte_t],
Expand Down Expand Up @@ -371,6 +373,15 @@ class IndexTest(NamedTuple):
encoded, start_index=pt.ExtractUint16(encoded, pt.Int(0))
),
),
IndexTest(
Copy link
Contributor

@tzaffi tzaffi Feb 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I tried this test case against master (and also the graviton roundtrip test that I proposed) and both tests passed. I'd like to get a better understanding of why that is. For the graviton roundtrip test, I guess the test is passing because it's decoding the entire tuple as opposed to a portion of it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point on reverse testing on master, I am also trying to replicate things on master and will get back to you what I have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, my first find: this is INDEED an insufficient test: for we are taking from byte, rather than a static array (with static type). So the compilation will use getbyte, which makes no difference with or without length.

I propose we should introduce another type spec (like static bytes) for this testcase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

made a change in 7bc748d, and it should be failing on master

types=[dynamic_array_t1, byte_t],
typeIndex=1,
expected=lambda output: output.decode(
encoded,
start_index=pt.Int(2),
length=pt.Int(1),
),
),
IndexTest(
types=[byte_t, dynamic_array_t1, byte_t],
typeIndex=1,
Expand Down
2 changes: 1 addition & 1 deletion ...integration/teal/roundtrip/app_roundtrip_(byte[4],(bool,bool),uint64,address)[]_7_v6.teal
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,7 @@ int 5
extract_uint64
store 2
load 19
extract 13 0
extract 13 32
Copy link
Contributor

Choose a reason for hiding this comment

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

So was the integration test giving a false negative?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think integration test (in general) is false negative, but the testcase is really a corner case we didn't account for.

Copy link
Contributor

@tzaffi tzaffi Feb 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

I think this is extracting the address which is at the end of the encoding, end therefore extract 13 0 $\equiv$ extract 13 32. Does that make sense given your changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, this two cases are equal only under a precondition, the last element in tuple (address in this case) has no leading dynamic type(s).

If we both agree on this precondition, then yes, it makes sense given my change.

store 3
load 0
callsub arraycomplement_4
Expand Down
2 changes: 1 addition & 1 deletion ...integration/teal/roundtrip/app_roundtrip_(byte[4],(bool,bool),uint64,address)[]_7_v8.teal
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ int 5
extract_uint64
store 2
frame_dig -1
extract 13 0
extract 13 32
store 3
load 0
callsub arraycomplement_4
Expand Down