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.

Sign up for GitHub

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

Jump to bottom

feat: use alloy Signature type #10758

Open
wants to merge 30 commits into
base: main
Choose a base branch
from
Open

feat: use alloy Signature type #10758

wants to merge 30 commits into from

Conversation

leruaa
Copy link
Contributor

@leruaa leruaa commented Sep 6, 2024

Closes #10750

@leruaa leruaa marked this pull request as ready for review September 15, 2024 21:35
emhane
emhane approved these changes Sep 16, 2024
View reviewed changes
Copy link
Member

@emhane emhane left a comment

Choose a reason for hiding this comment

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

lgtm

crates/optimism/evm/src/execute.rs Show resolved Hide resolved
mattsse
mattsse reviewed Sep 17, 2024
View reviewed changes
Copy link
Collaborator

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

cool!

@klkvr could you please take a closer look here

@mattsse mattsse added the C-debt Refactor of code section that is hard to understand or maintain label Sep 17, 2024
klkvr
klkvr reviewed Sep 17, 2024
View reviewed changes
crates/primitives/src/transaction/mod.rs
@@ -1083,14 +1112,14 @@ impl TransactionSigned {
return Some(from)
}
let signature_hash = self.signature_hash();
self.signature.recover_signer(signature_hash)
recover_signer(&self.signature, signature_hash)
Copy link
Collaborator

Choose a reason for hiding this comment

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

should we use signature.recover here? @mattsse

klkvr
klkvr requested changes Sep 17, 2024
View reviewed changes
Copy link
Collaborator

@klkvr klkvr left a comment •
edited
Loading

Choose a reason for hiding this comment

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

lgtm overall

unfortunately we're losing some safety for parity encoding with alloy signature, so would like us to make sure we handle all cases correctly

crates/primitives/src/transaction/signature.rs Show resolved Hide resolved
crates/primitives/src/transaction/mod.rs Show resolved Hide resolved
crates/primitives/src/transaction/mod.rs Outdated
@@ -1304,7 +1329,7 @@ impl TransactionSigned {

let tx_length = header.payload_length + header.length();
let hash = keccak256(&original_encoding[..tx_length]);
Ok((transaction, hash, signature))
Ok((transaction, hash, signature.with_parity_bool()))
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, why do we need .with_parity_bool here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right, this was incorrect.

@emhane
Copy link
Member

emhane commented Sep 17, 2024

lgtm overall

unfortunately we're losing some safety for parity encoding with alloy signature, so would like us to make sure we handle all cases correctly

can you open an issue to add the safety methods from reth in alloy in that case pls?

@klkvr
Copy link
Collaborator

klkvr commented Sep 17, 2024

can you open an issue to add the safety methods from reth in alloy in that case pls?

do you mean something like signature.ensure_legacy_parity()? I think eventually we'll just inherit those checks from alloy transaction decoding methods once we integrate TxEnvelope

though looks like we're missing checks there too, will create an issue

@klkvr klkvr mentioned this pull request Sep 18, 2024
Merged
3 tasks
@emhane
Copy link
Member

emhane commented Sep 20, 2024

do you need help finishing this @leruaa ? some other issues depend on this pr

@leruaa
Copy link
Contributor Author

leruaa commented Sep 20, 2024

I'll try to find some time today :)

@mattsse
Copy link
Collaborator

mattsse commented Sep 20, 2024

cool, once ready I can do a new alloy patch so we have the parity checks

@emhane
Copy link
Member

emhane commented Sep 21, 2024

let's get this beefy pr in before more merge conflicts arise @klkvr @mattsse

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@emhane emhane emhane approved these changes

@rakita rakita Awaiting requested review from rakita rakita is a code owner

@joshieDo joshieDo Awaiting requested review from joshieDo joshieDo is a code owner

@shekhirin shekhirin Awaiting requested review from shekhirin shekhirin is a code owner

@Rjected Rjected Awaiting requested review from Rjected Rjected is a code owner

@fgimenez fgimenez Awaiting requested review from fgimenez fgimenez is a code owner

@rkrasiuk rkrasiuk Awaiting requested review from rkrasiuk rkrasiuk is a code owner

@gakonst gakonst Awaiting requested review from gakonst gakonst is a code owner

@onbjerg onbjerg Awaiting requested review from onbjerg

@mattsse mattsse Awaiting requested review from mattsse mattsse is a code owner

@klkvr klkvr Awaiting requested review from klkvr

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
C-debt Refactor of code section that is hard to understand or maintain
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

use alloy Signature type
4 participants
@leruaa @emhane @klkvr @mattsse