feat: read only registry #1272
feat: read only registry #1272
Conversation
🦋 Changeset detectedLatest commit: e23e651 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
📝 WalkthroughWalkthroughThis change adds a ReadonlyRegistry wrapper class exposing read-only behavior by delegating reads to an inner registry and blocking write methods. It also introduces an IRegistryWriteMethod type and a RegistryType.Readonly enum member, plus tests and a version bump. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ 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}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/registry/ReadonlyRegistry.ts (1)
86-90: Consider usingwarnlevel instead ofinfofor blocked operations.When someone tries to write to a readonly registry, that's usually not the expected path - might be worth logging at
warnlevel so it stands out more in logs. Currently usinginfomight make these calls blend in with normal operations, and folks might miss that their writes are being silently swallowed.That said, if the intention is that this is expected behavior in your use cases (like exposing to untrusted code where writes are anticipated and intentionally ignored),
infois fine. Just something to chew on.private skipMethodExecution(methodName: IRegistryWriteMethod): MaybePromise<void> { const logger = this.logger ?? console; - logger.info(`Skipping "${methodName}" method call as registry is readonly`); + logger.warn(`Skipping "${methodName}" method call as registry is readonly`); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.changeset/strange-apricots-search.md(1 hunks)src/registry/IRegistry.ts(2 hunks)src/registry/ReadonlyRegistry.ts(1 hunks)test/unit/registry.test.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
test/unit/registry.test.ts (3)
src/registry/ReadonlyRegistry.ts (1)
ReadonlyRegistry(46-137)src/registry/IRegistry.ts (1)
IRegistryWriteMethod(50-52)src/registry/PartialRegistry.ts (1)
PartialRegistry(35-130)
src/registry/ReadonlyRegistry.ts (3)
src/index.ts (1)
RegistryType(13-13)src/types.ts (2)
MaybePromise(10-10)WarpDeployConfigMap(42-42)src/registry/IRegistry.ts (1)
IRegistryWriteMethod(50-52)
🪛 ast-grep (0.40.0)
test/unit/registry.test.ts
[warning] 373-373: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(Skipping "${methodName}" method call as registry is readonly)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (13)
.changeset/strange-apricots-search.md (1)
1-5: Aye, looks good to me!Minor version bump is the right call for adding a new feature like this. The description captures what this is all about - a registry that won't let anyone muck about with the data.
src/registry/IRegistry.ts (2)
29-52: Nice bit of type magic here, well done!The
satisfies Record<IRegistryMethods, boolean>is a proper smart move - if someone adds a new method toIRegistrybut forgets to add it to this map, TypeScript will start complaining like a donkey. Keeps everything in sync without any extra effort.The derived
IRegistryWriteMethodtype is clean and will catch any misuse at compile time.
78-78: New enum value fits right in.The
Readonlyregistry type slots in nicely with the existing types. No complaints here.test/unit/registry.test.ts (5)
70-77: Good to see the readonly wrapper getting a proper seat at the table.Including
readonlyRegistryin the main test loop ensures it behaves like any proper registry for read operations. Smart way to get broad coverage without duplicating tests.
89-97: Display name test now accounts for the readonly type - nice catch.The condition properly includes
RegistryType.Readonlysince it wraps the merged registry which has the partial overlay.
294-366: This test structure is as robust as an ogre's constitution.Using
Record<IRegistryWriteMethod, ...>for the test cases is clever - if a new write method gets added to the interface but not to these tests, TypeScript will let you know right quick. Exhaustive testing without the manual bookkeeping.
368-377: About that static analysis warning - it's a false alarm.The
methodNamehere comes from theIRegistryWriteMethodtype which is a fixed set of known string literals ('addChain','updateChain', etc.). It's not user input, so there's no ReDoS risk. The analyzer just doesn't have the context to know these are safe, controlled values.
379-402: Read delegation and merge behavior tests are solid.Good coverage of the core functionality - reads go through, and merging preserves the readonly nature. The assertion that
mergedis aninstanceOf ReadonlyRegistryis exactly what we need to verify the wrapper behavior is maintained.src/registry/ReadonlyRegistry.ts (5)
26-44: The documentation is like layers on an onion - helpful and well-structured.Good example usage and clear explanation of when to use this. Helps other developers understand the purpose without having to dig through the implementation.
46-52: Constructor is clean and simple.Storing the inner registry and copying over the URI makes sense. The readonly modifiers on constructor parameters are a nice touch for immutability.
131-136: Merge keeps the readonly wrapper - exactly right.Wrapping the
MergedRegistryin a newReadonlyRegistryensures the readonly guarantee isn't lost when combining registries. Good defensive design.
54-84: Read method delegation is straightforward and complete.All the read operations just pass through to the inner registry. Nothing fancy, nothing to break - exactly how a wrapper should work.
92-121: Write methods properly blocked with type-safe method names.Each write method passes its name to
skipMethodExecution, and TypeScript ensures we can only pass validIRegistryWriteMethodvalues. Clean and maintainable.
src/registry/IRegistry.ts
Outdated
| warpRouteId: WarpRouteId; | ||
| }; | ||
|
|
||
| export interface IRegistry { |
There was a problem hiding this comment.
did you consider separating this interface into IReadRegistry and IWriteRegistry and IRegistry is IReadRegistry, IWriteRegistry?
There was a problem hiding this comment.
Yes, the reason for not doing it, in the even if the ReadonlyRegistry will be used in specific contexts, is that imo it should still be possible to use it wherever a IRegistry might be used. If it implemented only a subset of the IRegistry interface methods, we wouldn't be able to use it everywhere and additionally client code (sdk, cli ...) should be aware of the new interface and conditionally call methods based on the interface type which would be a much bigger change.
There was a problem hiding this comment.
as suggested, we could maintain backward compatibility by having the IRegistry be the union of the two interfaces
I think its desirable for the client to only have access to the read methods when using the read interface
| addChain(_chain: UpdateChainParams): MaybePromise<void> { | ||
| return this.skipMethodExecution('addChain'); | ||
| } | ||
|
|
||
| updateChain(_chain: UpdateChainParams): MaybePromise<void> { | ||
| return this.skipMethodExecution('updateChain'); | ||
| } |
There was a problem hiding this comment.
imo it makes sense to encode this in the type and catch this at compile time rather than have an exception at runtime
There was a problem hiding this comment.
we wouldn't have an exception because the skipMethodExecution would only log the info message that the call will be skipped as the current registry is a readonly one
There was a problem hiding this comment.
that seems possibly worse to "silently" skip the operation requested by the client?
There was a problem hiding this comment.
Seems a pretty common pattern in the registry code as we already do something similar in the merged registry (not saying it is correct, I'd be in favor of changing it later to be more explicit tbh)
hyperlane-registry/src/registry/MergedRegistry.ts
Lines 160 to 179 in d5a2f9c
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/registry/IRegistry.ts (1)
66-76: CleanIBaseRegistry/IReadRegistrysplit; minor typing polish you might considerThe split into
IBaseRegistryplus a read‑only surface (IReadRegistry) lines up nicely with the earlier design discussion and keepsunimplementedMethodscentralized. Structurally this reads well and should make it straightforward to plug in a readonly implementation that only exposes reads.Two small nits you may want to consider, nothing blocking:
getWarpRoute/getWarpDeployConfigcurrently takerouteId: string; if you want tighter typing to match how you key maps and options, usingWarpRouteIdhere (like inAddWarpRouteConfigOptions) would keep things a bit more coherent.- The TODOs for “more deployment artifact related methods” are duplicated across read/write; if they’re meant to diverge (read vs write concerns), you might eventually want slightly more specific wording or separate TODOs so future you doesn’t get lost in the swamp.
Also applies to: 77-99
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/registry/IRegistry.ts(5 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/registry/IRegistry.ts (2)
src/types.ts (5)
MaybePromise(10-10)WarpRouteFilterParams(29-29)WarpRouteConfigMap(41-41)WarpDeployConfigMap(42-42)UpdateChainParams(36-38)src/index.ts (5)
WarpRouteFilterParams(29-29)WarpRouteConfigMap(28-28)UpdateChainParams(31-31)AddWarpRouteConfigOptions(9-9)IRegistry(11-11)
🔇 Additional comments (2)
src/registry/IRegistry.ts (2)
55-55:RegistryType.Readonlyenum member looks good; mind exhaustiveness checksThe new
Readonly = 'readonly'fits the existing enum pattern just fine. Only thing to watch is anyswitch/ifladders overRegistryTypethat assume the old set; they may need a new branch for the readonly case depending on how strict the handling is.
101-113:IWriteRegistry+ compositeIRegistrytype keep the public surface intactThe write surface hanging off
IWriteRegistry(chain mutations + warp additions) looks straightforward, and redefiningIRegistryasIReadRegistry & IWriteRegistry & { merge(...) }keeps the overall contract essentially the same while allowing the readonly flavor to implement only what it cares about and mark the rest as unimplemented.From a consumer’s point of view, classes can still
implements IRegistryjust fine, andmerge(otherRegistry: IRegistry): IRegistrykeeps the type story clear for registry composition.Also applies to: 115-118
| */ | ||
| export type IRegistryMethods = MethodsOf<Omit<IRegistry, 'unimplementedMethods'>>; | ||
|
|
||
| export type IRegistryWriteMethod = keyof IWriteRegistry; |
There was a problem hiding this comment.
Tighten IRegistryWriteMethod to only include method keys
IRegistryWriteMethod = keyof IWriteRegistry includes non-function properties (type, uri, unimplementedMethods) alongside the actual mutating methods. This creates a type mismatch when used with Set<IRegistryMethods> as intended for the unimplementedMethods field.
Use MethodsOf<IWriteRegistry> instead to filter to function-valued keys only:
-export type IRegistryWriteMethod = keyof IWriteRegistry;
+export type IRegistryWriteMethod = MethodsOf<IWriteRegistry>;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export type IRegistryWriteMethod = keyof IWriteRegistry; | |
| export type IRegistryWriteMethod = MethodsOf<IWriteRegistry>; |
🤖 Prompt for AI Agents
In src/registry/IRegistry.ts around line 29, the type alias IRegistryWriteMethod
is currently keyof IWriteRegistry which includes non-function properties and
causes a mismatch when used with Set<IRegistryMethods>; change it to use
MethodsOf<IWriteRegistry> (or the project’s equivalent utility type that selects
only function-valued keys) so IRegistryWriteMethod represents only method keys,
and update any imports/exports accordingly to reference MethodsOf.
| IWriteRegistry & { | ||
| merge(otherRegistry: IRegistry): IRegistry; | ||
| }; |
There was a problem hiding this comment.
maybe you can put this on the BaseRegistry?
Description
Adds a read only registry for environments where it is desirable not to write back to the underlying registry implementation
Backward compatibility
Testing
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.