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

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

Merged
merged 7 commits into from
Jun 17, 2022
2 changes: 1 addition & 1 deletion run_integration.sh
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

tzaffi marked this conversation as resolved.
Show resolved Hide resolved

## Copy feature files into the project resources
mkdir -p test/features
Expand Down
115 changes: 80 additions & 35 deletions test/steps/application_v2_steps.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,11 @@

from algosdk import abi, atomic_transaction_composer, encoding, mnemonic
from algosdk.abi.contract import NetworkInfo
from algosdk.error import ABITypeError, IndexerHTTPError
from algosdk.error import (
ABITypeError,
IndexerHTTPError,
AtomicTransactionComposerError,
)
from algosdk.future import transaction

from test.steps.other_v2_steps import read_program
Expand Down Expand Up @@ -85,16 +89,32 @@ def s512_256_uint64(witness):
return int.from_bytes(encoding.checksum(witness)[:8], "big")


@step(
Copy link
Contributor Author

@tzaffi tzaffi Jun 14, 2022

Choose a reason for hiding this comment

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

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

'I sign and submit the transaction, saving the txid. If there is an error it is "{error_string:MaybeString}".'
)
def sign_submit_save_txid_with_error(context, error_string):
try:
signed_app_transaction = context.app_transaction.sign(
context.transient_sk
)
context.app_txid = context.app_acl.send_transaction(
signed_app_transaction
)
except Exception as e:
if not error_string or error_string not in str(e):
raise RuntimeError(
"error string "
+ error_string
+ " not in actual error "
+ str(e)
)


@when("we make a GetApplicationByID call for applicationID {app_id}")
def application_info(context, app_id):
context.response = context.acl.application_info(int(app_id))


@when("we make a LookupApplications call with applicationID {app_id}")
def lookup_application(context, app_id):
context.response = context.icl.applications(int(app_id))


@when(
'we make a LookupApplicationLogsByID call with applicationID {app_id} limit {limit} minRound {min_round} maxRound {max_round} nextToken "{next_token:MaybeString}" sender "{sender:MaybeString}" and txID "{txid:MaybeString}"'
)
Expand Down Expand Up @@ -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}")
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 closer to similar function

def lookup_application(context, app_id):
context.response = context.icl.applications(int(app_id))


@when("I use {indexer} to lookup application with {application_id}")
def lookup_application(context, indexer, application_id):
def lookup_application2(context, indexer, application_id):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed to avoid method name collision

context.response = context.icls[indexer].applications(
application_id=int(application_id)
)
Expand Down Expand Up @@ -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:
Copy link
Contributor Author

@tzaffi tzaffi Jun 14, 2022

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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 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?

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@tzaffi tzaffi Jun 17, 2022

Choose a reason for hiding this comment

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

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

arg_tokens = [tok for tok in arg_tokens if tok]
method_args = []
for arg_index, arg in enumerate(method.args):
# Skip arg if it does not have a type
for arg_index, arg_token in enumerate(arg_tokens):
if arg_index >= len(method.args):
method_args.append(arg_token)
continue

arg = method.args[arg_index]
if isinstance(arg.type, abi.ABIType):
method_arg = arg.type.decode(
base64.b64decode(arg_tokens[arg_index])
)
method_arg = arg.type.decode(base64.b64decode(arg_token))
method_args.append(method_arg)
elif arg.type == abi.ABIReferenceType.ACCOUNT:
method_arg = abi.AddressType().decode(
base64.b64decode(arg_tokens[arg_index])
)
method_arg = abi.AddressType().decode(base64.b64decode(arg_token))
method_args.append(method_arg)
elif (
arg.type == abi.ABIReferenceType.APPLICATION
or arg.type == abi.ABIReferenceType.ASSET
):
parts = arg_tokens[arg_index].split(":")
parts = arg_token.split(":")
if len(parts) == 2 and parts[0] == "ctxAppIdx":
method_arg = context.app_ids[int(parts[1])]
else:
method_arg = abi.UintType(64).decode(
base64.b64decode(arg_tokens[arg_index])
base64.b64decode(arg_token)
)
method_args.append(method_arg)
else:
# Append the transaction signer as is
method_args.append(arg_tokens[arg_index])
method_args.append(arg_token)
return method_args


