feat(AccountAPI): sign raw hash32 #1966
Conversation
fubuloubu
changed the title
src/ape_accounts/accounts.py
Outdated
| display_msg = f"Signing raw bytes: '{msg.hex()}'" | ||
|
|
||
| if sign_raw_hash := ( | ||
| len(msg) == 32 and not click.confirm("Sign using EIP-191? (Select 'y' if unsure)") |
There was a problem hiding this comment.
Using click-isms outside of a CLI is a bit strange, I know we are already doing it elsewhere in this calss, but it'd be nice to keep prompting in the CLI components.
There was a problem hiding this comment.
This class needs to prompt the user directly to unlock or approve actions, so seems okay to me
|
Another reason a separate method might be better: Accounts that don't support this will be more obvious. Otherwise by the same rationale, we'd have to warn or raise exceptions in accounts trying to sign a bytes value of a certain length, which assumes stuff about the signer's intent. By having separate methods, the intent is clear, and when the account does not support such thing, it can raise a Just something I was thinking about ... |
I'm on board with this change, also makes it easier to roll out |
fubuloubu
force-pushed
the
feat/AccountAPI/sign-raw-hash32
branch
from
df3c28c
to
1045ec5
Compare
|
Refactored in fc4b250 as a separate method with |
| :class:`~ape.types.signatures.MessageSignature` (optional): | ||
| The signature corresponding to the message. | ||
| """ | ||
| raise NotImplementedError( |
There was a problem hiding this comment.
suggestion: The @raises_not_implemented decorator does exactly this! same messaging.
| @@ -307,6 +325,10 @@ def check_signature( | |||
| signature (Optional[:class:`~ape.types.signatures.MessageSignature`]): | |||
| The signature to check. Defaults to ``None`` and is not needed when the first | |||
| argument is a transaction class. | |||
| recover_using_eip191 (bool): | |||
| Perform recovery using EIP-191 signed message check. If set False, then will attempt | |||
There was a problem hiding this comment.
| Perform recovery using EIP-191 signed message check. If set False, then will attempt | |
| Perform recovery using EIP-191 signed message check. If False, then will attempt |
| @@ -315,6 +337,8 @@ def check_signature( | |||
| data = encode_defunct(text=data) | |||
| elif isinstance(data, int): | |||
| data = encode_defunct(hexstr=HexBytes(data).hex()) | |||
| elif isinstance(data, bytes) and (len(data) != 32 or recover_using_eip191): | |||
There was a problem hiding this comment.
small perf, maybe, for short-circuit eval on the or stmt
| elif isinstance(data, bytes) and (len(data) != 32 or recover_using_eip191): | |
| elif isinstance(data, bytes) and (recover_using_eip191 or len(data) != 32): |
as checking a bool is prolly faster than a length check
What I did
Add a new path to
AccountAPIcalled.sign_raw_msghashthat allows signing a raw 32 byte hash without treating it as an EIP191SignableMessagetypefixes: #1962
How I did it
I needed this for work I was doing on ape safe, but it is a really dangerous thing to allow, so I added a warning message and additionally disallowed autosigning for it. For
AccountAPI.recover_messageI added an additional kwargrecover_using_eip191=Truethat can be set false if you want to opt-in for that behavior for verifying a signature.How to verify it
Try the experience locally, I have added tests for it. Ultimately, when installled alongside the delegate feature for ape safe (ApeWorX/ape-safe#41) you should now be able to use it to directly sign something with the API
Checklist