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

Fix assertion error when calling the cdps getter of the Stabilization contract #45

Merged
merged 2 commits into from
Mar 5, 2024

Conversation

szemate
Copy link
Contributor

@szemate szemate commented Feb 29, 2024

Fixes AssertionError from the

assert isinstance(return_value, tuple)

assertion when calling the cdps getter of the Stabilization contract.

The reason for failing the assertion is that for this call the contract
function wrapper returns a list instead of the expected tuple.
To work around it, cast the return value to a tuple.

Note that according to web3.py docs1, the function wrapper's call()
method "returns the return value of the executed function" i.e. there is
no guarantee that the return value is either a list or a tuple, however
I don't think we should worry about it until we run into similar errors.

Fixes #37.

Fixes `AssertionError` from the

    assert isinstance(return_value, tuple)

assertion when calling the `cdps` getter of the Stabilization contract.

The reason for failing the assertion is that for this call the contract
function wrapper returns a list instead of the expected tuple.
To work around it, cast the return value to a tuple.

Note that according to web3.py docs[1], the function wrapper's call()
method "returns the return value of the executed function" i.e. there is
no guarantee that the return value is either a list or a tuple, however
I don't think we should worry about it until we run into similar errors.

[1]: https://web3py.readthedocs.io/en/stable/web3.contract.html#web3.contract.ContractFunction.call

Fixes #37.
The previous docstring was unhelpful in understanding what the function
does.
@aiman
Copy link
Contributor

aiman commented Feb 29, 2024

Do we know why we are getting back a list instead of a tuple? I think it is worth investigating. Otherwise, we won't be able to detect if there is a problem since we don't know what the expected behaviour of Web3 should be.

This is a good starting point: https://github.com/ethereum/web3.py/blob/e94e5e8cf79eed324534fdf77127a49545aba20a/web3/contract/utils.py#L66

@szemate szemate changed the title Fix assertion error when calling the cdps getter of the Stabilization contract. Fix assertion error when calling the cdps getter of the Stabilization contract Feb 29, 2024
@szemate
Copy link
Contributor Author

szemate commented Mar 1, 2024

Do we know why we are getting back a list instead of a tuple?

I've done the investigation.

The contract function call is performed by call_contract_function from which we should focus on the part:

    try:
        output_data = w3.codec.decode(output_types, return_data)
    except DecodingError as e:
        ...

    _normalizers = itertools.chain(
        BASE_RETURN_NORMALIZERS,
        normalizers,
    )
    normalized_data = map_abi_data(_normalizers, output_types, output_data)

It first calls w3.codec.decode that has the return type Tuple[Any, ...] i.e. we could assume that the return type is always a tuple.

However, map_abi_data passes the output data through a chain of normalizer functions, of which the first one, abi_data_tree converts the tuple into a list (has return type List[Any]).

Therefore, if the contract function has multiple outputs, the expected behaviour of Web3 is that the return type is a list.

I could verify this by performing other contract calls; e.g. calling the config function of the Autonity contract as:

aut -v contract call --abi ../autonity.py/autonity/abi/Autonity.abi --address 0xBd770416a3345F91E4B34576cb804a576fa48EB1 config

also results in assertion error.

@szemate
Copy link
Contributor Author

szemate commented Mar 1, 2024

Anyway, it seems to me that the entire decoding logic in autonity.abi_parser is redundant with built-in Web3 functionality.

Web3.Contract objects have a Contract.decode_tuples flag (default False):

If a Tuple/Struct is returned by a contract function, this flag defines whether to apply the field names from the ABI to the returned data.

I'd propose a refactoring that autcli should use the built-in ABI parser instead of our own implementation.

@aiman
Copy link
Contributor

aiman commented Mar 5, 2024

Great investigation, thanks!

I'd propose a refactoring that autcli should use the built-in ABI parser instead of our own implementation.

This sounds like a good proposal, let's discuss it soon.

@aiman aiman merged commit 502eef8 into develop Mar 5, 2024
0 of 8 checks passed
@aiman aiman deleted the fix-tuple-assertion-error branch March 5, 2024 08:50
@aiman aiman restored the fix-tuple-assertion-error branch March 5, 2024 10:41
@aiman aiman deleted the fix-tuple-assertion-error branch March 5, 2024 10:48
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