Expand Down Expand Up @@ -621,28 +648,46 @@ def int_if_given(given):
app_args = process_abi_args(
context, context.abi_method, context.method_args
)
context.app_args = app_args
note = None
if force_unique_transactions:
note = (
b"I should be unique thanks to this nonce: "
+ context.nonce.encode()
)

context.atomic_transaction_composer.add_method_call(
app_id=app_id,
method=context.abi_method,
sender=sender,
sp=context.suggested_params,
signer=context.transaction_signer,
method_args=app_args,
on_complete=operation_string_to_enum(operation),
local_schema=local_schema,
global_schema=global_schema,
approval_program=approval_program,
clear_program=clear_program,
extra_pages=extra_pages,
note=note,
)
context.most_recent_added_method_exception = None
try:
context.atomic_transaction_composer.add_method_call(
app_id=app_id,
method=context.abi_method,
sender=sender,
sp=context.suggested_params,
signer=context.transaction_signer,
method_args=app_args,
on_complete=operation_string_to_enum(operation),
local_schema=local_schema,
global_schema=global_schema,
approval_program=approval_program,
clear_program=clear_program,
extra_pages=extra_pages,
note=note,
)
except AtomicTransactionComposerError as atce:
context.most_recent_added_method_exception = atce


@then(
'the most recently added method call has an exception which satisfies "{regex}".'
tzaffi marked this conversation as resolved.
Show resolved Hide resolved
)
def most_recently_added_method_exception_satisfies(context, regex):
most_recent_exception = context.most_recent_added_method_exception
if regex == "none":
assert most_recent_exception is None
return
assert re.search(
regex, str(most_recent_exception)
), f"{most_recent_exception} did not satisfy {regex}. FYI: method_args={context.method_args}; app_args={context.app_args}"


@step(
Expand Down Expand Up @@ -827,7 +872,7 @@ def serialize_method_to_json(context):
@when(
'I create the Method object with name "{method_name}" method description "{method_desc}" first argument type "{first_arg_type}" first argument description "{first_arg_desc}" second argument type "{second_arg_type}" second argument description "{second_arg_desc}" and return type "{return_arg_type}"'
)
def create_method_from_test_with_arg_name(
def create_method_from_test_with_arg_name_and_desc(
context,
method_name,
method_desc,
Expand Down
23 changes: 1 addition & 22 deletions test/steps/other_v2_steps.py
Original file line number Diff line number Diff line change
Expand Up @@ -604,7 +604,7 @@ def txns_by_addr(
@when(
'we make a Lookup Account Transactions call against account "{account:MaybeString}" with NotePrefix "{notePrefixB64:MaybeString}" TxType "{txType:MaybeString}" SigType "{sigType:MaybeString}" txid "{txid:MaybeString}" round {block} minRound {minRound} maxRound {maxRound} limit {limit} beforeTime "{beforeTime:MaybeString}" afterTime "{afterTime:MaybeString}" currencyGreaterThan {currencyGreaterThan} currencyLessThan {currencyLessThan} assetIndex {index}'
)
def txns_by_addr(
def txns_by_addr2(
context,
account,
notePrefixB64,
Expand Down Expand Up @@ -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}".'
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 this to application_v2_steps.py

)
def sign_submit_save_txid_with_error(context, error_string):
try:
signed_app_transaction = context.app_transaction.sign(
context.transient_sk
)
context.app_txid = context.app_acl.send_transaction(
signed_app_transaction
)
except Exception as e:
if not error_string or error_string not in str(e):
raise RuntimeError(
"error string "
+ error_string
+ " not in actual error "
+ str(e)
)


@when('I compile a teal program "{program}"')
def compile_step(context, program):
data = load_resource(program)
Expand Down