ABI Method / Routing / Contract functionality

[tzaffi]

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

• Better assertion message for invariant predicates of 2 variables #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 DISALLOWED is_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

ABI-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_oc

1. 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

• Remove the temporarily_skip_iia variable in tests/integration/abi_test.py and start failing that test again
• 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.
• 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)
• 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)
• Test the @ABIReturnSubroutine conditional_factorial - this is being addressed in PyTEAL PR #391, and in particular in this test method.
• test _router_with_oc's clear state program expected_csp_with_oc
• test approval and clear state programs for _router_without_oc
• 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
  • Ben's Priority Queue
• 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

• Should strict num-args enforcement be added to ARC-4? - NO

PyTEAL

• 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: Assert that Atomic Transaction Composer's method adder checks for arg-length consistency algorand-sdk-testing#190
• 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"
• 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
• 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.

[tzaffi]

An upgrade to go-algorand's dry run is returning logs even when rejecting.

[michaeldiamant]

@tzaffi Thanks for the effort here to improve our confidence in PyTeal-generated ABI routing! ☕

We'll extend the effort via these stories in future iterations:

• ABI: Add Router routing graviton tests pyteal#394
• Transactions Groups in Graviton #22

[tzaffi]

Changed this to point to develop but still should not merge until the next release so that pull from PyPi

[tzaffi]

py-algorand-sdk v 1.15.0 is out, so I'll revert and merge very soon.