From 0eb247808c51a9ef724d46f80c44d09b14329230 Mon Sep 17 00:00:00 2001 From: antazoey Date: Fri, 29 Sep 2023 12:36:09 -0500 Subject: [PATCH] fix: issue with custom errors on estimate gas and static fee txns [APE-1421] (#1680) --- src/ape/api/networks.py | 41 +++++++-- src/ape/api/providers.py | 15 +++- src/ape/managers/config.py | 7 +- src/ape/managers/networks.py | 89 ++++++++----------- src/ape/plugins/__init__.py | 9 +- src/ape_ethereum/ecosystem.py | 9 +- tests/conftest.py | 4 +- tests/functional/conftest.py | 22 +++-- tests/functional/geth/test_provider.py | 1 + tests/functional/test_accounts.py | 54 ++++++----- tests/functional/test_config.py | 10 ++- tests/functional/test_contract_container.py | 6 +- tests/functional/test_contract_instance.py | 8 +- tests/functional/test_ecosystem.py | 25 +++++- tests/functional/test_history.py | 3 +- tests/functional/test_network_api.py | 35 ++++++++ ...st_networks.py => test_network_manager.py} | 56 +----------- tests/functional/test_provider.py | 11 +++ tests/integration/cli/test_networks.py | 11 ++- tests/integration/cli/test_test.py | 8 ++ 20 files changed, 249 insertions(+), 175 deletions(-) create mode 100644 tests/functional/test_network_api.py rename tests/functional/{test_networks.py => test_network_manager.py} (77%) diff --git a/src/ape/api/networks.py b/src/ape/api/networks.py index 3252a6cd2a..5d7e9ab40f 100644 --- a/src/ape/api/networks.py +++ b/src/ape/api/networks.py @@ -74,7 +74,7 @@ class EcosystemAPI(BaseInterfaceModel): fee_token_decimals: int = 18 """The number of the decimals the fee token has.""" - _default_network: str = LOCAL_NETWORK_NAME + _default_network: Optional[str] = None def __repr__(self) -> str: return f"<{self.name}>" @@ -254,7 +254,25 @@ def default_network(self) -> str: Returns: str """ - return self._default_network + + if network := self._default_network: + # Was set programatically. + return network + + elif network := self.config.get("default_network"): + # Default found in config. + return network + + elif LOCAL_NETWORK_NAME in self.networks: + # Default to the LOCAL_NETWORK_NAME, at last resort. + return LOCAL_NETWORK_NAME + + elif len(self.networks) >= 1: + # Use the first network. + return self.networks[0] + + # Very unlikely scenario. + raise ValueError("No networks found.") def set_default_network(self, network_name: str): """ @@ -425,7 +443,7 @@ def get_network_data(self, network_name: str) -> Dict: Returns: dict: A dictionary containing the providers in a network. """ - data: Dict[str, Any] = {"name": network_name} + data: Dict[str, Any] = {"name": str(network_name)} # Only add isDefault key when True if network_name == self.default_network: @@ -435,10 +453,10 @@ def get_network_data(self, network_name: str) -> Dict: network = self[network_name] if network.explorer: - data["explorer"] = network.explorer.name + data["explorer"] = str(network.explorer.name) for provider_name in network.providers: - provider_data = {"name": provider_name} + provider_data: Dict = {"name": str(provider_name)} # Only add isDefault key when True if provider_name == network.default_provider: @@ -906,12 +924,19 @@ def default_provider(self) -> Optional[str]: Optional[str] """ - if self._default_provider: - return self._default_provider + if provider := self._default_provider: + # Was set programatically. + return provider + + elif provider_from_config := self._network_config.get("default_provider"): + # The default is found in the Network's config class. + return provider_from_config - if len(self.providers) > 0: + elif len(self.providers) > 0: + # No default set anywhere - use the first installed. return list(self.providers)[0] + # There are no providers at all for this network. return None @property diff --git a/src/ape/api/providers.py b/src/ape/api/providers.py index 9da5605c48..5e52f79671 100644 --- a/src/ape/api/providers.py +++ b/src/ape/api/providers.py @@ -1420,8 +1420,13 @@ def prepare_transaction(self, txn: TransactionAPI) -> TransactionAPI: txn.max_fee = int(self.base_fee * multiplier + txn.max_priority_fee) # else: Assume user specified the correct amount or txn will fail and waste gas - if txn.gas_limit is None: - multiplier = self.network.auto_gas_multiplier + gas_limit = self.network.gas_limit if txn.gas_limit is None else txn.gas_limit + if gas_limit in (None, "auto") or isinstance(gas_limit, AutoGasLimit): + multiplier = ( + gas_limit.multiplier + if isinstance(gas_limit, AutoGasLimit) + else self.network.auto_gas_multiplier + ) if multiplier != 1.0: gas = min(int(self.estimate_gas_cost(txn) * multiplier), self.max_gas) else: @@ -1429,6 +1434,12 @@ def prepare_transaction(self, txn: TransactionAPI) -> TransactionAPI: txn.gas_limit = gas + elif gas_limit == "max": + txn.gas_limit = self.max_gas + + elif gas_limit is not None and isinstance(gas_limit, int): + txn.gas_limit = gas_limit + if txn.required_confirmations is None: txn.required_confirmations = self.network.required_confirmations elif not isinstance(txn.required_confirmations, int) or txn.required_confirmations < 0: diff --git a/src/ape/managers/config.py b/src/ape/managers/config.py index f5883eb59a..ba10efc5de 100644 --- a/src/ape/managers/config.py +++ b/src/ape/managers/config.py @@ -5,7 +5,7 @@ from ape._pydantic_compat import root_validator from ape.api import ConfigDict, DependencyAPI, PluginConfig -from ape.exceptions import ConfigError, NetworkError +from ape.exceptions import ConfigError from ape.logging import logger from ape.utils import BaseInterfaceModel, load_config @@ -182,11 +182,6 @@ def _plugin_configs(self) -> Dict[str, PluginConfig]: configs["compiler"] = compiler_dict self.compiler = CompilerConfig(**compiler_dict) - try: - self.network_manager.set_default_ecosystem(self.default_ecosystem) - except NetworkError as err: - logger.warning(str(err)) - dependencies = user_config.pop("dependencies", []) or [] if not isinstance(dependencies, list): raise ConfigError("'dependencies' config item must be a list of dicts.") diff --git a/src/ape/managers/networks.py b/src/ape/managers/networks.py index ef752fef9e..4e5b038fcf 100644 --- a/src/ape/managers/networks.py +++ b/src/ape/managers/networks.py @@ -1,13 +1,13 @@ +import json +from functools import cached_property from typing import Dict, Iterator, List, Optional, Set, Union import yaml from ape.api import EcosystemAPI, ProviderAPI, ProviderContextManager -from ape.api.networks import LOCAL_NETWORK_NAME, NetworkAPI +from ape.api.networks import NetworkAPI from ape.exceptions import ApeAttributeError, NetworkError -from ape.logging import logger - -from .base import BaseManager +from ape.managers.base import BaseManager class NetworkManager(BaseManager): @@ -27,7 +27,6 @@ class NetworkManager(BaseManager): _active_provider: Optional[ProviderAPI] = None _default: Optional[str] = None - _ecosystems_by_project: Dict[str, Dict[str, EcosystemAPI]] = {} def __repr__(self): provider = self.active_provider @@ -137,54 +136,21 @@ def provider_names(self) -> Set[str]: return names - @property + @cached_property def ecosystems(self) -> Dict[str, EcosystemAPI]: """ All the registered ecosystems in ``ape``, such as ``ethereum``. """ - project_name = self.config_manager.PROJECT_FOLDER.stem - if project_name in self._ecosystems_by_project: - return self._ecosystems_by_project[project_name] - - ecosystem_dict = {} - for plugin_name, ecosystem_class in self.plugin_manager.ecosystems: - ecosystem = ecosystem_class( # type: ignore - name=plugin_name, - data_folder=self.config_manager.DATA_FOLDER / plugin_name, - request_header=self.config_manager.REQUEST_HEADER, - ) - ecosystem_config = self.config_manager.get_config(plugin_name).dict() - default_network = ecosystem_config.get("default_network", LOCAL_NETWORK_NAME) + def to_kwargs(name: str) -> Dict: + return { + "name": name, + "data_folder": self.config_manager.DATA_FOLDER / name, + "request_header": self.config_manager.REQUEST_HEADER, + } - try: - ecosystem.set_default_network(default_network) - except NetworkError as err: - message = f"Failed setting default network: {err}" - logger.error(message) - - if ecosystem_config: - for network_name, network in ecosystem.networks.items(): - network_name = network_name.replace("-", "_") - if network_name not in ecosystem_config: - continue - - network_config = ecosystem_config[network_name] - if "default_provider" not in network_config: - continue - - default_provider = network_config["default_provider"] - if default_provider: - try: - network.set_default_provider(default_provider) - except NetworkError as err: - message = f"Failed setting default provider: {err}" - logger.error(message) - - ecosystem_dict[plugin_name] = ecosystem - - self._ecosystems_by_project[project_name] = ecosystem_dict - return ecosystem_dict + ecosystems = self.plugin_manager.ecosystems + return {n: cls(**to_kwargs(n)) for n, cls in ecosystems} # type: ignore def create_adhoc_geth_provider(self, uri: str) -> ProviderAPI: """ @@ -485,6 +451,9 @@ def default_ecosystem(self) -> EcosystemAPI: if self._default: return ecosystems[self._default] + elif self.config_manager.default_ecosystem: + return ecosystems[self.config_manager.default_ecosystem] + # If explicit default is not set, use first registered ecosystem elif len(ecosystems) > 0: return list(ecosystems.values())[0] @@ -529,16 +498,17 @@ def network_data(self) -> Dict: return data - def _get_ecosystem_data(self, ecosystem_name) -> Dict: + def _get_ecosystem_data(self, ecosystem_name: str) -> Dict: ecosystem = self[ecosystem_name] - ecosystem_data = {"name": ecosystem_name} + ecosystem_data: Dict = {"name": str(ecosystem_name)} # Only add isDefault key when True if ecosystem_name == self.default_ecosystem.name: ecosystem_data["isDefault"] = True ecosystem_data["networks"] = [] - for network_name in getattr(self, ecosystem_name).networks.keys(): + + for network_name in getattr(self, ecosystem_name).networks: network_data = ecosystem.get_network_data(network_name) ecosystem_data["networks"].append(network_data) @@ -556,7 +526,24 @@ def networks_yaml(self) -> str: str """ - return yaml.dump(self.network_data, sort_keys=False) + data = self.network_data + if not isinstance(data, dict): + raise TypeError( + f"Unexpected network data type: {type(data)}. " + f"Expecting dict. YAML dump will fail." + ) + + try: + return yaml.dump(data, sort_keys=False) + except ValueError as err: + try: + data_str = json.dumps(data) + except Exception: + data_str = str(data) + + raise NetworkError( + f"Network data did not dump to YAML: {data_str}\nAcual err: {err}" + ) from err def _validate_filter(arg: Optional[Union[List[str], str]], options: Set[str]): diff --git a/src/ape/plugins/__init__.py b/src/ape/plugins/__init__.py index bf42745450..d27ca8f90b 100644 --- a/src/ape/plugins/__init__.py +++ b/src/ape/plugins/__init__.py @@ -195,7 +195,9 @@ def _warn_not_fully_implemented_error(self, results, plugin_name): # Likely only ever a single class in a registration, but just in case. api_name = " - ".join([p.__name__ for p in classes]) for api_cls in classes: - if hasattr(api_cls, "__abstractmethods__") and api_cls.__abstractmethods__: + if ( + abstract_methods := getattr(api_cls, "__abstractmethods__", None) + ) and isinstance(abstract_methods, dict): unimplemented_methods.extend(api_cls.__abstractmethods__) else: @@ -204,8 +206,11 @@ def _warn_not_fully_implemented_error(self, results, plugin_name): elif hasattr(results, "__name__"): api_name = results.__name__ - if hasattr(results, "__abstractmethods__") and results.__abstractmethods__: + if (abstract_methods := getattr(results, "__abstractmethods__", None)) and isinstance( + abstract_methods, dict + ): unimplemented_methods.extend(results.__abstractmethods__) + else: api_name = results diff --git a/src/ape_ethereum/ecosystem.py b/src/ape_ethereum/ecosystem.py index 1b027ee88e..b6dfd5ce12 100644 --- a/src/ape_ethereum/ecosystem.py +++ b/src/ape_ethereum/ecosystem.py @@ -602,10 +602,13 @@ def create_transaction(self, **kwargs) -> TransactionAPI: if "type" in kwargs: if kwargs["type"] is None: version = TransactionType.DYNAMIC - elif not isinstance(kwargs["type"], int): - version = TransactionType(self.conversion_manager.convert(kwargs["type"], int)) - else: + elif isinstance(kwargs["type"], TransactionType): + version = kwargs["type"] + elif isinstance(kwargs["type"], int): version = TransactionType(kwargs["type"]) + else: + # Using hex values or alike. + version = TransactionType(self.conversion_manager.convert(kwargs["type"], int)) elif "gas_price" in kwargs: version = TransactionType.STATIC diff --git a/tests/conftest.py b/tests/conftest.py index d1c843c1a3..b1f3a794ef 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -203,9 +203,9 @@ def ethereum(networks): @pytest.fixture(autouse=True) -def eth_tester_provider(): +def eth_tester_provider(ethereum): if not ape.networks.active_provider or ape.networks.provider.name != "test": - with ape.networks.ethereum.local.use_provider("test") as provider: + with ethereum.local.use_provider("test") as provider: yield provider else: yield ape.networks.provider diff --git a/tests/functional/conftest.py b/tests/functional/conftest.py index 58d2378111..83e64bf7d9 100644 --- a/tests/functional/conftest.py +++ b/tests/functional/conftest.py @@ -8,8 +8,7 @@ from ethpm_types import ContractType, HexBytes import ape -from ape.api import EcosystemAPI, NetworkAPI, TransactionAPI -from ape.api.networks import LOCAL_NETWORK_NAME +from ape.api import TransactionAPI from ape.contracts import ContractContainer, ContractInstance from ape.exceptions import ChainError, ContractLogicError from ape.logging import LogLevel @@ -48,15 +47,6 @@ def pytest_collection_finish(session): yield -@pytest.fixture -def mock_network_api(mocker): - mock = mocker.MagicMock(spec=NetworkAPI) - mock_ecosystem = mocker.MagicMock(spec=EcosystemAPI) - mock_ecosystem.virtual_machine_error_class = _ContractLogicError - mock.ecosystem = mock_ecosystem - return mock - - @pytest.fixture def mock_web3(mocker): return mocker.MagicMock() @@ -416,9 +406,10 @@ def use_debug(logger): @pytest.fixture def dummy_live_network(chain): + original_network = chain.provider.network.name chain.provider.network.name = "goerli" yield chain.provider.network - chain.provider.network.name = LOCAL_NETWORK_NAME + chain.provider.network.name = original_network @pytest.fixture(scope="session") @@ -514,3 +505,10 @@ def minimal_proxy_container(): @pytest.fixture def minimal_proxy(owner, minimal_proxy_container): return owner.deploy(minimal_proxy_container) + + +@pytest.fixture +def mock_explorer(mocker): + explorer = mocker.MagicMock() + explorer.name = "mock" # Needed for network data serialization. + return explorer diff --git a/tests/functional/geth/test_provider.py b/tests/functional/geth/test_provider.py index feccaf493e..e5955ccf91 100644 --- a/tests/functional/geth/test_provider.py +++ b/tests/functional/geth/test_provider.py @@ -68,6 +68,7 @@ def test_chain_id_live_network_not_connected(networks): def test_chain_id_live_network_connected_uses_web3_chain_id(mocker, geth_provider): mock_network = mocker.MagicMock() mock_network.chain_id = 999999999 # Shouldn't use hardcoded network + mock_network.name = "mock" orig_network = geth_provider.network try: diff --git a/tests/functional/test_accounts.py b/tests/functional/test_accounts.py index 09ea66eac9..7054e6f2c9 100644 --- a/tests/functional/test_accounts.py +++ b/tests/functional/test_accounts.py @@ -11,6 +11,7 @@ from ape.types.signatures import recover_signer from ape.utils.testing import DEFAULT_NUMBER_OF_TEST_ACCOUNTS from ape_ethereum.ecosystem import ProxyType +from ape_ethereum.transactions import TransactionType from ape_test.accounts import TestAccount from tests.conftest import explorer_test @@ -204,16 +205,14 @@ def test_deploy_and_publish_live_network_no_explorer(owner, contract_container, @explorer_test -def test_deploy_and_publish(mocker, owner, contract_container, dummy_live_network): - mock_explorer = mocker.MagicMock() +def test_deploy_and_publish(owner, contract_container, dummy_live_network, mock_explorer): dummy_live_network.__dict__["explorer"] = mock_explorer contract = owner.deploy(contract_container, 0, publish=True, required_confirmations=0) mock_explorer.publish_contract.assert_called_once_with(contract.address) @explorer_test -def test_deploy_and_not_publish(mocker, owner, contract_container, dummy_live_network): - mock_explorer = mocker.MagicMock() +def test_deploy_and_not_publish(owner, contract_container, dummy_live_network, mock_explorer): dummy_live_network.__dict__["explorer"] = mock_explorer owner.deploy(contract_container, 0, publish=True, required_confirmations=0) assert not mock_explorer.call_count @@ -499,28 +498,33 @@ def test_declare(contract_container, sender): assert not receipt.failed -@pytest.mark.parametrize( - "tx_type,params", [(0, ["gas_price"]), (2, ["max_fee", "max_priority_fee"])] -) -def test_prepare_transaction(sender, ethereum, tx_type, params): +@pytest.mark.parametrize("tx_type", (TransactionType.STATIC, TransactionType.DYNAMIC)) +def test_prepare_transaction_using_auto_gas(sender, ethereum, tx_type): + params = ( + ("gas_price",) if tx_type is TransactionType.STATIC else ("max_fee", "max_priority_fee") + ) + def clear_network_property_cached(): for field in ("gas_limit", "auto_gas_multiplier"): - if field in tx.provider.network.__dict__: - del tx.provider.network.__dict__[field] + if field in ethereum.local.__dict__: + del ethereum.local.__dict__[field] - tx = ethereum.create_transaction(type=tx_type, gas_limit="auto") auto_gas = AutoGasLimit(multiplier=1.0) - original_limit = tx.provider.network.config.local.gas_limit + original_limit = ethereum.config.local.gas_limit try: - tx.provider.network.config.local.gas_limit = auto_gas + ethereum.config.local.gas_limit = auto_gas clear_network_property_cached() + assert ethereum.local.gas_limit == auto_gas, "Setup failed - auto gas not set." + + # NOTE: Must create tx _after_ setting network gas value. + tx = ethereum.create_transaction(type=tx_type) # Show tx doesn't have these by default. assert tx.nonce is None for param in params: # Custom fields depending on type. - assert getattr(tx, param) is None + assert getattr(tx, param) is None, f"'{param}' unexpectedly set." # Gas should NOT yet be estimated, as that happens closer to sending. assert tx.gas_limit is None @@ -530,24 +534,34 @@ def clear_network_property_cached(): # We expect these fields to have been set. assert tx.nonce is not None - assert tx.gas_limit is not None # Gas was estimated (using eth_estimateGas). + assert tx.gas_limit is not None # Show multipliers work. First, reset network to use one (hack). gas_smaller = tx.gas_limit - clear_network_property_cached() auto_gas.multiplier = 1.1 - tx.provider.network.config.local.gas_limit = auto_gas + ethereum.config.local.gas_limit = auto_gas + clear_network_property_cached() + assert ethereum.local.gas_limit == auto_gas, "Setup failed - auto gas multiplier not set." - tx2 = ethereum.create_transaction(type=tx_type, gas_limit="auto") + tx2 = ethereum.create_transaction(type=tx_type) tx2 = sender.prepare_transaction(tx2) gas_bigger = tx2.gas_limit - assert gas_smaller < gas_bigger for param in params: assert getattr(tx, param) is not None finally: - tx.provider.network.config.local.gas_limit = original_limit + ethereum.config.local.gas_limit = original_limit clear_network_property_cached() + + +@pytest.mark.parametrize("tx_type", (TransactionType.STATIC, TransactionType.DYNAMIC)) +def test_prepare_transaction_and_call_using_max_gas(tx_type, ethereum, sender, eth_tester_provider): + tx = ethereum.create_transaction(type=tx_type.value) + tx = sender.prepare_transaction(tx) + assert tx.gas_limit == eth_tester_provider.max_gas, "Test setup failed - gas limit unexpected." + + actual = sender.call(tx) + assert not actual.failed diff --git a/tests/functional/test_config.py b/tests/functional/test_config.py index d3648c9268..0b614c6cf2 100644 --- a/tests/functional/test_config.py +++ b/tests/functional/test_config.py @@ -24,14 +24,16 @@ def test_integer_deployment_addresses(networks): @pytest.mark.parametrize( - "ecosystems,networks,err_part", + "ecosystem_names,network_names,err_part", [(["ERRORS"], ["mainnet"], "ecosystem"), (["ethereum"], ["ERRORS"], "network")], ) -def test_bad_value_in_deployments(ecosystems, networks, err_part, ape_caplog, plugin_manager): +def test_bad_value_in_deployments( + ecosystem_names, network_names, err_part, ape_caplog, plugin_manager +): deployments = _create_deployments() all_ecosystems = dict(plugin_manager.ecosystems) - ecosystem_dict = {e: all_ecosystems[e] for e in ecosystems if e in all_ecosystems} - data = {**deployments, "valid_ecosystems": ecosystem_dict, "valid_networks": networks} + ecosystem_dict = {e: all_ecosystems[e] for e in ecosystem_names if e in all_ecosystems} + data = {**deployments, "valid_ecosystems": ecosystem_dict, "valid_networks": network_names} ape_caplog.assert_last_log_with_retries( lambda: DeploymentConfigCollection(__root__=data), f"Invalid {err_part}", diff --git a/tests/functional/test_contract_container.py b/tests/functional/test_contract_container.py index 3c3767ea8f..47b2eaa376 100644 --- a/tests/functional/test_contract_container.py +++ b/tests/functional/test_contract_container.py @@ -53,15 +53,13 @@ def test_deploy_and_publish_live_network_no_explorer(owner, contract_container, contract_container.deploy(0, sender=owner, publish=True, required_confirmations=0) -def test_deploy_and_publish(mocker, owner, contract_container, dummy_live_network): - mock_explorer = mocker.MagicMock() +def test_deploy_and_publish(owner, contract_container, dummy_live_network, mock_explorer): dummy_live_network.__dict__["explorer"] = mock_explorer contract = contract_container.deploy(0, sender=owner, publish=True, required_confirmations=0) mock_explorer.publish_contract.assert_called_once_with(contract.address) -def test_deploy_and_not_publish(mocker, owner, contract_container, dummy_live_network): - mock_explorer = mocker.MagicMock() +def test_deploy_and_not_publish(owner, contract_container, dummy_live_network, mock_explorer): dummy_live_network.__dict__["explorer"] = mock_explorer contract_container.deploy(0, sender=owner, publish=False, required_confirmations=0) assert not mock_explorer.call_count diff --git a/tests/functional/test_contract_instance.py b/tests/functional/test_contract_instance.py index 2ee7180943..48524e6787 100644 --- a/tests/functional/test_contract_instance.py +++ b/tests/functional/test_contract_instance.py @@ -19,7 +19,7 @@ MethodNonPayableError, ) from ape.types import AddressType -from ape_ethereum.transactions import TransactionStatusEnum +from ape_ethereum.transactions import TransactionStatusEnum, TransactionType MATCH_TEST_CONTRACT = re.compile(r"