ApeWorX / ape

The smart contract development tool for Pythonistas, Data Scientists, and Security Professionals
https://apeworx.io
Apache License 2.0
889 stars 131 forks source link

Can't use Union type for converter #1220

Closed fubuloubu closed 1 year ago

fubuloubu commented 1 year ago

Environment information

$ ape --version
0.5.10.dev9+gb57d4a9b

$ ape plugins list
Installed Plugins:
  infura       0.5.3
  ens          0.5.1
  etherscan    0.5.4
  foundry      0.5.2
  ledger       0.5.0

What went wrong?

When making a call with `my_contract.myMethod(..., gas_limit=), it fails to process because it is asking the conversion plugin system to convert to a Union type, which is not possible:

File ~/.pyenv/versions/3.9.14/envs/ape-safe/lib/python3.9/site-packages/ape/contracts/base.py:336, in ContractTransactionHandler.__call__(self, *args, **kwargs)
    334 function_arguments = self._convert_tuple(args)
    335 contract_transaction = self._as_transaction(*function_arguments)
--> 336 return contract_transaction(*function_arguments, **kwargs)

File ~/.pyenv/versions/3.9.14/envs/ape-safe/lib/python3.9/site-packages/ape/contracts/base.py:274, in ContractTransaction.__call__(self, *args, **kwargs)
    273 def __call__(self, *args, **kwargs) -> ReceiptAPI:
--> 274     txn = self.serialize_transaction(*args, **kwargs)
    276     if "sender" in kwargs and isinstance(kwargs["sender"], AccountAPI):
    277         return kwargs["sender"].call(txn, **kwargs)

File ~/.pyenv/versions/3.9.14/envs/ape-safe/lib/python3.9/site-packages/ape/contracts/base.py:268, in ContractTransaction.serialize_transaction(self, *args, **kwargs)
    264 if "sender" in kwargs and isinstance(kwargs["sender"], ContractInstance):
    265     # Automatically impersonate contracts (if API available) when sender
    266     kwargs["sender"] = self.account_manager.test_accounts[kwargs["sender"].address]
--> 268 kwargs = _convert_kwargs(kwargs, self.conversion_manager.convert)
    269 return self.provider.network.ecosystem.encode_transaction(
    270     self.address, self.abi, *args, **kwargs
    271 )

File ~/.pyenv/versions/3.9.14/envs/ape-safe/lib/python3.9/site-packages/ape/contracts/base.py:24, in _convert_kwargs(kwargs, converter)
     22 fields = TransactionAPI.__fields__
     23 kwargs_to_convert = {k: v for k, v in kwargs.items() if k == "sender" or k in fields}
---> 24 converted_fields = {
     25     k: converter(
     26         v,
     27         # TODO: Upstream, `TransactionAPI.sender` should be `AddressType` (not `str`)
     28         AddressType if k == "sender" else fields[k].type_,
     29     )
     30     for k, v in kwargs_to_convert.items()
     31 }
     32 return {**kwargs, **converted_fields}

File ~/.pyenv/versions/3.9.14/envs/ape-safe/lib/python3.9/site-packages/ape/contracts/base.py:25, in <dictcomp>(.0)
     22 fields = TransactionAPI.__fields__
     23 kwargs_to_convert = {k: v for k, v in kwargs.items() if k == "sender" or k in fields}
     24 converted_fields = {
---> 25     k: converter(
     26         v,
     27         # TODO: Upstream, `TransactionAPI.sender` should be `AddressType` (not `str`)
     28         AddressType if k == "sender" else fields[k].type_,
     29     )
     30     for k, v in kwargs_to_convert.items()
     31 }
     32 return {**kwargs, **converted_fields}

File ~/.pyenv/versions/3.9.14/envs/ape-safe/lib/python3.9/site-packages/ape/managers/converters.py:270, in ConversionManager.convert(self, value, type)
    268 if type not in self._converters:
    269     options = ", ".join([t.__name__ for t in self._converters])
--> 270     raise ConversionError(f"Type '{type}' must be one of [{options}].")
    272 elif self.is_type(value, type) and not isinstance(value, (list, tuple)):
    273     # NOTE: Always process lists and tuples
    274     return value

ConversionError: Type 'typing.Union[typing.Literal['auto', 'max'], int, str, NoneType]' must be one of [ChecksumAddress, bytes, int, Decimal, list, tuple].

This is because currently TransactionAPI.gas_limit is of type ape.types.GasLimit, which is a Union:

https://github.com/ApeWorX/ape/blob/b57d4a9b16b71903a8f76319e245e1f50a87dcd3/src/ape/api/transactions.py#L33

https://github.com/ApeWorX/ape/blob/b57d4a9b16b71903a8f76319e245e1f50a87dcd3/src/ape/types/__init__.py#L55

How can it be fixed?

Easy fix is that gas_limit should just be an int type, and probably go through and ensure all types of any BaseInterfaceModel subclass are basic Python types (and not Unions) since the intent behind these models is they are supposed to be indexable for caching solutions (although strictly speaking, TransactionAPI is not supposed to be cached, ReceiptAPI is)

Or alternatively, expand the conversion plugin system to work with Union types by adding something like the following psuedo code to here:

if instance(type, Union) and isinstance(value, type.options):
    return value

NOTE: the above code does not work, but to sketch an idea

milkyklim commented 1 year ago

Talked to @fubuloubu, seems like it is bigger issue, calling function as get_values(3, gas_limit="max") throws, too.

E           ape.exceptions.ConversionError: Type 'typing.Union[typing.Literal['auto', 'max'], int, str, NoneType]' must be one of [ChecksumAddress, bytes, int, Decimal, list, tuple]