feat(signer): sign txs via KMS - #1808
Open
Thegaram wants to merge 3 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds AWS KMS-backed ECDSA secp256k1 transaction signing to the rollup sender, including configuration, address validation, transaction integration, tests, documentation, and dependency wiring. The version tag is updated to ChangesAWS KMS Signer Integration
Version Bump
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant NewSender
participant NewTransactionSigner
participant kmsSigner
participant AWSKMS
NewSender->>NewTransactionSigner: context, AWS KMS config, chain ID
NewTransactionSigner->>kmsSigner: initialize signer
kmsSigner->>AWSKMS: GetPublicKey
AWSKMS-->>kmsSigner: DER SPKI public key
kmsSigner->>kmsSigner: derive and validate signer address
NewTransactionSigner->>kmsSigner: sign transaction
kmsSigner->>AWSKMS: Sign digest
AWSKMS-->>kmsSigner: DER ECDSA signature
kmsSigner-->>NewTransactionSigner: apply Ethereum signature
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## develop #1808 +/- ##
===========================================
+ Coverage 35.44% 35.47% +0.02%
===========================================
Files 262 261 -1
Lines 22596 22642 +46
===========================================
+ Hits 8010 8032 +22
- Misses 13748 13764 +16
- Partials 838 846 +8
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
georgehao
previously approved these changes
Jun 16, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
rollup/internal/controller/relayer/l2_relayer.go (1)
226-228: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winLog when the committed batch’s end chunk is missing.
If
GetChunkByIndexreturnsnil, the gauge remains at zero without any diagnostic, defeating this initialization and potentially triggering a false commit-lag alert. Emit a warning for this inconsistent database state, or fail startup if it must be treated as fatal.🤖 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 `@rollup/internal/controller/relayer/l2_relayer.go` around lines 226 - 228, Update the initialization flow around GetChunkByIndex and the endChunk check to emit a warning when endChunk is nil, including enough context to identify the missing committed batch end chunk; preserve the existing gauge update when endChunk is present.
🤖 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.
Outside diff comments:
In `@rollup/internal/controller/relayer/l2_relayer.go`:
- Around line 226-228: Update the initialization flow around GetChunkByIndex and
the endChunk check to emit a warning when endChunk is nil, including enough
context to identify the missing committed batch end chunk; preserve the existing
gauge update when endChunk is present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 27e150cf-14d6-464f-a016-a3e94905ca5c
📒 Files selected for processing (2)
common/version/version.gorollup/internal/controller/relayer/l2_relayer.go
🚧 Files skipped from review as they are similar to previous changes (1)
- common/version/version.go
There was a problem hiding this comment.
🧹 Nitpick comments (1)
rollup/internal/controller/sender/transaction_signer.go (1)
69-77: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftCover the
NewTransactionSignerKMS branch directly.The existing KMS test constructs
TransactionSignerwithkmsSigner: ks, so it bypasses this constructor path. Add a constructor-level test using an injectable KMS client/factory, or at least cover its validation/error paths, so regressions in initialization and address wiring are detected.🤖 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 `@rollup/internal/controller/sender/transaction_signer.go` around lines 69 - 77, Add a direct test for the AWSKMSSignerType branch in NewTransactionSigner, using an injectable KMS client or factory to exercise newKMSSigner, initialization, and address wiring; if injection is unavailable, cover the constructor’s validation and error paths instead. Keep the existing TransactionSigner-based KMS test, but ensure the new test invokes NewTransactionSigner itself and verifies successful signer creation or the expected error.
🤖 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.
Nitpick comments:
In `@rollup/internal/controller/sender/transaction_signer.go`:
- Around line 69-77: Add a direct test for the AWSKMSSignerType branch in
NewTransactionSigner, using an injectable KMS client or factory to exercise
newKMSSigner, initialization, and address wiring; if injection is unavailable,
cover the constructor’s validation and error paths instead. Keep the existing
TransactionSigner-based KMS test, but ensure the new test invokes
NewTransactionSigner itself and verifies successful signer creation or the
expected error.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6a9da4be-7519-46fa-85cb-39baa850226a
📒 Files selected for processing (6)
rollup/internal/config/relayer.gorollup/internal/controller/relayer/l2_relayer.gorollup/internal/controller/sender/README.mdrollup/internal/controller/sender/kms_signer.gorollup/internal/controller/sender/kms_signer_test.gorollup/internal/controller/sender/transaction_signer.go
🚧 Files skipped from review as they are similar to previous changes (5)
- rollup/internal/config/relayer.go
- rollup/internal/controller/sender/README.md
- rollup/internal/controller/sender/kms_signer_test.go
- rollup/internal/controller/relayer/l2_relayer.go
- rollup/internal/controller/sender/kms_signer.go
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
3 participants
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.
Purpose or design rationale of this PR
Adds a third
signer_type,AWSKMS, alongsidePrivateKeyandRemoteSigner.kms_signer.go— at startup, derives the key's address fromkms:GetPublicKeyand refuses to run unless it matches the configuredsigner_address. Per tx, sends only the 32-byte signing hash tokms:Signand converts the DER(r, s)reply into Ethereum's 65-byte form (low-s per EIP-2, recovery id found by checking which one recovers the key's own address). The private key never leaves KMS.RemoteSigner—web3signercan't sign blob txs, socommit_sendercan't use it. KMS signs a hash and the tx is assembled locally, so all tx types work, blobs included.aws_kms_signer_config:key_id, optionalregion, requiredsigner_address. Credentials come from the standard AWS chain (IRSA/instance role), never the config file.Deployment checklist
ECC_SECG_P256K1/SIGN_VERIFY, migrate the existing private keys into KMS.kms:Sign+kms:GetPublicKeyon that key.get-public-key, put it insigner_address(should match known key).signer_type: AWSKMS,key_id, andregionin the service config.Summary by CodeRabbit
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores