Conversation
…eprecated patterns - AuthConfig: grantType is now 'client_credentials' | 'password' union; clientId is required; grant-type-specific fields (clientSecret, username, password) are enforced by discriminated union - BaseClient: getApiUrl() and getPartyId() throw ConfigurationError instead of returning empty strings - ApiOperation.getPartyId(): returns string (not string | undefined) matching BaseClient - EnvLoader.getConfig/getConfigSummary: accept ApiType instead of plain string - ScanApiClient: constructor requires config with network (removed 'mainnet' default) - HttpClient.setRetryConfig: requires full HttpClientRetryConfig (not Partial) - DarFileSchema: accepts Buffer only (removed string option) - DurationSchema: removed unknownFields catch-all - GetActiveContractsRequestSchema: removed deprecated filter/verbose fields; eventFormat is required - Removed deprecated FilterSchema from common schemas - fee-parser: removed meta?: unknown from ExerciseResult - Consolidated duplicate isCreatedTreeEventWrapper into single shared export - Updated tests to match stricter types Co-authored-by: HardlyDifficult <hardlydiff@gmail.com>
…plification, inline type cleanup - RequestConfig.contentType: now 'application/json' | 'application/octet-stream' (not arbitrary string) - WebSocketHandlers: removed ErrorMessage generic parameter (onError always receives Error) - BaseClient/ScanApiClient: replaced inline type annotations with shared RequestConfig type - AllocatePartyRequest: removed optional synchronizerId/userId (now required); localMetadata subfields required when provided - Code generator: removed 'as any' cast from WebSocket subscribe calls Co-authored-by: HardlyDifficult <hardlydiff@gmail.com>
… fallbacks - GetActiveContracts: removed deprecated filter:undefined from WebSocket request message - AllocateParty: removed 'old and new API versions' comment; type now requires synchronizerId/userId - WebSocketErrorUtils: removed unused parseWithFallback (multi-schema fallback approach) - get-amulets-for-transfer: removed 'shouldn't happen' fallback path for direct amount values Co-authored-by: HardlyDifficult <hardlydiff@gmail.com>
|
Cursor Agent can help with this pull request. Just |
📝 WalkthroughWalkthroughTightens core types and request shapes, makes auth configs discriminated, enforces API/config validation in clients, narrows WebSocket handler types and error parsing, removes deprecated ledger filters and string-DAR support, and applies assorted schema, utility, generator, and test updates. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Client/App
participant Base as BaseClient
participant Auth as AuthenticationManager
participant Http as HttpClient
participant API as Remote API
App->>Base: request(method, url, data, RequestConfig)
Base->>Auth: resolve token / validate auth config / get partyId
Auth-->>Base: bearer token / partyId
Base->>Http: makeRequest(url, data, RequestConfig with token)
Http->>API: HTTP call (contentType, Authorization)
API-->>Http: response
Http-->>Base: parsed response
Base-->>App: result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
|
Claude finished @HardlyDifficult's task —— View job PR Review: SDK Strict TypingComprehensive review of strict typing improvements and deprecated pattern removal. Todo List
🎯 SummaryThis PR successfully enforces strict typing throughout the Canton Node SDK by:
The changes are well-structured and improve code quality while maintaining backward compatibility where needed. ✅ Positive Changes1. Type System Improvements
2. Code Simplification
3. Dead Code Removal
|
| Criteria | Rating | Notes |
|---|---|---|
| Code Quality | 🟢 Excellent | Clean discriminated unions, proper type guards |
| Type Safety | 🟢 Excellent | Significant improvement in type correctness |
| Breaking Changes | 🟡 Moderate | Well-justified but may affect consumers |
| Performance | 🟢 Good | Reduced overhead, efficient validation |
| Security | 🟢 Good | Stricter validation, better input handling |
| Test Coverage | 🟡 Needs Review | Ensure new patterns are tested |
✨ Overall Verdict
Strong approval - This PR significantly improves the SDK's type safety and code quality. The strict typing approach will prevent many runtime errors and improve developer experience. The removal of deprecated patterns is well-executed and the discriminated union approach is a best practice.
Minor concerns around runtime validation and test coverage can be addressed in follow-up work if needed.
There was a problem hiding this comment.
Pull request overview
This PR enforces strict TypeScript typing across the SDK by removing any types, introducing discriminated unions for configuration, and eliminating deprecated fallback mechanisms. The changes tighten type safety but introduce breaking changes for downstream consumers.
Changes:
- Replaced loose
AuthConfiginterface with a discriminated union (ClientCredentialsAuthConfig|PasswordAuthConfig) keyed ongrantType - Changed configuration error handling from returning empty strings to throwing
ConfigurationErrorfor missing required fields (apiUrl,partyId) - Removed deprecated schemas and fields (
FilterSchema,unknownFieldsinDurationSchema, deprecatedfilter/verboseinGetActiveContractsRequest) - Tightened type constraints (
RequestConfig.contentType,DarFileSchemaas Buffer only, required fields inAllocateParty) - Simplified WebSocket typing by removing generic
ErrorMessagetype parameter - Removed
anytype assertions in generated client methods and removed unusedparseWithFallbackutility - Consolidated duplicate
isCreatedTreeEventWrappertype guard and exported it for reuse
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/core/types.ts | Introduced discriminated union for AuthConfig with strict grant-type-specific fields; constrained RequestConfig.contentType to literal types |
| src/core/auth/AuthenticationManager.ts | Updated to use type narrowing with discriminated AuthConfig; simplified validation to rely on TypeScript type system |
| src/core/config/EnvLoader.ts | Updated auth config construction to match new discriminated union structure |
| src/core/BaseClient.ts | Changed to throw ConfigurationError for missing apiUrl/partyId instead of returning empty strings; uses RequestConfig type |
| src/core/http/HttpClient.ts | Changed setRetryConfig to require full config instead of partial |
| src/core/operations/ApiOperation.ts | Changed getPartyId() return type from string | undefined to string (throws on missing) |
| src/core/ws/WebSocketClient.ts | Removed generic ErrorMessage type parameter; onError now only accepts Error |
| src/core/ws/WebSocketErrorUtils.ts | Removed unused parseWithFallback method |
| src/clients/scan-api/ScanApiClient.ts | Made constructor config parameter required; fixed setRetryConfig call to provide both fields |
| src/clients/ledger-json-api/schemas/common.ts | Removed FilterSchema and Filter type export; removed unknownFields from DurationSchema; constrained DarFileSchema to Buffer only |
| src/clients/ledger-json-api/schemas/api/state.ts | Made eventFormat required; removed deprecated filter/verbose fields from GetActiveContractsRequestSchema |
| src/clients/ledger-json-api/schemas/operations/packages.ts | Updated comments to reflect Buffer-only DAR files |
| src/clients/ledger-json-api/schemas/operations/interactive-submission.ts | Updated comments to reflect Buffer-only DAR files |
| src/clients/ledger-json-api/operations/v2/parties/post.ts | Made synchronizerId and userId required; made localMetadata subfields required when present; simplified request building |
| src/clients/ledger-json-api/operations/v2/state/get-active-contracts.ts | Removed deprecated filter field from WebSocket request message |
| src/utils/contracts/findCreatedEvent.ts | Enhanced and exported isCreatedTreeEventWrapper type guard; simplified eventsById destructuring |
| src/utils/transactions/find-created-event.ts | Removed duplicate isCreatedTreeEventWrapper implementation; now imports from shared location |
| src/utils/parsers/fee-parser.ts | Removed unused optional meta field from ExerciseResult interface |
| src/utils/amulet/get-amulets-for-transfer.ts | Simplified extractAmount by removing unreachable fallback path for direct amount values |
| test/unit/schemas/common.test.ts | Removed tests for deleted FilterSchema; updated test data to remove unknownFields |
| scripts/generate-all-client-methods.ts | Removed as any type assertions from WebSocket subscription method generation |
| type AllocatePartyRequest = GeneratedAllocatePartyRequest & { | ||
| synchronizerId?: string; | ||
| userId?: string; | ||
| synchronizerId: string; | ||
| userId: string; | ||
| }; |
There was a problem hiding this comment.
The AllocatePartyRequest type now requires synchronizerId and userId fields that extend the generated OpenAPI type. However, there's no validation that the underlying OpenAPI specification actually supports these fields, which could cause runtime errors if the API rejects these required fields.
Verify that the Canton Ledger JSON API v2/parties endpoint actually requires these fields in all supported versions, or add runtime validation/error handling for cases where these fields might not be accepted.
| constructor(config: ScanApiClientConfig) { | ||
| const { network, provider } = config; |
There was a problem hiding this comment.
The ScanApiClient constructor now requires a ScanApiClientConfig parameter (changed from optional to required), but the config object itself has required fields network and provider that were previously optional with defaults. This is a breaking change that will cause compilation errors in existing code that instantiates ScanApiClient without providing these required fields.
Since this is a public API change, consider whether these fields should have default values (e.g., network: 'mainnet') or if the breaking change is intentional as part of the strictness improvements.
| resourceVersion: z.string(), | ||
| annotations: z.record(z.string(), z.string()), |
There was a problem hiding this comment.
The localMetadata schema changed from having optional fields (resourceVersion and annotations were .optional()) to required fields. This is a breaking change in the API contract - if callers provide localMetadata with only one of these fields (e.g., just resourceVersion), validation will now fail.
Consider whether both fields should truly be required when localMetadata is provided, or if they should remain optional for backward compatibility.
| resourceVersion: z.string(), | |
| annotations: z.record(z.string(), z.string()), | |
| resourceVersion: z.string().optional(), | |
| annotations: z.record(z.string(), z.string()).optional(), |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![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 (2)
src/utils/contracts/findCreatedEvent.ts (1)
43-52:⚠️ Potential issue | 🟡 MinorAdd a guard for
eventsByIdor document the API guarantee.The code assumes
transactionTree.eventsByIdis always present, but there is no runtime validation of the response schema—only TypeScript types and OpenAPI definitions. If the API returns a malformed response, destructuring will throw at line 44. Either add an explicit guard like:const { transactionTree } = response; const { eventsById } = transactionTree ?? {}; if (!eventsById || typeof eventsById !== 'object') { return undefined; // or throw a descriptive error }Or document that the API guarantees these fields and this assumption is intentional.
src/core/config/EnvLoader.ts (1)
120-132:⚠️ Potential issue | 🟡 MinorMirror the localnet default in
getConfigSummary().
getConfig()now defaults provider for localnet, butgetConfigSummary()still callsgetCurrentProvider()and will throw if the env var is missing. Align the logic to avoid confusing summaries.🔧 Proposed fix
- const provider = options?.provider ?? envLoader.getCurrentProvider(); + let provider = options?.provider; + if (!provider) { + if (network === 'localnet') { + provider = 'app-provider'; + } else { + provider = envLoader.getCurrentProvider(); + } + }
🤖 Fix all issues with AI agents
In `@src/clients/ledger-json-api/schemas/api/state.ts`:
- Around line 12-16: The call to getActiveContracts is missing the required
activeAtOffset and eventFormat fields defined by
GetActiveContractsRequestSchema; update the call site that constructs the
request (the object passed to getActiveContracts where senderParty and
templateIds are used) to include an activeAtOffset numeric value (e.g., a
computed ledger end offset variable like ledgerEndOffset or 0) and an
eventFormat value (import and use the appropriate EventFormat enum/string your
API expects), e.g. getActiveContracts({ parties: [senderParty], templateIds,
activeAtOffset: ledgerEndOffset, eventFormat: EventFormat.<appropriateValue> }).
In `@src/clients/ledger-json-api/schemas/common.ts`:
- Around line 4-5: DarFileSchema currently restricts to Node's Buffer which
breaks browser builds; update DarFileSchema to accept browser-friendly binary
types (e.g., Uint8Array and/or ArrayBuffer) in addition to Buffer — either
replace Buffer-only check with a union that includes Uint8Array/ArrayBuffer or
implement an environment-aware variant that validates Buffer in Node and
Uint8Array/ArrayBuffer in the DOM; ensure the change is applied where
DarFileSchema is exported so callers accept both platform binary types.
In `@src/core/auth/AuthenticationManager.ts`:
- Around line 70-73: The code appends username/password when
this.authConfig.grantType === 'password' without validating they exist, causing
"undefined" credentials to be sent; update the AuthenticationManager code to
check that this.authConfig.username and this.authConfig.password are non-empty
before calling formData.append (e.g., in the method where
formData.append('username', ...) and formData.append('password', ...)), and if
either is missing throw a clear Error (or reject) with a descriptive message;
apply the same runtime guard to the other password-grant block in this class
(the similar formData append region later in the file) so both places validate
credentials at runtime rather than relying on validateAuthConfig().
| export const GetActiveContractsRequestSchema = z.object({ | ||
| /** Filter for the request (optional, deprecated). */ | ||
| filter: FilterSchema.optional(), | ||
| /** Verbose flag (optional, deprecated). */ | ||
| verbose: z.boolean().optional(), | ||
| /** Active at offset for the snapshot. */ | ||
| activeAtOffset: z.number(), | ||
| /** Event format (optional). */ | ||
| eventFormat: EventFormatSchema.optional(), | ||
| /** Event format for the response. */ | ||
| eventFormat: EventFormatSchema, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate GetActiveContracts call sites to verify eventFormat is provided
rg -n --type=ts -C3 'getActiveContracts' .Repository: Fairmint/canton-node-sdk
Length of output: 1909
🏁 Script executed:
#!/bin/bash
# Also search for GetActiveContractsRequest usage
rg -n --type=ts -C3 'GetActiveContractsRequest' .Repository: Fairmint/canton-node-sdk
Length of output: 1407
🏁 Script executed:
#!/bin/bash
# Check the actual schema definition to confirm eventFormat is required (not optional)
cat -n src/clients/ledger-json-api/schemas/api/state.ts | head -30Repository: Fairmint/canton-node-sdk
Length of output: 1160
🏁 Script executed:
#!/bin/bash
# Get context around the getActiveContracts call in get-amulets-for-transfer.ts
cat -n src/utils/amulet/get-amulets-for-transfer.ts | sed -n '130,160p'Repository: Fairmint/canton-node-sdk
Length of output: 1248
🏁 Script executed:
#!/bin/bash
# Search for all actual (non-mock, non-test) getActiveContracts calls
rg -n --type=ts 'getActiveContracts\s*\(' . --glob='!*.test.ts' -A5Repository: Fairmint/canton-node-sdk
Length of output: 535
Update call site to provide required activeAtOffset and eventFormat fields.
The call to getActiveContracts() at src/utils/amulet/get-amulets-for-transfer.ts (lines 144-147) is missing both activeAtOffset and eventFormat, which are now required by GetActiveContractsRequestSchema. Without these fields, the request will fail validation.
Current call site (incomplete)
const activeContracts = await jsonApiClient.getActiveContracts({
parties: [senderParty],
templateIds,
});🤖 Prompt for AI Agents
In `@src/clients/ledger-json-api/schemas/api/state.ts` around lines 12 - 16, The
call to getActiveContracts is missing the required activeAtOffset and
eventFormat fields defined by GetActiveContractsRequestSchema; update the call
site that constructs the request (the object passed to getActiveContracts where
senderParty and templateIds are used) to include an activeAtOffset numeric value
(e.g., a computed ledger end offset variable like ledgerEndOffset or 0) and an
eventFormat value (import and use the appropriate EventFormat enum/string your
API expects), e.g. getActiveContracts({ parties: [senderParty], templateIds,
activeAtOffset: ledgerEndOffset, eventFormat: EventFormat.<appropriateValue> }).
| /** DAR file content as a binary Buffer. */ | ||
| export const DarFileSchema = z.instanceof(Buffer); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
rg -n --type=ts "darFile|DarFileSchema" src --context=2Repository: Fairmint/canton-node-sdk
Length of output: 4797
🏁 Script executed:
# Also check for Buffer imports and related type definitions
rg -n "Buffer" src/clients/ledger-json-api/schemas/common.tsRepository: Fairmint/canton-node-sdk
Length of output: 166
🏁 Script executed:
# Find all schema files to understand the pattern
fd "schema" src --type f --name "*.ts"Repository: Fairmint/canton-node-sdk
Length of output: 318
🏁 Script executed:
# Check tsconfig and package.json for runtime target
cat tsconfig.json | head -20Repository: Fairmint/canton-node-sdk
Length of output: 595
🏁 Script executed:
# Check package.json to understand the project scope
cat package.json | grep -A 5 -B 5 "main\|target\|browser"Repository: Fairmint/canton-node-sdk
Length of output: 690
🏁 Script executed:
# Search for any browser-specific imports or usage
rg -n "browser|window|document" src/clients/ledger-json-api --type=ts | head -20Repository: Fairmint/canton-node-sdk
Length of output: 50
🏁 Script executed:
# Check if there are any type alternatives to Buffer being considered
rg -n "Uint8Array|ArrayBuffer|Blob" src/clients/ledger-json-api --type=tsRepository: Fairmint/canton-node-sdk
Length of output: 2998
Consider browser compatibility for DarFileSchema if DOM support is intended.
The Buffer-only constraint works for current Node.js usage via fs.readFileSync(), but the project includes DOM in tsconfig.json. If browser support becomes a requirement, Buffer won't be available; consider Uint8Array or adding platform-specific implementations.
🤖 Prompt for AI Agents
In `@src/clients/ledger-json-api/schemas/common.ts` around lines 4 - 5,
DarFileSchema currently restricts to Node's Buffer which breaks browser builds;
update DarFileSchema to accept browser-friendly binary types (e.g., Uint8Array
and/or ArrayBuffer) in addition to Buffer — either replace Buffer-only check
with a union that includes Uint8Array/ArrayBuffer or implement an
environment-aware variant that validates Buffer in Node and
Uint8Array/ArrayBuffer in the DOM; ensure the change is applied where
DarFileSchema is exported so callers accept both platform binary types.
TypeScript discriminated unions only provide compile-time checks. When configs are loaded dynamically at runtime (from JSON, env vars, etc.), malformed configs with grantType: 'password' but missing username/password won't be caught. Without runtime validation, URLSearchParams.append() would convert undefined to 'undefined' string, causing confusing auth failures. This restores the runtime validation that was previously removed.
|
Claude finished @HardlyDifficult's task —— View job PR Review: SDK Strict Typing
|
| Criteria | Rating | Notes |
|---|---|---|
| Code Quality | 🟢 Excellent | Clean discriminated unions, proper type guards, fixed runtime validation |
| Type Safety | 🟢 Excellent | Significant improvement in type correctness |
| Breaking Changes | 🟡 Moderate | Well-justified but may affect consumers |
| Performance | 🟢 Good | Reduced overhead, efficient validation |
| Security | 🟢 Good | Stricter validation, better input handling |
| Test Coverage | 🟢 Good | Appropriate cleanup of deprecated tests |
✨ Overall Verdict
Strong approval ⭐ - This PR significantly improves the SDK's type safety and code quality. The strict typing approach will prevent many runtime errors and improve developer experience. The removal of deprecated patterns is well-executed and the discriminated union approach is a best practice.
Key Strengths:
- Fixes critical runtime validation issues that were identified by other reviewers
- Clean architectural improvements with discriminated unions
- Maintains backward compatibility where sensible
- Removes technical debt while improving type safety
The PR successfully addresses the main concerns from other reviews and represents a solid step forward in code quality and type safety.
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
|
…ssword Restore original credential priority: clientSecret (client_credentials grant) takes precedence over username/password (password grant). This prevents silent authentication method changes for users with both credential types.
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
|
|
Claude finished @HardlyDifficult's task —— View job PR Review: SDK Strict TypingComprehensive review of strict typing improvements and deprecated pattern removal. Todo List
🎯 SummaryThis PR successfully enforces strict typing throughout the Canton Node SDK by:
The changes are well-structured and improve code quality while maintaining backward compatibility where needed. ✅ Positive Changes1. Type System Improvements
2. Code Simplification
3. Dead Code Removal
4. Schema Strictness
|
| Criteria | Rating | Notes |
|---|---|---|
| Code Quality | 🟢 Excellent | Clean discriminated unions, proper type guards, fixed runtime validation |
| Type Safety | 🟢 Excellent | Significant improvement in type correctness |
| Breaking Changes | 🟡 Moderate | Well-justified but may affect consumers |
| Performance | 🟢 Good | Reduced overhead, efficient validation |
| Security | 🟢 Good | Stricter validation, better input handling |
| Test Coverage | 🟢 Good | Appropriate cleanup of deprecated tests |
✨ Overall Verdict
Strong approval ⭐ - This PR significantly improves the SDK's type safety and code quality. The strict typing approach will prevent many runtime errors and improve developer experience. The removal of deprecated patterns is well-executed and the discriminated union approach is a best practice.
Key Strengths:
- Fixes critical runtime validation issues that were identified by other reviewers
- Clean architectural improvements with discriminated unions
- Maintains backward compatibility where sensible
- Removes technical debt while improving type safety
The PR successfully addresses the main concerns from other reviews and represents a solid step forward in code quality and type safety.
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 (2)
src/core/config/EnvLoader.ts (2)
92-92: 🧹 Nitpick | 🔵 TrivialRedundant fallback:
provideris guaranteed truthy here.After the throw on lines 84-88,
provideris guaranteed to be a non-empty string. The|| undefinedfallback is unreachable.🧹 Suggested cleanup
- const apiConfig = envLoader.loadApiConfig(apiType, network, provider || undefined); + const apiConfig = envLoader.loadApiConfig(apiType, network, provider);
120-131:⚠️ Potential issue | 🟡 MinorInconsistent localnet handling between
getConfigandgetConfigSummary.
getConfig(lines 73-82) defaultsproviderto'app-provider'for localnet when not specified. However,getConfigSummaryat line 131 callsgetCurrentProvider()directly, which throws ifCANTON_CURRENT_PROVIDERis missing.This inconsistency means a user debugging configuration on localnet could have
getConfigsucceed butgetConfigSummarythrow an error.🔧 Suggested fix to align behavior
public static getConfigSummary( apiType: ApiType, options?: { network?: NetworkType; provider?: ProviderType } ): { network: NetworkType; provider?: ProviderType; envVars: Record<string, string | undefined>; missingVars: string[]; } { const envLoader = EnvLoader.getInstance(); const network = options?.network ?? envLoader.getCurrentNetwork(); - const provider = options?.provider ?? envLoader.getCurrentProvider(); + + // Get provider, with special handling for localnet (matching getConfig behavior) + let provider = options?.provider; + if (!provider) { + if (network === 'localnet') { + provider = 'app-provider'; + } else { + provider = envLoader.getCurrentProvider(); + } + }
… redundant fallback - getConfigSummary: default provider to 'app-provider' for localnet (matching getConfig behavior) - getConfig: remove redundant 'provider || undefined' (provider is guaranteed truthy after validation) Co-authored-by: HardlyDifficult <hardlydiff@gmail.com>
|
Claude finished @HardlyDifficult's task —— View job PR Review: SDK Strict TypingComprehensive review of strict typing improvements and deprecated pattern removal. Todo List
🎯 SummaryThis PR successfully enforces strict typing throughout the Canton Node SDK by:
The changes are well-structured and improve code quality while maintaining backward compatibility where needed. ✅ Positive Changes1. Type System Improvements
2. Code Simplification
3. Dead Code Removal
4. Schema Strictness
|
| Criteria | Rating | Notes |
|---|---|---|
| Code Quality | 🟢 Excellent | Clean discriminated unions, proper type guards, fixed runtime validation |
| Type Safety | 🟢 Excellent | Significant improvement in type correctness |
| Breaking Changes | 🟡 Moderate | Well-justified but may affect consumers |
| Performance | 🟢 Good | Reduced overhead, efficient validation |
| Security | 🟢 Good | Stricter validation, better input handling |
| Test Coverage | 🟢 Good | Appropriate cleanup of deprecated tests |
✨ Overall Verdict
Strong approval ⭐ - This PR significantly improves the SDK's type safety and code quality. The strict typing approach will prevent many runtime errors and improve developer experience. The removal of deprecated patterns is well-executed and the discriminated union approach is a best practice.
Key Strengths:
- Fixes critical runtime validation issues that were identified by other reviewers
- Clean architectural improvements with discriminated unions
- Maintains backward compatibility where sensible
- Removes technical debt while improving type safety
The PR successfully addresses the main concerns from other reviews and represents a solid step forward in code quality and type safety.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/core/config/EnvLoader.ts`:
- Around line 456-475: The current else branch constructs a client_credentials
AuthConfig without credentials; change the fallback so you fail fast instead of
returning an invalid auth: in EnvLoader.ts, inside the block that sets "auth:
AuthConfig" (symbols: auth, AuthConfig, clientSecret, username, password,
grantType 'client_credentials'), replace the final else branch so it does NOT
create a client_credentials object but instead returns undefined (or throws)
from the loader so getConfig can surface missing vars immediately; ensure
callers that expect AuthConfig (e.g., getConfig) handle the undefined/exception
path accordingly.
| // Priority: clientSecret (client_credentials) > username/password (password grant) | ||
| let auth: AuthConfig; | ||
|
|
||
| if (clientSecret) { | ||
| // Use client_credentials if client secret is available | ||
| grantType = 'client_credentials'; | ||
| auth = { | ||
| grantType, | ||
| grantType: 'client_credentials', | ||
| clientId, | ||
| clientSecret, | ||
| }; | ||
| } else if (username && password) { | ||
| // Use password grant if username and password are available | ||
| grantType = 'password'; | ||
| auth = { | ||
| grantType, | ||
| grantType: 'password', | ||
| clientId, | ||
| username, | ||
| password, | ||
| }; | ||
| } else { | ||
| // Fallback to client_credentials without secret (some providers may not require it) | ||
| grantType = 'client_credentials'; | ||
| auth = { | ||
| grantType, | ||
| grantType: 'client_credentials', | ||
| clientId, |
There was a problem hiding this comment.
Fail fast when no auth credentials are provided.
Line 472-476 currently builds a client_credentials auth without clientSecret or username/password, allowing a misconfigured env to pass and fail later. Return undefined (or throw) so getConfig can surface missing vars immediately.
🛠️ Proposed fix
- } else {
- auth = {
- grantType: 'client_credentials',
- clientId,
- };
- }
+ } else {
+ return undefined;
+ }📝 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.
| // Priority: clientSecret (client_credentials) > username/password (password grant) | |
| let auth: AuthConfig; | |
| if (clientSecret) { | |
| // Use client_credentials if client secret is available | |
| grantType = 'client_credentials'; | |
| auth = { | |
| grantType, | |
| grantType: 'client_credentials', | |
| clientId, | |
| clientSecret, | |
| }; | |
| } else if (username && password) { | |
| // Use password grant if username and password are available | |
| grantType = 'password'; | |
| auth = { | |
| grantType, | |
| grantType: 'password', | |
| clientId, | |
| username, | |
| password, | |
| }; | |
| } else { | |
| // Fallback to client_credentials without secret (some providers may not require it) | |
| grantType = 'client_credentials'; | |
| auth = { | |
| grantType, | |
| grantType: 'client_credentials', | |
| clientId, | |
| // Priority: clientSecret (client_credentials) > username/password (password grant) | |
| let auth: AuthConfig; | |
| if (clientSecret) { | |
| auth = { | |
| grantType: 'client_credentials', | |
| clientId, | |
| clientSecret, | |
| }; | |
| } else if (username && password) { | |
| auth = { | |
| grantType: 'password', | |
| clientId, | |
| username, | |
| password, | |
| }; | |
| } else { | |
| return undefined; | |
| } |
🤖 Prompt for AI Agents
In `@src/core/config/EnvLoader.ts` around lines 456 - 475, The current else branch
constructs a client_credentials AuthConfig without credentials; change the
fallback so you fail fast instead of returning an invalid auth: in EnvLoader.ts,
inside the block that sets "auth: AuthConfig" (symbols: auth, AuthConfig,
clientSecret, username, password, grantType 'client_credentials'), replace the
final else branch so it does NOT create a client_credentials object but instead
returns undefined (or throws) from the loader so getConfig can surface missing
vars immediately; ensure callers that expect AuthConfig (e.g., getConfig) handle
the undefined/exception path accordingly.
When auth config is constructed from external sources (JSON parsing, environment variables, dynamic objects), TypeScript compile-time checks don't apply. Without runtime validation, a missing grantType would result in 'grant_type=undefined' being sent to the OAuth server, causing confusing authentication errors. This fix adds explicit runtime validation for grantType before using it in the OAuth request, providing a clear error message when the field is missing.
|
Claude finished @HardlyDifficult's task —— View job PR Review: SDK Strict TypingComprehensive review of strict typing improvements and deprecated pattern removal. Todo List
🎯 SummaryThis PR successfully enforces strict typing throughout the Canton Node SDK by:
The changes are well-structured and improve code quality while maintaining backward compatibility where needed. ✅ Positive Changes1. Type System Improvements
2. Code Simplification
3. Dead Code Removal
4. Schema Strictness
|
| Criteria | Rating | Notes |
|---|---|---|
| Code Quality | 🟢 Excellent | Clean discriminated unions, proper type guards, fixed runtime validation |
| Type Safety | 🟢 Excellent | Significant improvement in type correctness |
| Breaking Changes | 🟡 Moderate | Well-justified but may affect consumers |
| Performance | 🟢 Good | Reduced overhead, efficient validation |
| Security | 🟢 Good | Stricter validation, better input handling |
| Test Coverage | 🟢 Good | Appropriate cleanup of deprecated tests |
✨ Overall Verdict
Strong approval ⭐ - This PR significantly improves the SDK's type safety and code quality. The strict typing approach will prevent many runtime errors and improve developer experience. The removal of deprecated patterns is well-executed and the discriminated union approach is a best practice.
Key Strengths:
- Fixes critical runtime validation issues that were identified by other reviewers
- Clean architectural improvements with discriminated unions
- Maintains backward compatibility where sensible
- Removes technical debt while improving type safety
The PR successfully addresses the main concerns from other reviews and represents a solid step forward in code quality and type safety.
**
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
|
|
Claude finished @HardlyDifficult's task —— View job PR Review: SDK Strict TypingComprehensive review of strict typing improvements and deprecated pattern removal. Todo List
🎯 SummaryThis PR successfully enforces strict typing throughout the Canton Node SDK by:
The changes are well-structured and improve code quality while maintaining backward compatibility where needed. ✅ Positive Changes1. Type System Improvements
2. Code Simplification
3. Dead Code Removal
4. Schema Strictness
|
| Criteria | Rating | Notes |
|---|---|---|
| Code Quality | 🟢 Excellent | Clean discriminated unions, proper type guards, fixed runtime validation |
| Type Safety | 🟢 Excellent | Significant improvement in type correctness |
| Breaking Changes | 🟡 Moderate | Well-justified but may affect consumers |
| Performance | 🟢 Good | Reduced overhead, efficient validation |
| Security | 🟢 Good | Stricter validation, better input handling |
| Test Coverage | 🟢 Good | Appropriate cleanup of deprecated tests |
✨ Overall Verdict
Strong approval ⭐ - This PR significantly improves the SDK's type safety and code quality. The strict typing approach will prevent many runtime errors and improve developer experience. The removal of deprecated patterns is well-executed and the discriminated union approach is a best practice.
Key Strengths:
- Fixes critical runtime validation issues that were identified by other reviewers
- Clean architectural improvements with discriminated unions
- Maintains backward compatibility where sensible
- Removes technical debt while improving type safety
The PR successfully addresses the main concerns from other reviews and represents a solid step forward in code quality and type safety.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/core/auth/AuthenticationManager.ts`:
- Around line 172-198: The validateAuthConfig method currently checks grantType,
clientId, and password-grant fields but omits checking clientSecret for the
client_credentials grant; add a runtime guard inside validateAuthConfig: when
this.authConfig.grantType === 'client_credentials' ensure
this.authConfig.clientSecret is present and throw an AuthenticationError with a
clear message (similar style to the existing password branch) if missing,
referencing the authConfig.grantType and required field name; update the
missing-field message to match existing format used for username/password and
reuse the AuthenticationError thrown by validateAuthConfig.
| private validateAuthConfig(): void { | ||
| const missingConfig: string[] = []; | ||
|
|
||
| if (!this.authConfig.clientId) missingConfig.push('clientId'); | ||
| if (!this.authConfig.grantType) missingConfig.push('grantType'); | ||
|
|
||
| // Check for grant type specific requirements | ||
| if (this.authConfig.grantType === 'password') { | ||
| if (!this.authConfig.username) missingConfig.push('username'); | ||
| if (!this.authConfig.password) missingConfig.push('password'); | ||
| // Runtime validation for grantType (TypeScript only provides compile-time checks) | ||
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- runtime guard for external config sources | ||
| if (!this.authConfig.grantType) { | ||
| throw new AuthenticationError(`Authentication configuration incomplete. Missing required field: grantType.`); | ||
| } | ||
| // Note: client_credentials grant type may not always require client_secret | ||
| // Some providers allow client_credentials without secret for public clients | ||
|
|
||
| if (missingConfig.length > 0) { | ||
| if (!this.authConfig.clientId) { | ||
| throw new AuthenticationError( | ||
| `Authentication configuration incomplete. Missing required fields: ${missingConfig.join(', ')}. ` + | ||
| `Authentication configuration incomplete. Missing required field: clientId. ` + | ||
| `Grant Type: ${this.authConfig.grantType}` | ||
| ); | ||
| } | ||
| // Runtime validation for password grant type (TypeScript only provides compile-time checks) | ||
| if (this.authConfig.grantType === 'password') { | ||
| const missingFields: string[] = []; | ||
| if (!this.authConfig.username) { | ||
| missingFields.push('username'); | ||
| } | ||
| if (!this.authConfig.password) { | ||
| missingFields.push('password'); | ||
| } | ||
| if (missingFields.length > 0) { | ||
| throw new AuthenticationError( | ||
| `Authentication configuration incomplete for password grant type. Missing required field(s): ${missingFields.join(', ')}.` | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add runtime validation for clientSecret in client_credentials grant.
client_credentials configs missing clientSecret will pass runtime validation and fail later with opaque auth errors. This contradicts the PR’s “fail fast” intent. Add a guard similar to the password-grant check.
🔧 Suggested fix
if (!this.authConfig.clientId) {
throw new AuthenticationError(
`Authentication configuration incomplete. Missing required field: clientId. ` +
`Grant Type: ${this.authConfig.grantType}`
);
}
+ if (this.authConfig.grantType === 'client_credentials' && !this.authConfig.clientSecret) {
+ throw new AuthenticationError(
+ `Authentication configuration incomplete for client_credentials grant type. Missing required field: clientSecret.`
+ );
+ }
// Runtime validation for password grant type (TypeScript only provides compile-time checks)
if (this.authConfig.grantType === 'password') {📝 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.
| private validateAuthConfig(): void { | |
| const missingConfig: string[] = []; | |
| if (!this.authConfig.clientId) missingConfig.push('clientId'); | |
| if (!this.authConfig.grantType) missingConfig.push('grantType'); | |
| // Check for grant type specific requirements | |
| if (this.authConfig.grantType === 'password') { | |
| if (!this.authConfig.username) missingConfig.push('username'); | |
| if (!this.authConfig.password) missingConfig.push('password'); | |
| // Runtime validation for grantType (TypeScript only provides compile-time checks) | |
| // eslint-disable-next-line @typescript-eslint/no-unnecessary-condition -- runtime guard for external config sources | |
| if (!this.authConfig.grantType) { | |
| throw new AuthenticationError(`Authentication configuration incomplete. Missing required field: grantType.`); | |
| } | |
| // Note: client_credentials grant type may not always require client_secret | |
| // Some providers allow client_credentials without secret for public clients | |
| if (missingConfig.length > 0) { | |
| if (!this.authConfig.clientId) { | |
| throw new AuthenticationError( | |
| `Authentication configuration incomplete. Missing required fields: ${missingConfig.join(', ')}. ` + | |
| `Authentication configuration incomplete. Missing required field: clientId. ` + | |
| `Grant Type: ${this.authConfig.grantType}` | |
| ); | |
| } | |
| // Runtime validation for password grant type (TypeScript only provides compile-time checks) | |
| if (this.authConfig.grantType === 'password') { | |
| const missingFields: string[] = []; | |
| if (!this.authConfig.username) { | |
| missingFields.push('username'); | |
| } | |
| if (!this.authConfig.password) { | |
| missingFields.push('password'); | |
| } | |
| if (missingFields.length > 0) { | |
| throw new AuthenticationError( | |
| `Authentication configuration incomplete for password grant type. Missing required field(s): ${missingFields.join(', ')}.` | |
| ); | |
| } | |
| } | |
| private validateAuthConfig(): void { | |
| // Runtime validation for grantType (TypeScript only provides compile-time checks) | |
| // eslint-disable-next-line `@typescript-eslint/no-unnecessary-condition` -- runtime guard for external config sources | |
| if (!this.authConfig.grantType) { | |
| throw new AuthenticationError(`Authentication configuration incomplete. Missing required field: grantType.`); | |
| } | |
| if (!this.authConfig.clientId) { | |
| throw new AuthenticationError( | |
| `Authentication configuration incomplete. Missing required field: clientId. ` + | |
| `Grant Type: ${this.authConfig.grantType}` | |
| ); | |
| } | |
| if (this.authConfig.grantType === 'client_credentials' && !this.authConfig.clientSecret) { | |
| throw new AuthenticationError( | |
| `Authentication configuration incomplete for client_credentials grant type. Missing required field: clientSecret.` | |
| ); | |
| } | |
| // Runtime validation for password grant type (TypeScript only provides compile-time checks) | |
| if (this.authConfig.grantType === 'password') { | |
| const missingFields: string[] = []; | |
| if (!this.authConfig.username) { | |
| missingFields.push('username'); | |
| } | |
| if (!this.authConfig.password) { | |
| missingFields.push('password'); | |
| } | |
| if (missingFields.length > 0) { | |
| throw new AuthenticationError( | |
| `Authentication configuration incomplete for password grant type. Missing required field(s): ${missingFields.join(', ')}.` | |
| ); | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/core/auth/AuthenticationManager.ts` around lines 172 - 198, The
validateAuthConfig method currently checks grantType, clientId, and
password-grant fields but omits checking clientSecret for the client_credentials
grant; add a runtime guard inside validateAuthConfig: when
this.authConfig.grantType === 'client_credentials' ensure
this.authConfig.clientSecret is present and throw an AuthenticationError with a
clear message (similar style to the existing password branch) if missing,
referencing the authConfig.grantType and required field name; update the
missing-field message to match existing format used for username/password and
reuse the AuthenticationError thrown by validateAuthConfig.
Since getPartyId() now throws ConfigurationError instead of returning an empty string, the partyId variable is guaranteed to be a non-empty string after the call. The 'partyId &&' truthiness check was therefore dead code.
|
Claude finished @HardlyDifficult's task —— View job PR Review: SDK Strict TypingComprehensive review of strict typing improvements and deprecated pattern removal. Todo List
🎯 SummaryThis PR successfully enforces strict typing throughout the Canton Node SDK by:
The changes are well-structured and improve code quality while maintaining backward compatibility where needed. ✅ Positive Changes1. Type System Improvements
2. Code Simplification
3. Dead Code Removal
4. Schema Strictness
|
| Criteria | Rating | Notes |
|---|---|---|
| Code Quality | 🟢 Excellent | Clean discriminated unions, proper type guards, fixed runtime validation |
| Type Safety | 🟢 Excellent | Significant improvement in type correctness |
| Breaking Changes | 🟡 Moderate | Well-justified but may affect consumers |
| Performance | 🟢 Good | Reduced overhead, efficient validation |
| Security | 🟢 Good | Stricter validation, better input handling |
| Test Coverage | 🟢 Good | Appropriate cleanup of deprecated tests |
✨ Overall Verdict
Strong approval ⭐ - This PR significantly improves the SDK's type safety and code quality. The strict typing approach will prevent many runtime errors and improve developer experience. The removal of deprecated patterns is well-executed and the discriminated union approach is a best practice.
Key Strengths:
- Fixes critical runtime validation issues that were identified by other reviewers
- Clean architectural improvements with discriminated unions
- Maintains backward compatibility where sensible
- Removes technical debt while improving type safety
The PR successfully addresses the main concerns from other reviews and represents a solid step forward in code quality and type safety.
|
Bugbot Autofix prepared fixes for 1 of the 1 bugs found in the latest run.
|
2 participants
Enforce strict typing and remove deprecated fields and fallback mechanisms to improve client correctness and simplify the SDK.
Note
Medium Risk
This is a breaking-change-style tightening (required params, stricter schemas, and new runtime throws) across core client/auth and ledger schemas, which may break existing integrations that relied on optional fields or permissive defaults.
Overview
The SDK is made strictly typed end-to-end by removing fallback/loose behaviors and requiring callers to provide previously-optional values.
Core client behavior is tightened:
BaseClientnow throws whenapiUrl/partyIdare missing (instead of returning empty strings), request helpers consistently use the sharedRequestConfig,HttpClient.setRetryConfignow requires a full config object, and OAuth2 config is a discriminated union with stricter runtime validation inAuthenticationManager.Ledger JSON API types/schemas are aligned to stricter inputs: deprecated
filter/verbosefields are removed from active-contracts request shapes, DAR uploads now require a binaryBuffer(no base64/string), andAllocatePartyenforces requiredsynchronizerId/userIdand passeslocalMetadatawithout defaulting/patching missing subfields. WebSocket typing is simplified (WebSocketHandlersonly acceptsError), generated WS client methods stop casting params/handlers toany, and shared event type guards are centralized/exported for reuse.Written by Cursor Bugbot for commit b28e014. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Bug Fixes
New Features
Chores