feat: add delegates #41

fubuloubu · 2024-03-11T22:27:41Z

What I did

Adds support for delegates to the Client classes (including MockClient), the CLI (via ape safe delegates ...), as well as to the main accounts class (SafeAccount.propose_safe_tx and SafeAccount.propose)

fixes: #16

How I did it

How to verify it

Checklist

Passes all linting checks (pre-commit and CI jobs)
New test cases have been added and are passing
~~- [ ] Documentation has been updated~~
PR title follows Conventional Commit standard (will be automatically included in the changelog)

antazoey

Pretty great PR! I had some nits.

Is there anything particular you want me to verify? I can take it for a spin soon.

ape_safe/_cli/delegates.py

antazoey · 2024-03-14T01:21:48Z

ape_safe/accounts.py

+        for delegates in self.client.get_delegates().values():
+            yield from delegates
+
+    def propose_safe_tx(


the propose cli method could probably be refactored to utilize this now

Looked into this, but seems more complicated than I'm willing to deal with now

Think I will try to refactor that CLI method later

ape_safe/client/mock.py

tests/functional/test_delegates.py

fubuloubu · 2024-03-15T14:28:04Z

Is there anything particular you want me to verify? I can take it for a spin soon.

If you can try adding and removing a delegate, I've been doing it and it works for me but curious on the experience (which may improve with ApeWorX/ape#1966)

antazoey · 2024-03-15T16:02:44Z

Thoughts from testing:

(future item) I wish there was a way for network detection to be smarter in the safe. I purposely named my safes the same alias as the one generated in the UI, like laughing-sepolia-safe... It'd be cool maybe if could detect sepolia somehow and my default sepolia provider. It feel redundant to have to specify the network. Maybe the network can be included in the safe account data.
.ens addresses are not handled in the submitter callback, causing a bug in the add CLI... the callback ends up returning None , and then the line safe.client.add_delegate(delegate, label, signer) passes None for signer and you get an error about it not being a KeyfileAccount (because it is None).
Bug: if you have the same account added in multiple plugins, and you pass in an address value for submitter, it should try and pick the KeyfileAccount first, else it fails because it uses the same account from a different plugin and you get the ActionNotPerformedError("Need access to private key for this method.")... work around is to use the alias
Similarly to the ens thing above, if you pass any str for delegator that is not an account, it ends up as None and the error doesnt make a ton of sense, could be improved.
Good news! I got it to work. The output is nice too:

Signer 0xE3747e6341E0d3430e6Ea9e2346cdDCc2F8a4b5b:
- 0x3a45d3C998476AF7191588332b8a49a8ad8CfbE6

but I am wondering if we would wanto enrich those values, like if they are known accounst, use aliases here?

btw I would fix the second and fourth items and make tickets for the first and third items

antazoey · 2024-03-15T16:11:47Z

Related to those, on both add / remove, if you slightly mispell the alias, it just comes back as None and tell you to use a keyfile account again and i was confused for a bit.

i think most of the annoyance stems from the submitter callback

antazoey · 2024-03-15T16:14:34Z

ohhh , one more thing i just noticed. When i do:

ape safe delegates list <safe> --network <net>

for a safe with no delegates, it outputs:

ERROR (ape-safe): No delegates for 0x872Df5f6CF2C2Eaa9e7511AB079D263AB8fdc79f (laughing-sepolia-safe)

feedback: Error seems a bit extreme here. Maybe drop it down to warning. no harm in not having delegates, just was checking it got removed.

fubuloubu · 2024-03-15T21:55:43Z

Maybe the network can be included in the safe account data.

Yes I want to do this too. There are some ways to deploy a safe at the same address on all networks, I have done this for one of our safes but not the other. But still need to detect if it is deployed on the network you wish to use

fubuloubu · 2024-03-15T21:57:24Z

Bug: if you have the same account added in multiple plugins, and you pass in an address value for submitter, it should try and pick the KeyfileAccount first, else it fails because it uses the same account from a different plugin and you get the ActionNotPerformedError("Need access to private key for this method.")... work around is to use the alias

I am working on a PR for core that'll bypass this issue, and then I'll make it accept a wider range of inputs so you can be more specific with it

fubuloubu · 2024-03-15T21:58:25Z

feedback: Error seems a bit extreme here. Maybe drop it down to warning. no harm in not having delegates, just was checking it got removed.

Agree, should really just be INFO

antazoey · 2024-04-12T17:01:35Z

@fubuloubu the signing features we wanted are in Core now, we should update this and get this out.

fubuloubu · 2024-06-05T17:56:05Z

ohhh , one more thing i just noticed. When i do:
ape safe delegates list <safe> --network <net>
for a safe with no delegates, it outputs:
ERROR (ape-safe): No delegates for 0x872Df5f6CF2C2Eaa9e7511AB079D263AB8fdc79f (laughing-sepolia-safe)
feedback: Error seems a bit extreme here. Maybe drop it down to warning. no harm in not having delegates, just was checking it got removed.

eddd1f8

fubuloubu · 2024-06-05T22:33:39Z

@fubuloubu the signing features we wanted are in Core now, we should update this and get this out.

7d8cafd

fubuloubu · 2024-06-05T22:49:15Z

btw I would fix the second and fourth items and make tickets for the first and third items

2, 3, and 4 solved with 2148cf0, I made issues for the 1st one

banteg · 2024-06-12T23:15:56Z

ape_safe/accounts.py

@@ -373,6 +373,60 @@ 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]:


generators are bad ux in interactive sessions.

change to delegates property and make it return a list or a dict of address->label

moreover, since delegates is a client feature, it should only live in SafeClient exclusively, so might as well remove it from SafeAccount.

we can have a method for the iterator such as get_all_delegates() and have the delegates property use the iterator to make a dict

my other point is delegates is not a feature of a safe wallet, but rather a feature of a safe backend api, this it should not be in SafeAccount at all, but rather in SafeClient.

it's completely off-chain and only affects the backend accepting txs from non-owners.

banteg · 2024-06-12T23:17:03Z

ape_safe/client/__init__.py

@@ -185,6 +188,53 @@ def estimate_gas_cost(
        gas = result.get("safeTxGas")
        return int(HexBytes(gas).hex(), 16)

+    def get_delegates(self) -> Dict[AddressType, List[AddressType]]:


labels got lost along the way, they are quite useful imo. the current anonymous structure is not immediately clear. maybe it should be a dict of delegates to their metadata like label and delegator. currently delegator is the key, but it's not the primary info here. or it could be a list[dataclass] where str() casts to address.

banteg · 2024-06-12T23:18:54Z

ape_safe/client/__init__.py

+
+        return delegates
+
+    def add_delegate(self, delegate: AddressType, label: str, delegator: AccountAPI):


instead of delegator we can say owner, should be less confusing.

Co-authored-by: antazoey <[email protected]>

antazoey · 2024-06-13T03:19:27Z

ape_safe/_cli/delegates.py

+    """
+    Add a delegate for a signer in a Safe
+    """
+    delegate = cli_ctx.conversion_manager.convert(delegate, AddressType)


this could be part of the argument:

@click.argument("delegate", callback=lambda ctx, _, val: ctx.obj.conversion_manager.convert(delegate, AddressType))

antazoey · 2024-06-13T03:21:21Z

ape_safe/_cli/delegates.py

+@delegates.command(cls=ConnectedProviderCommand)
+@safe_cli_ctx()
+@safe_option
+@click.argument("delegate", type=ChecksumAddress)


type=ChecksumAddress is a bit wrong, since this implicitly handles ENS strings and such.

antazoey · 2024-06-13T03:22:32Z

ape_safe/_cli/delegates.py

+@delegates.command(cls=ConnectedProviderCommand)
+@safe_cli_ctx()
+@safe_option
+@click.argument("delegate", type=ChecksumAddress)


same here re: the type is incorrect (since ENS domains work because we are converting them?)
this may just all be better with the callback validating the value into an address beforehand.

(and line 60)

antazoey · 2024-06-13T03:24:21Z

ape_safe/accounts.py

@@ -373,6 +373,60 @@ 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]:


we can have a method for the iterator such as get_all_delegates() and have the delegates property use the iterator to make a dict

antazoey · 2024-06-13T03:25:31Z

ape_safe/accounts.py

+        safe_tx: SafeTx,
+        submitter: Union[AccountAPI, AddressType, str, None] = None,
+        sigs_by_signer: Optional[dict[AddressType, MessageSignature]] = None,
+        contractTransactionHash: Optional[SafeTxID] = None,


a note for why this has to be camelCase would be helpful

antazoey · 2024-06-13T03:27:44Z

ape_safe/client/__init__.py

@@ -186,6 +189,53 @@ def estimate_gas_cost(
        gas = result.get("safeTxGas")
        return int(HexBytes(gas).hex(), 16)

+    def get_delegates(self) -> Dict[AddressType, List[AddressType]]:


Suggested change

def get_delegates(self) -> Dict[AddressType, List[AddressType]]:

def get_delegates(self) -> dict[AddressType, list[AddressType]]:

antazoey · 2024-06-13T03:28:55Z

ape_safe/client/__init__.py

+            data = response.json()
+
+            for delegate_info in map(DelegateInfo.model_validate, data.get("results", [])):
+                if delegate_info.delegator not in delegates:


idea: we can use defaultdict again here

antazoey · 2024-06-13T03:29:59Z

ape_safe/client/__init__.py

+            "delegate": delegate,
+            "delegator": delegator.address,
+            "label": label,
+            "signature": sig.encode_rsv().hex(),


warning: when we move to web3.py 7.0 series, this will not longer have a 0x prefix. I have been trying to use different ways of going to hex-str now in preparation for this

antazoey · 2024-06-13T03:30:29Z

ape_safe/client/__init__.py

+            "delegator": delegator.address,
+            "signature": sig.encode_rsv().hex(),
+        }
+        self._delete(f"delegates/{delegate}", json=payload)


warning: trailing slashes are important i think for all of Safe's APIs

true, the api states it needs a trailing slash https://safe-transaction-mainnet.safe.global/

antazoey · 2024-06-13T03:32:15Z

tests/conftest.py

@@ -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


it does now!

fubuloubu force-pushed the feat/add-delegates branch 3 times, most recently from 880928f to 1f62100 Compare March 12, 2024 01:29

fubuloubu marked this pull request as ready for review March 12, 2024 01:29

fubuloubu mentioned this pull request Mar 12, 2024

ape_accounts.TestAccount does not support .sign_message for eip712.EIP712Message types ApeWorX/ape#1961

Closed

fubuloubu force-pushed the feat/add-delegates branch from d28f656 to 74baeec Compare March 12, 2024 19:20

antazoey approved these changes Mar 14, 2024

View reviewed changes

fubuloubu mentioned this pull request Mar 14, 2024

feat(AccountAPI): sign raw hash32 ApeWorX/ape#1966

Merged

3 tasks

fubuloubu force-pushed the feat/add-delegates branch from 74baeec to 7439a4b Compare June 5, 2024 15:34

fubuloubu mentioned this pull request Jun 5, 2024

Add smarter network detection to use as default network #48

Open

fubuloubu force-pushed the feat/add-delegates branch 2 times, most recently from 6a7537d to 3710d25 Compare June 6, 2024 00:23

fubuloubu requested a review from antazoey June 6, 2024 00:58

banteg reviewed Jun 12, 2024

View reviewed changes

fubuloubu added 5 commits June 12, 2024 19:58

feat(API): add delegate support to base client

d04207f

feat(API): add delegate implementation to mock client

78a41c1

feat(API): add delegates implementation to Safe API client

cacad19

feat(CLI): add support for viewing and modifying delegates

267f343

