fix: various issues with propose cli (#24)

.pre-commit-config.yaml

@@ -10,7 +10,7 @@ repos:
       - id: isort
 
 -   repo: https://github.com/psf/black
-    rev: 23.11.0
+    rev: 23.12.0
     hooks:
       - id: black
         name: black

ape_safe/_cli/pending.py

@@ -2,6 +2,7 @@
 
 import click
 import rich
+from ape import accounts
 from ape.api import AccountAPI
 from ape.cli import ConnectedProviderCommand
 from ape.exceptions import SignatureError
@@ -132,21 +133,26 @@ 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("--execute", callback=_handle_execute_cli_arg)
-def propose(cli_ctx, ecosystem, safe, data, gas_price, value, receiver, nonce, execute):
+@click.option("--sender", callback=_handle_execute_cli_arg)
+@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):
     """
     Create a new transaction
     """
     nonce = safe.new_nonce if nonce is None else nonce
     txn = ecosystem.create_transaction(
-        value=value, data=data, gas_price=gas_price, nonce=nonce, receiver=receiver
+        value=value,
+        data=data,
+        gas_price=gas_price,
+        nonce=nonce,
+        receiver=receiver,
     )
     safe_tx = safe.create_safe_tx(txn)
     safe_tx_hash = get_safe_tx_hash(safe_tx)
     signatures = get_signatures(safe_tx_hash, safe.local_signers)
 
     num_confirmations = 0
