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

feat!: refactor sign_message and update to 0.7.0 eth-ape #5

Merged
merged 4 commits into from
Feb 27, 2024

Conversation

z80dev
Copy link
Contributor

@z80dev z80dev commented Aug 30, 2023

note: this will fail type checking until we release the changes in ApeWorX/ape#1614 (review)

now supports strings, SignableMessage, and EIP712Message

What I did

Updated sign_message and check_message to support the types they can handle

Fixes: APE-1351

How I did it

some type checking and lots of experimenting

How to verify it

I used this script

from eip712 import EIP712Message
from ape import accounts
from eth_account.messages import encode_defunct


class CustomMessage(EIP712Message):
    _name_ = "CustomMessage"
    _version_ = "1"
    _chainId_ = 1
    amount: "uint256"  # type: ignore
    recipient: "address"  # type: ignore


acc = accounts.load("frame")

c = CustomMessage(amount=1, recipient=acc.address)

sig = acc.sign_message("Hello World")
assert acc.check_signature("Hello World", sig)

strmsg = encode_defunct(text="Hello World")
sig = acc.sign_message(strmsg)
assert acc.check_signature(strmsg, sig)

sig = acc.sign_message(c)
assert acc.check_signature(c, sig)

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)

@z80dev z80dev marked this pull request as draft August 30, 2023 19:21
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.

Tests are failing... because there are no tests 😅

How could we test this? Does Frame provide an automated mock we could integrate with?

@linear
Copy link

linear bot commented Sep 11, 2023

APE-1351 "feat: refactor sign_message" (ApeWorX/ape-frame #5)

now supports strings, SignableMessage, and EIP712Message

What I did

Updated sign_message and check_message to support the types they can handle

How I did it

some type checking and lots of experimenting

How to verify it

I used this script

from eip712 import EIP712Message
from ape import accounts
from eth_account.messages import encode_defunct


class CustomMessage(EIP712Message):
    _name_ = "CustomMessage"
    _version_ = "1"
    _chainId_ = 1
    amount: "uint256"  # type: ignore
    recipient: "address"  # type: ignore


acc = accounts.load("frame")

c = CustomMessage(amount=1, recipient=acc.address)

sig = acc.sign_message("Hello World")
assert acc.check_signature("Hello World", sig)

strmsg = encode_defunct(text="Hello World")
sig = acc.sign_message(strmsg)
assert acc.check_signature(strmsg, sig)

sig = acc.sign_message(c)
assert acc.check_signature(c, sig)

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)

ApeWorX/ape-frame #5 by z80dev on GitHub

via LinearSync

ape_frame/accounts.py Show resolved Hide resolved
ape_frame/providers.py Outdated Show resolved Hide resolved
@NotPeopling2day NotPeopling2day force-pushed the signing-refactor branch 5 times, most recently from 4d2bcd1 to 4676202 Compare December 19, 2023 22:11
@NotPeopling2day NotPeopling2day marked this pull request as ready for review December 19, 2023 22:12
@NotPeopling2day NotPeopling2day changed the title feat: refactor sign_message feat: refactor sign_message and update to 0.7.0 Dec 19, 2023
@NotPeopling2day NotPeopling2day changed the title feat: refactor sign_message and update to 0.7.0 feat!: refactor sign_message and update to 0.7.0 eth-ape Dec 19, 2023
Copy link
Member

@antazoey antazoey left a comment

Choose a reason for hiding this comment

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

mostly nit comments though you may want to be careful assuming all ValueErrors have arguments, that is likely to cause a random IndexError down the line.

ape_frame/accounts.py Outdated Show resolved Hide resolved
ape_frame/accounts.py Outdated Show resolved Hide resolved
ape_frame/accounts.py Outdated Show resolved Hide resolved
ape_frame/accounts.py Outdated Show resolved Hide resolved
@fubuloubu
Copy link
Member

I think this PR needs some rebasing or something, not sure if it's up to date with core

elif isinstance(msg, EIP712Message):
raw_signature = wrap_sign(
lambda: self.web3.eth.sign_typed_data(
self.address, msg._body_ # type: ignore[arg-type]
Copy link
Member

Choose a reason for hiding this comment

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

@z80dev may know more about the type discrepancy here, but I created this issue: ethereum/web3.py#3255

@antazoey
Copy link
Member

I think this PR needs some rebasing or something, not sure if it's up to date with core

OK, I tidied it all up and updated everything, lemme know if that works

@antazoey antazoey merged commit 425aeca into main Feb 27, 2024
24 checks passed
@antazoey antazoey deleted the signing-refactor branch February 27, 2024 20:37
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.

None yet

3 participants