Repository navigation
Wrong and confusing type annotations #318
Description
The wrong type annotation was used in the uniswap.uniswap.Uniswap class. The parameters provider, web3, factory_contract_addr and router_contract_addr of the constructor default to None but are not declared as Optional. This is very unfriendly to static type checkers (such as Pyright with strict type checking enabled). And it will generate a lot of type error prompts, but actually no problem.
class Uniswap:
"""
Wrapper around Uniswap contracts.
"""
...
def __init__(
self,
address: Union[AddressLike, str, None],
private_key: Optional[str],
provider: str = None,
web3: Web3 = None,
version: int = 1,
default_slippage: float = 0.01,
use_estimate_gas: bool = True,
# use_eip1559: bool = True,
factory_contract_addr: str = None,
router_contract_addr: str = None,
enable_caching: bool = False,
) -> None:
...At the same time, the type annotation format of address and private_key is confusing. In Python3.9 and below, the recommended declaration method is Optional[T] to indicate that the parameter can be None. And in 3.10+, its recommended to use T | None(also Union[T, None]). But here we can see the two different formats of type annotations are being used at the same time, which is confusing.
I suggest changing it to the following:
class Uniswap:
"""
Wrapper around Uniswap contracts.
"""
...
def __init__(
self,
address: Optional[Union[AddressLike, str]],
private_key: Optional[str],
provider: Optional[str] = None,
web3: Optional[Web3] = None,
version: int = 1,
default_slippage: float = 0.01,
use_estimate_gas: bool = True,
# use_eip1559: bool = True,
factory_contract_addr: Optional[str] = None,
router_contract_addr: Optional[str] = None,
enable_caching: bool = False,
) -> None:
...Also, I noticed that some function parameters/return values also have such annotations. Returns possible None but no Optional is declared. This will confuse the user for wasting more time debugging the type.
Activity
ErikBjare commentedon Feb 24, 2023
Thanks for notifying me.
These annotations used to be valid, as CI has been running passing typechecking for a while.
However, recently typecheckers like mypy (not only pylance) have started enforcing no-implicit-optional, which I remedied in this PR along with other typing issues reported with newer mypy: #320