feat(arns): allow operator to override arns ttls and force refreshes … #212
Conversation
…based on env variable Intrdocues `ARNS_RESOLVER_OVERRIDE_TTL_SECONDS` env variable which can be used to override when to refresh arns names. If set to `0` the resolver will always attempt to fetch the latest resolution data from the resolvers.
📝 WalkthroughWalkthroughThe changes introduce new configuration options for the ARNS resolver, specifically related to time-to-live (TTL) settings. Two constants are added to manage TTL values from environment variables. The Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #212 +/- ##
===========================================
- Coverage 70.20% 68.71% -1.50%
===========================================
Files 32 32
Lines 7908 7917 +9
Branches 430 431 +1
===========================================
- Hits 5552 5440 -112
- Misses 2356 2476 +120
- Partials 0 1 +1 ☔ View full report in Codecov by Sentry. |
![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
🧹 Outside diff range and nitpick comments (5)
src/init/resolvers.ts (1)
73-73: LGTM! Consider adding a comment for clarity.The addition of the
overridesparameter aligns well with the PR objectives, allowing operators to control ARNs TTLs. The implementation is correct and doesn't affect existing functionality.Consider adding a brief comment explaining the purpose of the
overridesparameter, for example:/** * @param overrides Optional settings to override default behavior * @param overrides.ttlSeconds Optional TTL in seconds for ARNs resolution */ overrides?: { ttlSeconds?: number; };This will help future developers understand the purpose and usage of this parameter.
Also applies to: 80-82
src/resolution/composite-arns-resolver.ts (2)
27-32: LGTM! Consider adding JSDoc for theoverridesproperty.The new
overridesproperty aligns well with the PR objectives. The TODO comment is a good practice for future enhancements.Consider adding a JSDoc comment to explain the purpose and usage of the
overridesproperty. This will improve code documentation and maintainability. For example:/** * Optional overrides for resolver behavior. * @property {number} [ttlSeconds] - Override for the TTL in seconds. */ private overrides: { ttlSeconds?: number; // TODO: other overrides like fallback txId if not found in resolution } | undefined;
64-65: LGTM! TTL override logic implemented correctly. Consider extracting the TTL calculation.The changes correctly implement the TTL override functionality while maintaining backward compatibility. The cache validity check is properly updated to use the potentially overridden TTL.
For improved readability and maintainability, consider extracting the TTL calculation into a separate method. This would make the logic more explicit and easier to test. For example:
private getEffectiveTTL(cachedResolution: NameResolution): number | undefined { return this.overrides?.ttlSeconds ?? cachedResolution.ttl; } // Then in the resolve method: const ttlSeconds = this.getEffectiveTTL(cachedResolution); if ( cachedResolution !== undefined && cachedResolution.resolvedAt !== undefined && ttlSeconds !== undefined && cachedResolution.resolvedAt + ttlSeconds * 1000 > Date.now() ) { // ... rest of the code }Also applies to: 69-70
src/config.ts (1)
271-278: LGTM! Consider adding a comment for clarity.The implementation of
ARNS_RESOLVER_OVERRIDE_TTL_SECONDSis correct and aligns well with the existing codebase. It provides a flexible way to override the TTL for ARNS resolvers through an environment variable.Consider adding a brief comment explaining the purpose of these constants and how they affect the ARNS resolver behavior. This would improve code readability and maintainability. For example:
// Override the default TTL for ARNS resolvers. If set to 0, it forces a refresh on each resolution. export const ARNS_RESOLVER_OVERRIDE_TTL_SECONDS_STRING = env.varOrUndefined( 'ARNS_RESOLVER_OVERRIDE_TTL_SECONDS', ); export const ARNS_RESOLVER_OVERRIDE_TTL_SECONDS = ARNS_RESOLVER_OVERRIDE_TTL_SECONDS_STRING !== undefined ? +ARNS_RESOLVER_OVERRIDE_TTL_SECONDS_STRING : undefined;src/system.ts (1)
571-574: LGTM! Consider documenting the new configuration option.The addition of the
overridesobject with thettlSecondsproperty aligns well with the PR objectives. It provides the desired flexibility for operators to control the TTL for ARN resolution.A few suggestions:
- Consider adding documentation for the
ARNS_RESOLVER_OVERRIDE_TTL_SECONDSconfiguration option, including its purpose, expected values, and any default value.- Regarding the TODO comment, do you have any specific plans or timeline for implementing other overrides like the fallback
txId? It might be helpful to create an issue to track this future enhancement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- src/config.ts (1 hunks)
- src/init/resolvers.ts (2 hunks)
- src/resolution/composite-arns-resolver.ts (2 hunks)
- src/system.ts (1 hunks)
🔇 Additional comments (6)
src/init/resolvers.ts (1)
120-120: Verify CompositeArNSResolver implementationThe
overridesparameter is correctly passed to theCompositeArNSResolverconstructor. However, we should verify that theCompositeArNSResolverclass is prepared to handle this new parameter.Let's check the implementation of
CompositeArNSResolver:✅ Verification successful
CompositeArNSResolver correctly handles the
overridesparameterThe
overridesparameter is properly assigned and utilized within theCompositeArNSResolverclass constructor and methods.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify CompositeArNSResolver implementation # Expected result: CompositeArNSResolver constructor should accept an 'overrides' parameter # Search for the CompositeArNSResolver class definition ast-grep --lang typescript --pattern 'class CompositeArNSResolver { constructor($_) { $$$ } $$$ }' # Search for usage of 'overrides' within the CompositeArNSResolver class rg --type typescript -A 5 'overrides' src/resolution/composite-arns-resolver.tsLength of output: 225
Script:
#!/bin/bash # Description: Verify CompositeArNSResolver implementation with adjusted file search # Expected result: CompositeArNSResolver constructor should accept an 'overrides' parameter # Search for the CompositeArNSResolver class definition with a broader pattern ast-grep --lang typescript --pattern 'class CompositeArNSResolver { constructor($_) { $$$ } $$$ }' # Search for usage of 'overrides' within the CompositeArNSResolver class by targeting .ts files explicitly rg -A 5 'overrides' --glob '*.ts' src/resolution/composite-arns-resolver.tsLength of output: 1267
Script:
#!/bin/bash # Description: Verify CompositeArNSResolver implementation with improved patterns # Expected result: CompositeArNSResolver constructor should accept an 'overrides' parameter # Search for the CompositeArNSResolver class definition with a flexible pattern ast-grep --lang typescript --pattern 'class CompositeArNSResolver { constructor($_) { $$$ } $$$ }' # Search for usage of 'overrides' within the CompositeArNSResolver class by targeting .ts files explicitly rg -A 5 'overrides' --glob '*.ts' src/resolution/composite-arns-resolver.tsLength of output: 1267
src/resolution/composite-arns-resolver.ts (3)
38-38: LGTM! Constructor changes implement the overrides functionality correctly.The updated constructor signature and implementation properly handle the new
overridesparameter. The changes maintain backward compatibility by making the parameter optional.Also applies to: 43-45, 50-50
54-54: LGTM! Enhanced logging improves observability.The updated logging statement now includes the
overridesinformation, which is valuable for debugging and monitoring the resolver's behavior with respect to TTL overrides.
Line range hint
1-124: Overall, the changes successfully implement the TTL override functionality.The modifications to the
CompositeArNSResolverclass effectively introduce the ability to override TTLs for ARN resolution, aligning well with the PR objectives. The implementation is backward compatible and maintains existing functionality. The code quality is good, with clear type definitions and appropriate error handling.A few minor suggestions have been made to further improve code documentation and readability:
- Adding JSDoc for the
overridesproperty.- Extracting the TTL calculation logic into a separate method for better maintainability.
These changes enhance the flexibility of the ARN resolution process and provide operators with more control over caching behavior, as intended.
src/config.ts (2)
271-278: Summary: New ARNS resolver TTL override feature added.The changes introduce a new feature to override the TTL for ARNS resolvers through environment variables. This addition:
- Aligns well with the existing configuration patterns in the file.
- Provides more flexibility in managing ARNS resolution behavior.
- Has minimal impact on the existing codebase structure.
These changes enhance the configurability of the AR.IO Gateway, particularly for operators who need fine-grained control over ARNS resolution caching. Ensure that the team is aware of this new configuration option and updates any relevant documentation or deployment scripts accordingly.
271-278: Verify the usage of new constants in the codebase.The new constants
ARNS_RESOLVER_OVERRIDE_TTL_SECONDS_STRINGandARNS_RESOLVER_OVERRIDE_TTL_SECONDSprovide a mechanism to override the TTL for ARNS resolvers. To ensure correct implementation:
- Verify that the parts of the codebase using these constants handle the potential
undefinedvalue correctly.- Check if any existing ARNS resolver logic needs to be updated to incorporate this new override functionality.
To assist with this verification, you can run the following script:
This script will help identify where the new constants are used and where ARNS resolver logic might need to be updated.
…based on env variable
Introduces
ARNS_RESOLVER_OVERRIDE_TTL_SECONDSenv variable which can be used to override when to refresh arns names. If set to0the resolver will always attempt to fetch the latest resolution data from the resolvers. If it fails to refretch, it will fallback to what it has cached.Testing
First resolved at: 1727378491422
Second resolved at: 1727378509536 (18s later)
Logs