Implement new step asserting that AtomicTransactionComposer's attempt to add a method can fail with a particular error #347

tzaffi · 2022-06-14T18:02:29Z

Implementing the following step:

I add a method call with the signing account, the current application, suggested params, on complete "noop", current transaction signer, current method arguments; any resulting exception has key "{none-or-exception-key}".

Related Issues and PR's

Assert that Atomic Transaction Composer's method adder checks for arg-length consistency algorand-sdk-testing#190
Assert that Atomic Transaction Composer's method adder checks for arg-length consistency algorand-sdk-testing#189

…on..."

test/steps/application_v2_steps.py

…add a method call..."

…ld have been

tzaffi · 2022-06-14T21:33:34Z

run_integration.sh

@@ -7,7 +7,7 @@ pushd $rootdir

 # Reset test harness
 rm -rf test-harness
-git clone --single-branch --branch master https://github.com/algorand/algorand-sdk-testing.git test-harness
+git clone --single-branch --branch abi-too-many-args https://github.com/algorand/algorand-sdk-testing.git test-harness


TODO: revert to point back to master before merging this PR.

tzaffi · 2022-06-14T21:34:42Z

test/steps/application_v2_steps.py

@@ -85,16 +89,32 @@ def s512_256_uint64(witness):
    return int.from_bytes(encoding.checksum(witness)[:8], "big")


