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.

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

Pass-by-Ref / Dynamic Scratch Variables via the loads and stores opcodes #198

Merged
merged 87 commits into from
Mar 1, 2022

Conversation

tzaffi
Copy link
Contributor

@tzaffi tzaffi commented Feb 15, 2022

Dynamic Scratch Variables

New Capabilities

This PR aims to allow dynamically-indexed scratch variables. For example here are some nice-to-have examples:

Pass-by-Reference Semantics

You can see the resulting teal (including my comments) here.

@Subroutine(TealType.none)
def swap(x: ScratchVar, y: ScratchVar):
    z = ScratchVar(TealType.anytype)
    return Seq(
        z.store(x.load()),
        x.store(y.load()),
        y.store(z.load()),
    )

Use a Dynamic ScratchVar to "iterate" over other ScratchVar's

You can see the resulting teal with comments here.

def wilt_the_stilt():
    player_score = DynamicScratchVar(TealType.uint64)

    wilt = ScratchVar(TealType.uint64, 129)
    kobe = ScratchVar(TealType.uint64)
    dt = ScratchVar(TealType.uint64, 131)

    return Seq(
        player_score.set_index(wilt),
        player_score.store(Int(100)),
        player_score.set_index(kobe),
        player_score.store(Int(81)),
        player_score.set_index(dt),
        player_score.store(Int(73)),
        Assert(player_score.load() == Int(73)),
        Assert(player_score.index() == Int(131)),
        player_score.set_index(wilt),
        Assert(player_score.load() == Int(100)),
        Assert(player_score.index() == Int(129)),
        Int(100),
    )

Other improvements

I've also introduced another way to run end-to-end tests. The script tests/pass_by_ref_test.py works as follows:

  • iterates over a list of PyTEAL approval functions defined in the file
  • compiles the program and saves into tests/teal with a file name that is dynamically generated from the approval program function name
  • loads pre-compiled companion TEAL with an extra _expected suffix in the filename
  • compares the just-compiled TEAL against the expected TEAL line by line. The expected TEAL can have extra information -such as comments- but they have to start out exactly the same for the test to pass

Testing

  • Unit tests have been added to:
    • pyteal/ast/scratch_test.py
    • pyteal/ast/scratchvar_test.py
    • pyteal/ast/subroutine_test.py
  • The end to end test tests/pass_by_ref_test.py has been added
  • Temporary PR DO NOT MERGE: Test the current compiler against the future compilations #203 runs all the backwards compatible tests in tests/pass_by_ref_test.py to ensure that the generated TEAL hasn't changes due to this PR

