fix(Multisend): pass-through impersonate kwarg to execTransaction call (#90)

* fix(Multisend): use `sender=self.safe` for calling

* refactor(Multisend): use same exception pattern from `as_transaction`

* refactor: remove need to use alchemy as default plugin

NOTE: We now have `ape-node` with failover from evmchains

* test: remove unused `paramtrize`

* refactor(Utils): give back `HexBytes` instead of string

* refactor(Utils): add `encode_signatures` utility function

* refactor(Account): expose impersonation as private API; pass-thru kwarg

* refactor(Account): streamline `SafeAccount.create_safe_tx`

* refactor(Multisend): fix operation by using new `SafeAccount` API

* fix(Account): list `submitter` as `sender=` in execute txn

* fix(Test): wrong kwarg for `safe=` (not needed anyways)

* fix(Utils): use `HexStr` from `eth-pydantic-types` instead

* refactor(Accounts): raise better error on not enough signatures

* refactor(Multisend): just broadcast `.as_transaction` for `__call__`

* refactor(Test): batch send needed `submitter=` kwarg to work

* refactor(CLI): use `submitter=` kwarg to send transaction

* refactor(Accounts): move `submitter=` kwarg handling to `create_execute_transaction`

* refactor(Client): revert `SafeTxID` type back to `NewType`

* refactor(Account): add convert to int for `safeTxGas` in `create_safe_tx`

* docs(MultiSend): update docstrings for multisend feature's correctness

* refactor(MultiSend): add auto-detection of version using Safe arg

Author: fubuloubu

ape-config.yaml

@@ -5,11 +5,8 @@ plugins:
   - name: foundry
   - name: solidity
   - name: etherscan
-  - name: alchemy
 
 ethereum:
-  mainnet:
-    default_provider: alchemy
   local:
     default_provider: foundry
 
@@ -73,13 +70,3 @@ dependencies:
         version: 0.7.6
         import_remapping:
           - "@openzeppelin/contracts=openzeppelin/v3.4.0"
-
-foundry:
-  fork:
-    ethereum:
-      mainnet:
-        upstream_provider: alchemy
-        block_number: 15776634
-      sepolia:
-        upstream_provider: alchemy
-        block_number: 3091950

ape_safe/_cli/pending.py

@@ -152,7 +152,10 @@ def propose(cli_ctx, ecosystem, safe, data, gas_price, value, receiver, nonce, s
 
     sender = submitter if isinstance(submitter, AccountAPI) else safe.select_signer(for_="sender")
     safe.client.post_transaction(
-        safe_tx, signatures, sender=sender.address, contractTransactionHash=safe_tx_hash
+        safe_tx,
+        signatures,
+        submitter=sender.address,
+        contractTransactionHash=safe_tx_hash,
     )
 
     # Wait for new transaction to appear
@@ -277,6 +280,7 @@ def _execute(safe: "SafeAccount", txn: "UnexecutedTxData", submitter: "AccountAP
     signatures: dict[AddressType, MessageSignature] = {
         c.owner: MessageSignature.from_rsv(c.signature) for c in txn.confirmations
     }
+    tx_kwargs["submitter"] = submitter
     exc_tx = safe.create_execute_transaction(safe_tx, signatures, **tx_kwargs)
     submitter.call(exc_tx)
 

ape_safe/accounts.py

@@ -11,24 +11,19 @@
 from ape.contracts import ContractCall
 from ape.exceptions import ContractNotFoundError, ProviderNotConnectedError
 from ape.logging import logger
-from ape.types import AddressType, HexBytes, MessageSignature
+from ape.types import AddressType, MessageSignature
 from ape.utils import ZERO_ADDRESS, cached_property
 from ape_ethereum.proxies import ProxyInfo, ProxyType
 from ape_ethereum.transactions import TransactionType
 from eip712.common import SafeTxV1, SafeTxV2, create_safe_tx_def
-from eth_utils import keccak, to_bytes, to_int
+from eth_utils import keccak, to_bytes, to_canonical_address, to_int
 from ethpm_types import ContractType
 from ethpm_types.abi import ABIType, MethodABI
 from packaging.version import Version
 
-from .client import (
-    BaseSafeClient,
-    MockSafeClient,
-    SafeClient,
-    SafeTx,
-    SafeTxConfirmation,
-    SafeTxID,
-)
+from ape_safe.client.types import OperationType
+
+from .client import BaseSafeClient, MockSafeClient, SafeClient, SafeTx, SafeTxConfirmation, SafeTxID
 from .config import SafeConfig
 from .exceptions import (
     ApeSafeError,
@@ -43,7 +38,7 @@
 from .modules import SafeModuleManager
 from .packages import PackageType
 from .types import SafeCacheData
-from .utils import get_safe_tx_hash, order_by_signer
+from .utils import encode_signatures, get_safe_tx_hash
 
 if TYPE_CHECKING:
     from ape.api.address import BaseAddress
@@ -190,8 +185,7 @@ def get_signatures(
 ) -> dict[AddressType, MessageSignature]:
     signatures: dict[AddressType, MessageSignature] = {}
     for signer in signers:
-        signature = signer.sign_message(safe_tx)
-        if signature:
+        if signature := signer.sign_message(safe_tx):
             signatures[signer.address] = signature
 
     return signatures
@@ -417,27 +411,39 @@ def create_safe_tx(self, txn: Optional[TransactionAPI] = None, **safe_tx_kwargs)
 
         Args:
             txn (Optional[``TransactionAPI``]): The transaction
-            **safe_tx_kwargs: The safe transactions specifications, such as ``submitter``.
+            **safe_tx_kwargs: The safe transaction specifications, such as ``operation``.
 
         Returns:
             :class:`~ape_safe.client.SafeTx`: The Safe Transaction to be used.
         """
-        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,
-            "gasToken": ZERO_ADDRESS,
-            "refundReceiver": ZERO_ADDRESS,
-        }
-        safe_tx = {
-            **safe_tx,
-            **{k: v for k, v in safe_tx_kwargs.items() if k in safe_tx and v is not None},
-        }
-        return self.safe_tx_def(**safe_tx)
+        return self.safe_tx_def(  # type: ignore[call-arg]
+            # NOTE: Use `txn` only for these 3 commonly-used params
+            to=(
+                txn.receiver
+                if txn
+                else self.conversion_manager.convert(
+                    safe_tx_kwargs.get("to", self.address), AddressType
+                )
+            ),
+            value=(
+                txn.value
+                if txn
+                else self.conversion_manager.convert(safe_tx_kwargs.get("value", 0), int)
+            ),
+            data=(txn.data or b"") if txn else safe_tx_kwargs.get("data", b""),
+            # ...then use only kwargs for the rest
+            # NOTE: Use `.new_nonce` to get latest nonce in off-chain txn queue
+            nonce=safe_tx_kwargs.get("nonce", self.new_nonce),
+            operation=safe_tx_kwargs.get("operation", OperationType.CALL),
+            safeTxGas=self.conversion_manager.convert(safe_tx_kwargs.get("safeTxGas", 0), int),
+            gasPrice=self.conversion_manager.convert(safe_tx_kwargs.get("gasPrice", 0), int),
+            gasToken=self.conversion_manager.convert(
+                safe_tx_kwargs.get("gasToken", ZERO_ADDRESS), AddressType
+            ),
+            refundReceiver=self.conversion_manager.convert(
+                safe_tx_kwargs.get("refundReceiver", ZERO_ADDRESS), AddressType
+            ),
+        )
 
     def create_batch(self) -> "MultiSend":
         from ape_safe.multisend import MultiSend
@@ -530,24 +536,69 @@ def local_signers(self) -> list[AccountAPI]:
 
         return list(filter(lambda a: a in signers, accounts))
 
+    def _preapproved_signature(
+        self, signer: Union[AddressType, "BaseAddress", str]
+    ) -> MessageSignature:
+        # Get the Safe-style "preapproval" signature type, which is a sentinel value used to denote
+        # when a signer approved via some other method, such as `approveHash` or being `msg.sender`
+        # TODO: Link documentation for this
+        return MessageSignature(
+            v=1,  # Approved hash (e.g. submitter is approved)
+            r=(
+                b"\x00" * 12
+                + to_canonical_address(self.conversion_manager.convert(signer, AddressType))
+            ),
+            s=b"\x00" * 32,
+        )
+
+    def _impersonate_approvals(self, safe_tx: SafeTx) -> dict[AddressType, MessageSignature]:
+        safe_tx_hash = self.contract.getTransactionHash(*safe_tx)
+
+        # Bypass signature collection logic and attempt to submit by impersonation
+        # NOTE: Only works for fork and local network providers that support `set_storage`
+        signatures = dict()
+        for signer_address in self.signers[: self.confirmations_required]:
+            # NOTE: `approvedHashes` is `address => safe_tx_hash => num_confs` @ slot 8
+            # TODO: Use native ape slot indexing, once available
+            #       e.g. `self.contract.approvedHashes[signer_address][safe_tx_hash] = 1`
+            address_bytes32 = to_bytes(hexstr=signer_address)
+            address_bytes32 = b"\x00" * (32 - len(address_bytes32)) + address_bytes32
+            key_hash = keccak(address_bytes32 + b"\x00" * 31 + to_bytes(8))
+            slot = to_int(keccak(safe_tx_hash + key_hash))
+            self.provider.set_storage(self.address, slot, b"\x00" * 31 + b"\x01")
+
+            signatures[signer_address] = self._preapproved_signature(signer_address)
+
+        return signatures
+
     @handle_safe_logic_error()
     def create_execute_transaction(
         self,
         safe_tx: SafeTx,
         signatures: Mapping[AddressType, MessageSignature],
+        submitter: Union[AccountAPI, AddressType, str, None] = None,
+        impersonate: bool = False,
         **txn_options,
     ) -> TransactionAPI:
-        exec_args = list(safe_tx._body_["message"].values())[:-1]  # NOTE: Skip `nonce`
-        encoded_signatures = HexBytes(
-            b"".join(
-                sig.encode_rsv() if isinstance(sig, MessageSignature) else sig
-                for sig in order_by_signer(signatures)
-            )
-        )
+        if impersonate:
+            signatures = self._impersonate_approvals(safe_tx)
+
+        elif len(signatures) < self.confirmations_required:
+            raise NotEnoughSignatures(self.confirmations_required, len(signatures))
+
+        if not isinstance(submitter, AccountAPI):
+            submitter = self.load_submitter(submitter)
+            assert isinstance(submitter, AccountAPI)  # NOTE: mypy happy
+
+        exec_args = _safe_tx_exec_args(safe_tx)[:-1]  # NOTE: Skip `nonce`
+        encoded_signatures = encode_signatures(signatures)
 
         # NOTE: executes a `ProviderAPI.prepare_transaction`, which may produce `ContractLogicError`
         return self.contract.execTransaction.as_transaction(
-            *exec_args, encoded_signatures, **txn_options
+            *exec_args,
+            encoded_signatures,
+            **txn_options,
+            sender=submitter,
         )
 
     def compute_prev_signer(self, signer: Union[str, AddressType, "BaseAddress"]) -> AddressType:
@@ -606,56 +657,6 @@ def estimate_gas_cost(self, **kwargs) -> int:
             or 0
         )
 
-    def _preapproved_signature(
-        self, signer: Union[AddressType, "BaseAddress", str]
-    ) -> MessageSignature:
-        # Get the Safe-style "preapproval" signature type, which is a sentinel value used to denote
-        # when a signer approved via some other method, such as `approveHash` or being `msg.sender`
-        # TODO: Link documentation for this
-        return MessageSignature(
-            v=1,  # Approved hash (e.g. submitter is approved)
-            r=b"\x00" * 12 + to_bytes(hexstr=self.conversion_manager.convert(signer, AddressType)),
-            s=b"\x00" * 32,
-        )
-
-    @handle_safe_logic_error()
-    def _impersonated_call(self, txn: TransactionAPI, **safe_tx_and_call_kwargs) -> ReceiptAPI:
-        safe_tx = self.create_safe_tx(txn, **safe_tx_and_call_kwargs)
-        safe_tx_exec_args = _safe_tx_exec_args(safe_tx)
-        signatures = {}
-
-        # Bypass signature collection logic and attempt to submit by impersonation
-        # NOTE: Only works for fork and local network providers that support `set_storage`
-        safe_tx_hash = self.contract.getTransactionHash(*safe_tx_exec_args)
-        signer_address = None
-        for signer_address in self.signers[: self.confirmations_required]:
-            # NOTE: `approvedHashes` is `address => safe_tx_hash => num_confs` @ slot 8
-            # TODO: Use native ape slot indexing, once available
-            address_bytes32 = to_bytes(hexstr=signer_address)
-            address_bytes32 = b"\x00" * (32 - len(address_bytes32)) + address_bytes32
-            key_hash = keccak(address_bytes32 + b"\x00" * 31 + to_bytes(8))
-            slot = to_int(keccak(safe_tx_hash + key_hash))
-            self.provider.set_storage(self.address, slot, to_bytes(1))
-
-            signatures[signer_address] = self._preapproved_signature(signer_address)
-
-        # NOTE: Could raise a `SafeContractError`
-        safe_tx_and_call_kwargs["sender"] = safe_tx_and_call_kwargs.get(
-            "submitter",
-            # NOTE: Use whatever the last signer was if no `submitter`
-            self.account_manager.test_accounts[signer_address],
-        )
-        return self.contract.execTransaction(
-            *safe_tx_exec_args[:-1],  # NOTE: Skip nonce
-            HexBytes(
-                b"".join(
-                    sig.encode_rsv() if isinstance(sig, MessageSignature) else sig
-                    for sig in order_by_signer(signatures)
-                )
-            ),
-            **safe_tx_and_call_kwargs,
-        )
-
     @handle_safe_logic_error()
     def call(  # type: ignore[override]
         self,
@@ -665,13 +666,11 @@ def call(  # type: ignore[override]
     ) -> ReceiptAPI:
         # NOTE: This handles if given `submit=None'.
         default_submit = not impersonate
-        submit = (
+        call_kwargs["submit"] = (
             call_kwargs.pop("submit_transaction", call_kwargs.pop("submit", default_submit))
             or not default_submit
         )
-        call_kwargs["submit"] = submit
-        if impersonate:
-            return self._impersonated_call(txn, **call_kwargs)
+        call_kwargs["impersonate"] = impersonate
 
         return super().call(txn, **call_kwargs)
 
@@ -699,8 +698,7 @@ def get_api_confirmations(
         }
 
     def _contract_approvals(self, safe_tx: SafeTx) -> Mapping[AddressType, MessageSignature]:
-        safe_tx_exec_args = _safe_tx_exec_args(safe_tx)
-        safe_tx_hash = self.contract.getTransactionHash(*safe_tx_exec_args)
+        safe_tx_hash = self.contract.getTransactionHash(*safe_tx)
 
         return {
             signer: self._preapproved_signature(signer)
@@ -715,10 +713,11 @@ def _all_approvals(self, safe_tx: SafeTx) -> dict[AddressType, MessageSignature]
         approvals.update(self._contract_approvals(safe_tx))
         return approvals
 
+    @handle_safe_logic_error()
     def submit_safe_tx(
         self,
         safe_tx: Union[SafeTx, SafeTxID],
-        submitter: Union[AccountAPI, AddressType, str, None] = None,
+        impersonate: bool = False,
         **txn_options,
     ) -> ReceiptAPI:
         """
@@ -740,16 +739,22 @@ def submit_safe_tx(
 
         else:
             safe_tx_id = get_safe_tx_hash(safe_tx)
-
         assert isinstance(safe_tx, (SafeTxV1, SafeTxV2))
-        signatures = self._all_approvals(safe_tx)
-        txn = self.create_execute_transaction(safe_tx, signatures, **txn_options)
 
-        if not isinstance(submitter, AccountAPI):
-            submitter = self.load_submitter(submitter)
-            assert isinstance(submitter, AccountAPI)  # NOTE: mypy happy
+        if impersonate:
+            signatures = {}
 
-        return submitter.call(txn)
+        elif len(signatures := self._all_approvals(safe_tx)) < self.confirmations_required:
+            signatures = self.add_signatures(safe_tx)
+
+        txn = self.create_execute_transaction(
+            safe_tx,
+            signatures,
+            impersonate=impersonate,
+            **txn_options,
+        )
+
+        return self.provider.send_transaction(txn)
 
     def sign_transaction(
         self,
@@ -856,13 +861,8 @@ def skip_signer(signer: AccountAPI):
                 safe_tx,
                 sigs_by_signer,
                 **gas_args,
-                nonce=submitter_account.nonce,
-                # NOTE: Because of `ape_ethereum.transactions.BaseTransaction.serialize_transaction`
-                #       doing a recovered signer check, and we have to make sure the address matches
-                #       the recovered address of the signed transaction.
-                sender=submitter_account.address,
+                submitter=submitter_account,
             )
-            # NOTE: If `submitter` does not sign, this returns `None` which raises downstream
             return submitter_account.sign_transaction(exec_transaction, **signer_options)
 
         elif submit:

ape_safe/client/types.py

@@ -4,7 +4,7 @@
 
 from ape.types import AddressType, HexBytes
 from eip712.common import SafeTxV1, SafeTxV2, create_safe_tx_def
-from eth_typing import HexStr
+from eth_pydantic_types import HexStr
 from eth_utils import add_0x_prefix, to_hex
 from pydantic import BaseModel, BeforeValidator, Field, field_validator