-    submitter = execute if isinstance(execute, AccountAPI) else None
+    submitter = sender if isinstance(sender, AccountAPI) else None
     if execute is None and submitter is None:
         # Check if we _can_ execute and ask the user.
         do_execute = (
@@ -159,10 +165,9 @@ def propose(cli_ctx, ecosystem, safe, data, gas_price, value, receiver, nonce, e
             # So we prompt them.
             submitter = safe.select_signer(for_="submitter")
 
-    owner = submitter if isinstance(submitter, AccountAPI) else safe.select_signer(for_="owner")
-
+    sender = submitter if isinstance(submitter, AccountAPI) else safe.select_signer(for_="sender")
     safe.client.post_transaction(
-        safe_tx, signatures, sender=owner.address, contractTransactionHash=safe_tx_hash
+        safe_tx, signatures, sender=sender.address, contractTransactionHash=safe_tx_hash
     )
 
     # Wait for new transaction to appear
@@ -178,27 +183,29 @@ def propose(cli_ctx, ecosystem, safe, data, gas_price, value, receiver, nonce, e
         )
         timeout -= 1
 
-    if not new_tx:
+    if new_tx:
+        cli_ctx.logger.success(f"Proposed transaction '{safe_tx_hash}'.")
+    else:
         cli_ctx.abort("Failed to propose transaction.")
 
-    if submitter:
-        _execute(safe, new_tx, submitter)
+    if execute:
+        _execute(safe, new_tx, sender)
 
 
 def _load_submitter(ctx, param, val):
     if val is None:
         return None
 
-    elif val in ctx.obj.account_manager.aliases:
-        return ctx.obj.account_manager.load(val)
+    elif val in accounts.aliases:
+        return accounts.load(val)
 
     # Account address - execute using this account.
-    elif val in ctx.obj.account_manager:
-        return ctx.obj.account_manager[val]
+    elif val in accounts:
+        return accounts[val]
 
     # Saying "yes, execute". Use first "local signer".
     elif val.lower() in ("true", "t", "1"):
-        safe = ctx.obj.account_manager.load(ctx.params["alias"])
+        safe = accounts.load(ctx.params["alias"])
         if not safe.local_signers:
             ctx.obj.abort("Cannot use `--execute TRUE` without a local signer.")
 
@@ -295,12 +302,9 @@ def execute(cli_ctx, safe, txn_ids, submitter, nonce):
 
 def _execute(safe: SafeAccount, txn: UnexecutedTxData, submitter: AccountAPI, **tx_kwargs):
     safe_tx = safe.create_safe_tx(**txn.model_dump(mode="json", by_alias=True))
-
-    # TODO: Can remove type ignore after Ape 0.7.1
     signatures: Dict[AddressType, MessageSignature] = {
-        c.owner: MessageSignature.from_rsv(c.signature) for c in txn.confirmations  # type: ignore
+        c.owner: MessageSignature.from_rsv(c.signature) for c in txn.confirmations
     }
-
     exc_tx = safe.create_execute_transaction(safe_tx, signatures, **tx_kwargs)
     submitter.call(exc_tx)
 
@@ -339,7 +343,7 @@ def reject(cli_ctx: SafeCliContext, safe, txn_ids, execute):
         elif click.confirm(f"{txn}\nCancel Transaction?"):
             try:
                 safe.transfer(
-                    safe, "0 ether", nonce=txn.nonce, submit_transaction=submit, submitter=submitter
+                    safe, 0, nonce=txn.nonce, submit_transaction=submit, submitter=submitter
                 )
             except SignatureError:
                 # These are expected because of how the plugin works

ape_safe/accounts.py

@@ -13,7 +13,7 @@
 from ape.managers.accounts import AccountManager, TestAccountManager
 from ape.types import AddressType, HexBytes, MessageSignature
 from ape.utils import ZERO_ADDRESS, cached_property
-from ape_ethereum.transactions import TransactionType
+from ape_ethereum.transactions import StaticFeeTransaction, TransactionType
 from eip712.common import create_safe_tx_def
 from eth_account.messages import encode_defunct
 from eth_utils import keccak, to_bytes, to_int
@@ -39,6 +39,8 @@
 
 
 class SafeContainer(AccountContainerAPI):
+    _accounts: Dict[str, "SafeAccount"] = {}
+
     @property
     def _account_files(self) -> Iterator[Path]:
         yield from self.data_folder.glob("*.json")
@@ -56,7 +58,15 @@ def addresses(self) -> Iterator[str]:
     @property
     def accounts(self) -> Iterator[AccountAPI]:
         for account_file in self._account_files:
-            yield SafeAccount(account_file_path=account_file)
+            if account_file.stem in self._accounts:
+                yield self._accounts[account_file.stem]
+
+            else:
+                # Cache the accounts so their local state is maintained
+                # throughout the current Python session.
+                acct = SafeAccount(account_file_path=account_file)
+                self._accounts[account_file.stem] = acct
+                yield acct
 
     def __len__(self) -> int:
         return len([*self._account_files])
@@ -121,11 +131,16 @@ def load_account(self, alias: str) -> "SafeAccount":
         Returns:
             :class:`~ape_safe.accounts.SafeAccount`: The Safe account loaded.
         """
+        if alias in self._accounts:
+            return self._accounts[alias]
+
         account_path = self._get_path(alias)
         if not account_path.is_file():
             raise ApeSafeError(f"Safe with '{alias}' does not exist")
 
-        return SafeAccount(account_file_path=account_path)
+        acct = SafeAccount(account_file_path=account_path)
+        self._accounts[alias] = acct
+        return acct
 
     def delete_account(self, alias: str):
         """
@@ -330,14 +345,22 @@ def create_safe_tx(self, txn: Optional[TransactionAPI] = None, **safe_tx_kwargs)
         Returns:
             :class:`~ape_safe.client.SafeTx`: The Safe Transaction to be used.
         """
+        gas_price = safe_tx_kwargs.get(
+            "gas_price", safe_tx_kwargs.get("gasPrice", safe_tx_kwargs.get("gas"))
+        )
+        if gas_price is None and isinstance(txn, StaticFeeTransaction):
+            gas_price = txn.gas_price or 0
+        elif gas_price is None:
+            gas_price = 0
+
         safe_tx = {
             "to": txn.receiver if txn else self.address,  # Self-call, e.g. rejection
             "value": txn.value if txn else 0,
             "data": (txn.data or b"") if txn else b"",
             "nonce": self.new_nonce if txn is None or txn.nonce is None else txn.nonce,
             "operation": 0,
             "safeTxGas": 0,
-            "gasPrice": 0,
+            "gasPrice": gas_price,
             "gasToken": ZERO_ADDRESS,
             "refundReceiver": ZERO_ADDRESS,
         }

ape_safe/client/mock.py

@@ -73,8 +73,6 @@ def post_transaction(
         self, safe_tx: SafeTx, signatures: Dict[AddressType, MessageSignature], **kwargs
     ):
         safe_tx_data = UnexecutedTxData.from_safe_tx(safe_tx, self.safe_details.threshold)
-
-        # NOTE: Using `construct` to avoid HexBytes pydantic v1 backimports issue.
         safe_tx_data.confirmations.extend(
             SafeTxConfirmation(
                 owner=signer,
@@ -86,7 +84,6 @@ def post_transaction(
         )
         tx_id = cast(SafeTxID, HexBytes(safe_tx_data.safe_tx_hash).hex())
         self.transactions[tx_id] = safe_tx_data
-
         if safe_tx_data.nonce in self.transactions_by_nonce:
             self.transactions_by_nonce[safe_tx_data.nonce].append(tx_id)
         else:

pyproject.toml

@@ -38,3 +38,6 @@ force_grid_wrap = 0
 include_trailing_comma = true
 multi_line_output = 3
 use_parentheses = true
+
+[tool.mdformat]
+number = true

setup.py

@@ -12,13 +12,14 @@
         "ape-solidity",  # Needed for compiling the Safe contracts
     ],
     "lint": [
-        "black>=23.11.0,<24",  # Auto-formatter and linter
+        "black>=23.12.0,<24",  # Auto-formatter and linter
         "mypy>=1.7.1,<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
         "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
     ],
     "release": [  # `release` GitHub Action job uses this
         "setuptools",  # Installation tool
@@ -58,7 +59,7 @@
     url="https://github.com/ApeWorX/ape-safe",
     include_package_data=True,
     install_requires=[
-        "eth-ape>=0.7.0,<0.8",
+        "eth-ape>=0.7.1,<0.8",
         "requests>=2.31.0,<3",
         "eip712",  # Use same version as eth-ape
         "click",  # Use same version as eth-ape

tests/conftest.py

@@ -28,8 +28,13 @@ def data_folder(config):
 
 
 @pytest.fixture(scope="session")
-def deployer(accounts):
-    return accounts[-1]
+def deployer(OWNERS):
+    return OWNERS[-1]
+
+
+@pytest.fixture(scope="session")
+def receiver(accounts):
+    return accounts[9]
 
 
 @pytest.fixture(scope="session", params=["1.3.0"])  # TODO: Test more versions later?

tests/integration/test_pending_cli.py

@@ -3,14 +3,104 @@ def test_help(runner, cli):
     assert result.exit_code == 0, result.output
 
 
+def test_propose(runner, cli, one_safe, receiver):
+    nonce_at_start = one_safe.next_nonce
+    cmd = (
+        "pending",
+        "propose",
+        "--to",
+        receiver.address,
+        "--value",
+        "1",
+    )
+
+    # Sender is required by the API even for initial proposal,
+    # so it prompts the user.
+    sender_input = f"{one_safe.alias}\n"
+
+    result = runner.invoke(cli, cmd, catch_exceptions=False, input=sender_input)
+    assert result.exit_code == 0
+    assert "Proposed transaction" in result.output
+    safe_tx_hash = result.output.split("Proposed transaction '")[-1].split("'")[0].strip()
+
+    # Verify the transaction is in the service.
+    assert safe_tx_hash in one_safe.client.transactions
+
+    # The nonce is the same because we did not execute.
+    assert one_safe.next_nonce == nonce_at_start
+
+
+def test_propose_with_gas_price(runner, cli, one_safe, receiver, chain):
+    cmd = (
+        "pending",
+        "propose",
+        "--to",
+        receiver.address,
+        "--value",
+        "1",
+        "--gas-price",
+        chain.provider.gas_price,
+    )
+    result = runner.invoke(cli, cmd, catch_exceptions=False, input=f"{one_safe.alias}\n")
+    assert result.exit_code == 0
+    safe_tx_hash = result.output.split("Proposed transaction '")[-1].split("'")[0].strip()
+
+    # Verify gas price was used.
+    tx = one_safe.client.transactions[safe_tx_hash]
+    assert tx.gas_price > 0
+
+
+def test_propose_with_sender(runner, cli, one_safe, receiver):
+    # First, fund the safe so the tx does not fail.
+    receiver.transfer(one_safe, "1 ETH")
+
+    nonce_at_start = one_safe.next_nonce
+    cmd = (
+        "pending",
+        "propose",
+        "--to",
+        receiver.address,
+        "--value",
+        "1",
+        "--sender",
+        receiver.address,
+    )
+    result = runner.invoke(cli, cmd, catch_exceptions=False)
+    assert result.exit_code == 0, result.output
+
+    # The nonce is the same because we did not execute.
+    assert one_safe.next_nonce == nonce_at_start
+
+
+def test_propose_with_execute(runner, cli, one_safe, receiver):
+    # First, fund the safe so the tx does not fail.
+    receiver.transfer(one_safe, "1 ETH")
+
+    nonce_at_start = one_safe.next_nonce
+    cmd = (
+        "pending",
+        "propose",
+        "--to",
+        receiver.address,
+        "--value",
+        "1",
+        "--sender",
+        receiver.address,
+        "--execute",
+    )
+    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"])
     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, safe):
+def test_list_no_txns(runner, cli, one_safe):
     result = runner.invoke(cli, ["pending", "list"], catch_exceptions=False)
     assert result.exit_code == 0, result.output
     assert "There are no pending transactions" in result.output

tests/integration/test_safe_mgmt_cli.py

@@ -13,7 +13,7 @@ def test_list_one_safe(runner, cli, one_safe):
     result = runner.invoke(cli, ["list"], catch_exceptions=False)
     assert result.exit_code == 0, result.output
     assert "Found 1 Safe" in result.output
-    assert "0x5FbDB2315678afecb367f032d93F642f64180aa3" in result.output
+    assert one_safe.address in result.output
 
 
 def test_list_network_not_connected(runner, cli, one_safe):
@@ -22,7 +22,7 @@ def test_list_network_not_connected(runner, cli, one_safe):
     )
     assert result.exit_code == 0, result.output
     assert "Found 1 Safe" in result.output
-    assert "0x5FbDB2315678afecb367f032d93F642f64180aa3" in result.output
+    assert one_safe.address in result.output
     assert "not connected" in result.output
 
 
@@ -32,7 +32,7 @@ def test_list_network_connected(runner, cli, one_safe):
     )
     assert result.exit_code == 0, result.output
     assert "Found 1 Safe" in result.output
-    assert "0x5FbDB2315678afecb367f032d93F642f64180aa3" in result.output
+    assert one_safe.address in result.output
     assert "not connected" not in result.output