Summary of Changes

  • Various __init__*'s
    • To bubble up the visibility of the new DynamicScratchVar and make mypy happy
    • Also, modified scripts/generate_init.py now sorts and uniquifies __all__
  • Resolve mypy complaints:
    • pyteal/ast/multi_test.py
  • pyteal/ast/scratch.py:
    • ability to query a slot's index at runtime via the new ScratchIndex class and various index() methods
  • pyteal/ast/scratchvar.py - new DynamicScratchIndex class that allows subroutines to access and modify external ScratchVar's and also allows it to act as an "iterator" over pre-existing ScratchVar's. It uses the loads and stores op codes in order to achieve its capabilities.
  • pyteal/ast/subroutine.py - modify the to allow pass-by-reference semantics
    • SubroutineDefinition now:
      • has a by_ref_args member set so one can tell if an argument is meant to be by-reference or by-value
      • allows return types to be any subtype of Expr, not just Expr
      • allows argument types to be ScratchVar's in addition to the previous Expr
      • has a new arguments() method
    • SubroutineCall now can handle by-ref arguments, by detecting such arguments and calling their new .index() method to pass their index on the stack, instead of their value
    • SubroutineDeclaration now can handle by-ref arguments as well. THIS IS THE TRICKIEST PART OF THE PR. The code changes are deceptively small. There are some new comments which attempt to explain the goings on.
  • New end-to-end test tests/pass_by_ref_test.py
    • Various expected outputs of compilation tests/teal/*_expected.py

@tzaffi tzaffi marked this pull request as draft February 15, 2022 04:44
@tzaffi tzaffi changed the base branch from master to dynamic-scratch February 15, 2022 04:46
@tzaffi tzaffi mentioned this pull request Feb 17, 2022
@tzaffi tzaffi changed the base branch from dynamic-scratch to master February 17, 2022 22:41
@@ -17,156 +17,158 @@ from .errors import TealInternalError, TealTypeError, TealInputError, TealCompil
from .config import MAX_GROUP_SIZE, NUM_SLOTS

__all__ = [
"Expr",
"LeafExpr",
"AccountParam",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This file changed mostly as a result of getting the generator to sort and uniquify __all__

@@ -37,7 +37,7 @@ def generate_init_pyi() -> str:
start_idx = init_contents.index(begin_flag)
end_idx = init_contents.index(end_flag)

all_imports = ",\n ".join(['"{}"'.format(s) for s in static_all])
all_imports = ",\n ".join(['"{}"'.format(s) for s in sorted(set(static_all))])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorting and uniquifying

Copy link
Contributor

Choose a reason for hiding this comment

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

Why is deduplicating necessary? I'd argue that's an actual problem with our exports and this script should error if a duplicate is detected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Looking closer, it turns out no de-duping actually occurs. So I'll change the code to make an assertion instead.

Zeph Grunschlag added 2 commits March 1, 2022 00:36
Co-authored-by: Michael Diamant <michaeldiamant@users.noreply.github.com>
Copy link
Contributor

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

Implementation looks good, I just have lingering questions

Comment on lines 131 to 136
(
"mixed3 missing the non-sv",
fnWithMixedAnns4AndIntReturn,
[sv, sv],
TealInputError,
),
Copy link
Contributor

Choose a reason for hiding this comment

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

Why get rid of this test?

Copy link
Contributor Author

@tzaffi tzaffi Mar 1, 2022

Choose a reason for hiding this comment

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

I moved this test case to test_subroutine_definition_invalid() because we are no longer allowing Expr subclasses for annotation return types.

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 also renamed the function fnWithMixedAnns4AndBytesReturn (it was wrongly named before)

)

for fn, msg in cases:
with pytest.raises(TealInputError) as e:
print(f"case=[{msg}]")
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this print statement intentional?

Copy link
Contributor Author

@tzaffi tzaffi Mar 1, 2022

Choose a reason for hiding this comment

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

Yes. Instead of catching the exception and re-raising with the case as I was doing before, now I'm just printing out each case. So if you run pytest -vv you can see the last case=XYZ that got printed and infer the broken case.

Copy link
Contributor

@jasonpaulos jasonpaulos left a comment

Choose a reason for hiding this comment

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

LGTM

@tzaffi tzaffi merged commit cf95165 into master Mar 1, 2022
@tzaffi tzaffi deleted the pass-by-ref branch March 1, 2022 22:10
jasonpaulos added a commit that referenced this pull request Mar 7, 2022
jasonpaulos added a commit that referenced this pull request Mar 7, 2022
algoidurovic pushed a commit to algoidurovic/pyteal that referenced this pull request Mar 23, 2022
…opcodes (algorand#198)

* Pass-by-Reference Semantics
* Use a Dynamic ScratchVar to "iterate" over other ScratchVar's
* Another approach for E2E testing
algoidurovic pushed a commit to algoidurovic/pyteal that referenced this pull request Mar 23, 2022
algoidurovic pushed a commit to algoidurovic/pyteal that referenced this pull request Mar 23, 2022
algoidurovic added a commit that referenced this pull request Mar 31, 2022
* Optimization added for repeated int constants under 2**7 w/ tests

* fixed type problem and formatted

* Expanded test and added comment for clarification

* implement optimization utility with simple slot store/load canceling

* minor refactor

* reformat code

* Update pyteal/compiler/optimizer/optimizer.py

Co-authored-by: Michael Diamant <michaeldiamant@users.noreply.github.com>

* Update pyteal/compiler/optimizer/optimizer.py

Co-authored-by: Michael Diamant <michaeldiamant@users.noreply.github.com>

* Adding exponentiation to arithmatic ops docs (#134)

Add missing exponentiation operation in document

* updating to use new syntax for seq (#135)

* updating to use new syntax for seq

* rewording

* Make pylance recognize wildcard imports (#133)

* adding exports directly to top level __all__

* apply black formatter

* adding initial generate script

* fmt

* rm all from all

* adding check to travis

* reading in original __init__ and using its imports, dont write to filesystem if --check is passed

* make messages more profesh

* fix flags after black formatted them

* y

* flippin black formatter

* help text fix

* asdfasdf

* Include pyi files in build (#137)

* Revert "Optimization for constant assembly (#128)"

This reverts commit 5636ccd.

* Revert "String optimization and addition of Suffix() (#126)"

This reverts commit 7cb7b9a.

* Update to v0.9.1 (#138)

* Revert "Revert "String optimization and addition of Suffix() (#126)""

This reverts commit 564e602.

* Revert "Revert "Optimization for constant assembly (#128)""

This reverts commit cc405a5.

* Update examples.rst (#140)

* Fix type for App.globalGetEx in docs (#142)

* up max teal version (#146)

* up max teal version

* make test fail if its greater than version defined as MAX_TEAL_VERSION

* Fmt

* hardcode to 7

* Add version 6 test

* Formatting subroutines with name and newline (#148)

* using the subroutine name for the label

* adding newline before label declaration, fix tests to account for newline

* remove commented name, fix test

* only add newline for subroutines with comment

* naming with suffix

* adding test for invalid name

* Call type_of() in require_type() for better exception messages (#151)

* call type_of in require_type to catch exceptions

* fix formatting for types.py and types_test.py

* `method` pseudo-op support for ABI methods (#153)

- Add support for `method` pseudo-opcode in PyTeal.
- Add `name` field in `subroutine` to override __name__ from function implementation, for readability in generated code.

* Print diff of `__init__.pyi` (#166)

* Print diff of __init__.pyi

* Format

* Undo travis change

* C2C Feature Support (#149)

- `itxn_next` implementation / test
- `itxn_field` support for array field setting
- `gitxn / gitxna` implementation / test
- `gloadss` implementation / test

* Add BytesSqrt (#163)

* Add BytesSqrt

* Update pyteal/ast/unaryexpr_test.py

Co-authored-by: Jason Paulos <jasonpaulos@users.noreply.github.com>

Co-authored-by: Jason Paulos <jasonpaulos@users.noreply.github.com>

* adding new globals from teal6 (#168)

* adding new globals from teal6

* fmt

* Acct params get (#165)

* Adding account param getter

* Add to init

* fix op names and type

* adding tests

* allow bytes to be passed

* tweak docs, add require check for any

* Change Subroutine Wrapped Callable to a class with call method (#171)

Allows for more information (name, return type, has return) about the subroutine extractable from wrapped fnImpl by subroutine

* Subroutine Type Annotations (#182)

This PR requires that any type annotation of a Subroutine parameter or return value be of type `Expr`. Missing annotations are assumed to be `Expr`'s as well. In a follow up PR #183 this restriction will be loosened.

* fix docs referencing what apps should eval to (#191)

* Move from Travis to Github Actions (#190)

* MultiValue expression implemented to support opcodes that return multiple values (#196)

* Optimization added for repeated int constants under 2**7 w/ tests

* fixed type problem and formatted

* Expanded test and added comment for clarification

* add multivalue expr and change maybevalue to derive from multivalue

* updated tests and formatting

* reorder output slots to reflect stack ordering

* add additional assertion in MaybeValue test to enforce slot ordering

* Support TEAL 6 txn fields LastLog, StateProofPK and opcodes divw, itxnas, gitxnas (#174)

* adding new teal6 ops, no pyteal expressions defined for them yet

* Add opcode support for divw

* Add opcode support for divw (#192)

* Add opcode support for itxnas and gitxnas (#193)

* Add opcode support for itxnas and gitxnas

* Update stale reference to inner transaction limit

* Fix allowed types for GitxnaExpr txnIndex

* Remove obsolete logic for handling GitxnaExpr.teal construction

* Remove unnecessary cast and fix gitxna runtime type checking

* Move type validation to constructors for gtxn and gitxn variants

* Add missed tests from prior commit

* Fix duplicate test case

* Move index validation from subclasses to TxnaExpr

* Inline validation functions per PR feedback

* Remove unused imports

* Refactor to isinstance tupled check

* Remove TEAL v1 min version test per PR feedback

* Fix constructor type checking for GtxnExpr

* Refactor to remove duplicate type check function

* Update last_log docstring

Co-authored-by: Jason Paulos <jasonpaulos@users.noreply.github.com>

* Expose state_proof_pk txn field

* Update transaction field docs to reflect TEAL v6

* Update transaction field docs to reflect TEAL v6

Co-authored-by: michaeldiamant <michaeldiamant@users.noreply.github.com>
Co-authored-by: Jason Paulos <jasonpaulos@users.noreply.github.com>

* Fixed typo (#202)

* Add Github action to generate docset (#201)

* Add build docset step

* non-slim container

* Update docs to group transaction field tables like go-algorand (#204)

* Update accessing_transaction_field.rst to fix typo (#207)

* Add docs README to explain docs/ testing procedure (#205)

* v0.10.0 (#206)

* Update to v0.10.0

* Add latest commits to changelog

* fixing github actions to run on tags (#208)

* Update build.yml

* Update build.yml

* Fix typos in docstrings and error messages (#211)

* Test on Python 3.10 (#212)

* Update versions.rst (#210)

* Update versions.rst

content of [https://github.com/algorand/pyteal/releases] is not shown in [https://pyteal.readthedocs.io/en/latest/versions.html]

* Update docs/versions.rst

Co-authored-by: Jason Paulos <jasonpaulos@users.noreply.github.com>

Co-authored-by: Jason Paulos <jasonpaulos@users.noreply.github.com>

* Pass-by-Ref / Dynamic Scratch Variables via the `loads` and `stores` opcodes (#198)

* Pass-by-Reference Semantics
* Use a Dynamic ScratchVar to "iterate" over other ScratchVar's
* Another approach for E2E testing

* Fix build script invocation (#223)

* Bring #225 to master (#227)

* Ignore tests generating TEAL file outputs used for expected comparisons (#228)

* Fix typo in CONTRIBUTING.md (#229)

* Fix subroutine mutual recursion with different argument counts bug (#234)

* Fix mutual recursion bug

* Remove usage of set.pop

* Revert "Pass-by-Ref / Dynamic Scratch Variables via the `loads` and `stores` opcodes (#198)"

This reverts commit cf95165.

* v0.10.1 (#237)

* Revert "Revert "Pass-by-Ref / Dynamic Scratch Variables via the `loads` and `stores` opcodes (#198)""

This reverts commit 51ec8c9.

* Update user guide docs to reflect addition of DynamicScratchVar (#226)

* Update CONTRIBUTING.md on PEP 8 naming conventions policy (#241)

* implement optimization utility with simple slot store/load canceling

* minor refactor

* reformat code

* correct import format to match convention

* slot optimization awareness of reserved ids added

* fix typo

* remove dataclass usage

* slight reorg of compiler process in order to perform optimization on cfg

* clean up imports

* updated documentation and reformatted with new version of black

* remove unused imports and comments

* reformatting

* add additional optimizer unit tests

* improve testing and slight refactoring

* more renaming

* documentation and import changes

* fixed typos in docs

Co-authored-by: Michael Diamant <michaeldiamant@users.noreply.github.com>
Co-authored-by: Ben Guidarelli <ben.guidarelli@gmail.com>
Co-authored-by: Jason Paulos <jasonpaulos@users.noreply.github.com>
Co-authored-by: Edward D Gaudio <edwardgaudio@gmail.com>
Co-authored-by: Joe Polny <50534337+joe-p@users.noreply.github.com>
Co-authored-by: Hang Su <87964331+ahangsu@users.noreply.github.com>
Co-authored-by: Łukasz Ptak <StylishTriangles@users.noreply.github.com>
Co-authored-by: Zeph Grunschlag <tzaffi@users.noreply.github.com>
Co-authored-by: Jack <87339414+algojack@users.noreply.github.com>
Co-authored-by: Glory Agatevure <agatevureglory@gmail.com>
Co-authored-by: Adriano Di Luzio <aldur@users.noreply.github.com>
Co-authored-by: PabloLION <36828324+PabloLION@users.noreply.github.com>
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.

4 participants