Skip to content

refactor(builder): replace custom Signer with Alloy's PrivateKeySigner #426

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
Merged
danyalprout merged 1 commit into from
Jan 14, 2026
Merged

refactor(builder): replace custom Signer with Alloy's PrivateKeySigner #426

danyalprout merged 1 commit into from
Jan 14, 2026

Conversation

@Himess
Copy link
Contributor

@Himess Himess commented Jan 13, 2026

Closes #390

@cb-heimdall
Copy link
Collaborator

cb-heimdall commented Jan 13, 2026 •
edited
Loading

✅ Heimdall Review Status

Requirement Status More Info
Reviews ✅ 1/1
Denominator calculation
Show calculation
1 if user is bot 0
1 if user is external 0
2 if repo is sensitive 0
From .codeflow.yml 1
Additional review requirements
Show calculation
Max 0
0
From CODEOWNERS 0
Global minimum 0
Max 1
1
1 if commit is unverified 1
Sum 2

refcell
refcell reviewed Jan 13, 2026
View reviewed changes
refcell
refcell approved these changes Jan 13, 2026
View reviewed changes
Copy link
Contributor

@refcell refcell left a comment

Choose a reason for hiding this comment

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

This looks good to me, would appreciate a second pair of eyes though.

refcell
refcell requested changes Jan 13, 2026
View reviewed changes
Copy link
Contributor

@refcell refcell left a comment

Choose a reason for hiding this comment

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

Remove clones

@cb-heimdall
Copy link
Collaborator

cb-heimdall commented Jan 13, 2026

Review Error for refcell @ 2026-01-13 23:09:39 UTC
User failed mfa authentication, see go/mfa-help

@Himess Himess force-pushed the refactor/replace-custom-signer-with-alloy branch from 1bd7a11 to 06fee44 Compare January 14, 2026 13:33
@Himess Himess marked this pull request as draft January 14, 2026 16:32
@Himess Himess force-pushed the refactor/replace-custom-signer-with-alloy branch from 06fee44 to b5e5f2c Compare January 14, 2026 16:48
@Himess
Copy link
Contributor Author

Himess commented Jan 14, 2026 •
edited
Loading

Hi @refcell , Force-pushed due to upstream conflicts.
Clone : 44 → 31 (used &Signer references). Clones are mostly in tests.
Want me to use Arc<Signer> instead for zero clones?

@Himess Himess marked this pull request as ready for review January 14, 2026 16:48
@Himess Himess requested a review from refcell January 14, 2026 16:50
haardikk21
haardikk21 previously approved these changes Jan 14, 2026
View reviewed changes
@refcell
Copy link
Contributor

refcell commented Jan 14, 2026

Hi @refcell , Force-pushed due to upstream conflicts. Clone : 44 → 31 (used &Signer references). Clones are mostly in tests. Want me to use Arc<Signer> instead for zero clones?

Nice work on the rebase, there were quite a few changes!

Yes I think Arc should work here nicely. We shouldn't need to ever modify the Signer so an atomic reference makes sense to me.

@Himess Himess force-pushed the refactor/replace-custom-signer-with-alloy branch from b5e5f2c to 3330aec Compare January 14, 2026 17:17
@Himess
Copy link
Contributor Author

Himess commented Jan 14, 2026

@refcell I am so sorry for force-update again :( That's mistake
I think it's ready

@Himess Himess force-pushed the refactor/replace-custom-signer-with-alloy branch from 3330aec to 9f8a5e0 Compare January 14, 2026 18:27
@refcell refcell force-pushed the refactor/replace-custom-signer-with-alloy branch from ae466ff to f5edd69 Compare January 14, 2026 19:34
refcell
refcell reviewed Jan 14, 2026
View reviewed changes
crates/builder/op-rbuilder/src/tests/framework/driver.rs

// Create a temporary signer for the deposit
let signer = self.signer.unwrap_or_else(Signer::random);
let signer = self.signer.clone().unwrap_or_else(Signer::random);
Copy link
Contributor

@refcell refcell Jan 14, 2026

Choose a reason for hiding this comment

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

This is a light clone since the signer type is small

@Himess @refcell
refactor: replace custom Signer with Alloy's PrivateKeySigner
f1871d5 
Replace the custom secp256k1-based Signer implementation with a wrapper
around Alloy's PrivateKeySigner. This eliminates manual cryptographic
operations and consolidates signing logic using battle-tested upstream
code.

Changes:
- Replace secp256k1 usage with alloy-signer's SignerSync trait
- Use PrivateKeySigner::from_bytes() for key construction
- Use sign_hash_sync() for message signing
- Add SignerRef (Arc<Signer>) type alias for cheap cloning
- Update BuilderConfig and FlashblocksBuilderTx to use SignerRef

Closes base#390
@refcell refcell force-pushed the refactor/replace-custom-signer-with-alloy branch from f5edd69 to f1871d5 Compare January 14, 2026 19:51
refcell
refcell reviewed Jan 14, 2026
View reviewed changes
Copy link
Contributor

@refcell refcell left a comment

Choose a reason for hiding this comment

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

This looks good to me now

@refcell refcell requested a review from haardikk21 January 14, 2026 19:53
@cb-heimdall
Copy link
Collaborator

cb-heimdall commented Jan 14, 2026

Review Error for refcell @ 2026-01-14 19:53:25 UTC
User cannot review their own commit

danyalprout
danyalprout approved these changes Jan 14, 2026
View reviewed changes
Copy link
Collaborator

@danyalprout danyalprout 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 thank you @Himess

@danyalprout danyalprout added this pull request to the merge queue Jan 14, 2026
Merged via the queue into base:main with commit 3ad2e03 Jan 14, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@danyalprout danyalprout danyalprout approved these changes

@refcell refcell refcell approved these changes

@haardikk21 haardikk21 Awaiting requested review from haardikk21

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

refactor(op-rbuilder): Replace Custom Signer with Alloy's PrivateKeySigner

5 participants

@Himess @cb-heimdall @refcell @danyalprout @haardikk21