refactor: move /v1/nodes and /v1/version to a module#1458
refactor: move /v1/nodes and /v1/version to a module#1458stalniy merged 1 commit intoakash-network:mainfrom
Conversation
|
""" WalkthroughThis change refactors the handling of network nodes and version routes by removing legacy route files and introducing a new modular structure. It adds new service, controller, schema, and router layers for network-related endpoints, updates dependency injection and constants, and consolidates the HTTP SDK node logic with improved type safety and retry handling. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API (networkRouter)
participant NetworkController
participant NetworkService
participant NodeHttpService
Client->>API (networkRouter): GET /v1/nodes/{network}
API (networkRouter)->>NetworkController: getNodes(network)
NetworkController->>NetworkService: getNodes(network)
NetworkService->>NodeHttpService: getNodes(network)
NodeHttpService-->>NetworkService: [nodes]
NetworkService-->>NetworkController: [nodes]
NetworkController-->>API (networkRouter): [nodes]
API (networkRouter)-->>Client: JSON response
Client->>API (networkRouter): GET /v1/version/{network}
API (networkRouter)->>NetworkController: getVersion(network)
NetworkController->>NetworkService: getVersion(network)
NetworkService->>NodeHttpService: getVersion(network)
NodeHttpService-->>NetworkService: "version"
NetworkService-->>NetworkController: "version"
NetworkController-->>API (networkRouter): "version"
API (networkRouter)-->>Client: text response
Assessment against linked issues
Suggested reviewers
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (25)
💤 Files with no reviewable changes (9)
🚧 Files skipped from review as they are similar to previous changes (16)
⏰ Context from checks skipped due to timeout of 90000ms (6)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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 (
|
jzsfkzm
force-pushed
the
refactor/1271-nodes-and-versions
branch
from
586d295 to
bf01292
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1458 +/- ##
==========================================
- Coverage 40.34% 40.32% -0.02%
==========================================
Files 875 872 -3
Lines 21107 21097 -10
Branches 3846 3844 -2
==========================================
- Hits 8515 8508 -7
+ Misses 12420 11860 -560
- Partials 172 729 +557
*This pull request uses carry forward flags. Click here to find out more.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
apps/api/src/utils/constants.ts (1)
22-22: Suggest renaming constant tonodeApiBasePathfor clarity.The current name
nNodeApiBasePathhas an extra leading “n”; aligning with other constants (e.g.,apiNodeUrl) improves readability and reduces confusion.apps/api/test/seeders/node.seeder.ts (1)
6-9: Simplify faker usage pattern.The current usage
faker.faker.string.alphanumeric()is redundant. Modern faker API allows direct usage of methods.- id = faker.faker.string.alphanumeric(), - api = faker.faker.string.alphanumeric(), - rpc = faker.faker.string.alphanumeric() + id = faker.string.alphanumeric(), + api = faker.string.alphanumeric(), + rpc = faker.string.alphanumeric()apps/api/src/network/controllers/network/network.controller.ts (1)
6-17: LGTM! Clean controller implementation with proper dependency injection.The controller follows good patterns with dependency injection and proper type safety. Consider adding error handling to transform service-level exceptions into appropriate HTTP responses.
Consider adding error handling:
async getNodes(network: GetNodesParams["network"]): Promise<GetNodesResponse> { + try { return await this.networkService.getNodes(network); + } catch (error) { + // Transform service errors to appropriate HTTP responses + throw error; + } } async getVersion(network: GetNodesParams["network"]): Promise<string> { + try { return await this.networkService.getVersion(network); + } catch (error) { + // Transform service errors to appropriate HTTP responses + throw error; + } }packages/http-sdk/src/node/node-http.service.ts (1)
21-43: Consider making endpoint paths configurable.The hardcoded endpoint paths work but could be made more maintainable by extracting them to configuration constants. This would make it easier to update paths or support different environments.
Consider extracting endpoint paths to constants:
+const ENDPOINTS = { + mainnet: { + nodes: "console/main/config/mainnet-nodes.json", + version: "net/master/mainnet/version.txt" + }, + sandbox: { + nodes: "console/main/config/sandbox-nodes.json", + version: "net/master/sandbox/version.txt" + }, + testnet: { + nodes: "console/main/config/testnet-nodes.json", + version: "net/master/testnet-02/version.txt" + } +} as const; async getMainnetNodes() { - return this.extractData(await this.get<NetworkNode[]>("console/main/config/mainnet-nodes.json")); + return this.extractData(await this.get<NetworkNode[]>(ENDPOINTS.mainnet.nodes)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (24)
apps/api/src/app.ts(2 hunks)apps/api/src/core/providers/http-sdk.provider.ts(2 hunks)apps/api/src/network/controllers/network/network.controller.ts(1 hunks)apps/api/src/network/http-schemas/network.schema.ts(1 hunks)apps/api/src/network/index.ts(1 hunks)apps/api/src/network/routes/index.ts(1 hunks)apps/api/src/network/routes/network/network.router.ts(1 hunks)apps/api/src/network/services/network/network.service.ts(1 hunks)apps/api/src/routes/v1/index.ts(0 hunks)apps/api/src/routes/v1/nodes/mainnet.ts(0 hunks)apps/api/src/routes/v1/nodes/nodeClient.ts(0 hunks)apps/api/src/routes/v1/nodes/sandbox.ts(0 hunks)apps/api/src/routes/v1/nodes/testnet.ts(0 hunks)apps/api/src/routes/v1/version/mainnet.ts(0 hunks)apps/api/src/routes/v1/version/sandbox.ts(0 hunks)apps/api/src/routes/v1/version/testnet.ts(0 hunks)apps/api/src/utils/constants.ts(1 hunks)apps/api/test/functional/nodes-v1.spec.ts(1 hunks)apps/api/test/seeders/node.seeder.ts(1 hunks)apps/api/tsconfig.strict.json(1 hunks)packages/http-sdk/src/index.ts(1 hunks)packages/http-sdk/src/node/index.ts(1 hunks)packages/http-sdk/src/node/node-http.service.ts(1 hunks)packages/http-sdk/src/node/types.ts(1 hunks)
💤 Files with no reviewable changes (8)
- apps/api/src/routes/v1/version/mainnet.ts
- apps/api/src/routes/v1/version/testnet.ts
- apps/api/src/routes/v1/nodes/mainnet.ts
- apps/api/src/routes/v1/version/sandbox.ts
- apps/api/src/routes/v1/nodes/testnet.ts
- apps/api/src/routes/v1/nodes/sandbox.ts
- apps/api/src/routes/v1/nodes/nodeClient.ts
- apps/api/src/routes/v1/index.ts
🧰 Additional context used
🧬 Code Graph Analysis (8)
apps/api/src/app.ts (1)
apps/api/src/network/routes/network/network.router.ts (1)
networkRouter(45-45)
apps/api/src/core/providers/http-sdk.provider.ts (2)
packages/http-sdk/src/node/node-http.service.ts (1)
NodeHttpService(10-44)apps/api/src/utils/constants.ts (1)
nNodeApiBasePath(22-22)
packages/http-sdk/src/node/types.ts (1)
apps/api/src/network/http-schemas/network.schema.ts (1)
NetworkNode(16-16)
apps/api/src/network/controllers/network/network.controller.ts (2)
apps/api/src/network/services/network/network.service.ts (1)
singleton(8-35)apps/api/src/network/http-schemas/network.schema.ts (2)
GetNodesParams(15-15)GetNodesResponse(17-17)
apps/api/src/network/services/network/network.service.ts (4)
apps/api/src/network/controllers/network/network.controller.ts (1)
singleton(6-17)packages/http-sdk/src/node/node-http.service.ts (1)
NodeHttpService(10-44)apps/api/src/caching/helpers.ts (1)
Memoize(22-38)apps/api/src/network/http-schemas/network.schema.ts (2)
GetNodesParams(15-15)GetNodesResponse(17-17)
apps/api/src/network/http-schemas/network.schema.ts (1)
packages/http-sdk/src/node/types.ts (1)
NetworkNode(1-5)
apps/api/test/seeders/node.seeder.ts (2)
packages/http-sdk/src/node/types.ts (1)
NetworkNode(1-5)apps/api/src/network/http-schemas/network.schema.ts (1)
NetworkNode(16-16)
packages/http-sdk/src/node/node-http.service.ts (3)
packages/http-sdk/src/http/http.service.ts (1)
HttpService(3-15)packages/http-sdk/src/node/types.ts (1)
NetworkNode(1-5)apps/api/src/network/http-schemas/network.schema.ts (1)
NetworkNode(16-16)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: test-api-build
- GitHub Check: validate-api
- GitHub Check: validate-notifications
- GitHub Check: test-deploy-web-build
🔇 Additional comments (23)
apps/api/src/utils/constants.ts (1)
22-22: EnsureNODE_API_BASE_PATHis defined or provide a fallback.If
env.NODE_API_BASE_PATHis undefined, the DI registration ofNodeHttpServicewill receiveundefinedas itsbaseURL. Consider validating this value at startup (and throwing early) or supplying a sensible default to avoid runtime errors.apps/api/src/network/index.ts (1)
1-1: Approve central re-export of the network routes.Re-exporting all exports from
./routescorrectly exposes the network routing module interface.apps/api/src/app.ts (2)
48-48: Approve import ofnetworkRouter.The new network routes are correctly brought in via the module index and follow the project’s routing pattern.
133-133: Verify route ordering and overlaps.Mounting
networkRouterat root ("/") is consistent, but double-check that its/v1/...paths don’t conflict with neighbouring routers (pricingRouter,notificationsApiProxy, etc.) and that middleware ordering remains correct.packages/http-sdk/src/index.ts (1)
22-22: Approve exposing node API in HTTP SDK public API.Re-exporting the new
./nodesubmodule ensuresNodeHttpServiceand its types are available to consumers.apps/api/src/network/routes/index.ts (1)
1-1: LGTM! Clean barrel export pattern.The export statement follows standard patterns for centralizing module exports and integrates well with the network module refactoring.
packages/http-sdk/src/node/index.ts (1)
1-2: LGTM! Proper module exports for HTTP SDK node functionality.The exports provide clean access to both the
NodeHttpServiceand related types, following standard barrel export patterns.apps/api/tsconfig.strict.json (1)
29-29: LGTM! Appropriate addition to strict type checking.Adding the network module to strict TypeScript configuration ensures type safety for the new network-related code.
apps/api/src/core/providers/http-sdk.provider.ts (3)
11-11: LGTM! Proper import of NodeHttpService.The import follows the established pattern and integrates the new HTTP service into the dependency injection system.
16-16: LGTM! Appropriate import of the node API base path constant.The import of
nNodeApiBasePathprovides the necessary configuration for the NodeHttpService registration.
33-33: LGTM! Correct service registration pattern.The NodeHttpService registration follows the same pattern as other services with custom base URLs (GitHubHttpService, CoinGeckoHttpService). The separation from the SERVICES array is appropriate since it uses a different base URL than the other services.
packages/http-sdk/src/node/types.ts (1)
1-5:Details
✅ Verification successful
Type definition looks good, but verify consistency with Zod schema.
The
NetworkNodetype is well-defined with clear property names and types. However, there's also aNetworkNodetype defined via Zod schema inapps/api/src/network/http-schemas/network.schema.ts. Ensure both types are structurally identical to avoid type inconsistencies.
🏁 Script executed:
#!/bin/bash # Verify that both NetworkNode type definitions are structurally identical echo "=== HTTP SDK NetworkNode type ===" cat packages/http-sdk/src/node/types.ts echo -e "\n=== API Schema NetworkNode definition ===" ast-grep --pattern 'export const NodeSchema = $_'Length of output: 712
NetworkNode type definitions are consistent across modules
The
NetworkNodetype inpackages/http-sdk/src/node/types.tsand the ZodNodeSchemainapps/api/src/network/http-schemas/network.schema.tsboth defineid,api, andrpcas strings, so no further changes are needed.apps/api/test/seeders/node.seeder.ts (1)
1-2: Import changes look good.The migration to use
NetworkNodefrom the HTTP SDK package and the namespace import for faker are both appropriate changes that align with the refactoring objectives.apps/api/src/network/routes/network/network.router.ts (4)
8-26: Well-structured route definition.The nodes route is properly configured with OpenAPI metadata, parameter validation, and clear response schema. The implementation follows Hono and OpenAPI best practices.
28-43: Good version route implementation.The version route is well-documented with helpful description linking to the actual version files. The OpenAPI configuration is appropriate for a text response.
47-52: Clean handler implementation for nodes.The route handler properly extracts validated parameters, resolves the controller via dependency injection, and returns the response in the correct JSON format.
54-59: Appropriate text response for version.The version handler correctly returns the response as plain text, which is suitable for version information.
apps/api/src/network/services/network/network.service.ts (2)
8-11: Well-structured service with proper dependency injection.The singleton pattern and constructor injection are correctly implemented, following good dependency injection practices.
12-12: Excellent use of memoization for performance.The 5-minute cache TTL is appropriate for network configuration data that doesn't change frequently. This will significantly reduce external API calls and improve response times.
Also applies to: 24-24
packages/http-sdk/src/node/node-http.service.ts (1)
7-8: Good constants definition for retry configuration.The retry constants are appropriately defined and will make the retry behavior predictable.
apps/api/src/network/http-schemas/network.schema.ts (3)
3-5: Good network parameter validation.The enum correctly restricts network values to the supported networks, matching the implementation in
NodeHttpService.
15-17: Excellent type definitions with proper inference.The type exports provide good TypeScript integration and will ensure type safety across the network module.
7-11:Details
✅ Verification successful
Verify schema consistency with http-sdk NetworkNode type.
The NodeSchema matches the structure of
NetworkNodefrompackages/http-sdk/src/node/types.ts. Consider adding URL validation for theapiandrpcfields since they represent endpoints.Let me verify the type consistency between the schema and the http-sdk NetworkNode type:
Consider adding URL validation for endpoint fields:
export const NodeSchema = z.object({ id: z.string(), - api: z.string(), - rpc: z.string() + api: z.string().url("API endpoint must be a valid URL"), + rpc: z.string().url("RPC endpoint must be a valid URL") });
🏁 Script executed:
#!/bin/bash # Check if the NetworkNode type in http-sdk matches our schema cat packages/http-sdk/src/node/types.ts # Also check if there are any other NetworkNode type definitions that might conflict rg -A 5 "type NetworkNode" --type tsLength of output: 642
Schema matches NetworkNode; consider URL validation
- Verified that
NodeSchemaproperties (id,api,rpc) exactly align with theNetworkNodetype inpackages/http-sdk/src/node/types.ts.- No conflicting definitions of
NetworkNodewere found in the codebase.To enforce that
apiandrpcare valid URLs, you can update the schema as follows:-export const NodeSchema = z.object({ - id: z.string(), - api: z.string(), - rpc: z.string() -}); +export const NodeSchema = z.object({ + id: z.string(), + api: z.string().url("API endpoint must be a valid URL"), + rpc: z.string().url("RPC endpoint must be a valid URL") +});
jzsfkzm
force-pushed
the
refactor/1271-nodes-and-versions
branch
2 times, most recently
from
511bde9 to
9982207
Compare
jzsfkzm
force-pushed
the
refactor/1271-nodes-and-versions
branch
2 times, most recently
from
04edeed to
27dd512
Compare
Head branch was pushed to by a user without write access
jzsfkzm
force-pushed
the
refactor/1271-nodes-and-versions
branch
from
6e817e1 to
ac384ac
Compare
stalniy
force-pushed
the
refactor/1271-nodes-and-versions
branch
from
ac384ac to
3b971b7
Compare
3 participants
closes #1271
closes #1276
Summary by CodeRabbit
New Features
Improvements
Refactor
Bug Fixes