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

Prepare for Testing the ABI-Router in PyTeal #49

Merged
merged 129 commits into from
Feb 6, 2023

Conversation

tzaffi
Copy link
Contributor

@tzaffi tzaffi commented Jan 17, 2023

Summary of changes

  • class ABIMethodCallStrategy (formerly class ABIContractExecutor):
    • This is moved intograviton/abi_strategy.py (from graviton/blackbox.py)
    • Removing the following methods entirely:
      • validate_inputs()
      • dryrun_sequence()
    • New methods:
      • get()
      • num_args()
  • Introducing class Simulation in graviton/sim.py. See below for usage example.
  • New test file tests/integration/abi_router_test.py which contains some tests formerly in tests/integration/abi_test.py but which are -as you might guess- all about testing the Router
  • Fixing DryRunInspector's error() and error_message() which recently broke

PyTeal Sibling PR

algorand/pyteal#634

TODO

  • Timebox expired. Most likely culprit. Figure out the actual PR referenced above and in the CHANGELOG

Zeph Grunschlag and others added 30 commits December 22, 2022 17:37
* small typing fix + EncodingType

* type ignores
@@ -92,186 +90,6 @@ def test_encode_abi():
)


NONSENSE = "not a valid signature"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moved to new test file executor_test.py

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Contributor Author

@tzaffi tzaffi left a comment

Choose a reason for hiding this comment

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

wip

Copy link
Contributor

@ahangsu ahangsu left a comment

Choose a reason for hiding this comment

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

I think abi_strategy.py looks neat, looking into sim.py and related test. Hopefully we can wrap this up this week

Copy link
Contributor

@ahangsu ahangsu left a comment

Choose a reason for hiding this comment

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

I finished my pass on sim.py, should be almost good to go, some TODOs in code (mostly comments and documentation) need to be done.

CHANGELOG.md Outdated Show resolved Hide resolved
@@ -308,10 +308,9 @@ def msg():
err\ opcode # pyteal generated err's ok
| assert\ failed\ pc= # pyteal generated assert's ok
| invalid\ ApplicationArgs\ index # failing because an app arg wasn't provided
| extract\ range\ beyond\ length\ of\ string # failing because couldn't extract when omitted final arg or jammed in tuple
| extraction\ end\ 16\ is\ beyond\ length # failing because couldn't extract when omitted final arg or jammed in tuple
Copy link
Contributor Author

Choose a reason for hiding this comment

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

error messages have changed

Copy link
Contributor

@ahangsu ahangsu left a comment

Choose a reason for hiding this comment

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

I am leaning towards approving and merging, just one question on hypothesis and related TODO, but I don't expect we will take any action here (for it would be non trivial).

Comment on lines +20 to +22
TODO: when incorporating hypothesis strategies, we'll need a more holistic
approach that looks at relationships amongst various args.
Current approach only looks at each argument as a completely independent entity.
Copy link
Contributor

Choose a reason for hiding this comment

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

no action needed, I see hypothesis cross the PR, together with TODO, are you referring to hypothesis https://hypothesis.readthedocs.io/en/latest/data.html?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tzaffi tzaffi merged commit 86c9bc4 into algorand:main Feb 6, 2023
@tzaffi tzaffi deleted the ace-for-pyteal branch February 6, 2023 21:42
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.

2 participants