+@step(


moved unchanged from other_v2_steps after I realized that this is meant for application transactions

tzaffi · 2022-06-14T21:35:16Z

test/steps/application_v2_steps.py

@@ -159,8 +179,13 @@ def lookup_application_include_all2(
        context.response = json.loads(str(e))


+@when("we make a LookupApplications call with applicationID {app_id}")


moved closer to similar function

tzaffi · 2022-06-14T21:35:32Z

test/steps/application_v2_steps.py

 @when("I use {indexer} to lookup application with {application_id}")
-def lookup_application(context, indexer, application_id):
+def lookup_application2(context, indexer, application_id):


renamed to avoid method name collision

tzaffi · 2022-06-14T21:38:12Z

test/steps/application_v2_steps.py

@@ -512,35 +537,37 @@ def add_transaction_to_composer(context):
    )


-def process_abi_args(context, method, arg_tokens):
+def process_abi_args(context, method, arg_tokens, erase_empty_tokens=True):
+    if erase_empty_tokens:


Now that we no longer assume that the lengths of the method args and actual args are the same, a couple of side effects occurred. One side effect is that when "".split() is called on an empty string, you get a non-empty list. As in all existing cases, empty strings that are split were meant to indicate no values at all, so this change shouldn't break any tests.

I'm a little confused. Is the cause of this change that the step I append the encoded arguments "<app-args>" to the method arguments array now has to work when <app-args> is the empty string?

Or is there another cause?

Previously, all tests had exactly the same number of runtime args as defined in the method signature. The step logic iterated through the method signature args and accessed the run-time arg with index corresponding to the signature arg. When there were no method signature args, the loop was actually trivial (see the image below). However, in that trivial case, args was actually the list of length 1 containing an empty string [''] (because of how python split works):

>>> "".split(",") ['']

Now that we are no longer assuming that the number of args are the same, the 0 vs. 1 case no longer was correct so I needed to adjust the logic using this hack.

So yes, it has to work with empty string (which before was actually ignored in a sense).

Thanks for that explanation, I think I understand now. Alternatively, would it be an acceptable solution to modify the I append the encoded arguments "<app-args>" to the method arguments array step to check if <app-args> is an empty string and do nothing in that case?

The reason I suggest this is because we can semantically still pass empty string args with something like a,,b, and this would be ignored by the current code, but I think it should complain/error for this input.

yeah, you'd have to set erase_empty_tokens to false... still thinking about your first paragraph

I have a proposal. Rename this step to:

'I add a method call with the {account_type} account, the current application, suggested params, on complete "{operation}", current transaction signer, current method arguments ignoring empty ones; any resulting exception has key "{exception_key}".'

the phrase "ignoring empty ones" indicates that we should not be allowing empty arguments for this step. And in fact, this step will eventually call process_abi_args(... erase_empty_tokens=True) while all the other steps will continue as before and therefor have erase_empty_tokens=False

Sounds good?

That's better, but it still looks like we're addressing the symptoms of the problem, not the root cause. I'd find it simpler if no filtering of arguments needed to be done after the fact, since this seems error-prone and hard to reason about.

I'm trying your original suggestion with I append the encoded arguments. Seems to work. We'll see if all the tests pass.

tzaffi · 2022-06-14T21:39:50Z

test/steps/other_v2_steps.py

@@ -1406,27 +1406,6 @@ def algod_v2_client(context):
    context.app_acl = algod.AlgodClient(daemon_token, algod_address)


-@step(
-    'I sign and submit the transaction, saving the txid. If there is an error it is "{error_string:MaybeString}".'


moved this to application_v2_steps.py

…ray and stop erasing args

run_integration.sh

jasonpaulos

Looks good

Imlement new step "the most recently added method call has an excepti…

7ab587c

…on..."

tzaffi changed the title ~~Imlement new step "the most recently added method call has an exception"~~ Implement new step "the most recently added method call has an exception" Jun 14, 2022

tzaffi changed the title ~~Implement new step "the most recently added method call has an exception"~~ Implement new step "the most recently added method call has an exception..." Jun 14, 2022

jasonpaulos reviewed Jun 14, 2022

View reviewed changes

test/steps/application_v2_steps.py Outdated Show resolved Hide resolved

tzaffi added the Team Scytale label Jun 14, 2022

Zeph Grunschlag added 2 commits June 14, 2022 14:49

Per CR suggestion, make another variant with exception handler of "I …

c5f282b

…add a method call..."

forgot to include the case when there was no exception but there shou…

9bf3149

…ld have been

tzaffi mentioned this pull request Jun 14, 2022

Assert that Atomic Transaction Composer's method adder checks for arg-length consistency algorand/algorand-sdk-testing#190

Open

tzaffi commented Jun 14, 2022

View reviewed changes

tzaffi requested review from algochoi and jasonpaulos June 14, 2022 21:41

per cr suggestions

aaa7f77

tzaffi changed the title ~~Implement new step "the most recently added method call has an exception..."~~ Implement new step asserting that AtomicTransactionComposer's attempt to add a method can fail with a particular error Jun 16, 2022

Zeph Grunschlag added 2 commits June 17, 2022 14:42

modify append_app_args... to interpret the empty string as empty ar…

9ad1a76

…ray and stop erasing args

Merge remote-tracking branch 'origin' into abi-too-many-args-steps

608ebf1

tzaffi commented Jun 17, 2022

View reviewed changes

run_integration.sh Outdated Show resolved Hide resolved

Update run_integration.sh

7d79134

jasonpaulos approved these changes Jun 17, 2022

View reviewed changes

tzaffi merged commit 989893a into develop Jun 17, 2022

tzaffi deleted the abi-too-many-args-steps branch June 17, 2022 21:29

algojack mentioned this pull request Jul 22, 2022

FOR REVIEW ONLY - py-algorand-sdk v1.16.0 #364

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement new step asserting that AtomicTransactionComposer's attempt to add a method can fail with a particular error #347

Implement new step asserting that AtomicTransactionComposer's attempt to add a method can fail with a particular error #347

tzaffi commented Jun 14, 2022 •

edited

tzaffi Jun 14, 2022

tzaffi Jun 14, 2022 •

edited

tzaffi Jun 14, 2022

tzaffi Jun 14, 2022

tzaffi Jun 14, 2022 •

edited

jasonpaulos Jun 17, 2022

tzaffi Jun 17, 2022

tzaffi Jun 17, 2022

jasonpaulos Jun 17, 2022

tzaffi Jun 17, 2022

tzaffi Jun 17, 2022

jasonpaulos Jun 17, 2022

tzaffi Jun 17, 2022 •

edited

tzaffi Jun 14, 2022

jasonpaulos left a comment

		@@ -85,16 +89,32 @@ def s512_256_uint64(witness):
		return int.from_bytes(encoding.checksum(witness)[:8], "big")


		@step(

		@@ -159,8 +179,13 @@ def lookup_application_include_all2(
		context.response = json.loads(str(e))


		@when("we make a LookupApplications call with applicationID {app_id}")

Implement new step asserting that AtomicTransactionComposer's attempt to add a method can fail with a particular error #347

Implement new step asserting that AtomicTransactionComposer's attempt to add a method can fail with a particular error #347

Conversation

tzaffi commented Jun 14, 2022 • edited

Implementing the following step:

Related Issues and PR's

Choose a reason for hiding this comment

tzaffi Jun 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tzaffi Jun 14, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tzaffi Jun 17, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jasonpaulos left a comment

Choose a reason for hiding this comment

tzaffi commented Jun 14, 2022 •

edited

tzaffi Jun 14, 2022 •

edited

tzaffi Jun 14, 2022 •

edited

tzaffi Jun 17, 2022 •

edited