fix: API uses .signHash raw hash signing (dangerous)

fe29931

fubuloubu and others added 17 commits June 12, 2024 19:59

fix(API): client accepts empty signatures

a1ef8f8

feat(account): add method to propose a SafeTx directly

35cae2d

test: add tests for delegate feature (using MockClient)

a07dd45

refactor: apply suggestions from code review

b04382a

Co-authored-by: antazoey <[email protected]>

fix: update typing again

c3e1e1c

refactor: use asserts rather than excepiton for mock

9973b4b

refactor: don't add extra conversion logic in mock

5ce6c14

refactor(CLI): change ERROR to INFO for no delegates found

1aab28a

refactor: use new AccountAPI.sign_raw_msghash method

b9528e7

fix: forgot to update removal too

04184b2

refactor: use account_option instead of callback_factory

dd1478c

fix: unused imports

7c258a2

fix: asserts were wrong

54df67f

fix: use SafeClientException to mimic real environment

ff4a21f

chore: ignore not helpful logs to reduce display overload for testing

b0744d2

chore: run ape tests and integration tests with different commands

4879720

feat: add SafeAccount.propose as a shortcut to propose to Safe API

318f4e9

fubuloubu force-pushed the feat/add-delegates branch from 5e99cd1 to 318f4e9 Compare June 12, 2024 23:59

antazoey reviewed Jun 13, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add delegates #41

feat: add delegates #41

fubuloubu commented Mar 11, 2024 •

edited

Loading

antazoey left a comment

antazoey Mar 14, 2024

fubuloubu Jun 5, 2024

fubuloubu commented Mar 15, 2024

antazoey commented Mar 15, 2024 •

edited

Loading

antazoey commented Mar 15, 2024

antazoey commented Mar 15, 2024

fubuloubu commented Mar 15, 2024

fubuloubu commented Mar 15, 2024

fubuloubu commented Mar 15, 2024

antazoey commented Apr 12, 2024

fubuloubu commented Jun 5, 2024

fubuloubu commented Jun 5, 2024

fubuloubu commented Jun 5, 2024 •

edited

Loading

banteg Jun 12, 2024 •

edited

Loading

antazoey Jun 13, 2024

banteg Jun 14, 2024

banteg Jun 12, 2024

banteg Jun 12, 2024

antazoey Jun 13, 2024

antazoey Jun 13, 2024

antazoey Jun 13, 2024

antazoey Jun 13, 2024

antazoey Jun 13, 2024

antazoey Jun 13, 2024

antazoey Jun 13, 2024

antazoey Jun 13, 2024

antazoey Jun 13, 2024

antazoey Jun 13, 2024

banteg Jun 14, 2024

antazoey Jun 13, 2024


		return delegates

		def add_delegate(self, delegate: AddressType, label: str, delegator: AccountAPI):

	def get_delegates(self) -> Dict[AddressType, List[AddressType]]:
	def get_delegates(self) -> dict[AddressType, list[AddressType]]:

feat: add delegates #41

Are you sure you want to change the base?

feat: add delegates #41

Conversation

fubuloubu commented Mar 11, 2024 • edited Loading

What I did

How I did it

How to verify it

Checklist

antazoey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fubuloubu commented Mar 15, 2024

antazoey commented Mar 15, 2024 • edited Loading

antazoey commented Mar 15, 2024

antazoey commented Mar 15, 2024

fubuloubu commented Mar 15, 2024

fubuloubu commented Mar 15, 2024

fubuloubu commented Mar 15, 2024

antazoey commented Apr 12, 2024

fubuloubu commented Jun 5, 2024

fubuloubu commented Jun 5, 2024

fubuloubu commented Jun 5, 2024 • edited Loading

banteg Jun 12, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fubuloubu commented Mar 11, 2024 •

edited

Loading

antazoey commented Mar 15, 2024 •

edited

Loading

fubuloubu commented Jun 5, 2024 •

edited

Loading

banteg Jun 12, 2024 •

edited

Loading