From d04207fcfe72956730ac3f14969f3ba701290ef3 Mon Sep 17 00:00:00 2001 From: fubuloubu <3859395+fubuloubu@users.noreply.github.com> Date: Mon, 11 Mar 2024 17:38:43 -0400 Subject: [PATCH 01/22] feat(API): add delegate support to base client --- ape_safe/client/base.py | 23 +++++++++++++++++++++-- 1 file changed, 21 insertions(+), 2 deletions(-) diff --git a/ape_safe/client/base.py b/ape_safe/client/base.py index ad50fa2..28fd0b8 100644 --- a/ape_safe/client/base.py +++ b/ape_safe/client/base.py @@ -1,12 +1,16 @@ +import time from abc import ABC, abstractmethod from collections.abc import Iterator from functools import cached_property -from typing import Optional, Union +from typing import Iterator, Optional, Set, Union import certifi import requests import urllib3 -from ape.types import AddressType, MessageSignature +from ape.api import AccountAPI +from ape.types import AddressType, MessageSignature, SignableMessage +from eth_account.messages import encode_defunct as encode_eip191_signable_message +from eth_utils import keccak from requests import Response from requests.adapters import HTTPAdapter @@ -112,6 +116,21 @@ def get_transactions( yield txn + def create_delegate_message(self, delegate: AddressType) -> SignableMessage: + # NOTE: referencing https://github.com/safe-global/safe-eth-py/blob/ + # a0a5771622f143ee6301cfc381c5ed50832ff482/gnosis/safe/api/transaction_service_api.py#L34 + totp = int(time.time()) // 3600 + return encode_eip191_signable_message(keccak(text=delegate + str(totp))) + + @abstractmethod + def get_delegates(self) -> dict[AddressType, list[AddressType]]: ... + + @abstractmethod + def add_delegate(self, delegate: AddressType, label: str, delegator: AccountAPI): ... + + @abstractmethod + def remove_delegate(self, delegate: AddressType, delegator: AccountAPI): ... + """Request methods""" @cached_property From 78a41c185373ea5c4e1000bc4fb404add5beb2aa Mon Sep 17 00:00:00 2001 From: fubuloubu <3859395+fubuloubu@users.noreply.github.com> Date: Mon, 11 Mar 2024 17:39:38 -0400 Subject: [PATCH 02/22] feat(API): add delegate implementation to mock client --- ape_safe/client/mock.py | 54 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) diff --git a/ape_safe/client/mock.py b/ape_safe/client/mock.py index b424d60..b9f620f 100644 --- a/ape_safe/client/mock.py +++ b/ape_safe/client/mock.py @@ -2,6 +2,7 @@ from datetime import datetime, timezone from typing import Optional, Union, cast +from ape.api import AccountAPI from ape.contracts import ContractInstance from ape.types import AddressType, MessageSignature from ape.utils import ZERO_ADDRESS, ManagerAccessMixin @@ -18,6 +19,7 @@ SignatureType, UnexecutedTxData, ) +from ape_safe.exceptions import SafeClientException from ape_safe.utils import get_safe_tx_hash @@ -26,6 +28,7 @@ def __init__(self, contract: ContractInstance): self.contract = contract self.transactions: dict[SafeTxID, SafeApiTxData] = {} self.transactions_by_nonce: dict[int, list[SafeTxID]] = {} + self.delegates: dict[AddressType, list[AddressType]] = {} @property def safe_details(self) -> SafeDetails: @@ -75,6 +78,8 @@ def get_confirmations(self, safe_tx_hash: SafeTxID) -> Iterator[SafeTxConfirmati def post_transaction( self, safe_tx: SafeTx, signatures: dict[AddressType, MessageSignature], **kwargs ): + owners = self.safe_details.owners + safe_tx_data = UnexecutedTxData.from_safe_tx(safe_tx, self.safe_details.threshold) safe_tx_data.confirmations.extend( SafeTxConfirmation( @@ -84,7 +89,18 @@ def post_transaction( signatureType=SignatureType.EOA, ) for signer, sig in signatures.items() + if signer in owners ) + + # Ensure that if this is a zero-conf SafeTx, that at least one signature is from a delegate + # NOTE: More strict than Safe API check that silently ignores if no signatures are valid or + # from delegates, but should help us to get correct logic for mock testing purposes + if len(safe_tx_data.confirmations) == 0 and not any( + self.delegator_for_delegate(signer) in owners + for signer in filter(lambda signer: signer not in owners, signatures) + ): + raise SafeClientException("Not submitted by any valid signer or delegate.") + 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: @@ -117,3 +133,41 @@ def estimate_gas_cost( self, receiver: AddressType, value: int, data: bytes, operation: int = 0 ) -> Optional[int]: return None # Estimate gas normally + + def get_delegates(self) -> dict[AddressType, list[AddressType]]: + return self.delegates + + def delegator_for_delegate(self, delegate: AddressType) -> Optional[AddressType]: + for delegator, delegates in self.delegates.items(): + if delegate in delegates: + return delegator + + return None + + def add_delegate(self, delegate: AddressType, label: str, delegator: AccountAPI): + delegate = self.conversion_manager.convert(delegate, AddressType) + + if delegator.address not in self.safe_details.owners: + raise SafeClientException(f"'{delegator}' not a valid owner.") + + if delegator.address in self.delegates: + self.delegates[delegator.address].append(delegate) + + else: + self.delegates[delegator.address] = [delegate] + + def remove_delegate(self, delegate: AddressType, delegator: AccountAPI): + delegate = self.conversion_manager.convert(delegate, AddressType) + + if delegator.address not in self.safe_details.owners: + raise SafeClientException(f"'{delegator.address}' not a valid owner.") + + if delegator.address not in self.delegates: + raise SafeClientException( + f"'{delegate}' not a valid delegate for '{delegator.address}'." + ) + + self.delegates[delegator.address].remove(delegate) + + if len(self.delegates[delegator.address]) == 0: + del self.delegates[delegator.address] From cacad1929b1f96bfc0205ce31e744bc75a59b458 Mon Sep 17 00:00:00 2001 From: fubuloubu <3859395+fubuloubu@users.noreply.github.com> Date: Mon, 11 Mar 2024 17:41:58 -0400 Subject: [PATCH 03/22] feat(API): add delegates implementation to Safe API client --- ape_safe/client/__init__.py | 47 ++++++++++++++++++++++++++++++++++++- ape_safe/client/base.py | 9 ++++--- ape_safe/client/types.py | 7 ++++++ 3 files changed, 59 insertions(+), 4 deletions(-) diff --git a/ape_safe/client/__init__.py b/ape_safe/client/__init__.py index f1d350f..bcd629e 100644 --- a/ape_safe/client/__init__.py +++ b/ape_safe/client/__init__.py @@ -1,8 +1,9 @@ import json from datetime import datetime from functools import reduce -from typing import Dict, Iterator, Optional, Union, cast +from typing import Dict, Iterator, List, Optional, Union, cast +from ape.api import AccountAPI from ape.types import AddressType, HexBytes, MessageSignature from ape.utils.misc import USER_AGENT, get_package_version from eip712.common import SafeTxV1, SafeTxV2 @@ -10,6 +11,7 @@ from ape_safe.client.base import BaseSafeClient from ape_safe.client.mock import MockSafeClient from ape_safe.client.types import ( + DelegateInfo, ExecutedTxData, OperationType, SafeApiTxData, @@ -21,6 +23,7 @@ UnexecutedTxData, ) from ape_safe.exceptions import ( + ActionNotPerformedError, ClientResponseError, ClientUnsupportedChainError, MultisigTransactionNotFoundError, @@ -186,6 +189,48 @@ def estimate_gas_cost( gas = result.get("safeTxGas") return int(HexBytes(gas).hex(), 16) + def get_delegates(self) -> Dict[AddressType, List[AddressType]]: + url = "delegates" + delegates: Dict[AddressType, List[AddressType]] = {} + + while url: + response = self._get(url, params={"safe": self.address}) + data = response.json() + + for delegate_info in map(DelegateInfo.model_validate, data.get("results", [])): + if delegate_info.delegator not in delegates: + delegates[delegate_info.delegator] = [delegate_info.delegate] + else: + delegates[delegate_info.delegator].append(delegate_info.delegate) + + url = data.get("next") + + return delegates + + def add_delegate(self, delegate: AddressType, label: str, delegator: AccountAPI): + msg = self.create_delegate_message(delegate) + if not (sig := delegator.sign_message(msg)): + raise ActionNotPerformedError("Must sign request to add delegate.") + + payload = { + "safe": self.address, + "delegate": delegate, + "delegator": delegator.address, + "label": label, + "signature": sig.encode_rsv().hex(), + } + self._post("delegates", json=payload) + + def remove_delegate(self, delegate: AddressType, delegator: AccountAPI): + msg = self.create_delegate_message(delegate) + if not (sig := delegator.sign_message(msg)): + raise ActionNotPerformedError("Must sign request to remove delegate.") + + payload = { + "signature": sig.encode_rsv().hex(), + } + self._delete(f"delegates/{delegate}", json=payload) + __all__ = [ "ExecutedTxData", diff --git a/ape_safe/client/base.py b/ape_safe/client/base.py index 28fd0b8..a098dcb 100644 --- a/ape_safe/client/base.py +++ b/ape_safe/client/base.py @@ -120,7 +120,7 @@ def create_delegate_message(self, delegate: AddressType) -> SignableMessage: # NOTE: referencing https://github.com/safe-global/safe-eth-py/blob/ # a0a5771622f143ee6301cfc381c5ed50832ff482/gnosis/safe/api/transaction_service_api.py#L34 totp = int(time.time()) // 3600 - return encode_eip191_signable_message(keccak(text=delegate + str(totp))) + return encode_eip191_signable_message(keccak(text=(delegate + str(totp)))) @abstractmethod def get_delegates(self) -> dict[AddressType, list[AddressType]]: ... @@ -145,12 +145,15 @@ def session(self) -> requests.Session: session.mount("https://", adapter) return session - def _get(self, url: str) -> Response: - return self._request("GET", url) + def _get(self, url: str, params: Optional[Dict] = None) -> Response: + return self._request("GET", url, params=params) def _post(self, url: str, json: Optional[dict] = None, **kwargs) -> Response: return self._request("POST", url, json=json, **kwargs) + def _delete(self, url: str, json: Optional[Dict] = None, **kwargs) -> Response: + return self._request("DELETE", url, json=json, **kwargs) + @cached_property def _http(self): return urllib3.PoolManager(ca_certs=certifi.where()) diff --git a/ape_safe/client/types.py b/ape_safe/client/types.py index f3603dd..6478213 100644 --- a/ape_safe/client/types.py +++ b/ape_safe/client/types.py @@ -124,3 +124,10 @@ class ExecutedTxData(UnexecutedTxData): SafeApiTxData = Union[ExecutedTxData, UnexecutedTxData] + + +class DelegateInfo(BaseModel): + safe: AddressType + delegate: AddressType + delegator: AddressType + label: str = "" From 267f343d69690ac652eb3581e55ac3830a459a27 Mon Sep 17 00:00:00 2001 From: fubuloubu <3859395+fubuloubu@users.noreply.github.com> Date: Mon, 11 Mar 2024 17:47:26 -0400 Subject: [PATCH 04/22] feat(CLI): add support for viewing and modifying delegates --- ape_safe/_cli/__init__.py | 2 ++ ape_safe/_cli/delegates.py | 63 ++++++++++++++++++++++++++++++++++++++ 2 files changed, 65 insertions(+) create mode 100644 ape_safe/_cli/delegates.py diff --git a/ape_safe/_cli/__init__.py b/ape_safe/_cli/__init__.py index 58cd73a..4d9529f 100644 --- a/ape_safe/_cli/__init__.py +++ b/ape_safe/_cli/__init__.py @@ -1,5 +1,6 @@ import click +from ape_safe._cli.delegates import delegates from ape_safe._cli.pending import pending from ape_safe._cli.safe_mgmt import _list, add, all_txns, remove @@ -17,3 +18,4 @@ def cli(): cli.add_command(remove) cli.add_command(all_txns) cli.add_command(pending) +cli.add_command(delegates) diff --git a/ape_safe/_cli/delegates.py b/ape_safe/_cli/delegates.py new file mode 100644 index 0000000..a98dfba --- /dev/null +++ b/ape_safe/_cli/delegates.py @@ -0,0 +1,63 @@ +import click +from ape.cli import ConnectedProviderCommand +from ape.types import AddressType +from eth_typing import ChecksumAddress + +from ape_safe._cli.click_ext import callback_factory, safe_argument, safe_cli_ctx, safe_option + + +@click.group() +def delegates(): + """ + Commands for viewing and configuring delegates thru the configured Safe API + """ + + +@delegates.command("list", cls=ConnectedProviderCommand) +@safe_cli_ctx() +@safe_argument +def _list(cli_ctx, safe): + """ + Show delegates for signers in a Safe + """ + if delegates := safe.client.get_delegates(): + cli_ctx.logger.success(f"Found delegates for {safe.address} ({safe.alias})") + for delegator in delegates: + click.echo(f"\nSigner {delegator}:") + click.echo("- " + "\n- ".join(delegates[delegator])) + + else: + cli_ctx.logger.error(f"No delegates for {safe.address} ({safe.alias})") + + +@delegates.command(cls=ConnectedProviderCommand) +@safe_cli_ctx() +@safe_option +@click.argument("delegate", type=ChecksumAddress) +@click.argument("label") +@click.argument("signer", callback=callback_factory.submitter_callback) +def add(cli_ctx, safe, delegate, label, signer): + """ + Add a delegate for a specific signer in a Safe + """ + delegate = cli_ctx.conversion_manager.convert(delegate, AddressType) + safe.client.add_delegate(delegate, label, signer) + cli_ctx.logger.success( + f"Added delegate {delegate} ({label}) for {signer.address} in {safe.address} ({safe.alias})" + ) + + +@delegates.command(cls=ConnectedProviderCommand) +@safe_cli_ctx() +@safe_option +@click.argument("delegate", type=ChecksumAddress) +@click.argument("signer", callback=callback_factory.submitter_callback) +def remove(cli_ctx, safe, delegate, signer): + """ + Remove a delegate for a specific signer in a Safe + """ + delegate = cli_ctx.conversion_manager.convert(delegate, AddressType) + safe.client.remove_delegate(delegate, signer) + cli_ctx.logger.success( + f"Removed delegate {delegate} for {signer.address} in {safe.address} ({safe.alias})" + ) From fe29931b46bcd5e15028196881cdf4d25856ef56 Mon Sep 17 00:00:00 2001 From: fubuloubu <3859395+fubuloubu@users.noreply.github.com> Date: Mon, 11 Mar 2024 21:29:04 -0400 Subject: [PATCH 05/22] fix: API uses `.signHash` raw hash signing (dangerous) --- ape_safe/client/__init__.py | 40 +++++++++++++++++++++++++++++-------- ape_safe/client/base.py | 7 +++---- 2 files changed, 35 insertions(+), 12 deletions(-) diff --git a/ape_safe/client/__init__.py b/ape_safe/client/__init__.py index bcd629e..c4a6286 100644 --- a/ape_safe/client/__init__.py +++ b/ape_safe/client/__init__.py @@ -4,9 +4,12 @@ from typing import Dict, Iterator, List, Optional, Union, cast from ape.api import AccountAPI +from ape.logging import logger from ape.types import AddressType, HexBytes, MessageSignature from ape.utils.misc import USER_AGENT, get_package_version +from ape_accounts import KeyfileAccount from eip712.common import SafeTxV1, SafeTxV2 +from eth_account import Account as EthAccount from ape_safe.client.base import BaseSafeClient from ape_safe.client.mock import MockSafeClient @@ -208,26 +211,47 @@ def get_delegates(self) -> Dict[AddressType, List[AddressType]]: return delegates def add_delegate(self, delegate: AddressType, label: str, delegator: AccountAPI): - msg = self.create_delegate_message(delegate) - if not (sig := delegator.sign_message(msg)): - raise ActionNotPerformedError("Must sign request to add delegate.") + # TODO: Replace this by adding raw hash signing into supported account plugins + if not isinstance(delegator, KeyfileAccount): + raise ActionNotPerformedError("Need access to private key for this method.") + + logger.warning("Need to unlock account to add a delegate.") + delegator.unlock() # NOTE: Ensures we have the key handy + + msg_hash = self.create_delegate_message(delegate) + # NOTE: This is required as Safe API uses an antiquated .signHash method + sig = EthAccount.signHash( + msg_hash, + delegator._KeyfileAccount__cached_key, # type: ignore[attr-defined] + ) payload = { "safe": self.address, "delegate": delegate, "delegator": delegator.address, "label": label, - "signature": sig.encode_rsv().hex(), + "signature": sig.signature.hex(), } self._post("delegates", json=payload) def remove_delegate(self, delegate: AddressType, delegator: AccountAPI): - msg = self.create_delegate_message(delegate) - if not (sig := delegator.sign_message(msg)): - raise ActionNotPerformedError("Must sign request to remove delegate.") + # TODO: Replace this by adding raw hash signing into supported account plugins + if not isinstance(delegator, KeyfileAccount): + raise ActionNotPerformedError("Need access to private key for this method.") + + logger.warning("Need to unlock account to add a delegate.") + delegator.unlock() # NOTE: Ensures we have the key handy + + msg_hash = self.create_delegate_message(delegate) + # NOTE: This is required as Safe API uses an antiquated .signHash method + sig = EthAccount.signHash( + msg_hash, + delegator._KeyfileAccount__cached_key, # type: ignore[attr-defined] + ) payload = { - "signature": sig.encode_rsv().hex(), + "delegator": delegator.address, + "signature": sig.signature.hex(), } self._delete(f"delegates/{delegate}", json=payload) diff --git a/ape_safe/client/base.py b/ape_safe/client/base.py index a098dcb..b34b3b9 100644 --- a/ape_safe/client/base.py +++ b/ape_safe/client/base.py @@ -8,8 +8,7 @@ import requests import urllib3 from ape.api import AccountAPI -from ape.types import AddressType, MessageSignature, SignableMessage -from eth_account.messages import encode_defunct as encode_eip191_signable_message +from ape.types import AddressType, MessageSignature from eth_utils import keccak from requests import Response from requests.adapters import HTTPAdapter @@ -116,11 +115,11 @@ def get_transactions( yield txn - def create_delegate_message(self, delegate: AddressType) -> SignableMessage: + def create_delegate_message(self, delegate: AddressType) -> bytes: # NOTE: referencing https://github.com/safe-global/safe-eth-py/blob/ # a0a5771622f143ee6301cfc381c5ed50832ff482/gnosis/safe/api/transaction_service_api.py#L34 totp = int(time.time()) // 3600 - return encode_eip191_signable_message(keccak(text=(delegate + str(totp)))) + return keccak(text=(delegate + str(totp))) @abstractmethod def get_delegates(self) -> dict[AddressType, list[AddressType]]: ... From a1ef8f86fc0f2a8c49235913c61916e43b486c31 Mon Sep 17 00:00:00 2001 From: fubuloubu <3859395+fubuloubu@users.noreply.github.com> Date: Mon, 11 Mar 2024 22:31:52 -0400 Subject: [PATCH 06/22] fix(API): client accepts empty signatures --- ape_safe/client/__init__.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/ape_safe/client/__init__.py b/ape_safe/client/__init__.py index c4a6286..8da2642 100644 --- a/ape_safe/client/__init__.py +++ b/ape_safe/client/__init__.py @@ -133,7 +133,7 @@ def post_transaction( b"", ) ) - post_dict: Dict = {"signature": signature.hex(), "origin": ORIGIN} + post_dict: Dict = {"signature": signature.hex() if signature else None, "origin": ORIGIN} for key, value in tx_data.model_dump(by_alias=True, mode="json").items(): if isinstance(value, HexBytes): @@ -212,6 +212,7 @@ def get_delegates(self) -> Dict[AddressType, List[AddressType]]: def add_delegate(self, delegate: AddressType, label: str, delegator: AccountAPI): # TODO: Replace this by adding raw hash signing into supported account plugins + # See: https://github.com/ApeWorX/ape/issues/1962 if not isinstance(delegator, KeyfileAccount): raise ActionNotPerformedError("Need access to private key for this method.") @@ -236,6 +237,7 @@ def add_delegate(self, delegate: AddressType, label: str, delegator: AccountAPI) def remove_delegate(self, delegate: AddressType, delegator: AccountAPI): # TODO: Replace this by adding raw hash signing into supported account plugins + # See: https://github.com/ApeWorX/ape/issues/1962 if not isinstance(delegator, KeyfileAccount): raise ActionNotPerformedError("Need access to private key for this method.") From 35cae2d0a2c385e5c3f0e535aecc433540892665 Mon Sep 17 00:00:00 2001 From: fubuloubu <3859395+fubuloubu@users.noreply.github.com> Date: Mon, 11 Mar 2024 22:40:11 -0400 Subject: [PATCH 07/22] feat(account): add method to propose a SafeTx directly --- ape_safe/accounts.py | 61 ++++++++++++++++++++++++++++++++++++-------- 1 file changed, 51 insertions(+), 10 deletions(-) diff --git a/ape_safe/accounts.py b/ape_safe/accounts.py index 04c4bf7..047488f 100644 --- a/ape_safe/accounts.py +++ b/ape_safe/accounts.py @@ -2,7 +2,7 @@ import os from collections.abc import Iterable, Iterator, Mapping from pathlib import Path -from typing import Any, Dict, Optional, Union, cast +from typing import Any, Optional, Union, cast from ape.api import AccountAPI, AccountContainerAPI, ReceiptAPI, TransactionAPI from ape.api.address import BaseAddress @@ -180,7 +180,7 @@ def _get_path(self, alias: str) -> Path: def get_signatures( safe_tx: SafeTx, signers: Iterable[AccountAPI], -) -> Dict[AddressType, MessageSignature]: +) -> dict[AddressType, MessageSignature]: signatures: Dict[AddressType, MessageSignature] = {} for signer in signers: signature = signer.sign_message(safe_tx) @@ -362,6 +362,48 @@ def create_safe_tx(self, txn: Optional[TransactionAPI] = None, **safe_tx_kwargs) } return self.safe_tx_def(**safe_tx) + def all_delegates(self) -> Iterator[AddressType]: + for delegates in self.client.get_delegates().values(): + yield from delegates + + def propose_safe_tx( + self, + safe_tx: SafeTx, + submitter: Union[AccountAPI, AddressType, str, None] = None, + sigs_by_signer: Optional[dict[AddressType, MessageSignature]] = None, + contractTransactionHash: Optional[SafeTxID] = None, + ) -> SafeTxID: + """ + Propose a transaction to the Safe API client + """ + if not contractTransactionHash: + contractTransactionHash = get_safe_tx_hash(safe_tx) + + if not sigs_by_signer: + sigs_by_signer = {} + + if submitter is not None and not isinstance(submitter, AccountAPI): + submitter = self.load_submitter(submitter) + + if ( + submitter is not None + and submitter.address not in sigs_by_signer + and len(sigs_by_signer) < self.confirmations_required + and (submitter.address in self.signers or submitter.address in self.all_delegates()) + ): + if sig := submitter.sign_message(safe_tx): + sigs_by_signer[submitter.address] = sig + + # NOTE: Signatures don't have to be in order for Safe API post + self.client.post_transaction( + safe_tx, + sigs_by_signer, + sender=submitter.address if submitter else None, + contractTransactionHash=contractTransactionHash, + ) + + return contractTransactionHash + def pending_transactions(self) -> Iterator[tuple[SafeTx, list[SafeTxConfirmation]]]: for executed_tx in self.client.get_transactions(confirmed=False): yield self.create_safe_tx( @@ -533,7 +575,7 @@ def call( # type: ignore[override] return super().call(txn, **call_kwargs) - def get_api_confirmations(self, safe_tx: SafeTx) -> Dict[AddressType, MessageSignature]: + def get_api_confirmations(self, safe_tx: SafeTx) -> dict[AddressType, MessageSignature]: safe_tx_id = get_safe_tx_hash(safe_tx) try: client_confirmations = self.client.get_confirmations(safe_tx_id) @@ -558,7 +600,7 @@ def _contract_approvals(self, safe_tx: SafeTx) -> Mapping[AddressType, MessageSi if self.contract.approvedHashes(signer, safe_tx_hash) > 0 } - def _all_approvals(self, safe_tx: SafeTx) -> Dict[AddressType, MessageSignature]: + def _all_approvals(self, safe_tx: SafeTx) -> dict[AddressType, MessageSignature]: approvals = self.get_api_confirmations(safe_tx) # NOTE: Do this last because it should take precedence @@ -609,7 +651,7 @@ def sign_transaction( submit (bool): The option to submit the transaction. Defaults to ``True``. submitter (Union[``AccountAPI``, ``AddressType``, str, None]): Determine who is submitting the transaction. Defaults to ``None``. - skip (Optional[List[Union[``AccountAPI, `AddressType``, str]]]): + skip (Optional[list[Union[``AccountAPI, `AddressType``, str]]]): Allow bypassing any specified signer. Defaults to ``None``. signatures_required (Optional[int]): The amount of signers required to confirm the transaction. Defaults to ``None``. @@ -709,12 +751,11 @@ def skip_signer(signer: AccountAPI): f"for Safe {self.address}#{safe_tx.nonce}" # TODO: put URI ) - # NOTE: Signatures don't have to be in order for Safe API post - self.client.post_transaction( + self.propose_safe_tx( safe_tx, - sigs_by_signer, + submitter=submitter_account, + sigs_by_signer=sigs_by_signer, contractTransactionHash=safe_tx_hash, - sender=submitter_account.address, ) # Return None so that Ape does not try to submit the transaction. @@ -722,7 +763,7 @@ def skip_signer(signer: AccountAPI): def add_signatures( self, safe_tx: SafeTx, confirmations: Optional[list[SafeTxConfirmation]] = None - ) -> Dict[AddressType, MessageSignature]: + ) -> dict[AddressType, MessageSignature]: confirmations = confirmations or [] if not self.local_signers: raise ApeSafeError("Cannot sign without local signers.") From a07dd450a88de756b00c094c7b8fd563f6b37124 Mon Sep 17 00:00:00 2001 From: fubuloubu <3859395+fubuloubu@users.noreply.github.com> Date: Tue, 12 Mar 2024 15:15:37 -0400 Subject: [PATCH 08/22] test: add tests for delegate feature (using MockClient) --- tests/conftest.py | 22 ++++++++++++++++ tests/functional/test_delegates.py | 41 ++++++++++++++++++++++++++++++ 2 files changed, 63 insertions(+) create mode 100644 tests/functional/test_delegates.py diff --git a/tests/conftest.py b/tests/conftest.py index 66db46f..cbff25e 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -6,6 +6,8 @@ import pytest from ape.contracts import ContractContainer from ape.utils import ZERO_ADDRESS +from ape_test import TestAccount +from eip712 import EIP712Message from ethpm_types import ContractType from ape_safe import MultiSend @@ -15,6 +17,21 @@ TESTS_DIR = Path(__file__).parent.absolute() +@pytest.fixture(autouse=True) +def fix_eip712_signing(monkeypatch): + # TODO: `ape_test.TestAccount.sign_message` doesn't support `EIP712Message` yet + # See: https://github.com/ApeWorX/ape/issues/1961 + original_sign_message = TestAccount.sign_message + + def modified_sign_message(self, msg): + if isinstance(msg, EIP712Message): + msg = msg.signable_message + + return original_sign_message(self, msg) + + monkeypatch.setattr(TestAccount, "sign_message", modified_sign_message) + + @pytest.fixture(scope="session", autouse=True) def config(): cfg = ape.config @@ -40,6 +57,11 @@ def receiver(accounts): return accounts[9] +@pytest.fixture(scope="session") +def delegate(accounts): + return accounts[8] + + @pytest.fixture(scope="session", params=["1.3.0"]) # TODO: Test more versions later? def VERSION(request): return request.param diff --git a/tests/functional/test_delegates.py b/tests/functional/test_delegates.py new file mode 100644 index 0000000..9b1eeec --- /dev/null +++ b/tests/functional/test_delegates.py @@ -0,0 +1,41 @@ +import pytest + +from ape_safe.exceptions import SafeClientException + + +def test_manage_delegates(safe, delegate, OWNERS): + owner = OWNERS[0] + assert owner.address not in safe.client.get_delegates() + + safe.client.add_delegate(delegate, "pepito", owner) + assert delegate.address in safe.client.get_delegates()[owner.address] + assert delegate.address in safe.all_delegates() + # NOTE: Only in MockSafeClient + assert safe.client.delegator_for_delegate(delegate.address) == owner.address + + safe.client.remove_delegate(delegate, owner) + assert owner.address not in safe.client.get_delegates() + + with pytest.raises(SafeClientException): + # Only signers can create a delegate + safe.client.add_delegate(owner, "root privledges", delegate) + + +def test_delegate_can_propose_safe_tx(safe, delegate, OWNERS): + owner = OWNERS[0] + + safe_tx = safe.create_safe_tx( + safe.contract.addOwnerWithThreshold.as_transaction(delegate, safe.confirmations_required) + ) + + with pytest.raises(SafeClientException): + # Not a delegate or signer + safe.propose_safe_tx(safe_tx, submitter=delegate) + + safe.client.add_delegate(delegate, "pepito", owner) + safe.propose_safe_tx(safe_tx, submitter=delegate) + + assert len(safe.get_api_confirmations(safe_tx)) == 0 + assert list( + (safe_tx.signable_message, confs) for safe_tx, confs in safe.pending_transactions() + ) == [(safe_tx.signable_message, [])] From b04382a234f06b96c06b28ffea1483957181f68d Mon Sep 17 00:00:00 2001 From: El De-dog-lo <3859395+fubuloubu@users.noreply.github.com> Date: Wed, 5 Jun 2024 11:36:01 -0400 Subject: [PATCH 09/22] refactor: apply suggestions from code review Co-authored-by: antazoey --- ape_safe/_cli/delegates.py | 4 ++-- ape_safe/client/mock.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/ape_safe/_cli/delegates.py b/ape_safe/_cli/delegates.py index a98dfba..50299be 100644 --- a/ape_safe/_cli/delegates.py +++ b/ape_safe/_cli/delegates.py @@ -9,7 +9,7 @@ @click.group() def delegates(): """ - Commands for viewing and configuring delegates thru the configured Safe API + View and configure delegates """ @@ -38,7 +38,7 @@ def _list(cli_ctx, safe): @click.argument("signer", callback=callback_factory.submitter_callback) def add(cli_ctx, safe, delegate, label, signer): """ - Add a delegate for a specific signer in a Safe + Add a delegate for a signer in a Safe """ delegate = cli_ctx.conversion_manager.convert(delegate, AddressType) safe.client.add_delegate(delegate, label, signer) diff --git a/ape_safe/client/mock.py b/ape_safe/client/mock.py index b9f620f..00cd155 100644 --- a/ape_safe/client/mock.py +++ b/ape_safe/client/mock.py @@ -162,7 +162,7 @@ def remove_delegate(self, delegate: AddressType, delegator: AccountAPI): if delegator.address not in self.safe_details.owners: raise SafeClientException(f"'{delegator.address}' not a valid owner.") - if delegator.address not in self.delegates: + elif delegator.address not in self.delegates: raise SafeClientException( f"'{delegate}' not a valid delegate for '{delegator.address}'." ) From c3e1e1c9397f672f8df8fc80384a4b14e3959421 Mon Sep 17 00:00:00 2001 From: doggie <3859395+fubuloubu@users.noreply.github.com> Date: Wed, 5 Jun 2024 11:39:06 -0400 Subject: [PATCH 10/22] fix: update typing again --- ape_safe/accounts.py | 4 ++-- ape_safe/client/base.py | 7 +++---- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/ape_safe/accounts.py b/ape_safe/accounts.py index 047488f..7538cf9 100644 --- a/ape_safe/accounts.py +++ b/ape_safe/accounts.py @@ -39,7 +39,7 @@ class SafeContainer(AccountContainerAPI): - _accounts: Dict[str, "SafeAccount"] = {} + _accounts: dict[str, "SafeAccount"] = {} @property def _account_files(self) -> Iterator[Path]: @@ -181,7 +181,7 @@ def get_signatures( safe_tx: SafeTx, signers: Iterable[AccountAPI], ) -> dict[AddressType, MessageSignature]: - signatures: Dict[AddressType, MessageSignature] = {} + signatures: dict[AddressType, MessageSignature] = {} for signer in signers: signature = signer.sign_message(safe_tx) if signature: diff --git a/ape_safe/client/base.py b/ape_safe/client/base.py index b34b3b9..138d6dd 100644 --- a/ape_safe/client/base.py +++ b/ape_safe/client/base.py @@ -1,8 +1,7 @@ import time from abc import ABC, abstractmethod -from collections.abc import Iterator from functools import cached_property -from typing import Iterator, Optional, Set, Union +from typing import Iterator, Optional, Union import certifi import requests @@ -144,13 +143,13 @@ def session(self) -> requests.Session: session.mount("https://", adapter) return session - def _get(self, url: str, params: Optional[Dict] = None) -> Response: + def _get(self, url: str, params: Optional[dict] = None) -> Response: return self._request("GET", url, params=params) def _post(self, url: str, json: Optional[dict] = None, **kwargs) -> Response: return self._request("POST", url, json=json, **kwargs) - def _delete(self, url: str, json: Optional[Dict] = None, **kwargs) -> Response: + def _delete(self, url: str, json: Optional[dict] = None, **kwargs) -> Response: return self._request("DELETE", url, json=json, **kwargs) @cached_property From 9973b4b140e1977b8a79990e018bf9623d99575c Mon Sep 17 00:00:00 2001 From: doggie <3859395+fubuloubu@users.noreply.github.com> Date: Wed, 5 Jun 2024 12:34:05 -0400 Subject: [PATCH 11/22] refactor: use asserts rather than excepiton for mock --- ape_safe/client/mock.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/ape_safe/client/mock.py b/ape_safe/client/mock.py index 00cd155..a8b6862 100644 --- a/ape_safe/client/mock.py +++ b/ape_safe/client/mock.py @@ -95,11 +95,12 @@ def post_transaction( # Ensure that if this is a zero-conf SafeTx, that at least one signature is from a delegate # NOTE: More strict than Safe API check that silently ignores if no signatures are valid or # from delegates, but should help us to get correct logic for mock testing purposes - if len(safe_tx_data.confirmations) == 0 and not any( + # NOTE: Using `assert` because this client is only meant for mock testing purposes + assert len(safe_tx_data.confirmations) > 0 + assert all( self.delegator_for_delegate(signer) in owners for signer in filter(lambda signer: signer not in owners, signatures) - ): - raise SafeClientException("Not submitted by any valid signer or delegate.") + ) tx_id = cast(SafeTxID, HexBytes(safe_tx_data.safe_tx_hash).hex()) self.transactions[tx_id] = safe_tx_data From 5ce6c14480534a69955411f7f5a4de3842a51405 Mon Sep 17 00:00:00 2001 From: doggie <3859395+fubuloubu@users.noreply.github.com> Date: Wed, 5 Jun 2024 13:51:02 -0400 Subject: [PATCH 12/22] refactor: don't add extra conversion logic in mock --- ape_safe/client/mock.py | 4 ---- tests/functional/test_delegates.py | 6 +++--- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/ape_safe/client/mock.py b/ape_safe/client/mock.py index a8b6862..28b859b 100644 --- a/ape_safe/client/mock.py +++ b/ape_safe/client/mock.py @@ -146,8 +146,6 @@ def delegator_for_delegate(self, delegate: AddressType) -> Optional[AddressType] return None def add_delegate(self, delegate: AddressType, label: str, delegator: AccountAPI): - delegate = self.conversion_manager.convert(delegate, AddressType) - if delegator.address not in self.safe_details.owners: raise SafeClientException(f"'{delegator}' not a valid owner.") @@ -158,8 +156,6 @@ def add_delegate(self, delegate: AddressType, label: str, delegator: AccountAPI) self.delegates[delegator.address] = [delegate] def remove_delegate(self, delegate: AddressType, delegator: AccountAPI): - delegate = self.conversion_manager.convert(delegate, AddressType) - if delegator.address not in self.safe_details.owners: raise SafeClientException(f"'{delegator.address}' not a valid owner.") diff --git a/tests/functional/test_delegates.py b/tests/functional/test_delegates.py index 9b1eeec..9270098 100644 --- a/tests/functional/test_delegates.py +++ b/tests/functional/test_delegates.py @@ -7,13 +7,13 @@ def test_manage_delegates(safe, delegate, OWNERS): owner = OWNERS[0] assert owner.address not in safe.client.get_delegates() - safe.client.add_delegate(delegate, "pepito", owner) + safe.client.add_delegate(delegate.address, "pepito", owner) assert delegate.address in safe.client.get_delegates()[owner.address] assert delegate.address in safe.all_delegates() # NOTE: Only in MockSafeClient assert safe.client.delegator_for_delegate(delegate.address) == owner.address - safe.client.remove_delegate(delegate, owner) + safe.client.remove_delegate(delegate.address, owner) assert owner.address not in safe.client.get_delegates() with pytest.raises(SafeClientException): @@ -32,7 +32,7 @@ def test_delegate_can_propose_safe_tx(safe, delegate, OWNERS): # Not a delegate or signer safe.propose_safe_tx(safe_tx, submitter=delegate) - safe.client.add_delegate(delegate, "pepito", owner) + safe.client.add_delegate(delegate.address, "pepito", owner) safe.propose_safe_tx(safe_tx, submitter=delegate) assert len(safe.get_api_confirmations(safe_tx)) == 0 From 1aab28a05146c8553b260605d0e8d61636c65ac8 Mon Sep 17 00:00:00 2001 From: doggie <3859395+fubuloubu@users.noreply.github.com> Date: Wed, 5 Jun 2024 13:55:38 -0400 Subject: [PATCH 13/22] refactor(CLI): change `ERROR` to `INFO` for no delegates found --- ape_safe/_cli/delegates.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ape_safe/_cli/delegates.py b/ape_safe/_cli/delegates.py index 50299be..d28664c 100644 --- a/ape_safe/_cli/delegates.py +++ b/ape_safe/_cli/delegates.py @@ -27,7 +27,7 @@ def _list(cli_ctx, safe): click.echo("- " + "\n- ".join(delegates[delegator])) else: - cli_ctx.logger.error(f"No delegates for {safe.address} ({safe.alias})") + cli_ctx.logger.info(f"No delegates for {safe.address} ({safe.alias})") @delegates.command(cls=ConnectedProviderCommand) From b9528e78cd0080c78c98156e68a1a2428ee1526a Mon Sep 17 00:00:00 2001 From: doggie <3859395+fubuloubu@users.noreply.github.com> Date: Wed, 5 Jun 2024 18:32:47 -0400 Subject: [PATCH 14/22] refactor: use new `AccountAPI.sign_raw_msghash` method --- ape_safe/client/__init__.py | 17 ++++------------- ape_safe/client/base.py | 6 +++--- 2 files changed, 7 insertions(+), 16 deletions(-) diff --git a/ape_safe/client/__init__.py b/ape_safe/client/__init__.py index 8da2642..2265a5e 100644 --- a/ape_safe/client/__init__.py +++ b/ape_safe/client/__init__.py @@ -211,27 +211,18 @@ def get_delegates(self) -> Dict[AddressType, List[AddressType]]: return delegates def add_delegate(self, delegate: AddressType, label: str, delegator: AccountAPI): - # TODO: Replace this by adding raw hash signing into supported account plugins - # See: https://github.com/ApeWorX/ape/issues/1962 - if not isinstance(delegator, KeyfileAccount): - raise ActionNotPerformedError("Need access to private key for this method.") - - logger.warning("Need to unlock account to add a delegate.") - delegator.unlock() # NOTE: Ensures we have the key handy - msg_hash = self.create_delegate_message(delegate) + # NOTE: This is required as Safe API uses an antiquated .signHash method - sig = EthAccount.signHash( - msg_hash, - delegator._KeyfileAccount__cached_key, # type: ignore[attr-defined] - ) + if not (sig := delegator.sign_raw_msghash(msg_hash)): + raise ActionNotPerformedError("Did not sign delegation") payload = { "safe": self.address, "delegate": delegate, "delegator": delegator.address, "label": label, - "signature": sig.signature.hex(), + "signature": sig.encode_rsv().hex(), } self._post("delegates", json=payload) diff --git a/ape_safe/client/base.py b/ape_safe/client/base.py index 138d6dd..662a7b2 100644 --- a/ape_safe/client/base.py +++ b/ape_safe/client/base.py @@ -7,7 +7,7 @@ import requests import urllib3 from ape.api import AccountAPI -from ape.types import AddressType, MessageSignature +from ape.types import AddressType, HexBytes, MessageSignature from eth_utils import keccak from requests import Response from requests.adapters import HTTPAdapter @@ -114,11 +114,11 @@ def get_transactions( yield txn - def create_delegate_message(self, delegate: AddressType) -> bytes: + def create_delegate_message(self, delegate: AddressType) -> HexBytes: # NOTE: referencing https://github.com/safe-global/safe-eth-py/blob/ # a0a5771622f143ee6301cfc381c5ed50832ff482/gnosis/safe/api/transaction_service_api.py#L34 totp = int(time.time()) // 3600 - return keccak(text=(delegate + str(totp))) + return HexBytes(keccak(text=(delegate + str(totp)))) @abstractmethod def get_delegates(self) -> dict[AddressType, list[AddressType]]: ... From 04184b21a6666017b28af56fb1847e8d856d9375 Mon Sep 17 00:00:00 2001 From: doggie <3859395+fubuloubu@users.noreply.github.com> Date: Wed, 5 Jun 2024 18:43:03 -0400 Subject: [PATCH 15/22] fix: forgot to update removal too --- ape_safe/client/__init__.py | 19 +++++-------------- 1 file changed, 5 insertions(+), 14 deletions(-) diff --git a/ape_safe/client/__init__.py b/ape_safe/client/__init__.py index 2265a5e..e663eca 100644 --- a/ape_safe/client/__init__.py +++ b/ape_safe/client/__init__.py @@ -215,7 +215,7 @@ def add_delegate(self, delegate: AddressType, label: str, delegator: AccountAPI) # NOTE: This is required as Safe API uses an antiquated .signHash method if not (sig := delegator.sign_raw_msghash(msg_hash)): - raise ActionNotPerformedError("Did not sign delegation") + raise ActionNotPerformedError("Did not sign delegate approval") payload = { "safe": self.address, @@ -227,24 +227,15 @@ def add_delegate(self, delegate: AddressType, label: str, delegator: AccountAPI) self._post("delegates", json=payload) def remove_delegate(self, delegate: AddressType, delegator: AccountAPI): - # TODO: Replace this by adding raw hash signing into supported account plugins - # See: https://github.com/ApeWorX/ape/issues/1962 - if not isinstance(delegator, KeyfileAccount): - raise ActionNotPerformedError("Need access to private key for this method.") - - logger.warning("Need to unlock account to add a delegate.") - delegator.unlock() # NOTE: Ensures we have the key handy - msg_hash = self.create_delegate_message(delegate) + # NOTE: This is required as Safe API uses an antiquated .signHash method - sig = EthAccount.signHash( - msg_hash, - delegator._KeyfileAccount__cached_key, # type: ignore[attr-defined] - ) + if not (sig := delegator.sign_raw_msghash(msg_hash)): + raise ActionNotPerformedError("Did not sign delegate removal") payload = { "delegator": delegator.address, - "signature": sig.signature.hex(), + "signature": sig.encode_rsv().hex(), } self._delete(f"delegates/{delegate}", json=payload) From dd1478c7022781677143092bf37ec7c1d1111d8d Mon Sep 17 00:00:00 2001 From: doggie <3859395+fubuloubu@users.noreply.github.com> Date: Wed, 5 Jun 2024 18:48:08 -0400 Subject: [PATCH 16/22] refactor: use `account_option` instead of `callback_factory` --- ape_safe/_cli/delegates.py | 21 +++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/ape_safe/_cli/delegates.py b/ape_safe/_cli/delegates.py index d28664c..58dc12a 100644 --- a/ape_safe/_cli/delegates.py +++ b/ape_safe/_cli/delegates.py @@ -1,9 +1,9 @@ import click -from ape.cli import ConnectedProviderCommand +from ape.cli import ConnectedProviderCommand, account_option from ape.types import AddressType from eth_typing import ChecksumAddress -from ape_safe._cli.click_ext import callback_factory, safe_argument, safe_cli_ctx, safe_option +from ape_safe._cli.click_ext import safe_argument, safe_cli_ctx, safe_option @click.group() @@ -35,15 +35,16 @@ def _list(cli_ctx, safe): @safe_option @click.argument("delegate", type=ChecksumAddress) @click.argument("label") -@click.argument("signer", callback=callback_factory.submitter_callback) -def add(cli_ctx, safe, delegate, label, signer): +@account_option() +def add(cli_ctx, safe, delegate, label, account): """ Add a delegate for a signer in a Safe """ delegate = cli_ctx.conversion_manager.convert(delegate, AddressType) - safe.client.add_delegate(delegate, label, signer) + safe.client.add_delegate(delegate, label, account) cli_ctx.logger.success( - f"Added delegate {delegate} ({label}) for {signer.address} in {safe.address} ({safe.alias})" + f"Added delegate {delegate} ({label}) for {account.address} " + f"in {safe.address} ({safe.alias})" ) @@ -51,13 +52,13 @@ def add(cli_ctx, safe, delegate, label, signer): @safe_cli_ctx() @safe_option @click.argument("delegate", type=ChecksumAddress) -@click.argument("signer", callback=callback_factory.submitter_callback) -def remove(cli_ctx, safe, delegate, signer): +@account_option() +def remove(cli_ctx, safe, delegate, account): """ Remove a delegate for a specific signer in a Safe """ delegate = cli_ctx.conversion_manager.convert(delegate, AddressType) - safe.client.remove_delegate(delegate, signer) + safe.client.remove_delegate(delegate, account) cli_ctx.logger.success( - f"Removed delegate {delegate} for {signer.address} in {safe.address} ({safe.alias})" + f"Removed delegate {delegate} for {account.address} in {safe.address} ({safe.alias})" ) From 7c258a2b9a561a0fc137fc0166239cf43b1ac35c Mon Sep 17 00:00:00 2001 From: doggie <3859395+fubuloubu@users.noreply.github.com> Date: Wed, 5 Jun 2024 18:52:09 -0400 Subject: [PATCH 17/22] fix: unused imports --- ape_safe/client/__init__.py | 3 --- 1 file changed, 3 deletions(-) diff --git a/ape_safe/client/__init__.py b/ape_safe/client/__init__.py index e663eca..24b5b0b 100644 --- a/ape_safe/client/__init__.py +++ b/ape_safe/client/__init__.py @@ -4,12 +4,9 @@ from typing import Dict, Iterator, List, Optional, Union, cast from ape.api import AccountAPI -from ape.logging import logger from ape.types import AddressType, HexBytes, MessageSignature from ape.utils.misc import USER_AGENT, get_package_version -from ape_accounts import KeyfileAccount from eip712.common import SafeTxV1, SafeTxV2 -from eth_account import Account as EthAccount from ape_safe.client.base import BaseSafeClient from ape_safe.client.mock import MockSafeClient From 54df67fe1fc2e484c20d54d1558ccdacc3103239 Mon Sep 17 00:00:00 2001 From: doggie <3859395+fubuloubu@users.noreply.github.com> Date: Wed, 5 Jun 2024 19:05:23 -0400 Subject: [PATCH 18/22] fix: asserts were wrong --- ape_safe/client/mock.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/ape_safe/client/mock.py b/ape_safe/client/mock.py index 28b859b..d31b964 100644 --- a/ape_safe/client/mock.py +++ b/ape_safe/client/mock.py @@ -95,12 +95,12 @@ def post_transaction( # Ensure that if this is a zero-conf SafeTx, that at least one signature is from a delegate # NOTE: More strict than Safe API check that silently ignores if no signatures are valid or # from delegates, but should help us to get correct logic for mock testing purposes - # NOTE: Using `assert` because this client is only meant for mock testing purposes - assert len(safe_tx_data.confirmations) > 0 - assert all( - self.delegator_for_delegate(signer) in owners - for signer in filter(lambda signer: signer not in owners, signatures) - ) + if len(safe_tx_data.confirmations) == 0: + # NOTE: Using `assert` because this client is only meant for mock testing purposes + assert any( + self.delegator_for_delegate(signer) in owners + for signer in filter(lambda signer: signer not in owners, signatures) + ), "At least one signature must be from a valid owner of the safe" tx_id = cast(SafeTxID, HexBytes(safe_tx_data.safe_tx_hash).hex()) self.transactions[tx_id] = safe_tx_data From ff4a21f770723289202a4e6f14acac8bb6412fa4 Mon Sep 17 00:00:00 2001 From: doggie <3859395+fubuloubu@users.noreply.github.com> Date: Wed, 5 Jun 2024 19:45:37 -0400 Subject: [PATCH 19/22] fix: use SafeClientException to mimic real environment --- ape_safe/client/mock.py | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/ape_safe/client/mock.py b/ape_safe/client/mock.py index d31b964..2d3a6f1 100644 --- a/ape_safe/client/mock.py +++ b/ape_safe/client/mock.py @@ -95,12 +95,14 @@ def post_transaction( # Ensure that if this is a zero-conf SafeTx, that at least one signature is from a delegate # NOTE: More strict than Safe API check that silently ignores if no signatures are valid or # from delegates, but should help us to get correct logic for mock testing purposes - if len(safe_tx_data.confirmations) == 0: - # NOTE: Using `assert` because this client is only meant for mock testing purposes - assert any( - self.delegator_for_delegate(signer) in owners - for signer in filter(lambda signer: signer not in owners, signatures) - ), "At least one signature must be from a valid owner of the safe" + if len(safe_tx_data.confirmations) == 0 and not any( + self.delegator_for_delegate(delegate) in owners + for delegate in filter(lambda signer: signer not in owners, signatures) + ): + # NOTE: mimic real exception for mock testing purposes + raise SafeClientException( + "At least one signature must be from a valid owner of the safe" + ) tx_id = cast(SafeTxID, HexBytes(safe_tx_data.safe_tx_hash).hex()) self.transactions[tx_id] = safe_tx_data From b0744d2483a98e22f9fa15f62be500af71b9ccad Mon Sep 17 00:00:00 2001 From: doggie <3859395+fubuloubu@users.noreply.github.com> Date: Wed, 5 Jun 2024 19:46:06 -0400 Subject: [PATCH 20/22] chore: ignore not helpful logs to reduce display overload for testing --- .github/workflows/test.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index b3c148f..dc3a96e 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -96,4 +96,4 @@ jobs: run: npm install "@gnosis.pm/safe-contracts@${{ matrix.safe-version }}" - name: Run Tests - run: pytest -n 0 -s --cov + run: ape test -s --cov -v WARNING From 4879720caeea5bebc6c7a7f5d3807d4ceba8eb73 Mon Sep 17 00:00:00 2001 From: doggie <3859395+fubuloubu@users.noreply.github.com> Date: Wed, 5 Jun 2024 20:03:25 -0400 Subject: [PATCH 21/22] chore: run ape tests and integration tests with different commands --- .github/workflows/test.yaml | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/.github/workflows/test.yaml b/.github/workflows/test.yaml index dc3a96e..2d3d6b6 100644 --- a/.github/workflows/test.yaml +++ b/.github/workflows/test.yaml @@ -95,5 +95,8 @@ jobs: - name: Install gnosis safe run: npm install "@gnosis.pm/safe-contracts@${{ matrix.safe-version }}" - - name: Run Tests - run: ape test -s --cov -v WARNING + - name: Run Functional Tests + run: ape test tests/functional/ -s --cov -v WARNING + + - name: Run Integration Tests + run: pytest tests/integration/ -s --cov From 318f4e9f1b103cbe7542674b0f3a21d51046ca5f Mon Sep 17 00:00:00 2001 From: doggie <3859395+fubuloubu@users.noreply.github.com> Date: Wed, 5 Jun 2024 20:52:59 -0400 Subject: [PATCH 22/22] feat: add `SafeAccount.propose` as a shortcut to propose to Safe API --- ape_safe/accounts.py | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/ape_safe/accounts.py b/ape_safe/accounts.py index 7538cf9..08f9fa2 100644 --- a/ape_safe/accounts.py +++ b/ape_safe/accounts.py @@ -374,7 +374,7 @@ def propose_safe_tx( contractTransactionHash: Optional[SafeTxID] = None, ) -> SafeTxID: """ - Propose a transaction to the Safe API client + Propose a safe_tx to the Safe API client """ if not contractTransactionHash: contractTransactionHash = get_safe_tx_hash(safe_tx) @@ -404,6 +404,18 @@ def propose_safe_tx( return contractTransactionHash + def propose( + self, + txn: Optional[TransactionAPI] = None, + submitter: Union[AccountAPI, AddressType, str, None] = None, + **safe_tx_kwargs, + ) -> SafeTxID: + """ + Propose a transaction to the Safe API client + """ + safe_tx = self.create_safe_tx(txn=txn, **safe_tx_kwargs) + return self.propose_safe_tx(safe_tx, submitter=submitter) + def pending_transactions(self) -> Iterator[tuple[SafeTx, list[SafeTxConfirmation]]]: for executed_tx in self.client.get_transactions(confirmed=False): yield self.create_safe_tx(