feat: KMS-backed signer for the write path (createWithKms / initWithKms) - #31
Merged
Merged
Conversation
The SDK could only sign from a raw 32-byte private key, so a production liquidator had to hold key material in process memory. This adds a KMS signing path (eth.zig KmsSigner) so the money path signs with the key staying in AWS KMS. - EthChainClient.createWithKms(rpc_url, region, key_id): builds the wallet via eth.signer.Signer.fromKms over a heap-owned, stable KmsSigner; owns a copy of key_id (the signer borrows it) and derives+caches the wallet address from KMS at construction. destroy() deinits and frees the signer. - PerpCityContext.initWithKms(rpc_url, region, key_id, deployments): the KMS counterpart of init(). The raw-key init() is unchanged. The KMS key must be ECC_SECG_P256K1; credentials resolve from the env / container role at call time. Tests: create/destroy on the raw-key path is network-free (address derives locally), so it regression-guards the destroy() change (kms_signer == null frees clean under the testing allocator); the KMS constructors are referenced at compile time. The KMS signing path itself hits kms:GetPublicKey and is exercised by integration, not CI. zig build test 497, contract-test 96 (Debug + ReleaseFast).
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds AWS KMS-backed signing construction to ChangesAWS KMS signing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant PerpCityContext
participant EthChainClient
participant KmsSigner
participant Wallet
PerpCityContext->>EthChainClient: createWithKms(region, key_id)
EthChainClient->>KmsSigner: initialize(region, key_id)
EthChainClient->>Wallet: create from KmsSigner
EthChainClient-->>PerpCityContext: initialized client
EthChainClient->>KmsSigner: deinitialize and free during destroy()
Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@tests/contract/chain_client_test.zig`:
- Around line 33-41: Update the compile-time-only test “KMS constructors are
wired” to assign sdk.context.PerpCityContext.initWithKms and
EthChainClient.createWithKms to explicitly declared function types matching
their intended parameter order, parameter types, and return types. Replace the
generic `@typeInfo` checks while preserving the no-invocation behavior that avoids
network calls.
🪄 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: 7c3ec07a-8c63-40c6-8d8d-e89a1d4b776e
📒 Files selected for processing (4)
src/chain_client.zigsrc/context.zigtests/contract/chain_client_test.zigtests/contract_tests.zig
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
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
Adds a KMS signing path so the SDK's write/money path signs with the private key staying in AWS KMS, instead of requiring a raw 32-byte key in process memory.
Why
Until now
PerpCityContext.initaccepted only a raw[32]u8key -- so a production liquidator could not run its full flow (discover -> simulate -> sign -> send) through the SDK without holding key material. eth.zig ships aKmsSignerand aSignerabstraction; this wires them in.Changes
EthChainClient.createWithKms(rpc_url, region, key_id): builds the wallet viaeth.signer.Signer.fromKmsover a heap-owned, stableKmsSigner(the wallet'sSignerholds a borrowed pointer). Owns a copy ofkey_id(the signer borrows it) and derives+caches the wallet address from KMS at construction.destroy()deinits and frees the signer + key id.PerpCityContext.initWithKms(rpc_url, region, key_id, deployments): the KMS counterpart ofinit(). The raw-keyinit()is unchanged.ECC_SECG_P256K1; credentials resolve from the env / container role at call time.Tests
create/destroyon the raw-key path is network-free (the address derives locally from the key), so it regression-guards thedestroy()change: thekms_signer == nullbranch must free cleanly under the testing allocator.kms:GetPublicKey(needs AWS credentials), so it is exercised by integration, not CI -- consistent with how the eth-backed path is tested.zig build test: 497/497zig build contract-test(Debug + ReleaseFast): 96/96zig fmt --check: cleanSummary by CodeRabbit