feat: Add signer registry URIs for GCP and Foundry keystores#1364
Open
nambrot-agent wants to merge 1 commit intomainfrom
Open
feat: Add signer registry URIs for GCP and Foundry keystores#1364nambrot-agent wants to merge 1 commit intomainfrom
nambrot-agent wants to merge 1 commit intomainfrom
Conversation
Add support for signer-specific registry URIs that allow users to configure signing keys without creating YAML configuration files: - gcp://project/secret-name - Load private key from GCP Secret Manager - foundry://account-name - Load key from Foundry keystore (~/.foundry/keystores) - foundry:///path/to/keystores/account - Load from custom keystore path New components: - GCPSignerRegistry: Exposes GCP secrets as gcpSecret signer type - FoundryKeystoreRegistry: Exposes Foundry keystores as foundryKeystore signer type - Updated registry-utils.ts to detect and handle these URI schemes Both registries implement IRegistry but only provide signer configuration - chain metadata and warp routes return empty/null values. This allows them to be merged with other registries that provide chain data. Also adds signer-related methods to IRegistry interface: - getSigners(): Get all named signers - getSigner(id): Get a specific signer by name - getDefaultSigner(chainName?): Get the default signer - getSignerConfiguration(): Get full signer config with defaults Includes unit tests for both new registry types.
7 tasks
Contributor
📝 WalkthroughWalkthroughThis PR adds a multi-layered signer configuration system to the registry infrastructure. New specialized registries for GCP and Foundry keystores are introduced, along with signer resolution logic in the base and merged registry classes. The file system registry gains signer configuration loading and caching capabilities, with all pieces wired through updated exports. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant MergedRegistry
participant SubReg1 as SubRegistry<br/>(e.g., FileSystem)
participant SubReg2 as SubRegistry<br/>(e.g., GCP)
participant Config as Signer<br/>Config
Client->>MergedRegistry: getDefaultSigner(chainName)
MergedRegistry->>SubReg1: getSignerConfiguration()
SubReg1-->>MergedRegistry: SignerConfiguration
MergedRegistry->>SubReg2: getSignerConfiguration()
SubReg2-->>MergedRegistry: SignerConfiguration
MergedRegistry->>Config: Merge configs<br/>(SubReg2 overrides SubReg1)
MergedRegistry->>Config: Check chain-specific<br/>default
MergedRegistry->>Config: Fall back to protocol<br/>or global default
MergedRegistry->>Config: Resolve signer<br/>references
MergedRegistry-->>Client: SignerConfig
sequenceDiagram
participant App
participant RegistryDispatch as getRegistry()<br/>Dispatch Logic
participant Parser as URI Parsers
participant FKReg as FoundryKeystore<br/>Registry
participant GCPReg as GCP Signer<br/>Registry
App->>RegistryDispatch: getRegistry(uri)
RegistryDispatch->>Parser: parseFoundryKeystoreUri(uri)
alt Match: foundry://...
Parser-->>RegistryDispatch: {accountName, keystorePath?}
RegistryDispatch->>FKReg: new FoundryKeystoreRegistry(options)
FKReg-->>RegistryDispatch: instance
else No match
RegistryDispatch->>Parser: parseGCPUri(uri)
alt Match: gcp://...
Parser-->>RegistryDispatch: {project, secretName}
RegistryDispatch->>GCPReg: new GCPSignerRegistry(options)
GCPReg-->>RegistryDispatch: instance
else Fall through
RegistryDispatch->>RegistryDispatch: Try GitHub/HTTP<br/>(existing logic)
end
end
RegistryDispatch-->>App: IRegistry instance
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes This change introduces two new specialized registry classes with substantial logic, integrates them into the dispatch system, adds multi-registry signer merging with precedence rules, and extends the base registry contracts. Multiple files contain non-trivial signer resolution and URI parsing logic, though test coverage is comprehensive and homogeneous patterns help offset the diversity. Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 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 |
![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: 2
🤖 Fix all issues with AI agents
In `@src/fs/FileSystemRegistry.ts`:
- Around line 154-206: getSignerConfiguration has two issues: mergedConfig is
declared with let but never reassigned (ESLint), and signer file processing
order is non-deterministic; fix by declaring mergedConfig as const in
getSignerConfiguration() and sorting signerFiles (e.g., signerFiles.sort())
before the for loop so overrides are deterministic; keep existing merge logic
that spreads parsed.signers and parsed.defaults (including protocols/chains) and
preserve the try/catch/logger.warn behavior in the same function.
In `@src/fs/registry-utils.ts`:
- Around line 3-7: Currently malformed gcp:// or foundry:// URIs fall through to
file-path validation and produce misleading "Invalid file system path" errors;
change the logic that handles input URIs so that any input starting with
"gcp://" calls parseGCPUri and instantiates GCPSignerRegistry (using
GCPSignerRegistry constructor) and any input starting with "foundry://" calls
parseFoundryKeystoreUri and instantiates FoundryKeystoreRegistry (using
FoundryKeystoreRegistry constructor) so parse/constructor errors are surfaced
immediately; specifically, detect the scheme prefix before file-path validation,
invoke parseGCPUri/parseFoundryKeystoreUri and let or rethrow their errors (or
wrap them with a clearer message) instead of falling through to the file-path
branch.
🧹 Nitpick comments (3)
src/registry/MergedRegistry.ts (1)
160-182: Consider warning on signer ID overrides during merge.
Later registries silently replace earlier signer IDs; a small warning when a key already exists would make collisions easier to spot.src/registry/GCPSignerRegistry.ts (1)
140-195: DRY up signer config construction.
The same signer shape is built in four methods; a private helper keeps them in sync if fields change.♻️ Possible refactor
+ private buildSignerConfig(): SignerConfig { + return { + type: SignerType.GCP_SECRET, + project: this.project, + secretName: this.secretName, + }; + } ... - getSignerConfiguration(): SignerConfiguration { - return { - signers: { - [this.signerName]: { - type: SignerType.GCP_SECRET, - project: this.project, - secretName: this.secretName, - }, - }, - defaults: { - default: { ref: this.signerName }, - }, - }; - } + getSignerConfiguration(): SignerConfiguration { + return { + signers: { [this.signerName]: this.buildSignerConfig() }, + defaults: { default: { ref: this.signerName } }, + }; + }src/registry/FoundryKeystoreRegistry.ts (1)
194-254: Avoidanycasts in buildSignerConfig.
You can keep type safety by using a local intersection type for the optional fields instead of(config as any).🛠️ Example approach
+ private buildSignerConfig(): SignerConfig & { + keystorePath?: string; + passwordFile?: string; + passwordEnvVar?: string; + } { const config: SignerConfig = { type: SignerType.FOUNDRY_KEYSTORE, accountName: this.accountName, }; if (this.keystorePath) { - (config as any).keystorePath = this.keystorePath; + config.keystorePath = this.keystorePath; } if (this.passwordFile) { - (config as any).passwordFile = this.passwordFile; + config.passwordFile = this.passwordFile; } if (this.passwordEnvVar) { - (config as any).passwordEnvVar = this.passwordEnvVar; + config.passwordEnvVar = this.passwordEnvVar; } return config; }
Comment on lines
+154
to
+206
| /** | ||
| * Get the full signer configuration from the signers/ directory | ||
| * Parses all .yaml files in the signers/ directory and merges them | ||
| */ | ||
| getSignerConfiguration(): SignerConfiguration | null { | ||
| if (this.signerConfigCache) return this.signerConfigCache; | ||
|
|
||
| const signersPath = path.join(this.uri, this.getSignersPath()); | ||
| if (!fs.existsSync(signersPath)) return null; | ||
|
|
||
| const signerFiles = this.listFiles(signersPath).filter( | ||
| (f) => f.endsWith('.yaml') || f.endsWith('.yml'), | ||
| ); | ||
|
|
||
| if (signerFiles.length === 0) return null; | ||
|
|
||
| // Merge all signer config files | ||
| let mergedConfig: SignerConfiguration = {}; | ||
|
|
||
| for (const filePath of signerFiles) { | ||
| try { | ||
| const data = fs.readFileSync(filePath, 'utf8'); | ||
| const parsed = yamlParse(data) as SignerConfiguration; | ||
|
|
||
| // Merge signers | ||
| if (parsed.signers) { | ||
| mergedConfig.signers = { ...mergedConfig.signers, ...parsed.signers }; | ||
| } | ||
|
|
||
| // Merge defaults (later files override earlier ones) | ||
| if (parsed.defaults) { | ||
| mergedConfig.defaults = { | ||
| ...mergedConfig.defaults, | ||
| ...parsed.defaults, | ||
| // Deep merge protocols and chains | ||
| protocols: { | ||
| ...mergedConfig.defaults?.protocols, | ||
| ...parsed.defaults.protocols, | ||
| }, | ||
| chains: { | ||
| ...mergedConfig.defaults?.chains, | ||
| ...parsed.defaults.chains, | ||
| }, | ||
| }; | ||
| } | ||
| } catch (error) { | ||
| this.logger.warn(`Failed to parse signer config file ${filePath}: ${error}`); | ||
| } | ||
| } | ||
|
|
||
| this.signerConfigCache = mergedConfig; | ||
| return mergedConfig; | ||
| } |
Contributor
There was a problem hiding this comment.
Fix lint + make override order deterministic.
ESLint flags mergedConfig as never reassigned, and file order currently depends on filesystem traversal. Sorting makes overrides predictable.
🛠️ Suggested tweak
- const signerFiles = this.listFiles(signersPath).filter(
- (f) => f.endsWith('.yaml') || f.endsWith('.yml'),
- );
+ const signerFiles = this.listFiles(signersPath)
+ .filter((f) => f.endsWith('.yaml') || f.endsWith('.yml'))
+ .sort();
...
- let mergedConfig: SignerConfiguration = {};
+ const mergedConfig: SignerConfiguration = {};🧰 Tools
🪛 ESLint
[error] 171-171: 'mergedConfig' is never reassigned. Use 'const' instead.
(prefer-const)
🪛 GitHub Check: lint
[failure] 171-171:
'mergedConfig' is never reassigned. Use 'const' instead
🤖 Prompt for AI Agents
In `@src/fs/FileSystemRegistry.ts` around lines 154 - 206, getSignerConfiguration
has two issues: mergedConfig is declared with let but never reassigned (ESLint),
and signer file processing order is non-deterministic; fix by declaring
mergedConfig as const in getSignerConfiguration() and sorting signerFiles (e.g.,
signerFiles.sort()) before the for loop so overrides are deterministic; keep
existing merge logic that spreads parsed.signers and parsed.defaults (including
protocols/chains) and preserve the try/catch/logger.warn behavior in the same
function.
Comment on lines
+3
to
+7
| import { GCPSignerRegistry, parseGCPUri } from '../registry/GCPSignerRegistry.js'; | ||
| import { | ||
| FoundryKeystoreRegistry, | ||
| parseFoundryKeystoreUri, | ||
| } from '../registry/FoundryKeystoreRegistry.js'; |
Contributor
There was a problem hiding this comment.
Handle invalid gcp:// or foundry:// URIs with scheme-specific errors.
Right now Line 83–97 only instantiates when parsing succeeds; malformed URIs fall through to file-path validation and throw “Invalid file system path,” which is pretty confusing for users. I'd route any gcp:// or foundry:// scheme straight to their registries so the constructor emits the proper error.
🛠️ Suggested fix
-import { GCPSignerRegistry, parseGCPUri } from '../registry/GCPSignerRegistry.js';
+import { GCPSignerRegistry } from '../registry/GCPSignerRegistry.js';
import {
FoundryKeystoreRegistry,
- parseFoundryKeystoreUri,
} from '../registry/FoundryKeystoreRegistry.js';
@@
- // Check for GCP signer registry (gcp://project/secret-name)
- if (parseGCPUri(uri)) {
+ // Check for GCP signer registry (gcp://project/secret-name)
+ if (uri.startsWith('gcp://')) {
return new GCPSignerRegistry({
uri,
logger: childLogger,
});
}
// Check for Foundry keystore registry (foundry://accountName or foundry:///path/accountName)
- if (parseFoundryKeystoreUri(uri)) {
+ if (uri.startsWith('foundry://')) {
return new FoundryKeystoreRegistry({
uri,
logger: childLogger,
});
}Also applies to: 83-97
🤖 Prompt for AI Agents
In `@src/fs/registry-utils.ts` around lines 3 - 7, Currently malformed gcp:// or
foundry:// URIs fall through to file-path validation and produce misleading
"Invalid file system path" errors; change the logic that handles input URIs so
that any input starting with "gcp://" calls parseGCPUri and instantiates
GCPSignerRegistry (using GCPSignerRegistry constructor) and any input starting
with "foundry://" calls parseFoundryKeystoreUri and instantiates
FoundryKeystoreRegistry (using FoundryKeystoreRegistry constructor) so
parse/constructor errors are surfaced immediately; specifically, detect the
scheme prefix before file-path validation, invoke
parseGCPUri/parseFoundryKeystoreUri and let or rethrow their errors (or wrap
them with a clearer message) instead of falling through to the file-path branch.
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
2 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.
Summary
Add support for signer-specific registry URIs that allow users to configure signing keys without creating YAML configuration files.
Related PR: hyperlane-xyz/hyperlane-monorepo#7938
Motivation
Currently, CLI users must either:
--key <private-key>on the command line (exposes keys in shell history)This PR adds URI-based signer configuration for quick, secure key management:
gcp://project/secret-name- Load private key from GCP Secret Managerfoundry://account-name- Load key from Foundry keystoreImplementation
New Registry Types
GCPSignerRegistry (
src/registry/GCPSignerRegistry.ts)gcp://project/secret-nameURIsgcpSecretsigner typegcloudCLIFoundryKeystoreRegistry (
src/registry/FoundryKeystoreRegistry.ts)foundry://accountNameorfoundry:///path/to/keystores/accountNameURIsfoundryKeystoresigner typeETH_PASSWORDenv var for password file pathIRegistry Interface Changes
Added signer-related methods to
IRegistry:getSigners(): Get all named signersgetSigner(id): Get a specific signer by namegetDefaultSigner(chainName?): Get the default signergetSignerConfiguration(): Get full signer config with defaultsUsage Examples
Testing
GCPSignerRegistry(13 tests)FoundryKeystoreRegistry(33 tests)Checklist
Summary by CodeRabbit
Release Notes
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.