feat: client refresh rate#62
Conversation
WalkthroughA new configurable Changes
sequenceDiagram
autonumber
participant Config as Config Loader
participant Wallet as WalletWorker
participant Controller as ClientController
participant DB as Client DB
rect rgb(240,248,255)
Note over Config,Wallet: Startup / Worker init
Config->>Wallet: provide clientRefreshRate
end
rect rgb(245,255,240)
Note over Wallet,Controller: Periodic client refresh flow
Wallet->>Controller: getClientsToUpdate(chainId, peers, clientRefreshRate)
Controller->>DB: query clients and lastUpdated
alt missing client
DB-->>Controller: no client found
Controller->>DB: addClient(...) -- awaited
DB-->>Controller: new client record
end
Controller-->>Wallet: list of clients needing update (based on clientRefreshRate)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Fix all issues with AI Agents 🤖
In @src/lib/config.ts:
- Around line 47-48: The env CLIENT_REFRESH_RATE is parsed into
envConfig.clientRefreshRate but mergeConfigs does not include that field; update
mergeConfigs to copy envConfig.clientRefreshRate into the resulting config
(respecting existing config.clientRefreshRate if present), referencing
envConfig.clientRefreshRate and the mergeConfigs function so the environment
override is applied to the final config.
🧹 Nitpick comments (1)
README.md (1)
56-56: Clarify the example value vs. default.The example shows
clientRefreshRate: 0.1, but Line 121 documents the default as1/3(≈0.333). This inconsistency may confuse users. Consider either using the default value in the example or adding a comment explaining why a non-default value is shown.Suggested clarification
"rpcRequestTimeout": 5000, - "clientRefreshRate": 0.1, + "clientRefreshRate": 0.333, // Default: 1/3 of trusting period "chains": [
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (5)
README.mdsrc/db/controller/client.tssrc/lib/config.tssrc/workers/index.tssrc/workers/wallet.ts
🧰 Additional context used
🧬 Code graph analysis (2)
src/workers/wallet.ts (1)
src/db/controller/packet.ts (1)
PacketFilter(800-805)
src/workers/index.ts (1)
src/lib/config.ts (1)
config(308-308)
🔇 Additional comments (6)
src/workers/wallet.ts (2)
40-41: LGTM!The
clientRefreshRateparameter is properly added to the constructor and threaded through to the client update logic.
226-230: LGTM!The
clientRefreshRateis correctly passed togetClientsToUpdate, enabling configurable refresh behavior.src/lib/config.ts (1)
319-319: LGTM!The
clientRefreshRatefield is correctly added to the Config interface as an optional number.src/db/controller/client.ts (2)
139-139: Good fix for async consistency.Adding
awaitensuresgetClientconsistently returnsClientTablerather than potentially returningPromise<ClientTable>. This improves type safety and prevents runtime issues.
144-145: LGTM!The
clientRefreshRateparameter is properly integrated with a sensible default of 0.333 (1/3 of the trusting period). The threshold calculation correctly applies the rate to determine when clients need updates.Also applies to: 170-172
src/workers/index.ts (1)
140-141: LGTM!The
config.clientRefreshRateis correctly passed to theWalletWorkerconstructor. Note that this currently only works when set inconfig.jsondue to the missing merge logic insrc/lib/config.ts(see comment on that file).
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/lib/config.ts (1)
47-48: Consider adding validation for numeric environment variables.The code uses
Number()to parseCLIENT_REFRESH_RATE, which returnsNaNfor invalid input. While this follows the existing pattern for other numeric environment variables in this file, adding validation could prevent silent failures, especially for rate values that likely need to be positive.🔎 Example validation approach
if (env.CLIENT_REFRESH_RATE) - envConfig.clientRefreshRate = Number(env.CLIENT_REFRESH_RATE) + const rate = Number(env.CLIENT_REFRESH_RATE) + if (!isNaN(rate) && rate > 0) { + envConfig.clientRefreshRate = rate + } else { + throw new Error('CLIENT_REFRESH_RATE must be a positive number') + }Note: This same validation pattern could be applied to other numeric environment variables for consistency.
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Cache: Disabled due to data retention organization setting
Knowledge base: Disabled due to Reviews -> Disable Knowledge Base setting
📒 Files selected for processing (1)
src/lib/config.ts
🔇 Additional comments (2)
src/lib/config.ts (2)
282-283: Previous critical issue resolved – merge logic now present.The merge logic for
clientRefreshRateis now correctly implemented, resolving the critical issue flagged in the previous review. The implementation follows the same pattern as other optional configuration properties and ensures that environment variables are properly merged into the final config.
321-321: LGTM!The type definition is correct. Marking
clientRefreshRateas optional is appropriate, and thenumbertype aligns with the usage pattern.
Summary by CodeRabbit
New Features
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.