feat: provider-proxy accepts REST_API_NODE_URL #2225
Conversation
WalkthroughThe PR refactors provider-proxy to eliminate network-based configuration. It introduces environment-based REST_API_NODE_URL configuration via env-loader and dotenv, removes the Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Container as Container
participant EnvConfig as EnvConfig
participant ProviderSvc as ProviderService
participant CertValidator as CertificateValidator
App->>Container: createContainer()
Container->>EnvConfig: Parse envSchema
EnvConfig-->>Container: REST_API_NODE_URL
Container->>ProviderSvc: new ProviderService(chainBaseUrl)
Container->>CertValidator: new CertificateValidator(providerService)
Container-->>App: {providerService, certificateValidator, ...}
App->>CertValidator: validate(certificate, providerAddress)
CertValidator->>ProviderSvc: getCertificate(providerAddress, serialNumber)
ProviderSvc->>ProviderSvc: fetch(chainBaseUrl + queryParams)
ProviderSvc-->>CertValidator: X509Certificate
CertValidator-->>App: CertValidationResult
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ 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 |
❌ 27 Tests Failed:
View the top 3 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/provider-proxy/src/services/ProviderService/ProviderService.ts (1)
15-31: Add input validation for serialNumber to prevent BigInt conversion errors.Line 19 converts the
serialNumberfrom hex to decimal usingBigInt(\0x${serialNumber}`)without validating thatserialNumber` contains only valid hexadecimal characters. The pipeline failure confirms this causes a runtime error when invalid serial numbers are provided (e.g., "provider" instead of hex digits).Add validation before the BigInt conversion:
async getCertificate(providerAddress: string, serialNumber: string): Promise<X509Certificate | null> { + // Validate serialNumber is a valid hex string + if (!/^[0-9a-fA-F]+$/.test(serialNumber)) { + this.logger?.warn(`Invalid serial number format: ${serialNumber}`); + return null; + } + const queryParams = new URLSearchParams({ "filter.state": "valid", "filter.owner": providerAddress, "filter.serial": BigInt(`0x${serialNumber}`).toString(10), "pagination.limit": "1" });
♻️ Duplicate comments (2)
apps/deploy-web/env/.env.sample (1)
15-17: Consider alphabetical ordering (same as other env files).As noted in the
.env.productionreview, these keys should ideally be placed beforeNEXT_PUBLIC_STATS_APP_URLfor alphabetical consistency.apps/deploy-web/env/.env.staging (1)
17-19: Consider alphabetical ordering (same as other env files).As noted in other environment file reviews, these keys should ideally be placed before
NEXT_PUBLIC_STATS_APP_URLfor alphabetical consistency.
🧹 Nitpick comments (2)
apps/deploy-web/env/.env.production (1)
17-19: Consider alphabetical ordering for consistency.The static analysis tool suggests these keys should be placed before
NEXT_PUBLIC_STATS_APP_URLto maintain alphabetical order. While not functionally critical, consistent ordering improves maintainability.Apply this diff to reorder the keys:
NEXT_PUBLIC_STATS_APP_URL=https://stats.akash.network +NEXT_PUBLIC_PROVIDER_PROXY_MAINNET_URL=https://provider-proxy-mainnet.akash.network +NEXT_PUBLIC_PROVIDER_PROXY_SANDBOX_URL=https://provider-proxy-sandbox.akash.network +NEXT_PUBLIC_PROVIDER_PROXY_TESTNET_URL=https://provider-proxy-testnet.akash.network -NEXT_PUBLIC_PROVIDER_PROXY_MAINNET_URL=https://provider-proxy-mainnet.akash.network -NEXT_PUBLIC_PROVIDER_PROXY_SANDBOX_URL=https://provider-proxy-sandbox.akash.network -NEXT_PUBLIC_PROVIDER_PROXY_TESTNET_URL=https://provider-proxy-testnet.akash.networkNote: This same pattern applies to
.env,.env.sample, and.env.stagingfiles.apps/deploy-web/env/.env (1)
28-34: Consider alphabetical ordering for the PROVIDER_PROXY variables.The static analysis tool flags that
PROVIDER_PROXY_SANDBOX_URL(line 34) should come beforePROVIDER_PROXY_TESTNET_URL(line 33) to maintain alphabetical order. While this doesn't affect functionality, consistent ordering improves maintainability.Apply this diff:
PROVIDER_PROXY_MAINNET_URL=$NEXT_PUBLIC_PROVIDER_PROXY_MAINNET_URL +PROVIDER_PROXY_SANDBOX_URL=$NEXT_PUBLIC_PROVIDER_PROXY_SANDBOX_URL PROVIDER_PROXY_TESTNET_URL=$NEXT_PUBLIC_PROVIDER_PROXY_TESTNET_URL -PROVIDER_PROXY_SANDBOX_URL=$NEXT_PUBLIC_PROVIDER_PROXY_SANDBOX_URL
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
apps/deploy-web/env/.env(1 hunks)apps/deploy-web/env/.env.production(1 hunks)apps/deploy-web/env/.env.sample(1 hunks)apps/deploy-web/env/.env.staging(1 hunks)apps/deploy-web/src/config/browser-env.config.ts(1 hunks)apps/deploy-web/src/config/env-config.schema.ts(2 hunks)apps/deploy-web/src/config/ws.config.ts(0 hunks)apps/deploy-web/src/services/api-url/api-url.service.ts(2 hunks)apps/deploy-web/src/services/app-di-container/app-di-container.ts(1 hunks)apps/deploy-web/src/services/app-di-container/browser-di-container.ts(0 hunks)apps/deploy-web/src/services/app-di-container/server-di-container.service.ts(0 hunks)apps/provider-proxy/env/.env.sample(1 hunks)apps/provider-proxy/package.json(1 hunks)apps/provider-proxy/src/config/env.config.ts(1 hunks)apps/provider-proxy/src/container.ts(2 hunks)apps/provider-proxy/src/routes/proxyProviderRequest.ts(1 hunks)apps/provider-proxy/src/server.ts(1 hunks)apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.ts(2 hunks)apps/provider-proxy/src/services/ProviderProxy.ts(2 hunks)apps/provider-proxy/src/services/ProviderService/ProviderService.ts(1 hunks)apps/provider-proxy/src/services/WebsocketServer.ts(1 hunks)apps/provider-proxy/src/utils/schema.ts(2 hunks)
💤 Files with no reviewable changes (3)
- apps/deploy-web/src/config/ws.config.ts
- apps/deploy-web/src/services/app-di-container/browser-di-container.ts
- apps/deploy-web/src/services/app-di-container/server-di-container.service.ts
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-12T13:52:38.708Z
Learnt from: stalniy
Repo: akash-network/console PR: 1800
File: apps/deploy-web/next.config.js:163-165
Timestamp: 2025-08-12T13:52:38.708Z
Learning: In the Akash Console project, akashnetwork/env-loader is used at the top of next.config.js files to automatically load environment variables from env/.env files into process.env. SENTRY_ORG and SENTRY_PROJECT are stored as public configuration values in apps/deploy-web/env/.env and are loaded this way, while only SENTRY_AUTH_TOKEN is handled as a GitHub secret in workflows.
Applied to files:
apps/provider-proxy/package.jsonapps/provider-proxy/src/server.tsapps/deploy-web/env/.env.sampleapps/deploy-web/env/.envapps/deploy-web/env/.env.stagingapps/deploy-web/env/.env.productionapps/provider-proxy/env/.env.sampleapps/deploy-web/src/config/browser-env.config.tsapps/deploy-web/src/config/env-config.schema.ts
📚 Learning: 2025-09-04T04:27:50.638Z
Learnt from: stalniy
Repo: akash-network/console PR: 1868
File: apps/api/src/billing/services/managed-signer/managed-signer.service.ts:1-1
Timestamp: 2025-09-04T04:27:50.638Z
Learning: In the akash-network/console project, importing MsgCreateLease from "akashnetwork/akash-api/v1beta3" instead of v1beta4 is considered non-critical by the maintainers, likely due to backward compatibility or project-specific requirements.
Applied to files:
apps/provider-proxy/package.json
📚 Learning: 2025-08-12T13:52:38.708Z
Learnt from: stalniy
Repo: akash-network/console PR: 1800
File: apps/deploy-web/next.config.js:163-165
Timestamp: 2025-08-12T13:52:38.708Z
Learning: In the Akash Console project, SENTRY_ORG and SENTRY_PROJECT are stored as public configuration values in apps/deploy-web/env/.env file and loaded by akashnetwork/env-loader, not as GitHub secrets. Only SENTRY_AUTH_TOKEN should be handled as a secret.
Applied to files:
apps/deploy-web/env/.env.stagingapps/deploy-web/env/.env.production
📚 Learning: 2025-11-12T09:03:40.132Z
Learnt from: stalniy
Repo: akash-network/console PR: 0
File: :0-0
Timestamp: 2025-11-12T09:03:40.132Z
Learning: For backend services (like the Indexer), prefer using createOtelLogger from "akashnetwork/logging/otel" to include OpenTelemetry trace context in logs.
Applied to files:
apps/provider-proxy/src/container.tsapps/provider-proxy/src/services/WebsocketServer.ts
📚 Learning: 2025-09-25T20:34:55.117Z
Learnt from: jigar-arc10
Repo: akash-network/console PR: 1970
File: apps/provider-console/src/config/network.config.ts:17-28
Timestamp: 2025-09-25T20:34:55.117Z
Learning: In cosmos-kit integration, nodesUrl and versionUrl should point to Cosmos API endpoints (not console backend API endpoints) as cosmos-kit expects these to be Cosmos REST endpoints for proper functionality.
Applied to files:
apps/provider-proxy/env/.env.sample
🧬 Code graph analysis (4)
apps/deploy-web/src/services/app-di-container/app-di-container.ts (1)
apps/deploy-web/src/services/provider-proxy/provider-proxy.service.ts (1)
ProviderProxyService(18-319)
apps/provider-proxy/src/container.ts (1)
apps/provider-proxy/src/config/env.config.ts (1)
envConfig(7-7)
apps/deploy-web/src/services/api-url/api-url.service.ts (2)
apps/deploy-web/src/config/env-config.schema.ts (2)
ServerEnvConfig(75-75)BrowserEnvConfig(74-74)apps/deploy-web/src/config/browser-env.config.ts (1)
BrowserEnvConfig(45-45)
apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.ts (1)
apps/provider-proxy/src/utils/validateCertificateAttrs.ts (1)
validateCertificateAttrs(11-49)
🪛 dotenv-linter (4.0.0)
apps/deploy-web/env/.env.sample
[warning] 15-15: [UnorderedKey] The NEXT_PUBLIC_PROVIDER_PROXY_MAINNET_URL key should go before the NEXT_PUBLIC_STATS_APP_URL key
(UnorderedKey)
[warning] 16-16: [UnorderedKey] The NEXT_PUBLIC_PROVIDER_PROXY_SANDBOX_URL key should go before the NEXT_PUBLIC_STATS_APP_URL key
(UnorderedKey)
[warning] 17-17: [UnorderedKey] The NEXT_PUBLIC_PROVIDER_PROXY_TESTNET_URL key should go before the NEXT_PUBLIC_STATS_APP_URL key
(UnorderedKey)
apps/deploy-web/env/.env
[warning] 34-34: [UnorderedKey] The PROVIDER_PROXY_SANDBOX_URL key should go before the PROVIDER_PROXY_TESTNET_URL key
(UnorderedKey)
apps/deploy-web/env/.env.staging
[warning] 17-17: [UnorderedKey] The NEXT_PUBLIC_PROVIDER_PROXY_MAINNET_URL key should go before the NEXT_PUBLIC_STATS_APP_URL key
(UnorderedKey)
[warning] 18-18: [UnorderedKey] The NEXT_PUBLIC_PROVIDER_PROXY_SANDBOX_URL key should go before the NEXT_PUBLIC_STATS_APP_URL key
(UnorderedKey)
[warning] 19-19: [UnorderedKey] The NEXT_PUBLIC_PROVIDER_PROXY_TESTNET_URL key should go before the NEXT_PUBLIC_STATS_APP_URL key
(UnorderedKey)
apps/deploy-web/env/.env.production
[warning] 17-17: [UnorderedKey] The NEXT_PUBLIC_PROVIDER_PROXY_MAINNET_URL key should go before the NEXT_PUBLIC_STATS_APP_URL key
(UnorderedKey)
[warning] 18-18: [UnorderedKey] The NEXT_PUBLIC_PROVIDER_PROXY_SANDBOX_URL key should go before the NEXT_PUBLIC_STATS_APP_URL key
(UnorderedKey)
[warning] 19-19: [UnorderedKey] The NEXT_PUBLIC_PROVIDER_PROXY_TESTNET_URL key should go before the NEXT_PUBLIC_STATS_APP_URL key
(UnorderedKey)
🪛 GitHub Actions: Provider Proxy CI
apps/provider-proxy/package.json
[error] 1-1: npm run test:cov (workspace @akashnetwork/provider-proxy) failed. Command failed with exit code 1 during jest test run (unit/functional suites).
apps/provider-proxy/src/server.ts
[error] 1-1: npm run test:cov (workspace @akashnetwork/provider-proxy) failed. Command failed with exit code 1 during jest test run (unit/functional suites).
apps/provider-proxy/src/services/ProviderService/ProviderService.ts
[error] 19-19: Cannot convert 0xprovider to a BigInt. Likely an invalid serial number format when constructing the BigInt from a hex string.
[error] 1-1: npm run test:cov (workspace @akashnetwork/provider-proxy) failed. Command failed with exit code 1 during jest test run (unit/functional suites).
apps/provider-proxy/src/config/env.config.ts
[error] 7-7: ZodError: REST_API_NODE_URL is required. Environment configuration invalid.
[error] 1-1: npm run test:cov (workspace @akashnetwork/provider-proxy) failed. Command failed with exit code 1 during jest test run (unit/functional suites).
apps/provider-proxy/src/utils/schema.ts
[error] 1-1: npm run test:cov (workspace @akashnetwork/provider-proxy) failed. Command failed with exit code 1 during jest test run (unit/functional suites).
apps/provider-proxy/src/container.ts
[error] 1-1: npm run test:cov (workspace @akashnetwork/provider-proxy) failed. Command failed with exit code 1 during jest test run (unit/functional suites).
apps/provider-proxy/env/.env.sample
[error] 1-1: npm run test:cov (workspace @akashnetwork/provider-proxy) failed. Command failed with exit code 1 during jest test run (unit/functional suites).
apps/provider-proxy/src/services/WebsocketServer.ts
[error] 1-1: npm run test:cov (workspace @akashnetwork/provider-proxy) failed. Command failed with exit code 1 during jest test run (unit/functional suites).
apps/provider-proxy/src/routes/proxyProviderRequest.ts
[error] 1-1: npm run test:cov (workspace @akashnetwork/provider-proxy) failed. Command failed with exit code 1 during jest test run (unit/functional suites).
apps/provider-proxy/src/services/ProviderProxy.ts
[error] 1-1: npm run test:cov (workspace @akashnetwork/provider-proxy) failed. Command failed with exit code 1 during jest test run (unit/functional suites).
apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.ts
[error] 1-1: npm run test:cov (workspace @akashnetwork/provider-proxy) failed. Command failed with exit code 1 during jest test run (unit/functional suites).
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: validate (apps/deploy-web) / validate-unsafe
- GitHub Check: validate (apps/api) / validate-unsafe
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
🔇 Additional comments (18)
apps/provider-proxy/package.json (2)
1-66: Investigate pipeline test failures.The pipeline indicates test failures in the provider-proxy workspace. The addition of
@akashnetwork/env-loaderand changes to environment configuration may have affected existing tests.Please ensure:
- Tests are updated to account for the new
REST_API_NODE_URLenvironment variable requirement- Mock configurations in test setup include the new environment variables
- The env-loader import in
server.tsdoesn't break test isolationYou may need to update test fixtures or add environment variable mocks for the test suite.
26-26: No issues found. The dependency specification is correct.The verification confirms that
@akashnetwork/env-loaderis a workspace package located at./packages/env-loader/package.json, matching the root workspace pattern./packages/*. Using"*"as the version specifier for internal workspace dependencies is standard practice and prevents version mismatches across the monorepo.apps/provider-proxy/env/.env.sample (1)
1-3: LGTM! Clear documentation for the new environment variable.The sample file clearly documents the purpose of
REST_API_NODE_URLand provides a working example. This aligns well with the PR objective to accept REST API node URLs in provider-proxy.apps/provider-proxy/src/server.ts (1)
1-2: LGTM! Correct placement of env-loader import.The import is correctly placed at the very top to ensure environment variables are loaded before any other modules execute. This follows the established pattern in the Akash Console project.
However, ensure test files mock or skip this import to prevent side effects during testing.
Based on learnings
apps/deploy-web/src/config/browser-env.config.ts (1)
7-9: LGTM! Consistent with environment configuration changes.The replacement of a single
NEXT_PUBLIC_PROVIDER_PROXY_URLwith three network-specific URLs properly reflects the new per-network proxy configuration. The validation schema correctly handles all three new variables.apps/provider-proxy/src/routes/proxyProviderRequest.ts (1)
57-85: LGTM! Clean removal of network parameter.The network field has been properly removed from the request payload, logging, and downstream service calls. The logic remains sound and the changes align with the broader refactor to simplify provider proxy URL handling.
apps/provider-proxy/src/services/WebsocketServer.ts (1)
288-337: LGTM! Certificate validation simplified.The validation call correctly updated to pass only
certificateandproviderAddress, eliminating the network parameter. This aligns with the broader refactor and simplifies the validation surface.apps/deploy-web/src/services/app-di-container/app-di-container.ts (1)
77-85: LGTM! Provider proxy URL now dynamically resolved.The refactor correctly derives the provider proxy base URL using
apiUrlService.getProviderProxyUrlFor(config.MANAGED_WALLET_NETWORK_ID)and constructs the WebSocket URL appropriately. This aligns with the broader shift to network-specific provider proxy URLs.apps/provider-proxy/src/services/ProviderProxy.ts (2)
60-76: Certificate validation simplified correctly.The validation flow now relies solely on
providerAddresswithout network context, which aligns with the broader refactor. The implementation is clean and the error handling is preserved.
118-148: I need to gather more context about whether provider addresses are globally unique or network-specific.Now I need to check the
ProxyConnectOptionsinterface and understand what parameters are available and how they're being used:Based on my research and code analysis, I can now provide a definitive assessment:
No critical issues found with agent cache key design.
The review comment raises a concern about agent cache key uniqueness across networks, but verification shows this is mitigated by several factors:
For mTLS auth: Different networks using different certificates result in different cache keys since the key includes a hash of the cert and keyPem.
For JWT auth: While cache keys would theoretically be identical across networks, the underlying https.Agent is scoped by hostname, and different Akash networks use different provider URLs/hostnames.
Deliberate deprecation: The schema explicitly marks
networkandchainNetworkas "Deprecated blockchain network parameter - no longer used," indicating this was an intentional design decision rather than an oversight.In practice: Each network (mainnet, testnet, sandbox) has separate on-chain provider registrations with typically different hosting infrastructure, so reusing an agent wouldn't occur in real-world scenarios.
apps/provider-proxy/src/container.ts (1)
15-24: ProviderService URL source updated correctly.The switch from
netConfig.getBaseAPIUrl(network)toenvConfig.REST_API_NODE_URLsimplifies the URL resolution. TheTEST_CHAIN_NETWORK_URLoverride for functional tests is well-documented and preserved appropriately.Note: This depends on fixing the critical issue in
apps/provider-proxy/src/config/env.config.tsline 7, as the current module-level parsing will prevent container initialization ifREST_API_NODE_URLis missing.apps/provider-proxy/src/utils/schema.ts (1)
23-25: LGTM! Backward-compatible deprecation pattern.The schema correctly handles deprecated fields:
- Accepts
networkandchainNetworkas optional strings with deprecation notes to avoid breaking existing clients- Omits them from the exported TypeScript type to discourage new usage
- Silently discards them during preprocessing
This ensures existing clients continue to work while guiding new implementations away from the deprecated fields.
Also applies to: 35-45
apps/deploy-web/src/services/api-url/api-url.service.ts (2)
8-26: LGTM! Constructor signature properly extended.The constructor now accepts provider proxy URL fields for both server and browser configurations, mirroring the existing pattern for base API URLs. The type unions correctly constrain the config to include the necessary URL fields.
50-70: LGTM! Provider proxy URL resolution follows established pattern.The
getProviderProxyUrlFormethod correctly mirrors the logic ingetBaseApiUrlFor:
- Checks for server-side config first
- Falls back to browser config
- Handles all three networks (TESTNET, SANDBOX, default MAINNET)
The implementation is consistent and maintainable.
apps/provider-proxy/src/services/ProviderService/ProviderService.ts (1)
9-13: LGTM: Simplified constructor aligns with PR objectives.The constructor change from a network-to-URL resolver function to a direct
chainBaseUrlstring simplifies the service architecture and aligns with the PR's goal of using environment-based configuration instead of network-based URL resolution.apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.ts (3)
21-44: LGTM: Validation logic correctly updated to remove network dependency.The
validatemethod signature and all instrumentation callbacks have been consistently updated to remove the network parameter. The validation logic remains intact while aligning with the PR's objective to eliminate network-based routing.
70-92: LGTM: Instrumentation interface consistently refactored.The instrumentation interface and factory implementation have been correctly updated to remove the network parameter from all method signatures. Log messages remain informative by referencing
providerAddressinstead of network.
46-62: The cache key change is safe; the review comment is based on incorrect architectural assumptions.The
provider-proxyservice is designed as a single-network proxy per deployment instance. Each application startup creates exactly oneCertificateValidatorinstance with a fixedchainBaseUrlfrom environment configuration (REST_API_NODE_URL). There is no multi-network support, dynamic network switching, or instance sharing across networks—separate deployments serve separate networks.Removing the network segment from the cache key is the correct design for this architecture. The concern about cache collisions across networks cannot occur because:
- One
createContainer()call per application startup- One
ProviderServiceinstance with one fixedchainBaseUrl- One
CertificateValidatorinstance per deployment- No logic to share validator instances across multiple networks
The original review assumes a multi-network aggregator pattern that does not exist in this codebase.
Likely an incorrect or invalid review comment.
| NEXT_PUBLIC_PROVIDER_PROXY_MAINNET_URL: z.string(), | ||
| NEXT_PUBLIC_PROVIDER_PROXY_TESTNET_URL: z.string(), | ||
| NEXT_PUBLIC_PROVIDER_PROXY_SANDBOX_URL: z.string(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add URL validation for consistency and correctness.
These provider proxy URL fields use z.string() without .url() validation, while other URL fields in the schema (e.g., line 15, lines 24-26, lines 58-60) use z.string().url(). This inconsistency could allow invalid URLs to pass validation.
Apply this diff to add URL validation:
- NEXT_PUBLIC_PROVIDER_PROXY_MAINNET_URL: z.string(),
- NEXT_PUBLIC_PROVIDER_PROXY_TESTNET_URL: z.string(),
- NEXT_PUBLIC_PROVIDER_PROXY_SANDBOX_URL: z.string(),
+ NEXT_PUBLIC_PROVIDER_PROXY_MAINNET_URL: z.string().url(),
+ NEXT_PUBLIC_PROVIDER_PROXY_TESTNET_URL: z.string().url(),
+ NEXT_PUBLIC_PROVIDER_PROXY_SANDBOX_URL: z.string().url(),📝 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.
| NEXT_PUBLIC_PROVIDER_PROXY_MAINNET_URL: z.string(), | |
| NEXT_PUBLIC_PROVIDER_PROXY_TESTNET_URL: z.string(), | |
| NEXT_PUBLIC_PROVIDER_PROXY_SANDBOX_URL: z.string(), | |
| NEXT_PUBLIC_PROVIDER_PROXY_MAINNET_URL: z.string().url(), | |
| NEXT_PUBLIC_PROVIDER_PROXY_TESTNET_URL: z.string().url(), | |
| NEXT_PUBLIC_PROVIDER_PROXY_SANDBOX_URL: z.string().url(), |
🤖 Prompt for AI Agents
In apps/deploy-web/src/config/env-config.schema.ts around lines 16 to 18, the
three NEXT_PUBLIC_PROVIDER_PROXY_* fields are defined as z.string() but should
validate as URLs; change each to z.string().url() to match other URL fields in
the schema and ensure invalid proxy URLs are rejected during validation.
| PROVIDER_PROXY_MAINNET_URL: z.string(), | ||
| PROVIDER_PROXY_TESTNET_URL: z.string(), | ||
| PROVIDER_PROXY_SANDBOX_URL: z.string(), |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Add URL validation for consistency and correctness.
These server-side provider proxy URL fields use z.string() without .url() validation, while other URL fields in the same schema (e.g., lines 58-60) use z.string().url(). This inconsistency could allow invalid URLs to pass validation.
Apply this diff to add URL validation:
- PROVIDER_PROXY_MAINNET_URL: z.string(),
- PROVIDER_PROXY_TESTNET_URL: z.string(),
- PROVIDER_PROXY_SANDBOX_URL: z.string(),
+ PROVIDER_PROXY_MAINNET_URL: z.string().url(),
+ PROVIDER_PROXY_TESTNET_URL: z.string().url(),
+ PROVIDER_PROXY_SANDBOX_URL: z.string().url(),📝 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.
| PROVIDER_PROXY_MAINNET_URL: z.string(), | |
| PROVIDER_PROXY_TESTNET_URL: z.string(), | |
| PROVIDER_PROXY_SANDBOX_URL: z.string(), | |
| PROVIDER_PROXY_MAINNET_URL: z.string().url(), | |
| PROVIDER_PROXY_TESTNET_URL: z.string().url(), | |
| PROVIDER_PROXY_SANDBOX_URL: z.string().url(), |
🤖 Prompt for AI Agents
In apps/deploy-web/src/config/env-config.schema.ts around lines 65 to 67, the
PROVIDER_PROXY_MAINNET_URL, PROVIDER_PROXY_TESTNET_URL, and
PROVIDER_PROXY_SANDBOX_URL fields currently use z.string() without URL
validation; update each to use z.string().url() to match the other URL fields in
the schema and ensure invalid URLs are rejected by the validator.
jzsfkzm
force-pushed
the
features/2189-accept-rest-api-node-url
branch
from
eac2240 to
44dd3ca
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/provider-proxy/src/services/ProviderService/ProviderService.spec.ts (1)
106-107: Fix type error: Pass string directly instead of function.The constructor now expects a concrete
chainBaseUrlstring, but a function() => "/"is being passed, causing a TypeScript compilation error.Apply this diff to fix the type error:
function setup(input?: { httpFetch?: typeof global.fetch }) { return new ProviderService( - () => "/", + "/", input?.httpFetch || jest.fn().mockResolvedValue( new Response("", {
🧹 Nitpick comments (1)
apps/deploy-web/env/.env (1)
32-34: Consider reordering for consistency.The derived server-side provider proxy variables work correctly, but alphabetical ordering (mainnet → sandbox → testnet) would align with the public variables above and satisfy the static analysis hint.
Apply this diff to reorder:
PROVIDER_PROXY_MAINNET_URL=$NEXT_PUBLIC_PROVIDER_PROXY_MAINNET_URL +PROVIDER_PROXY_SANDBOX_URL=$NEXT_PUBLIC_PROVIDER_PROXY_SANDBOX_URL PROVIDER_PROXY_TESTNET_URL=$NEXT_PUBLIC_PROVIDER_PROXY_TESTNET_URL -PROVIDER_PROXY_SANDBOX_URL=$NEXT_PUBLIC_PROVIDER_PROXY_SANDBOX_URL
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (24)
apps/deploy-web/env/.env(1 hunks)apps/deploy-web/env/.env.production(1 hunks)apps/deploy-web/env/.env.sample(1 hunks)apps/deploy-web/env/.env.staging(1 hunks)apps/deploy-web/src/config/browser-env.config.ts(1 hunks)apps/deploy-web/src/config/env-config.schema.ts(2 hunks)apps/deploy-web/src/config/ws.config.ts(0 hunks)apps/deploy-web/src/services/api-url/api-url.service.ts(2 hunks)apps/deploy-web/src/services/app-di-container/app-di-container.ts(1 hunks)apps/deploy-web/src/services/app-di-container/browser-di-container.ts(0 hunks)apps/deploy-web/src/services/app-di-container/server-di-container.service.ts(0 hunks)apps/provider-proxy/env/.env.sample(1 hunks)apps/provider-proxy/package.json(1 hunks)apps/provider-proxy/src/config/env.config.ts(1 hunks)apps/provider-proxy/src/container.ts(2 hunks)apps/provider-proxy/src/routes/proxyProviderRequest.ts(1 hunks)apps/provider-proxy/src/server.ts(1 hunks)apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.spec.ts(10 hunks)apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.ts(2 hunks)apps/provider-proxy/src/services/ProviderProxy.ts(2 hunks)apps/provider-proxy/src/services/ProviderService/ProviderService.spec.ts(5 hunks)apps/provider-proxy/src/services/ProviderService/ProviderService.ts(1 hunks)apps/provider-proxy/src/services/WebsocketServer.ts(1 hunks)apps/provider-proxy/src/utils/schema.ts(2 hunks)
💤 Files with no reviewable changes (3)
- apps/deploy-web/src/services/app-di-container/server-di-container.service.ts
- apps/deploy-web/src/services/app-di-container/browser-di-container.ts
- apps/deploy-web/src/config/ws.config.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/provider-proxy/src/server.ts
- apps/deploy-web/src/services/app-di-container/app-di-container.ts
- apps/provider-proxy/env/.env.sample
- apps/provider-proxy/src/container.ts
- apps/provider-proxy/package.json
- apps/provider-proxy/src/services/ProviderService/ProviderService.ts
- apps/provider-proxy/src/config/env.config.ts
- apps/deploy-web/src/config/env-config.schema.ts
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-06-08T03:07:13.871Z
Learnt from: stalniy
Repo: akash-network/console PR: 1436
File: apps/api/src/provider/repositories/provider/provider.repository.ts:79-90
Timestamp: 2025-06-08T03:07:13.871Z
Learning: The getProvidersHostUriByAttributes method in apps/api/src/provider/repositories/provider/provider.repository.ts already has comprehensive test coverage in provider.repository.spec.ts, including tests for complex HAVING clause logic with COUNT(*) FILTER (WHERE ...) syntax, signature conditions (anyOf/allOf), and glob pattern matching.
Applied to files:
apps/provider-proxy/src/services/ProviderService/ProviderService.spec.ts
📚 Learning: 2025-08-12T13:52:38.708Z
Learnt from: stalniy
Repo: akash-network/console PR: 1800
File: apps/deploy-web/next.config.js:163-165
Timestamp: 2025-08-12T13:52:38.708Z
Learning: In the Akash Console project, akashnetwork/env-loader is used at the top of next.config.js files to automatically load environment variables from env/.env files into process.env. SENTRY_ORG and SENTRY_PROJECT are stored as public configuration values in apps/deploy-web/env/.env and are loaded this way, while only SENTRY_AUTH_TOKEN is handled as a GitHub secret in workflows.
Applied to files:
apps/deploy-web/env/.envapps/deploy-web/src/config/browser-env.config.tsapps/deploy-web/env/.env.productionapps/deploy-web/env/.env.sampleapps/deploy-web/env/.env.staging
📚 Learning: 2025-11-12T09:03:40.132Z
Learnt from: stalniy
Repo: akash-network/console PR: 0
File: :0-0
Timestamp: 2025-11-12T09:03:40.132Z
Learning: For backend services (like the Indexer), prefer using createOtelLogger from "akashnetwork/logging/otel" to include OpenTelemetry trace context in logs.
Applied to files:
apps/provider-proxy/src/services/WebsocketServer.ts
📚 Learning: 2025-08-12T13:52:38.708Z
Learnt from: stalniy
Repo: akash-network/console PR: 1800
File: apps/deploy-web/next.config.js:163-165
Timestamp: 2025-08-12T13:52:38.708Z
Learning: In the Akash Console project, SENTRY_ORG and SENTRY_PROJECT are stored as public configuration values in apps/deploy-web/env/.env file and loaded by akashnetwork/env-loader, not as GitHub secrets. Only SENTRY_AUTH_TOKEN should be handled as a secret.
Applied to files:
apps/deploy-web/env/.env.productionapps/deploy-web/env/.env.staging
🧬 Code graph analysis (3)
apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.spec.ts (2)
apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.ts (1)
CertValidationResultError(66-69)apps/provider-proxy/src/services/ProviderService/ProviderService.ts (1)
getCertificate(15-31)
apps/deploy-web/src/services/api-url/api-url.service.ts (2)
apps/deploy-web/src/config/env-config.schema.ts (2)
ServerEnvConfig(75-75)BrowserEnvConfig(74-74)apps/deploy-web/src/config/browser-env.config.ts (1)
BrowserEnvConfig(45-45)
apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.ts (1)
apps/provider-proxy/src/utils/validateCertificateAttrs.ts (1)
validateCertificateAttrs(11-49)
🪛 dotenv-linter (4.0.0)
apps/deploy-web/env/.env
[warning] 34-34: [UnorderedKey] The PROVIDER_PROXY_SANDBOX_URL key should go before the PROVIDER_PROXY_TESTNET_URL key
(UnorderedKey)
apps/deploy-web/env/.env.production
[warning] 17-17: [UnorderedKey] The NEXT_PUBLIC_PROVIDER_PROXY_MAINNET_URL key should go before the NEXT_PUBLIC_STATS_APP_URL key
(UnorderedKey)
[warning] 18-18: [UnorderedKey] The NEXT_PUBLIC_PROVIDER_PROXY_SANDBOX_URL key should go before the NEXT_PUBLIC_STATS_APP_URL key
(UnorderedKey)
[warning] 19-19: [UnorderedKey] The NEXT_PUBLIC_PROVIDER_PROXY_TESTNET_URL key should go before the NEXT_PUBLIC_STATS_APP_URL key
(UnorderedKey)
apps/deploy-web/env/.env.sample
[warning] 15-15: [UnorderedKey] The NEXT_PUBLIC_PROVIDER_PROXY_MAINNET_URL key should go before the NEXT_PUBLIC_STATS_APP_URL key
(UnorderedKey)
[warning] 16-16: [UnorderedKey] The NEXT_PUBLIC_PROVIDER_PROXY_SANDBOX_URL key should go before the NEXT_PUBLIC_STATS_APP_URL key
(UnorderedKey)
[warning] 17-17: [UnorderedKey] The NEXT_PUBLIC_PROVIDER_PROXY_TESTNET_URL key should go before the NEXT_PUBLIC_STATS_APP_URL key
(UnorderedKey)
apps/deploy-web/env/.env.staging
[warning] 17-17: [UnorderedKey] The NEXT_PUBLIC_PROVIDER_PROXY_MAINNET_URL key should go before the NEXT_PUBLIC_STATS_APP_URL key
(UnorderedKey)
[warning] 18-18: [UnorderedKey] The NEXT_PUBLIC_PROVIDER_PROXY_SANDBOX_URL key should go before the NEXT_PUBLIC_STATS_APP_URL key
(UnorderedKey)
[warning] 19-19: [UnorderedKey] The NEXT_PUBLIC_PROVIDER_PROXY_TESTNET_URL key should go before the NEXT_PUBLIC_STATS_APP_URL key
(UnorderedKey)
🪛 GitHub Actions: Provider Proxy CI
apps/provider-proxy/src/services/ProviderService/ProviderService.spec.ts
[error] 107-107: TS2345: Argument of type '() => string' is not assignable to parameter of type 'string'.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
🔇 Additional comments (24)
apps/provider-proxy/src/services/ProviderService/ProviderService.spec.ts (1)
12-12: LGTM! Test calls correctly updated to match the new API signature.All
getCertificatecalls have been correctly updated to use the two-parameter signature(providerAddress, serialNumber), removing the network parameter as intended by the refactoring.Also applies to: 25-25, 42-42, 51-51, 60-60
apps/deploy-web/env/.env.production (1)
17-19: LGTM! Per-network provider proxy URLs configured correctly.The addition of separate provider proxy URLs for mainnet, sandbox, and testnet aligns with the PR objective and enables network-specific routing.
apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.ts (3)
21-44: LGTM! Validation logic correctly refactored to remove network dependency.The validation flow now operates using
providerAddressinstead ofnetwork, which aligns with the architectural shift to per-network provider proxy instances.
70-92: LGTM! Instrumentation interface properly updated.All instrumentation callbacks and logging have been consistently updated to remove the network parameter and use providerAddress instead.
46-62: No change required — provider-proxy is network-scoped at startup.CertificateValidator is created with a ProviderService initialized from process.env.TEST_CHAIN_NETWORK_URL || envConfig.REST_API_NODE_URL (apps/provider-proxy/src/container.ts — providerService ≈ line 15, CertificateValidator ≈ lines 25–29). Each process is therefore bound to a single network, so omitting the network from the cache key does not create cross-network collisions. Reintroduce the network discriminator only if you switch to a single process serving multiple networks.
apps/deploy-web/env/.env.staging (1)
17-19: LGTM! Staging environment properly configured.The per-network provider proxy URLs for staging are correctly configured and consistent with the production environment changes.
apps/deploy-web/env/.env.sample (1)
15-17: LGTM! Sample environment file updated correctly.The environment sample properly reflects the new per-network provider proxy URL configuration.
apps/provider-proxy/src/routes/proxyProviderRequest.ts (2)
56-86: LGTM! Request handling properly refactored.The proxy request handler has been correctly updated to remove network from the payload, logging, and downstream provider proxy connection calls.
99-163: LGTM! Error handling updated consistently.All error logging and handling paths have been properly updated to remove network references.
apps/deploy-web/src/config/browser-env.config.ts (1)
7-9: LGTM! Browser environment configuration updated correctly.The browser config now properly exposes the three per-network provider proxy URLs, replacing the single URL configuration.
apps/provider-proxy/src/services/WebsocketServer.ts (4)
127-145: LGTM! WebSocket message attributes updated correctly.The message handling has been properly updated to remove
chainNetworkfrom attributes and metrics.
210-254: LGTM! Provider socket creation updated correctly.The provider socket creation flow has been properly refactored to remove network from the options and pass only
providerAddressto validation.
286-337: LGTM! Certificate validation call updated correctly.The validation call on line 289 now correctly passes
(certificate, options.providerAddress)instead of including network, matching the updatedCertificateValidator.validatesignature.
443-447: LGTM! Interface updated correctly.The
CreateProviderSocketOptionsinterface has been properly updated to remove the network field.apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.spec.ts (4)
12-27: LGTM! Test updated correctly for unknown certificate scenario.The test properly validates the new signature
validate(cert, "provider")and verifies the expectedgetCertificate("provider", cert.serialNumber)call.
29-53: LGTM! Test updated correctly for fingerprint mismatch scenario.The test correctly uses the updated validation and service call signatures.
55-86: LGTM! Caching test updated correctly.The test properly verifies that caching now works based on
providerAddressandserialNumber(without network).
88-178: LGTM! All remaining tests updated correctly.All test scenarios (future validation, expiration, serial number checks, bech32 validation, and concurrent fetching) have been properly updated to use the new API signatures.
apps/provider-proxy/src/utils/schema.ts (2)
23-25: Clean deprecation strategy.The approach to deprecate network and chainNetwork fields is well-executed: fields remain in the schema for backward compatibility but are marked optional with clear deprecation messages, then stripped during preprocessing at line 41 and omitted from the exported type at line 35.
38-46: Preprocessing correctly removes deprecated fields.The preprocessing logic properly destructures and discards the deprecated
networkandchainNetworkfields while preserving the backward-compatible migration of legacycertPem/keyPemto the newauthstructure.apps/provider-proxy/src/services/ProviderProxy.ts (2)
60-62: Validation correctly uses providerAddress instead of network.The certificate validation has been appropriately refactored to use
providerAddressinstead ofnetwork, aligning with the PR's goal to remove network context. Theoptions.providerAddresscheck at line 60 is technically redundant sinceproviderAddressis a required field in theProxyConnectOptionsinterface (line 169), but it provides defensive programming.
118-127: Agent caching now correctly keyed by providerAddress.The cache key derivation has been updated to use
providerAddressinstead of network, which is the appropriate identifier for per-provider agent caching in the network-agnostic design.apps/deploy-web/src/services/api-url/api-url.service.ts (2)
6-26: Constructor typing correctly extended for per-network provider proxy URLs.The type definition properly includes all six new configuration keys (three server-side
PROVIDER_PROXY_*and three browser-sideNEXT_PUBLIC_PROVIDER_PROXY_*variants), maintaining the existing pattern used for base API URLs.
50-70: Provider proxy URL resolution follows established patterns.The
getProviderProxyUrlFormethod correctly mirrors thegetBaseApiUrlForimplementation with proper prioritization (server-side config first, then browser-side fallback) and appropriate network switching logic.
jzsfkzm
force-pushed
the
features/2189-accept-rest-api-node-url
branch
from
44dd3ca to
5d6fc74
Compare
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| "apps/provider-proxy": { | ||
| "name": "@akashnetwork/provider-proxy", | ||
| "version": "2.2.0", | ||
| "license": "Apache-2.0", |
There was a problem hiding this comment.
🔄 Carefully review the package-lock.json diff
Resolve the comment if everything is ok
+ apps/provider-proxy/node_modules/dotenv 17.2.3
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
apps/deploy-web/env/.env (1)
32-34: Reorder for consistency.The
PROVIDER_PROXY_SANDBOX_URLshould be placed beforePROVIDER_PROXY_TESTNET_URLto maintain alphabetical ordering and consistency with theNEXT_PUBLIC_PROVIDER_PROXY_*variables above.Apply this diff:
PROVIDER_PROXY_MAINNET_URL=$NEXT_PUBLIC_PROVIDER_PROXY_MAINNET_URL -PROVIDER_PROXY_TESTNET_URL=$NEXT_PUBLIC_PROVIDER_PROXY_TESTNET_URL PROVIDER_PROXY_SANDBOX_URL=$NEXT_PUBLIC_PROVIDER_PROXY_SANDBOX_URL +PROVIDER_PROXY_TESTNET_URL=$NEXT_PUBLIC_PROVIDER_PROXY_TESTNET_URL
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (30)
apps/deploy-web/env/.env(1 hunks)apps/deploy-web/env/.env.production(1 hunks)apps/deploy-web/env/.env.sample(1 hunks)apps/deploy-web/env/.env.staging(1 hunks)apps/deploy-web/src/config/browser-env.config.ts(1 hunks)apps/deploy-web/src/config/env-config.schema.ts(2 hunks)apps/deploy-web/src/config/ws.config.ts(0 hunks)apps/deploy-web/src/services/api-url/api-url.service.ts(2 hunks)apps/deploy-web/src/services/app-di-container/app-di-container.ts(1 hunks)apps/deploy-web/src/services/app-di-container/browser-di-container.ts(0 hunks)apps/deploy-web/src/services/app-di-container/server-di-container.service.ts(0 hunks)apps/provider-proxy/env/.env(1 hunks)apps/provider-proxy/env/.env.functional.test(1 hunks)apps/provider-proxy/env/.env.sample(1 hunks)apps/provider-proxy/jest.config.js(1 hunks)apps/provider-proxy/package.json(2 hunks)apps/provider-proxy/src/config/env.config.ts(1 hunks)apps/provider-proxy/src/container.ts(2 hunks)apps/provider-proxy/src/routes/proxyProviderRequest.ts(1 hunks)apps/provider-proxy/src/server.ts(1 hunks)apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.spec.ts(10 hunks)apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.ts(2 hunks)apps/provider-proxy/src/services/ProviderProxy.ts(2 hunks)apps/provider-proxy/src/services/ProviderService/ProviderService.spec.ts(6 hunks)apps/provider-proxy/src/services/ProviderService/ProviderService.ts(1 hunks)apps/provider-proxy/src/services/WebsocketServer.ts(1 hunks)apps/provider-proxy/src/utils/schema.ts(2 hunks)apps/provider-proxy/test/functional/provider-proxy-http.spec.ts(18 hunks)apps/provider-proxy/test/functional/provider-proxy-ws.spec.ts(8 hunks)apps/provider-proxy/test/setup-functional-env.ts(1 hunks)
💤 Files with no reviewable changes (3)
- apps/deploy-web/src/services/app-di-container/server-di-container.service.ts
- apps/deploy-web/src/services/app-di-container/browser-di-container.ts
- apps/deploy-web/src/config/ws.config.ts
✅ Files skipped from review due to trivial changes (1)
- apps/provider-proxy/env/.env.functional.test
🚧 Files skipped from review as they are similar to previous changes (4)
- apps/deploy-web/src/config/browser-env.config.ts
- apps/provider-proxy/src/server.ts
- apps/provider-proxy/env/.env.sample
- apps/deploy-web/src/config/env-config.schema.ts
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-12T13:52:38.708Z
Learnt from: stalniy
Repo: akash-network/console PR: 1800
File: apps/deploy-web/next.config.js:163-165
Timestamp: 2025-08-12T13:52:38.708Z
Learning: In the Akash Console project, akashnetwork/env-loader is used at the top of next.config.js files to automatically load environment variables from env/.env files into process.env. SENTRY_ORG and SENTRY_PROJECT are stored as public configuration values in apps/deploy-web/env/.env and are loaded this way, while only SENTRY_AUTH_TOKEN is handled as a GitHub secret in workflows.
Applied to files:
apps/provider-proxy/env/.envapps/provider-proxy/package.jsonapps/deploy-web/env/.envapps/deploy-web/env/.env.productionapps/deploy-web/env/.env.sampleapps/deploy-web/env/.env.staging
📚 Learning: 2025-11-12T09:03:40.132Z
Learnt from: stalniy
Repo: akash-network/console PR: 0
File: :0-0
Timestamp: 2025-11-12T09:03:40.132Z
Learning: For backend services (like the Indexer), prefer using createOtelLogger from "akashnetwork/logging/otel" to include OpenTelemetry trace context in logs.
Applied to files:
apps/provider-proxy/src/container.tsapps/provider-proxy/src/services/WebsocketServer.ts
📚 Learning: 2025-06-08T03:07:13.871Z
Learnt from: stalniy
Repo: akash-network/console PR: 1436
File: apps/api/src/provider/repositories/provider/provider.repository.ts:79-90
Timestamp: 2025-06-08T03:07:13.871Z
Learning: The getProvidersHostUriByAttributes method in apps/api/src/provider/repositories/provider/provider.repository.ts already has comprehensive test coverage in provider.repository.spec.ts, including tests for complex HAVING clause logic with COUNT(*) FILTER (WHERE ...) syntax, signature conditions (anyOf/allOf), and glob pattern matching.
Applied to files:
apps/provider-proxy/src/services/ProviderService/ProviderService.spec.ts
📚 Learning: 2025-09-04T04:27:50.638Z
Learnt from: stalniy
Repo: akash-network/console PR: 1868
File: apps/api/src/billing/services/managed-signer/managed-signer.service.ts:1-1
Timestamp: 2025-09-04T04:27:50.638Z
Learning: In the akash-network/console project, importing MsgCreateLease from "akashnetwork/akash-api/v1beta3" instead of v1beta4 is considered non-critical by the maintainers, likely due to backward compatibility or project-specific requirements.
Applied to files:
apps/provider-proxy/package.json
📚 Learning: 2025-08-12T13:52:38.708Z
Learnt from: stalniy
Repo: akash-network/console PR: 1800
File: apps/deploy-web/next.config.js:163-165
Timestamp: 2025-08-12T13:52:38.708Z
Learning: In the Akash Console project, SENTRY_ORG and SENTRY_PROJECT are stored as public configuration values in apps/deploy-web/env/.env file and loaded by akashnetwork/env-loader, not as GitHub secrets. Only SENTRY_AUTH_TOKEN should be handled as a secret.
Applied to files:
apps/deploy-web/env/.env.productionapps/deploy-web/env/.env.staging
🧬 Code graph analysis (7)
apps/provider-proxy/test/functional/provider-proxy-http.spec.ts (1)
apps/provider-proxy/test/setup/proxyServer.ts (1)
startServer(6-9)
apps/provider-proxy/src/container.ts (1)
apps/provider-proxy/src/config/env.config.ts (1)
envConfig(7-7)
apps/provider-proxy/test/functional/provider-proxy-ws.spec.ts (1)
apps/provider-proxy/test/setup/proxyServer.ts (1)
startServer(6-9)
apps/deploy-web/src/services/api-url/api-url.service.ts (2)
apps/deploy-web/src/config/env-config.schema.ts (2)
ServerEnvConfig(75-75)BrowserEnvConfig(74-74)apps/deploy-web/src/config/browser-env.config.ts (1)
BrowserEnvConfig(45-45)
apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.spec.ts (2)
apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.ts (1)
CertValidationResultError(66-69)apps/provider-proxy/src/services/ProviderService/ProviderService.ts (1)
getCertificate(15-31)
apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.ts (1)
apps/provider-proxy/src/utils/validateCertificateAttrs.ts (1)
validateCertificateAttrs(11-49)
apps/deploy-web/src/services/app-di-container/app-di-container.ts (1)
apps/deploy-web/src/services/provider-proxy/provider-proxy.service.ts (1)
ProviderProxyService(18-319)
🪛 dotenv-linter (4.0.0)
apps/deploy-web/env/.env
[warning] 34-34: [UnorderedKey] The PROVIDER_PROXY_SANDBOX_URL key should go before the PROVIDER_PROXY_TESTNET_URL key
(UnorderedKey)
apps/deploy-web/env/.env.production
[warning] 17-17: [UnorderedKey] The NEXT_PUBLIC_PROVIDER_PROXY_MAINNET_URL key should go before the NEXT_PUBLIC_STATS_APP_URL key
(UnorderedKey)
[warning] 18-18: [UnorderedKey] The NEXT_PUBLIC_PROVIDER_PROXY_SANDBOX_URL key should go before the NEXT_PUBLIC_STATS_APP_URL key
(UnorderedKey)
[warning] 19-19: [UnorderedKey] The NEXT_PUBLIC_PROVIDER_PROXY_TESTNET_URL key should go before the NEXT_PUBLIC_STATS_APP_URL key
(UnorderedKey)
apps/deploy-web/env/.env.sample
[warning] 15-15: [UnorderedKey] The NEXT_PUBLIC_PROVIDER_PROXY_MAINNET_URL key should go before the NEXT_PUBLIC_STATS_APP_URL key
(UnorderedKey)
[warning] 16-16: [UnorderedKey] The NEXT_PUBLIC_PROVIDER_PROXY_SANDBOX_URL key should go before the NEXT_PUBLIC_STATS_APP_URL key
(UnorderedKey)
[warning] 17-17: [UnorderedKey] The NEXT_PUBLIC_PROVIDER_PROXY_TESTNET_URL key should go before the NEXT_PUBLIC_STATS_APP_URL key
(UnorderedKey)
apps/deploy-web/env/.env.staging
[warning] 17-17: [UnorderedKey] The NEXT_PUBLIC_PROVIDER_PROXY_MAINNET_URL key should go before the NEXT_PUBLIC_STATS_APP_URL key
(UnorderedKey)
[warning] 18-18: [UnorderedKey] The NEXT_PUBLIC_PROVIDER_PROXY_SANDBOX_URL key should go before the NEXT_PUBLIC_STATS_APP_URL key
(UnorderedKey)
[warning] 19-19: [UnorderedKey] The NEXT_PUBLIC_PROVIDER_PROXY_TESTNET_URL key should go before the NEXT_PUBLIC_STATS_APP_URL key
(UnorderedKey)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: Socket Security: Pull Request Alerts
🔇 Additional comments (33)
apps/provider-proxy/test/setup-functional-env.ts (1)
1-3: LGTM!The functional test environment setup correctly uses dotenv to preload test-specific environment variables from the dedicated test environment file. This aligns with the new REST_API_NODE_URL configuration approach.
apps/provider-proxy/package.json (2)
26-26: LGTM!Adding
@akashnetwork/env-loaderas a runtime dependency is consistent with the project's pattern for loading environment variables from env/.env files.Based on learnings
53-53: LGTM!The
dotenvdev dependency is appropriately added to support the new functional test setup script that preloads environment variables.apps/deploy-web/env/.env.production (1)
17-19: LGTM!The per-network provider proxy URLs are correctly added, enabling the deploy-web application to route requests to the appropriate network-specific provider-proxy instance. This architectural change moves network resolution from the provider-proxy service to the client application.
apps/provider-proxy/src/services/WebsocketServer.ts (2)
443-447: LGTM!The
CreateProviderSocketOptionsinterface correctly removes thenetworkfield, aligning with the PR's architectural change to eliminate network-based resolution from the provider-proxy service.
289-289: Verification complete — no issues found.The
CertificateValidator.validate()method signature has been correctly updated to accept only two parameters (certificateandproviderAddress), and all call sites throughout the codebase are consistent with this signature. No orphaned calls using the old three-parameter version were found.apps/provider-proxy/src/utils/schema.ts (3)
23-25: LGTM!Excellent backward compatibility approach. The deprecated fields are retained as optional strings with clear deprecation messages, allowing existing clients to continue working while guiding them to remove the obsolete parameters.
35-35: LGTM!The type correctly omits the deprecated fields from the exported schema type, ensuring new code doesn't rely on these parameters while maintaining runtime backward compatibility.
38-46: LGTM!The preprocessing logic correctly destructures and discards the deprecated
networkandchainNetworkfields while preserving the MTLS authentication migration from the legacycertPem/keyPemformat. This maintains backward compatibility without complex migration logic.apps/provider-proxy/jest.config.js (1)
28-29: LGTM!The
setupFilesconfiguration correctly wires the functional test environment setup script, ensuring that environment variables fromenv/.env.functional.testare loaded before tests execute.apps/provider-proxy/test/functional/provider-proxy-http.spec.ts (1)
31-31: LGTM!The shift from global server setup to per-test
startServer()calls is necessary because the provider-proxy now reads configuration from environment variables at startup. This ensures proper test isolation, especially for tests that modifyprocess.envvalues (e.g., line 235 setsTEST_CHAIN_NETWORK_URL).Also applies to: 56-56, 82-82, 133-133, 158-158, 193-193, 233-233, 271-271, 312-312, 357-357, 391-391, 415-415, 449-449, 481-481, 540-540, 586-586, 625-625, 694-694
apps/provider-proxy/env/.env (1)
1-1: The REST_API_NODE_URL endpoint is valid and appropriate.Verification confirms the endpoint
https://rest-akash.ecostake.comis accessible and serves the required Akash certificate endpoints that ProviderService depends on. ECO Stake is a known and reliable validator in the Cosmos ecosystem. No issues identified.apps/deploy-web/src/services/app-di-container/app-di-container.ts (1)
77-85: LGTM! Provider proxy URL derivation is well-structured.The refactoring to use
apiUrlService.getProviderProxyUrlFor()for both HTTP and WebSocket URLs is clean and aligns with the PR's objective to centralize per-network URL resolution.apps/deploy-web/env/.env.staging (1)
17-19: LGTM! Per-network provider proxy URLs added correctly.The three new environment variables support the per-network URL resolution strategy. The static analysis warnings about key ordering are stylistic and can be safely ignored or addressed as an optional cleanup.
apps/deploy-web/env/.env.sample (1)
15-17: LGTM! Sample environment variables added correctly.The new placeholder variables align with the updated environment schema and provide clear guidance for configuration.
apps/provider-proxy/src/routes/proxyProviderRequest.ts (2)
57-76: LGTM! Network parameter removal is consistent.The refactoring correctly removes network from the request payload and downstream calls. URL resolution now relies on environment configuration, which aligns with the PR's architecture shift.
100-106: LGTM! Error logging updated consistently.Network field removed from error logging, maintaining consistency with the broader refactoring.
apps/provider-proxy/test/functional/provider-proxy-ws.spec.ts (1)
39-40: LGTM! Test initialization improved.Moving
startServer()closer to WebSocket creation improves test clarity and isolation.apps/provider-proxy/src/config/env.config.ts (1)
1-9: LGTM! Environment configuration module is well-structured.The new environment configuration provides type-safe access to
REST_API_NODE_URL. The synchronous parsing at module level was previously flagged but marked as addressed in commit 44dd3ca.apps/provider-proxy/src/services/ProviderService/ProviderService.spec.ts (1)
12-26: LGTM! Test calls updated to match new API signature.The
getCertificatecalls correctly use the new two-parameter signature(providerAddress, serialNumber), removing the network parameter as expected.apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.spec.ts (2)
22-27: LGTM! Certificate validation tests updated correctly.The test calls reflect the API changes where
validate()now takes(cert, providerAddress)andgetCertificate()takes(providerAddress, serialNumber), removing network parameters throughout.
167-178: LGTM! Concurrent validation tests maintain coverage.The concurrent validation tests correctly use the new signature while preserving the original test intent to verify single certificate fetches during parallel validations.
apps/deploy-web/env/.env (1)
28-31: LGTM! Per-network provider proxy URLs added.The addition of separate provider proxy URLs for each network (mainnet, sandbox, testnet) provides better configuration flexibility and aligns with the existing
BASE_API_*URL pattern.apps/provider-proxy/src/services/ProviderProxy.ts (2)
60-76: LGTM! Certificate validation simplified.The removal of the network parameter from certificate validation simplifies the logic while maintaining correctness. The validation now relies solely on
providerAddress, which is consistent with the refactoring goals across the PR.
126-126: LGTM! Cache key simplified.The agent cache key now correctly uses only
providerAddressinstead of a network-prefixed key, simplifying the caching logic while maintaining uniqueness.apps/deploy-web/src/services/api-url/api-url.service.ts (2)
6-26: LGTM! Constructor type correctly expanded.The constructor parameter types are properly expanded to include the new provider proxy URL fields for both server and browser configurations, maintaining consistency with the existing
BASE_API_*URL pattern.
50-70: LGTM! Provider proxy URL resolution implemented correctly.The new
getProviderProxyUrlFormethod follows the exact same pattern asgetBaseApiUrlFor, providing consistent URL resolution logic for provider proxy URLs across server and browser environments.apps/provider-proxy/src/services/ProviderService/ProviderService.ts (2)
9-13: LGTM! Constructor simplified.The constructor correctly accepts a concrete
chainBaseUrlstring instead of a network-based resolver, simplifying the service initialization and removing network-scoped logic.
15-31: LGTM! Method signature simplified.The
getCertificatemethod signature is correctly updated to remove the network parameter and usethis.chainBaseUrldirectly. This is a breaking change but aligns with the PR's goal of removing network-scoped logic.apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.ts (3)
21-44: LGTM! Validation method signature simplified.The
validatemethod signature is correctly updated to remove the network parameter. All instrumentation calls are consistently updated to passproviderAddresswithout network context, maintaining the integrity of the validation logic.
46-62: LGTM! Cache key simplified appropriately.The
getProviderCertificatemethod correctly simplifies the cache key fromnetwork.providerAddress.serialNumbertoproviderAddress.serialNumber, removing network context while maintaining uniqueness. The call toproviderService.getCertificateis properly updated to match its new signature.
70-92: LGTM! Instrumentation interface consistently updated.The
CertificateValidatorIntrumentationinterface and factory implementation are thoroughly updated to remove network parameters from all callbacks. Log messages remain clear and informative while focusing onproviderAddressinstead of network context.apps/provider-proxy/src/container.ts (1)
4-24: LGTM! Simplified URL resolution with proper validation.The refactoring correctly simplifies provider service initialization by using a direct environment variable (
REST_API_NODE_URL) instead of network-based resolution. The TEST_CHAIN_NETWORK_URL override for functional tests is well-documented and justified. Verification confirmsREST_API_NODE_URLis properly defined in the environment configuration with Zod URL validation (z.string().url()), ensuring type safety and URL format correctness.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
|
@jzsfkzm could you please revert changes in deploy-web? I will adjust it in your PR which changes helm values and gha |
stalniy
left a comment
stalniy
left a comment
There was a problem hiding this comment.
there are few minor issues which I'd like to fix but I like how it looks 👍
apps/provider-proxy/test/functional/provider-proxy-http.spec.ts
Outdated
Show resolved
Hide resolved
jzsfkzm
force-pushed
the
features/2189-accept-rest-api-node-url
branch
from
5d6fc74 to
3a6d0f0
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
apps/provider-proxy/src/config/env.config.ts (1)
7-7: Move environment parsing to dependency injection.The module-level synchronous parse creates a side effect that will crash the application at import time if
REST_API_NODE_URLis missing. This prevents tests and environments from controlling when validation occurs.As previously requested, move this to DI. Apply this pattern:
-export const envConfig = envSchema.parse(process.env); +export function parseEnvConfig() { + return envSchema.parse(process.env); +}Then update consumers (e.g.,
apps/provider-proxy/src/container.ts) to callparseEnvConfig()during container initialization, allowing proper error handling and test environment setup.Based on coding guidelines
apps/provider-proxy/src/utils/schema.ts (1)
23-25: Address the previous review comment about deprecated field definitions.A previous review from stalniy indicated these deprecated field definitions are unnecessary since zod doesn't validate extra fields strictly by default—old clients can still send
networkandchainNetwork, and they'll be ignored. However, keeping these definitions does provide explicit API documentation and OpenAPI schema clarity about the deprecation.If you prefer explicit documentation of the deprecation in the API schema, the current approach is acceptable. Otherwise, consider removing these field definitions as stalniy suggested.
🧹 Nitpick comments (1)
apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.spec.ts (1)
55-55: Update test description to reflect network removal.The test description mentions caching "per network, provider and serial number," but network is no longer part of the cache key after this PR's changes.
- it("caches provider certificate per network, provider and serial number", async () => { + it("caches provider certificate per provider and serial number", async () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
apps/provider-proxy/env/.env(1 hunks)apps/provider-proxy/env/.env.functional.test(1 hunks)apps/provider-proxy/env/.env.sample(1 hunks)apps/provider-proxy/jest.config.js(1 hunks)apps/provider-proxy/package.json(2 hunks)apps/provider-proxy/src/config/env.config.ts(1 hunks)apps/provider-proxy/src/container.ts(2 hunks)apps/provider-proxy/src/routes/proxyProviderRequest.ts(1 hunks)apps/provider-proxy/src/server.ts(1 hunks)apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.spec.ts(10 hunks)apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.ts(2 hunks)apps/provider-proxy/src/services/ProviderProxy.ts(2 hunks)apps/provider-proxy/src/services/ProviderService/ProviderService.spec.ts(6 hunks)apps/provider-proxy/src/services/ProviderService/ProviderService.ts(1 hunks)apps/provider-proxy/src/services/WebsocketServer.ts(1 hunks)apps/provider-proxy/src/utils/schema.ts(2 hunks)apps/provider-proxy/test/functional/provider-proxy-http.spec.ts(18 hunks)apps/provider-proxy/test/functional/provider-proxy-ws.spec.ts(8 hunks)apps/provider-proxy/test/setup-functional-env.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- apps/provider-proxy/env/.env
- apps/provider-proxy/env/.env.sample
- apps/provider-proxy/env/.env.functional.test
- apps/provider-proxy/src/container.ts
- apps/provider-proxy/test/setup-functional-env.ts
- apps/provider-proxy/package.json
- apps/provider-proxy/src/services/WebsocketServer.ts
- apps/provider-proxy/test/functional/provider-proxy-ws.spec.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-06-08T03:07:13.871Z
Learnt from: stalniy
Repo: akash-network/console PR: 1436
File: apps/api/src/provider/repositories/provider/provider.repository.ts:79-90
Timestamp: 2025-06-08T03:07:13.871Z
Learning: The getProvidersHostUriByAttributes method in apps/api/src/provider/repositories/provider/provider.repository.ts already has comprehensive test coverage in provider.repository.spec.ts, including tests for complex HAVING clause logic with COUNT(*) FILTER (WHERE ...) syntax, signature conditions (anyOf/allOf), and glob pattern matching.
Applied to files:
apps/provider-proxy/src/services/ProviderService/ProviderService.spec.tsapps/provider-proxy/test/functional/provider-proxy-http.spec.ts
📚 Learning: 2025-08-12T13:52:38.708Z
Learnt from: stalniy
Repo: akash-network/console PR: 1800
File: apps/deploy-web/next.config.js:163-165
Timestamp: 2025-08-12T13:52:38.708Z
Learning: In the Akash Console project, akashnetwork/env-loader is used at the top of next.config.js files to automatically load environment variables from env/.env files into process.env. SENTRY_ORG and SENTRY_PROJECT are stored as public configuration values in apps/deploy-web/env/.env and are loaded this way, while only SENTRY_AUTH_TOKEN is handled as a GitHub secret in workflows.
Applied to files:
apps/provider-proxy/src/server.ts
🧬 Code graph analysis (3)
apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.ts (1)
apps/provider-proxy/src/utils/validateCertificateAttrs.ts (1)
validateCertificateAttrs(11-49)
apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.spec.ts (2)
apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.ts (1)
CertValidationResultError(66-69)apps/provider-proxy/src/services/ProviderService/ProviderService.ts (1)
getCertificate(15-31)
apps/provider-proxy/test/functional/provider-proxy-http.spec.ts (1)
apps/provider-proxy/test/setup/proxyServer.ts (1)
startServer(6-9)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (13)
- GitHub Check: log-collector-ci
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
🔇 Additional comments (19)
apps/provider-proxy/src/server.ts (1)
1-1: LGTM! Environment loader import correctly placed.The side-effect import of
@akashnetwork/env-loaderat the top of the file follows the project's established pattern for loading environment variables from env/.env files into process.env.Based on learnings
apps/provider-proxy/jest.config.js (1)
28-29: LGTM! Functional test environment setup configured correctly.The setupFiles entry ensures environment variables are loaded before functional tests execute, supporting the new REST_API_NODE_URL configuration pattern.
apps/provider-proxy/test/functional/provider-proxy-http.spec.ts (1)
17-19: Verify server lifecycle management.Each test now calls
await startServer(), but the server is only stopped once inafterAll. This could cause resource leaks or port conflicts if multiple server instances are created. The previous review comment questioned whetherafterEachshould be used instead to properly clean up after each test.Please verify whether
startServer()creates a new server instance or reuses an existing one. If it creates new instances, consider this approach:- afterAll(async () => { + afterEach(async () => { await stopServer(); });Alternatively, if servers should be shared across tests, move
await startServer()back tobeforeAlland useafterAllfor cleanup.apps/provider-proxy/src/routes/proxyProviderRequest.ts (1)
57-57: LGTM! Network parameter consistently removed.The removal of the
networkfield from payload extraction, logging, and downstream calls aligns with the PR's objective to centralize configuration viaREST_API_NODE_URLand eliminate network-based routing.apps/provider-proxy/src/services/ProviderService/ProviderService.spec.ts (1)
12-60: LGTM! Test expectations correctly updated.All
getCertificatecalls have been updated to use the new two-argument signature(providerAddress, serialNumber), removing the network parameter. The test coverage is preserved and assertions remain valid.apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.spec.ts (1)
22-178: LGTM! Test expectations correctly updated for network removal.All
validateandgetCertificatecalls have been updated to the new two-argument signatures, removing the network parameter. Test coverage for certificate validation scenarios is preserved.apps/provider-proxy/src/services/ProviderProxy.ts (2)
60-62: LGTM! Certificate validation correctly simplified.The validation flow now uses only
providerAddress, eliminating network-based routing. The conditional logic and certificate validation call are correctly updated.
126-126: LGTM! Agent cache key simplified.The cache key now uses only
providerAddress, consistent with the removal of network context throughout the proxy service.apps/provider-proxy/src/utils/schema.ts (2)
35-35: LGTM: Type definition correctly omits deprecated fields.The updated type properly excludes both
networkandchainNetworkalong with the other deprecated fields, ensuring TypeScript consumers won't reference them.
41-41: LGTM: Backward compatibility maintained while removing migration logic.The destructuring correctly strips out deprecated
networkandchainNetworkfields while maintaining backward compatibility for existing clients. The removal of the migration logic (copyingchainNetworktonetwork) aligns with the shift to environment-based configuration.apps/provider-proxy/src/services/ProviderService/ProviderService.ts (3)
10-10: LGTM: Constructor simplified with direct URL injection.The change from a network-based URL resolver to direct
chainBaseUrlinjection centralizes configuration and simplifies the service's dependencies. This aligns with the PR's goal of usingREST_API_NODE_URLfrom environment configuration.
15-15: LGTM: Method signature simplified by removing network parameter.The updated signature
getCertificate(providerAddress, serialNumber)is cleaner and aligns with the shift to environment-based URL configuration. The network context is no longer needed since the base URL is injected at construction time.
23-23: Verification confirmed: Environment configuration is properly validated.REST_API_NODE_URL is properly validated in
env.config.tsusing Zod's URL schema validation, and the value is correctly passed to ProviderService incontainer.tswith a fallback toTEST_CHAIN_NETWORK_URLfor functional tests. The direct URL usage correctly eliminates network-based resolution.apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.ts (6)
21-21: LGTM: Validation signature simplified to provider-address-centric approach.The updated signature
validate(certificate, providerAddress)removes the network context dependency, aligning with the environment-based URL configuration. The validation logic remains intact and correct.
26-26: LGTM: Instrumentation callbacks consistently updated.All instrumentation callback invocations correctly use the updated two-parameter signature, removing the network context. The callbacks now properly receive
(certificate, providerAddress, ...)arguments.Also applies to: 32-32, 37-37, 41-41
54-54: LGTM: Provider service call updated to match new signature.The call to
this.providerService.getCertificate(providerAddress, cert.serialNumber)correctly uses the updated two-parameter signature, removing the network context.
71-75: LGTM: Instrumentation interface updated consistently.The
CertificateValidatorIntrumentationinterface correctly removes the network parameter from all callback signatures, maintaining consistency with the updated validation flow.
77-92: LGTM: Factory function implementation matches updated interface.The
createCertificateValidatorInstrumentationfactory correctly implements all callbacks with the updated signatures, removing network references from logging statements. The log messages remain informative with provider address context.
46-47: Cache key format change confirmed safe for single-network deployment model.The cache key change from
${network}.${providerAddress}.${cert.serialNumber}to${providerAddress}.${cert.serialNumber}is appropriate and safe. The deployment model enforces a single network per provider-proxy instance:
REST_API_NODE_URLenvironment variable is singular and network-specific (one URL per deployment)CertificateValidatoris instantiated once per containerProviderServiceaccepts a single network URL at initialization- Cache is instance-scoped; no cross-instance collisions possible
Code changes are correct and ready to merge.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.spec.ts (1)
132-140: Update the test description to avoid duplication.This test has an identical description to the test at lines 122-130, but tests a different scenario (default common name vs explicit non-bech32). Consider renaming to something like "returns error if certificate has default common name (not bech32 format)" for clarity.
- it("returns error if certificate subject common name is not in bech32 format", async () => { + it("returns error if certificate has default common name (not bech32 format)", async () => {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
apps/provider-proxy/src/config/env.config.ts(1 hunks)apps/provider-proxy/src/container.ts(1 hunks)apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.spec.ts(10 hunks)apps/provider-proxy/src/utils/schema.ts(1 hunks)apps/provider-proxy/test/functional/provider-proxy-http.spec.ts(19 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- apps/provider-proxy/src/config/env.config.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-11-12T09:03:40.132Z
Learnt from: stalniy
Repo: akash-network/console PR: 0
File: :0-0
Timestamp: 2025-11-12T09:03:40.132Z
Learning: For backend services (like the Indexer), prefer using createOtelLogger from "akashnetwork/logging/otel" to include OpenTelemetry trace context in logs.
Applied to files:
apps/provider-proxy/src/container.ts
📚 Learning: 2025-06-08T03:07:13.871Z
Learnt from: stalniy
Repo: akash-network/console PR: 1436
File: apps/api/src/provider/repositories/provider/provider.repository.ts:79-90
Timestamp: 2025-06-08T03:07:13.871Z
Learning: The getProvidersHostUriByAttributes method in apps/api/src/provider/repositories/provider/provider.repository.ts already has comprehensive test coverage in provider.repository.spec.ts, including tests for complex HAVING clause logic with COUNT(*) FILTER (WHERE ...) syntax, signature conditions (anyOf/allOf), and glob pattern matching.
Applied to files:
apps/provider-proxy/test/functional/provider-proxy-http.spec.ts
🧬 Code graph analysis (3)
apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.spec.ts (2)
apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.ts (1)
CertValidationResultError(66-69)apps/provider-proxy/src/services/ProviderService/ProviderService.ts (1)
getCertificate(15-31)
apps/provider-proxy/src/container.ts (2)
apps/provider-proxy/src/config/env.config.ts (1)
envSchema(3-5)apps/provider-proxy/src/services/ProviderService/ProviderService.ts (1)
ProviderService(6-55)
apps/provider-proxy/test/functional/provider-proxy-http.spec.ts (3)
apps/provider-proxy/test/setup/proxyServer.ts (2)
stopServer(11-13)startServer(6-9)apps/provider-proxy/test/setup/providerServer.ts (1)
stopProviderServer(73-75)apps/provider-proxy/test/setup/chainApiServer.ts (1)
stopChainAPIServer(52-54)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (10)
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: test-build
- GitHub Check: validate / validate-app
- GitHub Check: test-build
- GitHub Check: test-build
🔇 Additional comments (5)
apps/provider-proxy/src/container.ts (1)
4-4: LGTM! Clean refactoring to environment-based configuration.The changes successfully implement environment-based configuration for REST_API_NODE_URL:
- Environment validation happens early with fail-fast behavior (line 11)
- Type safety is maintained through zod schema validation
- The URL is correctly passed to ProviderService (line 16)
This aligns with the PR objectives and addresses the previous review feedback.
Also applies to: 11-11, 16-16
apps/provider-proxy/src/services/CertificateValidator/CertificateValidator.spec.ts (1)
1-195: LGTM! Tests correctly updated to reflect API changes.All test cases have been properly updated to reflect the removal of the
networkparameter from bothvalidateandgetCertificatemethods. The shift from network-scoped to provider-scoped caching is correctly reflected in the test at lines 55-86, and all test scenarios (validation errors, caching, concurrency) remain comprehensively covered.apps/provider-proxy/src/utils/schema.ts (2)
32-32: LGTM! Type definition correctly reflects schema changes.The type now properly omits only the deprecated
certPemandkeyPemfields after removing network-based configuration. This maintains a clean public API surface while the runtime schema still accepts deprecated fields for backwards compatibility.
38-42: LGTM! Preprocessing correctly updated for network-based config removal.The destructuring now handles only
certPemandkeyPem, with the chainNetwork migration logic appropriately removed. The backwards compatibility migration for deprecated certificate fields (lines 39-41) is correctly preserved, ensuring old clients continue to work seamlessly.apps/provider-proxy/test/functional/provider-proxy-http.spec.ts (1)
17-19: LGTM! This correctly implements per-test server lifecycle cleanup.The
afterEachhook properly stops all servers after each test, which aligns with the per-test server startup pattern and directly addresses the previous blocking question about server lifecycle management.
| await startServer(); | ||
|
|
||
| process.env.TEST_CHAIN_NETWORK_URL = "http://localhost:31234"; | ||
| process.env.REST_API_NODE_URL = "http://localhost:31234"; |
There was a problem hiding this comment.
🧩 Analysis chain
Restore process.env.REST_API_NODE_URL after mutation to prevent test pollution.
Mutating process.env.REST_API_NODE_URL on line 231 without restoration could affect subsequent tests. Additionally, setting this environment variable after startServer() (line 229) may not have the intended effect if the configuration is loaded at server startup rather than per-request.
Consider one of the following approaches:
Option 1: Capture and restore in this test
+ const originalUrl = process.env.REST_API_NODE_URL;
process.env.REST_API_NODE_URL = "http://localhost:31234";
const responsePromise = request("/", {
method: "POST",
body: JSON.stringify({
method: "GET",
url: `${providerUrl}/200.txt`,
providerAddress,
network
})
});
await wait(200);
await startChainApiServer([validCertPair.cert]);
const response = await responsePromise;
+ process.env.REST_API_NODE_URL = originalUrl;Option 2: Add cleanup to afterEach
afterEach(async () => {
await Promise.all([stopServer(), stopProviderServer(), stopChainAPIServer()]);
delete process.env.REST_API_NODE_URL; // or restore to original value
});Additionally, verify whether the configuration is loaded at startup or per-request, as the current placement suggests the latter.
---
🏁 Script executed:
```shell
#!/bin/bash
# Examine the test file around lines 229-231 and inspect cleanup patterns
cat apps/provider-proxy/test/functional/provider-proxy-http.spec.ts | head -250 | tail -+220
Length of output: 67
🏁 Script executed:
#!/bin/bash
# Search for afterEach and beforeEach patterns in the test file
rg -n "afterEach|beforeEach" apps/provider-proxy/test/functional/provider-proxy-http.spec.tsLength of output: 28
🏁 Script executed:
#!/bin/bash
# Search for all REST_API_NODE_URL references in the test file
rg -n "REST_API_NODE_URL" apps/provider-proxy/test/functional/provider-proxy-http.spec.ts -B2 -A2Length of output: 172
🏁 Script executed:
#!/bin/bash
# Check the overall structure of the test file - how many describe/it blocks
wc -l apps/provider-proxy/test/functional/provider-proxy-http.spec.ts
rg -n "describe\(|it\(" apps/provider-proxy/test/functional/provider-proxy-http.spec.ts | head -30Length of output: 1598
---
🏁 Script executed:
```shell
#!/bin/bash
# View the afterEach block
sed -n '17,30p' apps/provider-proxy/test/functional/provider-proxy-http.spec.ts
Length of output: 619
🏁 Script executed:
#!/bin/bash
# View the full test starting at line 222
sed -n '222,250p' apps/provider-proxy/test/functional/provider-proxy-http.spec.tsLength of output: 921
🏁 Script executed:
#!/bin/bash
# Check if REST_API_NODE_URL is cleaned up or used in afterEach
sed -n '17,25p' apps/provider-proxy/test/functional/provider-proxy-http.spec.tsLength of output: 445
🏁 Script executed:
#!/bin/bash
# Search for all uses of REST_API_NODE_URL in entire test file to see if it affects multiple tests
rg -c "REST_API_NODE_URL" apps/provider-proxy/test/functional/provider-proxy-http.spec.ts
rg -n "REST_API_NODE_URL" apps/provider-proxy/test/functional/provider-proxy-http.spec.tsLength of output: 67
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for REST_API_NODE_URL in source code to understand when/how it's used
rg -n "REST_API_NODE_URL" apps/provider-proxy/src/ -B3 -A3
Length of output: 1081
🏁 Script executed:
#!/bin/bash
# Check startServer function to see if it reads REST_API_NODE_URL
rg -n "function startServer|const startServer|export.*startServer" apps/provider-proxy/test/functional/provider-proxy-http.spec.ts -A5Length of output: 0
🏁 Script executed:
#!/bin/bash
# Look for REST_API_NODE_URL in test setup/helpers
fd "\.ts$" apps/provider-proxy/test/ --exec rg -l "REST_API_NODE_URL" {} \;Length of output: 63
---
🏁 Script executed:
```shell
#!/bin/bash
# Find where container is created and how startServer uses it
rg -n "createContainer|new Container" apps/provider-proxy/test/functional/ -B2 -A2
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Check if startServer is defined in the test file or imported
rg -n "import.*startServer|startServer.*=" apps/provider-proxy/test/functional/provider-proxy-http.spec.ts | head -5Length of output: 66
🏁 Script executed:
#!/bin_bash
# Look at the test helpers to see how startServer works
fd "helpers\.|setup\." apps/provider-proxy/test/ --type f
rg -n "export.*startServer" apps/provider-proxy/test/ -A10Length of output: 846
🏁 Script executed:
#!/bin/bash
# Check where envConfig is parsed/created to understand if it's read at startup or per-request
rg -n "envSchema\.parse|envConfig.*=" apps/provider-proxy/src/ -B2 -A2Length of output: 347
---
🏁 Script executed:
```shell
#!/bin/bash
# Check startAppServer to see when createContainer is called
rg -n "createContainer|startAppServer" apps/provider-proxy/src/app/ -B2 -A5
Length of output: 68
🏁 Script executed:
#!/bin/bash
# View the full startAppServer file
cat apps/provider-proxy/src/app/startAppServer.tsLength of output: 77
---
🏁 Script executed:
```shell
#!/bin/bash
# Find where startAppServer is defined
fd "startAppServer" apps/provider-proxy/
Length of output: 0
🏁 Script executed:
#!/bin/bash
# Search for startAppServer definition
rg -n "export.*startAppServer|function startAppServer" apps/provider-proxy/Length of output: 106
🏁 Script executed:
#!/bin/bash
# Check the imports in proxyServer.ts
cat apps/provider-proxy/test/setup/proxyServer.tsLength of output: 683
---
🏁 Script executed:
```shell
#!/bin/bash
# View app.ts to see when createContainer is called
cat apps/provider-proxy/src/app.ts
Length of output: 3073
Restore process.env.REST_API_NODE_URL after mutation to prevent test pollution, and move the mutation before startServer() for it to take effect.
The mutation at line 231 cannot work as intended: startServer() calls createContainer(), which immediately parses process.env into envConfig and initializes ProviderService with that parsed config (app.ts:56, container.ts:11, 16). Setting process.env.REST_API_NODE_URL after this happens has no effect on the already-instantiated service.
Additionally, the mutation is never restored in afterEach, causing test pollution for subsequent tests.
Consider either:
Option 1: Mutation before startServer()
const providerAddress = generateBech32();
const validCertPair = createX509CertPair({
commonName: providerAddress
});
const { providerUrl } = await startProviderServer({ certPair: validCertPair });
+ const originalUrl = process.env.REST_API_NODE_URL;
+ process.env.REST_API_NODE_URL = "http://localhost:31234";
await startServer();
- process.env.REST_API_NODE_URL = "http://localhost:31234";
const responsePromise = request("/", {
...
});Option 2: Add cleanup to afterEach
afterEach(async () => {
await Promise.all([stopServer(), stopProviderServer(), stopChainAPIServer()]);
delete process.env.REST_API_NODE_URL; // or restore to original value
});🤖 Prompt for AI Agents
In apps/provider-proxy/test/functional/provider-proxy-http.spec.ts around lines
229 to 231, the test mutates process.env.REST_API_NODE_URL after startServer()
so the server already created its config and the mutation has no effect, and the
env var is never restored causing test pollution; move the assignment of
process.env.REST_API_NODE_URL to before calling startServer() so the created
container picks it up, and add cleanup in afterEach to restore the original
value (save original at test start and restore it, or delete the key) along with
the existing stopServer/stopProviderServer/stopChainAPIServer calls.
2 participants
refs #2189
Summary by CodeRabbit
Configuration Updates
Chores