Still working on more test cases but the core functionality for testing ABI Router / ARC-4 compliant programs is in place.
Changes
Issues addressed
5
Bugs Fixed
Fixed a bug in dry run execution which made all app calls run as if during app creation
Python Support
Require 3.9 (previously 3.8 was supported)
New Functionality
Makefile - run mypy with --show-error-codes
graviton/abi_strategy.py - introducing class RandomABIStrategyHalfSized which allows restricting integers to ½ their bit size for testing multiplication and addition without oveflow
graviton/blackbox.py - introducing class ABIContractExecutor which applies the functionality of DryRunExecutor to the case of ABI-routable programs
graviton/invariant.py - make mypy a little happier and other small refactorings. Class Invariant was heavily utilized in the new tests
tests/integration/abi_test.py - adding tests of ABI-routable programs. In particular:
test_method_or_barecall_positive - test all valid method and bare app calls for the contract including consideration of:
is_app_create - simulate the program execution during app creation, or after it already exists as appropriate
on_complete - simulate all appropriate OnComplete actions
args - simulate NUM_ROUTER_DRYRUNS random sets of valid args as inputs for each method (currently NUM_ROUTER_DRYRUNS = 7)
assert that the program passes
assert that the output of each method call is exactly as expected (i.e. sub() actually subtracts, add() adds, etc.)
test_method_or_barecall_negative - test all INVALID method and bare app calls for the contract. In particular:
consider all DISALLOWEDis_app_create and on_complete combinations with all other parameters valid and verify that the app call is rejected with error
remove an argument from an otherwise valid app call and verify that the app call is rejected with error
append an extra argument to an otherwise valid app call and verify that the app call is rejected with error
edit the method selector (arg[0]) by removing/adding/replacing a byte for an otherwise valid app call and verify that the app call is rejected with error
THIS IS NO LONGER CONSIDERED A BUG ~Apart from enforcing txn NumAppArgs > 0, there is no enforcement that the number of arguments provided to a method is the number expected. Strictly speaking, this is not specified in ARC-4 however one can read between the lines in methods and implementing a method and argue that this is the intent. in particular "Invoking a method involves creating an Application call transaction to specifically call that method." seems like it would imply providing exactly the arguments definied in a method's signature.~
~when sub was provided arguments 2837233049, 2497150219, 2497150219 it PASSED instead of rejected (arg[3] was ignored, but should be disallowed IMO)~
~all the other contract methods suffer the same problem. EG add~
~similarly, when all_laid_args which can handle 16 non-selector args by converting the last 2 into a tuple was provided 17 non-selector args, it PASSED. I believe this is a separate issue from the first one which requires a different approach in PyTEAL than the first (i.e., you can't just enforce with txn NumAppArgs; int {NUMBER_OF_ACTUAL_ARGS}; ==; assert)~
Remaining Actions
Tasks
[x] Remove the temporarily_skip_iia variable in tests/integration/abi_test.py and start failing that test again
[x] Create a PyTEAL issue regarding more efficient scratch slot assignments in the ABI-Router. We can see here that even though sub and add do not depend on each-other, they enforce the usage of separate scratch slots. For a contract with many methods, this could result in needlessly running out of available slots. Please see PyTEAL issue 390.
[x] ~Add test_bad_method_selectors to verifies that ABI-router program rejects with error invalid method selectors~ (this is actually covered by test_method_or_barecall_negative's "edit the method selector..." scenarios)
[x] ~Test that REJECT with error or assert when use arg[0] different from the selector of any contract method - "If an Application is called with greater than zero Application call arguments (NOT a bare Application call), the Application MUST always treat the first argument as a method selector and invoke the specified method, regardless of the OnCompletion action of the Application call. This applies to Application creation transactions as well, where the supplied Application ID is 0."~ (Similarly to the previous, this is also covered by test_method_or_barecall_negative's "edit the method selector..." scenarios)
[x] Add graviton ABI Routing functionality in PyTEAL via ABIContractExecutor and use it to test more realistic programs such as. This is punted to PyTEAL issue #394
[ ] Ben's demo abi which includes reference and transaction type method arguments
[x] After addressing the related question below and assuming a relevant answer here, ~we would need to add test_barecall_for_action_implies_methods_allowed: test that if a bare app call is allowed for a particular is_app_create, on_complete combination, then all the contract methods can be called for such combo. This guidance is provided in ARC-4 according to: "If a Contract elects to allow bare Application calls for some OnCompletion actions, then that Contract SHOULD also allow any of its methods to be called with those OnCompletion actions, as long as this would not cause undesirable or nonsensical behavior."~ UPSHOT: the answer is back and it's "NO - not right now". That is, we are not holding up release on account of this not being implemented. Hence, testing of this functionality is on hold for now.
Questions
ARC-4
[x] Should strict num-args enforcement be added to ARC-4? - NO
PyTEAL
[x] Should we strictly enforce the number of arguments in PyTEAL's ABI-Router implementation? (A counter argument is that it is useful to allow optional extra undocumented arguments for debugging and/or admin purposes... but I can imagine counter-counter arguments as well). ANSWER: probably no, as the SDK's likely enforce this. @tzaffi commits to investigating further and writing a cucumber test asserting of this. UPDATE: here is the resulting ticket: https://github.com/algorand/algorand-sdk-testing/issues/190
[x] Should we enforce and/or allow the following in PyTEAL? According to ARC-4, "If a Contract elects to allow bare Application calls for some OnCompletion actions, then that Contract SHOULD also allow any of its methods to be called with those OnCompletion actions, as long as this would not cause undesirable or nonsensical behavior." However, this is not the current behavior of PyTEAL generated ABI-Routable programs. In particular, we can see in this branch that there is an allowable bare app call for opting in but only 2 of the methods (empty_return_subroutine() and log_1()) is allowed for opting in. ANSWER: Ideally, we should follow ARC-4 and allow method calls right after an opt-in bare app call, and right before close-out bare app calls, but this is not what is currently supported. Currently, we don't compose bare-app calls with method calls. This should not hold up release, however it is important that we be able to upgrade the router and -in order to enable this in the smart contract space- we need to introduce strict ABI Router versioning. @michaeldiamant will draft a story around this. TLDR; the answer "NO - not right now"
[x] Should an ABI Router generated clear state program explicitly enforce txn OnCompletion; int ClearState; ==; assert at the beginning? We can see that this isn't currently the case. - NO however, we should probably make it very easy to generate clear state programs whose entire contents is int 1
[x] Should we allow/encourage/mandate ABI Router-type checking in PyTEAL? I.e., should a PyTEAL ABI Subroutine enforce that its received arguments are of the type expected in the strictest way possible ? (I would argue not at this time because it's a big can of worms with definite downsides, but it's worthy of discussion and possibly a new ticket/issue). ANSWER: We should have a way to automatically inject type checking for ABI types, though we may not want to make this the default. @jasonpaulos has thought of the idea of adding a verify() method on ABI types.
Future Work - cf #22
[ ] Determine if dry runs can be made to handle transaction groups
[ ] If the determination of the previous is "yes", enable graviton to handle such transaction groups and then use to test abi methods with transaction arguments. In particular, it should verify that when the expected pay / axfr / ... transactions are too far away (compared to the number of such args in the method signature) the program is rejected with error.
Graviton ABI-Router Functionality
Still working on more test cases but the core functionality for testing ABI Router / ARC-4 compliant programs is in place.
Changes
Issues addressed
5
Bugs Fixed
Python Support
New Functionality
Makefile
- runmypy
with--show-error-codes
graviton/abi_strategy.py
- introducing classRandomABIStrategyHalfSized
which allows restricting integers to ½ their bit size for testing multiplication and addition without oveflowgraviton/blackbox.py
- introducing classABIContractExecutor
which applies the functionality ofDryRunExecutor
to the case of ABI-routable programsgraviton/invariant.py
- makemypy
a little happier and other small refactorings. ClassInvariant
was heavily utilized in the new teststests/integration/abi_test.py
- adding tests of ABI-routable programs. In particular:test_method_or_barecall_positive
- test all valid method and bare app calls for the contract including consideration of:is_app_create
- simulate the program execution during app creation, or after it already exists as appropriateon_complete
- simulate all appropriate OnComplete actionsargs
- simulate NUM_ROUTER_DRYRUNS random sets of validargs
as inputs for each method (currentlyNUM_ROUTER_DRYRUNS = 7
)sub()
actually subtracts,add()
adds, etc.)test_method_or_barecall_negative
- test all INVALID method and bare app calls for the contract. In particular:is_app_create
andon_complete
combinations with all other parameters valid and verify that the app call is rejected with errorarg[0]
) by removing/adding/replacing a byte for an otherwise valid app call and verify that the app call is rejected with errorABI-Router Bugs? Catalog
Approval Program Routing
These (alleged) bugs were discovered for the approval ABI contract and program generated by PyTeal in
compiler_test.py
as _router_with_octxn NumAppArgs > 0
, there is no enforcement that the number of arguments provided to a method is the number expected. Strictly speaking, this is not specified in ARC-4 however one can read between the lines in methods and implementing a method and argue that this is the intent. in particular "Invoking a method involves creating an Application call transaction to specifically call that method." seems like it would imply providing exactly the arguments definied in a method's signature.~sub
was provided arguments2837233049, 2497150219, 2497150219
it PASSED instead of rejected (arg[3]
was ignored, but should be disallowed IMO)~all_laid_args
which can handle 16 non-selector args by converting the last 2 into a tuple was provided 17 non-selector args, it PASSED. I believe this is a separate issue from the first one which requires a different approach in PyTEAL than the first (i.e., you can't just enforce withtxn NumAppArgs; int {NUMBER_OF_ACTUAL_ARGS}; ==; assert
)~Remaining Actions
Tasks
temporarily_skip_iia
variable intests/integration/abi_test.py
and start failing that test againsub
andadd
do not depend on each-other, they enforce the usage of separate scratch slots. For a contract with many methods, this could result in needlessly running out of available slots. Please see PyTEAL issue 390.test_bad_method_selectors
to verifies that ABI-router program rejects with error invalid method selectors~ (this is actually covered bytest_method_or_barecall_negative
's "edit the method selector..." scenarios)arg[0]
different from the selector of any contract method - "If an Application is called with greater than zero Application call arguments (NOT a bare Application call), the Application MUST always treat the first argument as a method selector and invoke the specified method, regardless of the OnCompletion action of the Application call. This applies to Application creation transactions as well, where the supplied Application ID is 0."~ (Similarly to the previous, this is also covered bytest_method_or_barecall_negative
's "edit the method selector..." scenarios)@ABIReturnSubroutine
conditional_factorial - this is being addressed in PyTEAL PR #391, and in particular in this test method.ABIContractExecutor
and use it to test more realistic programs such as. This is punted to PyTEAL issue #394test_barecall_for_action_implies_methods_allowed
: test that if a bare app call is allowed for a particularis_app_create, on_complete
combination, then all the contract methods can be called for such combo. This guidance is provided in ARC-4 according to: "If a Contract elects to allow bare Application calls for some OnCompletion actions, then that Contract SHOULD also allow any of its methods to be called with those OnCompletion actions, as long as this would not cause undesirable or nonsensical behavior."~ UPSHOT: the answer is back and it's "NO - not right now". That is, we are not holding up release on account of this not being implemented. Hence, testing of this functionality is on hold for now.Questions
ARC-4
PyTEAL
empty_return_subroutine()
andlog_1()
) is allowed for opting in. ANSWER: Ideally, we should follow ARC-4 and allow method calls right after anopt-in
bare app call, and right beforeclose-out
bare app calls, but this is not what is currently supported. Currently, we don't compose bare-app calls with method calls. This should not hold up release, however it is important that we be able to upgrade the router and -in order to enable this in the smart contract space- we need to introduce strict ABI Router versioning. @michaeldiamant will draft a story around this. TLDR; the answer "NO - not right now"txn OnCompletion; int ClearState; ==; assert
at the beginning? We can see that this isn't currently the case. - NO however, we should probably make it very easy to generate clear state programs whose entire contents isint 1
verify()
method on ABI types.Future Work - cf #22
pay / axfr / ...
transactions are too far away (compared to the number of such args in the method signature) the program is rejected with error.