Skip to content

Support external signers in ethfinalizer - #218

Merged
taylanpince merged 2 commits into
masterfrom
codex/kms-finalizer-signer
Sep 22, 2026
Merged

taylanpince merged 2 commits into
masterfrom
codex/kms-finalizer-signer

Conversation

@taylanpince

@taylanpince taylanpince commented Sep 18, 2026 •

Copy link
Copy Markdown
Contributor

ethfinalizer currently requires a concrete wallet, which prevents using a signer whose private key lives outside the process. Add a context-aware Signer interface for initial transactions and fee replacements, plus NewWalletSigner for local wallets.

Local wallets remain a supported signing option for development, CI, testing, and other deployments. Existing callers can continue setting FinalizerOptions.Wallet; callers using an external signer set Signer instead. Construction requires exactly one. Persisted signed transactions are rebroadcast without signing again.

Validation: go test -race ./ethfinalizer passed with Go 1.25.9. Added coverage for option compatibility, cancellation, signing failure before persistence, replacement signing, and rebroadcasting stored signatures.

@taylanpince

Copy link
Copy Markdown
Contributor Author

🧞 Codegenie Review

Two verified configuration-validation issues can defer invalid signer setup to a panic during transaction submission.

Coverage

Reviewed 10/10 hunks.
Coverage levels: deep 8, normal 2, light 0, skip 0.

⚠️ Findings

🔵 Medium: Reject a nil wallet in NewWalletSigner

File: ethfinalizer/signer.go:22 ↗
Confidence: high

NewWalletSigner(nil) returns a non-nil Signer that passes FinalizerOptions validation, then panics when Send calls s.wallet.SignTransaction.

func NewWalletSigner(wallet *ethwallet.Wallet) Signer { return &walletSigner{wallet: wallet} }
...
return s.wallet.SignTransaction(tx, chainID)

Impact: A recoverable invalid configuration (Signer: NewWalletSigner(nil), no Wallet) becomes a runtime panic while submitting a transaction.

Suggested fix: Reject a nil wallet in NewWalletSigner (for example, return (Signer, error)), or return a nil Signer so existing finalizer validation rejects the configuration.

⚪ Low: Reject typed-nil Signer values during option validation

File: ethfinalizer/ethfinalizer.go:68 ↗
Confidence: high

FinalizerOptions.IsValid accepts a typed-nil Signer, allowing construction with no usable signer and deferring failure to Send.

if (o.Wallet == nil) == (o.Signer == nil) {
 return fmt.Errorf("exactly one of wallet or signer is required")
}

In Go, an interface holding a nil pointer is non-nil. NewFinalizer retains that interface, and Send invokes f.signer.Address() and f.signer.SignTransaction(...); pointer-backed implementations that dereference their receiver panic.

Suggested fix: Treat nil underlying values of nil-capable Signer implementations as absent during validation and signer selection, or use an option representation that cannot carry typed-nil signers.

Stats

  • 🤖 Model: openai-codex gpt-5.6-terra high
  • 🧞 Codegenie: v0.5.6 (a662388fde)
  • Elapsed time: 2m 48s
  • Git: 0xsequence/ethkit from master to codex/kms-finalizer-signer (8afd52cb73)
  • Review completeness: complete.
  • Usage: model calls 49, tokens 692609, cost $0.9751.
  • Effective caps: tokens 8000000.
  • Local context pressure: 41 tool-budget rejections, 29 degraded tool results, 5 degraded hunks.

@pkieltyka

Copy link
Copy Markdown
Member

just wondering where it the KMS part here..?

@taylanpince

Copy link
Copy Markdown
Contributor Author

just wondering where it the KMS part here..?

Google KMS signer is only on relayer.

ethkit only gets a generic signer interface so it doesn't acquire unnecessary dependencies.

@pkieltyka

Copy link
Copy Markdown
Member

cool sounds good :)

PR LGTM to merge

@ScreamingHawk ScreamingHawk left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM!
This feature is great even without the broader KMS context ❤️

@taylanpince
taylanpince marked this pull request as ready for review September 22, 2026 11:03
@taylanpince
taylanpince requested a review from a team September 22, 2026 11:03
@taylanpince
taylanpince merged commit 72f37f6 into master Sep 22, 2026
14 checks passed
@taylanpince
taylanpince deleted the codex/kms-finalizer-signer branch September 22, 2026 11:03
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 22, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-22T11:06:27.377046Z f4f1141 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

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