Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor!: accept any msg type via AccountAPI.sign_message [APE-1305] #1614

Merged
merged 20 commits into from
Dec 8, 2023

Conversation

z80dev
Copy link
Contributor

@z80dev z80dev commented Aug 22, 2023

What I did

Refactor the definition of sign_message to accept Any. Signers implementing this class can raise when they receive a message type they can't handle.

Account plugins would each individual handle msg objects by type, including how they might display information from them for user feedback in terms of signing. This will improve UX because now we can display the text of a string message to sign (useful for displaying SIWE EIP-4361 authorizations), the complete object body and header for EIP-712 structured messages, and other future types of messages that might become necessary to support within Ape (EIP-4337?)

fixes: APE-1103

How I did it

Change type signature of sign_message function, and update implementors to match the new definition.

How to verify it

Run existing test suite

Checklist

  • All changes are completed
  • New test cases have been added
  • Documentation has been updated

@linear
Copy link

linear bot commented Aug 22, 2023

APE-1103 Refactor `AccountAPI.sign_message` arguments

Currently, this function only accepts a SignableMessage type, which limits it's ability to be used in situations where more information is required (such as signing through eth_signTypedData RPC endpoint)

My proposal is to refactor to sign_message(self, msg: Any, **signer_options) -> MessageSignatureand have it raise if the particular signer is unable to handle the type of msg (which could be str, eip712.EIP712Message subclass, SignableMessage, or something else)

Account plugins would each individual handle msg objects by type, including how they might display information from them for user feedback in terms of signing. This will improve UX because now we can display the text of a string message to sign (useful for displaying SIWE EIP-4361 authorizations), the complete object body and header for EIP-712 structured messages, and other future types of messages that might become necessary to support within Ape (EIP-4337?)

This is driven by a specific need right now, as I've been trying to get an ape-frame plugin working, and this is currently a blocker for that to be fully implemented since sign_message must call out to frame via eth_signTypedData_v4

Rating 5 points because all account plugins will need to be updated to handle the new arguments

@vany365 vany365 changed the title implement new sign_message fn accepting Any implement new sign_message fn accepting Any [APE-1305] Aug 22, 2023
@linear
Copy link

linear bot commented Aug 22, 2023

APE-1305 "implement new sign_message fn accepting Any" (ApeWorX/ape #1614)

What I did

Refactor the definition of sign_message to accept Any. Signers implementing this class can raise when they receive a message type they can't handle.

Account plugins would each individual handle msg objects by type, including how they might display information from them for user feedback in terms of signing. This will improve UX because now we can display the text of a string message to sign (useful for displaying SIWE EIP-4361 authorizations), the complete object body and header for EIP-712 structured messages, and other future types of messages that might become necessary to support within Ape (EIP-4337?)

fixes: APE-1103

How I did it

Change type signature of sign_message function, and update implementors to match the new definition.

How to verify it

Run existing test suite

Checklist

  • All changes are completed
  • New test cases have been added
  • Documentation has been updated

ApeWorX/ape #1614 by z80dev on GitHub

via LinearSync

@fubuloubu fubuloubu changed the title implement new sign_message fn accepting Any [APE-1305] refactor!: accept any message type via AccountAPI.sign_message [APE-1305] Aug 22, 2023
@fubuloubu fubuloubu changed the title refactor!: accept any message type via AccountAPI.sign_message [APE-1305] refactor!: accept any msg type via AccountAPI.sign_message [APE-1305] Aug 22, 2023
src/ape_accounts/accounts.py Show resolved Hide resolved
src/ape/api/accounts.py Outdated Show resolved Hide resolved
src/ape_accounts/accounts.py Outdated Show resolved Hide resolved
fubuloubu
fubuloubu previously approved these changes Aug 22, 2023
Copy link
Member

@fubuloubu fubuloubu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good to me, I'll let @antazoey give final approval (and see if she likes the warning log)

src/ape_accounts/accounts.py Outdated Show resolved Hide resolved
src/ape_accounts/accounts.py Show resolved Hide resolved
src/ape/api/accounts.py Outdated Show resolved Hide resolved
src/ape/api/accounts.py Outdated Show resolved Hide resolved
src/ape_accounts/accounts.py Outdated Show resolved Hide resolved
src/ape_test/accounts.py Show resolved Hide resolved
tests/functional/test_accounts.py Outdated Show resolved Hide resolved
src/ape_accounts/accounts.py Outdated Show resolved Hide resolved
src/ape_accounts/accounts.py Show resolved Hide resolved
antazoey
antazoey previously approved these changes Nov 18, 2023
Copy link

linear bot commented Nov 18, 2023

APE-1305 "implement new sign_message fn accepting Any" (ApeWorX/ape #1614)

What I did

Refactor the definition of sign_message to accept Any. Signers implementing this class can raise when they receive a message type they can't handle.

Account plugins would each individual handle msg objects by type, including how they might display information from them for user feedback in terms of signing. This will improve UX because now we can display the text of a string message to sign (useful for displaying SIWE EIP-4361 authorizations), the complete object body and header for EIP-712 structured messages, and other future types of messages that might become necessary to support within Ape (EIP-4337?)

fixes: APE-1103

How I did it

Change type signature of sign_message function, and update implementors to match the new definition.

How to verify it

Run existing test suite

Checklist

  • All changes are completed
  • New test cases have been added
  • Documentation has been updated

ApeWorX/ape #1614 by z80dev on GitHub

via LinearSync

fubuloubu
fubuloubu previously approved these changes Nov 18, 2023
@z80dev z80dev dismissed stale reviews from antazoey and fubuloubu via e04a794 November 28, 2023 21:09
@z80dev z80dev enabled auto-merge (squash) November 28, 2023 21:09
src/ape/api/accounts.py Outdated Show resolved Hide resolved
@z80dev z80dev merged commit d033f85 into main Dec 8, 2023
27 checks passed
@z80dev z80dev deleted the new-sign-message-api branch December 8, 2023 19:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants