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

[cast] Fix cast wallet verify #7215

Merged
merged 4 commits into from
Feb 23, 2024

Conversation

mdehoog
Copy link
Contributor

@mdehoog mdehoog commented Feb 22, 2024

Motivation

#7088 broke cast wallet verify. The following worked previously and no longer works:

cast wallet verify --address 0x28A4F420a619974a2393365BCe5a7b560078Cc13 hello $(cast wallet sign --private-key 0x678d52cc9fdedbf5be1cd9b479725aab5ed8b9fbfbfeff9b660dff8b5766bf2a hello)

Error:

Odd number of digits

Solution

This PR reverts a subset of the PR above which fixes the issue. Happy to take suggestions on alternative paths. Thanks!

Closes #7088

@stevieraykatz
Copy link

This addresses #7214

Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

Thanks! Indeed, this is a fix, but not the one we want—we want to keep the old types and not import any additional ethers types.

The fix would be to switch to using recover_address_from_msg instead of recover_address_from_prehash.

println!("Validation succeeded. Address {address} signed this message.");
} else {
println!("Validation failed. Address {address} did not sign this message.");
match signature.verify(Self::hex_str_to_bytes(&message)?, address.to_ethers()) {
Copy link
Member

Choose a reason for hiding this comment

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

We should not be using the old ethers type here. We should actually use recover_address_from_msg from the alloy Signature type.

Copy link
Member

@Evalir Evalir left a comment

Choose a reason for hiding this comment

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

I've added a commit that applies the fix described—thanks!

Copy link
Member

@DaniPopes DaniPopes left a comment

Choose a reason for hiding this comment

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

Can you please add a test

@Evalir
Copy link
Member

Evalir commented Feb 22, 2024

@DaniPopes yep was just gonna push one

@Evalir Evalir merged commit b5fc4dc into foundry-rs:master Feb 23, 2024
20 checks passed
zeroXbrock pushed a commit to flashbots/suavex-foundry that referenced this pull request Apr 2, 2024
* Fix cast wallet verify

* fmt

* fix(cast): use recover_address_from_msg

* chore: add test, abstract addr recovery for testing

---------

Co-authored-by: Enrique Ortiz <hi@enriqueortiz.dev>
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.

4 participants