Add Signer abstraction with LocalSigner and AWS KMS-backed KmsSigner#103
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR adds a KMS client, splits signer behavior into local and KMS-backed implementations, and updates wallet and Flashbots auth code to use the new signer API. Related tests and root module wiring are updated accordingly. ChangesKMS-backed signing support
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/flashbots.zig (1)
256-258: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winZero the Flashbots auth key during Relay deinit.
auth_signernow owns aLocalSigner, butRelay.deinitonly closes the HTTP client. The auth private key remains in memory after teardown.Proposed fix
pub fn deinit(self: *Relay) void { + self.auth_signer.deinit(); self.client.deinit(); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/flashbots.zig` around lines 256 - 258, Relay.deinit currently only tears down the HTTP client, leaving auth_signer’s LocalSigner private key in memory. Update Relay.deinit to also clean up auth_signer by invoking the LocalSigner teardown/zeroing logic before or alongside client.deinit, using the Relay and auth_signer symbols to locate the cleanup path.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/flashbots.zig`:
- Line 196: `computeAuthHeader` only avoids one by-value copy, but `LocalSigner`
is still copied in `signHash` and `address`, and `Relay.deinit` leaves
`auth_signer` resident in memory. Update the `LocalSigner` methods to use
pointer receivers and adjust the call sites in `computeAuthHeader`/`Relay` so
the signer is passed by reference, then make `Relay.deinit` explicitly wipe or
zero the `auth_signer` storage before releasing the relay.
In `@src/kms.zig`:
- Around line 399-410: decodeDerSignature currently parses only the DER prefix
and can accept malformed signatures with trailing data or empty INTEGER values.
Update decodeDerSignature to read and validate the SEQUENCE length up front,
ensure the full input length matches the encoded DER structure, and reject any
extra bytes. Also tighten readDerInteger so zero-length INTEGERs for r or s
return KmsError.ResponseInvalid instead of being accepted.
- Around line 207-211: The ECS credential duplication in the return literal can
leak already-allocated fields if a later duplicate fails. Refactor the
credential-building logic in the function that returns the parsed credentials so
each allocation is tracked and any failure frees previously duplicated values
before returning; use the existing symbols parsed.value.AccessKeyId,
parsed.value.SecretAccessKey, parsed.value.Token, and allocator.dupe to locate
the block and move the construction out of the literal into a cleanup-safe
sequence.
In `@src/signer.zig`:
- Around line 202-236: The borrowed-pointer contract in Signer.fromKms is broken
because Signer.deinit currently calls KmsSigner.deinit on a caller-owned signer.
Update the Signer union and its deinit path so the KMS variant is treated as
borrowed and not released here, or switch fromKms to store an owned KmsSigner
value/box if Wallet is meant to own it. Keep the fix localized to
Signer.fromKms, Signer.deinit, and the .kms handling in
address/signHash/signMessage.
---
Outside diff comments:
In `@src/flashbots.zig`:
- Around line 256-258: Relay.deinit currently only tears down the HTTP client,
leaving auth_signer’s LocalSigner private key in memory. Update Relay.deinit to
also clean up auth_signer by invoking the LocalSigner teardown/zeroing logic
before or alongside client.deinit, using the Relay and auth_signer symbols to
locate the cleanup path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: caad1054-efdc-4f0a-aebe-065a1642c604
📒 Files selected for processing (6)
src/abigen.zigsrc/flashbots.zigsrc/kms.zigsrc/root.zigsrc/signer.zigsrc/wallet.zig
… dupes, borrowed-kms deinit contract
Contributor
Author
|
Addressed the review findings:
Not changed:
|
koko1123
added a commit
that referenced
this pull request
Jul 9, 2026
v0.8.0 was cut out-of-band (git tag on 8c9a20d / #103 only, no GitHub Release), so release-please's manifest and the version-marked extra-files were never bumped and stayed at 0.7.0. With the manifest at 0.7.0, release-please recomputes 0.8.0 from the feat commits since v0.7.0 and collides with the existing v0.8.0 tag ("proposes wrong versions"). Adopt v0.8.0 as the released version by bumping the manifest plus the three release-please extra-files (build.zig.zon, README.md, installation.mdx) to match the existing tag. After this merges, release-please diffs from v0.8.0 and proposes 0.8.1 for the #104 fix. Also fixes build.zig.zon self-reporting 0.7.0 while downstream (gator, perpcity-zig-sdk) pins v0.8.0. Does not delete the v0.8.0 tag (load-bearing: downstream pins its SHA) and leaves CHANGELOG.md for the separate v0.8.0 GitHub Release backfill.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Introduces a general signer abstraction, modeled on alloy's Signer trait, and an AWS KMS implementation:
signer.Signer- tagged union interface (address/signHash/signMessage) owned inline byWallet(value semantics, no lifetime fix-ups for by-value consumers like the zig-sdk'sContext).signer.LocalSigner- the existing in-memory secp256k1 signer, renamed fromSigner(same behavior; keepssignAuthorization,hashPersonalMessage).signer.KmsSigner+ newsrc/kms.zig- signs via AWS KMSECC_SECG_P256K1keys that never leave the HSM. The KMS module implements only what a signer needs:Sign+GetPublicKey, SigV4 request signing (HMAC-SHA256 - baseline-CPU safe for Fargate), credential resolution (static env vars, then the ECS container-credentials endpoint), DER signature decode, EIP-2 low-s normalization, and parity recovery by matching the cached address.Wallet.initnow takes aSigner;Wallet.initLocal(allocator, private_key, provider)is the drop-in convenience for the previous raw-keyinit.wallet.signer_instancerenamed towallet.signer.LocalSigner(always a local key by nature).Why
Part of moving Strobe bot/service keys off plaintext env vars onto AWS KMS (operators fund an address; IAM
kms:Signgates signing; key material is non-exportable). gator-liquidators and future Zig bots consume this viaWallet.initWithSigner-style wiring; a follow-up bumps gator to this eth.zig and swapsLIQUIDATOR_PRIVATE_KEYfor a KMS key id.Breaking changes (intentional, semantics-honest naming)
signer.Signer(struct) ->signer.LocalSigner;signer.Signeris now the union interface.Wallet.init(allocator, [32]u8, provider)->Wallet.initLocal(...);Wallet.initnow takes asigner.Signer.Zig 0.16 notes
Wall-clock time reads through
std.Io.Clock.now(.real, io)(std.time.timestamp was removed); env access uses libcstd.c.getenv(0.16 moved ambient env behind the Io model; eth.zig already links libc for its vendored crypto C).Verification
zig build(0.16.0) - clean.zig build test- full suite green, including new unit tests: SigV4 signature against the AWS documented test vector, DER decode (padding + sign-byte stripping), SPKI pubkey extraction, sha256 vector.zig fmt --check- clean.Summary by CodeRabbit