diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 3688199..d1d3496 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -10,18 +10,18 @@ repos: - id: isort - repo: https://github.com/psf/black - rev: 23.12.0 + rev: 24.1.1 hooks: - id: black name: black - repo: https://github.com/pycqa/flake8 - rev: 6.1.0 + rev: 7.0.0 hooks: - id: flake8 - repo: https://github.com/pre-commit/mirrors-mypy - rev: v1.7.1 + rev: v1.8.0 hooks: - id: mypy additional_dependencies: [types-requests, types-setuptools, pydantic] diff --git a/ape-config.yaml b/ape-config.yaml index 78de3e6..84759f1 100644 --- a/ape-config.yaml +++ b/ape-config.yaml @@ -1,5 +1,11 @@ contracts_folder: tests/contracts +plugins: + - name: foundry + - name: solidity + - name: etherscan + - name: alchemy + ethereum: mainnet: default_provider: alchemy diff --git a/ape_safe/_cli/click_ext.py b/ape_safe/_cli/click_ext.py index ade047d..4854392 100644 --- a/ape_safe/_cli/click_ext.py +++ b/ape_safe/_cli/click_ext.py @@ -1,9 +1,10 @@ -from typing import NoReturn, Sequence, Union, cast +from typing import NoReturn, Optional, Sequence, Union, cast import click -from ape import accounts, config +from ape.api import AccountAPI from ape.cli import ApeCliContextObject, ape_cli_context from ape.exceptions import Abort +from ape.utils import ManagerAccessMixin from click import BadOptionUsage, MissingParameter from ape_safe.accounts import SafeContainer @@ -26,37 +27,6 @@ def safe_cli_ctx(): return ape_cli_context(obj_type=SafeCliContext) -def _safe_callback(ctx, param, value): - # NOTE: For some reason, the Cli CTX object is not the SafeCliCtx yet at this point. - safes = accounts.containers["safe"] - if value is None: - # First, check config for a default. If one is there, - # we must use that. - safe_config = config.get_config("safe") - if alias := safe_config.default_safe: - return accounts.load(alias) - - # If there is only 1 safe, just use that. - elif len(safes) == 1: - return next(safes.accounts) - - elif len(safes) == 0: - raise Abort("First, add a safe account using command:\n\t`ape safe add`") - - options = ", ".join(safes.aliases) - raise MissingParameter(message=f"Must specify one of '{options}').") - - elif value in safes.aliases: - return accounts.load(value) - - else: - raise BadOptionUsage("--safe", f"No safe with alias '{value}'") - - -safe_option = click.option("--safe", callback=_safe_callback) -safe_argument = click.argument("safe", callback=_safe_callback) - - def _txn_ids_callback(ctx, param, value): value_ls = value or [] return [int(x) if x.isnumeric() else x for x in value_ls if x] @@ -65,3 +35,106 @@ def _txn_ids_callback(ctx, param, value): txn_ids_argument = click.argument( "txn_ids", nargs=-1, callback=_txn_ids_callback, metavar="NONCE_OR_SAFE_TX_HASH(s)" ) + + +class CallbackFactory(ManagerAccessMixin): + """ + Helper class to prevent circular import and have access + to Ape objects. + """ + + @classmethod + def safe_callback(cls, ctx, param, value): + # NOTE: For some reason, the Cli CTX object is not the SafeCliCtx yet at this point. + safes = cls.account_manager.containers["safe"] + if value is None: + # First, check config for a default. If one is there, + # we must use that. + safe_config = cls.config_manager.get_config("safe") + if alias := safe_config.default_safe: + return cls.account_manager.load(alias) + + # If there is only 1 safe, just use that. + elif len(safes) == 1: + return next(safes.accounts) + + elif len(safes) == 0: + raise Abort("First, add a safe account using command:\n\t`ape safe add`") + + options = ", ".join(safes.aliases) + raise MissingParameter(message=f"Must specify one of '{options}').") + + elif value in safes.aliases: + return cls.account_manager.load(value) + + else: + raise BadOptionUsage("--safe", f"No safe with alias '{value}'") + + @classmethod + def submitter_callback(cls, ctx, param, val): + if val is None: + return None + + elif val in cls.account_manager.aliases: + return cls.account_manager.load(val) + + # Account address - execute using this account. + elif val in cls.account_manager: + return cls.account_manager[val] + + # Saying "yes, execute". Use first "local signer". + elif val.lower() in ("true", "t", "1"): + safe = cls.account_manager.load(ctx.params["alias"]) + if not safe.local_signers: + ctx.obj.abort("Cannot use `--execute TRUE` without a local signer.") + + return safe.select_signer(for_="submitter") + + return None + + @classmethod + def sender_callback(cls, ctx, param, val) -> Optional[Union[AccountAPI, bool]]: + """ + Either returns the account or ``False`` meaning don't execute. + NOTE: The handling of the `--execute` flag in the `pending` CLI + all happens here EXCEPT if a pending tx is executable and no + value of `--execute` was provided. + """ + return cls._get_execute_callback(ctx, param, val, name="sender") + + @classmethod + def execute_callback(cls, ctx, param, val) -> Optional[Union[AccountAPI, bool]]: + """ + Either returns the account or ``False`` meaning don't execute. + """ + return cls._get_execute_callback(ctx, param, val) + + @classmethod + def _get_execute_callback(cls, ctx, param, val, name: str = "execute"): + if val is None: + # Was not given any value. + # If it is determined in `pending` that a tx can execute, + # the user will get prompted. + # Avoid this by always doing `--execute false`. + return None + + elif submitter := cls.submitter_callback(ctx, param, val): + return submitter + + # Saying "no, do not execute", even if we could. + elif val.lower() in ("false", "f", "0"): + return False + + raise BadOptionUsage( + f"--{name}", f"`--{name}` value '{val}` not a boolean or account identifier." + ) + + +callback_factory = CallbackFactory() +safe_option = click.option("--safe", callback=callback_factory.safe_callback) +safe_argument = click.argument("safe", callback=callback_factory.safe_callback) +submitter_option = click.option( + "--submitter", help="Account to execute", callback=callback_factory.submitter_callback +) +sender_option = click.option("--sender", callback=callback_factory.sender_callback) +execute_option = click.option("--execute", callback=callback_factory.execute_callback) diff --git a/ape_safe/_cli/pending.py b/ape_safe/_cli/pending.py index fbef52b..83f364d 100644 --- a/ape_safe/_cli/pending.py +++ b/ape_safe/_cli/pending.py @@ -2,18 +2,24 @@ import click import rich -from ape import accounts from ape.api import AccountAPI from ape.cli import ConnectedProviderCommand from ape.exceptions import SignatureError from ape.types import AddressType, MessageSignature -from click.exceptions import BadOptionUsage from eth_typing import ChecksumAddress, Hash32 from eth_utils import humanize_hash from hexbytes import HexBytes from ape_safe import SafeAccount -from ape_safe._cli.click_ext import SafeCliContext, safe_cli_ctx, safe_option, txn_ids_argument +from ape_safe._cli.click_ext import ( + SafeCliContext, + execute_option, + safe_cli_ctx, + safe_option, + sender_option, + submitter_option, + txn_ids_argument, +) from ape_safe.accounts import get_signatures from ape_safe.client import UnexecutedTxData from ape_safe.utils import get_safe_tx_hash @@ -98,33 +104,6 @@ def _list(cli_ctx, safe, verbose) -> None: click.echo() -# NOTE: The handling of the `--execute` flag in the `pending` CLI -# all happens here EXCEPT if a pending tx is executable and no -# value of `--execute` was provided. -def _handle_execute_cli_arg(ctx, param, val) -> Optional[Union[AccountAPI, bool]]: - """ - Either returns the account or ``False`` meaning don't execute - """ - - if val is None: - # Was not given any value. - # If it is determined in `pending` that a tx can execute, - # the user will get prompted. - # Avoid this by always doing `--execute false`. - return None - - elif submitter := _load_submitter(ctx, param, val): - return submitter - - # Saying "no, do not execute", even if we could. - elif val.lower() in ("false", "f", "0"): - return False - - raise BadOptionUsage( - "--execute", f"`--execute` value '{val}` not a boolean or account identifier." - ) - - @pending.command(cls=ConnectedProviderCommand) @safe_cli_ctx() @safe_option @@ -133,7 +112,7 @@ def _handle_execute_cli_arg(ctx, param, val) -> Optional[Union[AccountAPI, bool] @click.option("--value", type=int, help="Transaction value", default=0) @click.option("--to", "receiver", type=ChecksumAddress, help="Transaction receiver") @click.option("--nonce", type=int, help="Transaction nonce") -@click.option("--sender", callback=_handle_execute_cli_arg) +@sender_option @click.option("--execute", help="Execute if possible after proposal", is_flag=True) def propose(cli_ctx, ecosystem, safe, data, gas_price, value, receiver, nonce, sender, execute): """ @@ -192,33 +171,11 @@ def propose(cli_ctx, ecosystem, safe, data, gas_price, value, receiver, nonce, s _execute(safe, new_tx, sender) -def _load_submitter(ctx, param, val): - if val is None: - return None - - elif val in accounts.aliases: - return accounts.load(val) - - # Account address - execute using this account. - elif val in accounts: - return accounts[val] - - # Saying "yes, execute". Use first "local signer". - elif val.lower() in ("true", "t", "1"): - safe = accounts.load(ctx.params["alias"]) - if not safe.local_signers: - ctx.obj.abort("Cannot use `--execute TRUE` without a local signer.") - - return safe.select_signer(for_="submitter") - - return None - - @pending.command(cls=ConnectedProviderCommand) @safe_cli_ctx() @safe_option @txn_ids_argument -@click.option("--execute", callback=_handle_execute_cli_arg) +@execute_option def approve(cli_ctx: SafeCliContext, safe, txn_ids, execute): submitter: Optional[AccountAPI] = execute if isinstance(execute, AccountAPI) else None pending_transactions = list( @@ -272,7 +229,7 @@ def approve(cli_ctx: SafeCliContext, safe, txn_ids, execute): @safe_option @txn_ids_argument # NOTE: Doesn't use --execute because we don't need BOOL values. -@click.option("--submitter", help="Account to execute", callback=_load_submitter) +@submitter_option @click.option("--nonce", help="Submitter nonce") def execute(cli_ctx, safe, txn_ids, submitter, nonce): """ @@ -313,7 +270,7 @@ def _execute(safe: SafeAccount, txn: UnexecutedTxData, submitter: AccountAPI, ** @safe_cli_ctx() @safe_option @txn_ids_argument -@click.option("--execute", callback=_handle_execute_cli_arg) +@execute_option def reject(cli_ctx: SafeCliContext, safe, txn_ids, execute): """ Reject one or more pending transactions diff --git a/ape_safe/accounts.py b/ape_safe/accounts.py index 3b85f81..21f357f 100644 --- a/ape_safe/accounts.py +++ b/ape_safe/accounts.py @@ -158,7 +158,8 @@ def create_client(self, key: str) -> BaseSafeClient: return safe.client elif key in self.addresses: - return self[cast(AddressType, key)].client + account = cast(SafeAccount, self[cast(AddressType, key)]) + return account.client elif key in self.aliases: return self.load_account(key).client @@ -166,7 +167,8 @@ def create_client(self, key: str) -> BaseSafeClient: else: address = self.conversion_manager.convert(key, AddressType) if address in self.addresses: - return self[cast(AddressType, key)].client + account = cast(SafeAccount, self[cast(AddressType, key)]) + return account.client # Is not locally managed. return SafeClient(address=address, chain_id=self.chain_manager.provider.chain_id) diff --git a/ape_safe/client/base.py b/ape_safe/client/base.py index 5832c85..3863510 100644 --- a/ape_safe/client/base.py +++ b/ape_safe/client/base.py @@ -34,34 +34,28 @@ def __init__(self, transaction_service_url: str): @property @abstractmethod - def safe_details(self) -> SafeDetails: - ... + def safe_details(self) -> SafeDetails: ... @abstractmethod - def get_next_nonce(self) -> int: - ... + def get_next_nonce(self) -> int: ... @abstractmethod - def _all_transactions(self) -> Iterator[SafeApiTxData]: - ... + def _all_transactions(self) -> Iterator[SafeApiTxData]: ... @abstractmethod - def get_confirmations(self, safe_tx_hash: SafeTxID) -> Iterator[SafeTxConfirmation]: - ... + def get_confirmations(self, safe_tx_hash: SafeTxID) -> Iterator[SafeTxConfirmation]: ... @abstractmethod def post_transaction( self, safe_tx: SafeTx, signatures: Dict[AddressType, MessageSignature], **kwargs - ): - ... + ): ... @abstractmethod def post_signatures( self, safe_tx_or_hash: Union[SafeTx, SafeTxID], signatures: Dict[AddressType, MessageSignature], - ): - ... + ): ... """Shared methods""" diff --git a/ape_safe/exceptions.py b/ape_safe/exceptions.py index 505aed5..4565dfc 100644 --- a/ape_safe/exceptions.py +++ b/ape_safe/exceptions.py @@ -85,10 +85,10 @@ def __exit__(self, exc_type: Type[BaseException], exc: BaseException, tb): if ( isinstance(exc, ContractLogicError) # NOTE: Just for mypy and exc_type == ContractLogicError - and exc.message.startswith("GS") - and exc.message in SAFE_ERROR_CODES ): - raise SafeLogicError(exc.message) from exc + message = exc.message.replace("revert: ", "").strip() + if message.startswith("GS") and message in SAFE_ERROR_CODES: + raise SafeLogicError(exc.message.replace("revert: ", "")) from exc # NOTE: Will raise `exc` by default because we did not return anything diff --git a/setup.py b/setup.py index 7ae5572..4c758b2 100644 --- a/setup.py +++ b/setup.py @@ -12,11 +12,11 @@ "ape-solidity", # Needed for compiling the Safe contracts ], "lint": [ - "black>=23.12.0,<24", # Auto-formatter and linter - "mypy>=1.7.1,<2", # Static type analyzer + "black>=24.1.1,<25", # Auto-formatter and linter + "mypy>=1.8.0,<2", # Static type analyzer "types-requests", # Needed for mypy type shed "types-setuptools", # Needed for mypy type shed - "flake8>=6.1.0,<7", # Style linter + "flake8>=7.0.0,<8", # Style linter "isort>=5.10.1,<6", # Import sorting linter "mdformat>=0.7.17,<0.8", # Docs formatter and linter "mdformat-pyproject>=0.0.1", # Allows configuring in pyproject.toml @@ -59,7 +59,7 @@ url="https://github.com/ApeWorX/ape-safe", include_package_data=True, install_requires=[ - "eth-ape>=0.7.1,<0.8", + "eth-ape>=0.7.7,<0.8", "requests>=2.31.0,<3", "eip712", # Use same version as eth-ape "click", # Use same version as eth-ape diff --git a/tests/integration/test_pending_cli.py b/tests/integration/test_pending_cli.py index c2204c8..ba6665e 100644 --- a/tests/integration/test_pending_cli.py +++ b/tests/integration/test_pending_cli.py @@ -3,7 +3,7 @@ def test_help(runner, cli): assert result.exit_code == 0, result.output -def test_propose(runner, cli, one_safe, receiver): +def test_propose(runner, cli, one_safe, receiver, chain): nonce_at_start = one_safe.next_nonce cmd = ( "pending", @@ -12,6 +12,8 @@ def test_propose(runner, cli, one_safe, receiver): receiver.address, "--value", "1", + "--network", + chain.provider.network_choice, ) # Sender is required by the API even for initial proposal, @@ -39,7 +41,9 @@ def test_propose_with_gas_price(runner, cli, one_safe, receiver, chain): "--value", "1", "--gas-price", - chain.provider.gas_price, + chain.provider.gas_price + 1, + "--network", + chain.provider.network_choice, ) result = runner.invoke(cli, cmd, catch_exceptions=False, input=f"{one_safe.alias}\n") assert result.exit_code == 0 @@ -50,7 +54,7 @@ def test_propose_with_gas_price(runner, cli, one_safe, receiver, chain): assert tx.gas_price > 0 -def test_propose_with_sender(runner, cli, one_safe, receiver): +def test_propose_with_sender(runner, cli, one_safe, receiver, chain): # First, fund the safe so the tx does not fail. receiver.transfer(one_safe, "1 ETH") @@ -64,6 +68,8 @@ def test_propose_with_sender(runner, cli, one_safe, receiver): "1", "--sender", receiver.address, + "--network", + chain.provider.network_choice, ) result = runner.invoke(cli, cmd, catch_exceptions=False) assert result.exit_code == 0, result.output @@ -72,7 +78,7 @@ def test_propose_with_sender(runner, cli, one_safe, receiver): assert one_safe.next_nonce == nonce_at_start -def test_propose_with_execute(runner, cli, one_safe, receiver): +def test_propose_with_execute(runner, cli, one_safe, receiver, chain): # First, fund the safe so the tx does not fail. receiver.transfer(one_safe, "1 ETH") @@ -87,20 +93,24 @@ def test_propose_with_execute(runner, cli, one_safe, receiver): "--sender", receiver.address, "--execute", + "--network", + chain.provider.network_choice, ) result = runner.invoke(cli, cmd, catch_exceptions=False) assert result.exit_code == 0, result.output assert one_safe.next_nonce == nonce_at_start + 1 -def test_list_no_safes(runner, cli, no_safes): - result = runner.invoke(cli, ["pending", "list"]) +def test_list_no_safes(runner, cli, no_safes, chain): + result = runner.invoke(cli, ["pending", "list", "--network", chain.provider.network_choice]) assert result.exit_code != 0, result.output assert "First, add a safe account using command" in result.output assert "ape safe add" in result.output -def test_list_no_txns(runner, cli, one_safe): - result = runner.invoke(cli, ["pending", "list"], catch_exceptions=False) +def test_list_no_txns(runner, cli, one_safe, chain): + result = runner.invoke( + cli, ["pending", "list", "--network", chain.provider.network_choice], catch_exceptions=False + ) assert result.exit_code == 0, result.output assert "There are no pending transactions" in result.output diff --git a/tests/integration/test_safe_mgmt_cli.py b/tests/integration/test_safe_mgmt_cli.py index be4cec0..4ed6f20 100644 --- a/tests/integration/test_safe_mgmt_cli.py +++ b/tests/integration/test_safe_mgmt_cli.py @@ -36,9 +36,12 @@ def test_list_network_connected(runner, cli, one_safe): assert "not connected" not in result.output -def test_add_safe(runner, cli, no_safes, safe): +def test_add_safe(runner, cli, no_safes, safe, chain): result = runner.invoke( - cli, ["add", safe.address, safe.alias], catch_exceptions=False, input="y\n" + cli, + ["add", safe.address, safe.alias, "--network", chain.provider.network_choice], + catch_exceptions=False, + input="y\n", ) assert result.exit_code == 0, result.output assert "SUCCESS" in result.output, result.output