Add exact public configuration typing and runtime validation - #417
HardlyDifficult wants to merge 54 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds typed configuration contracts, immutable runtime snapshots, strict environment and client validation, atomic factory overrides, safer observability handling, explicit error property types, snapshot-based command execution, and comprehensive runtime and exact-optional-property tests. ChangesConfiguration and client safety
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant Caller
participant OcpClient
participant Environment
participant AuthorizeIssuer
participant Ledger
participant Observability
Caller->>OcpClient: construct with validated dependencies and options
OcpClient->>Environment: validate and resolve environment
Environment-->>OcpClient: immutable resolved configuration
Caller->>OcpClient: authorize issuer with optional factory
OcpClient->>AuthorizeIssuer: pass snapshotted issuer and factory
AuthorizeIssuer->>Ledger: resolve network and submit authorization transaction
Ledger-->>AuthorizeIssuer: transaction result or error
AuthorizeIssuer->>Observability: record bounded submission diagnostics
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
CI is green on the latest head, including the full OCP QuickStart integration suite. @copilot review |
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Pull request overview
This PR tightens the public TypeScript configuration/client/error surface (notably for exactOptionalPropertyTypes) by separating caller input vs resolved runtime configuration, restructuring authentication and factory coordinates into more explicit/atomic shapes, and adding declaration-level compile-time probes to keep the published .d.ts contracts exact.
Changes:
- Introduces stricter/discriminated environment configuration inputs and fully resolved runtime config, including a dedicated
clientOptionspublic contract. - Reworks issuer authorization to use an atomic
factoryoverride object, and makesOcpClientruntime state properties explicit (T | undefined) instead of optional. - Adds
exactOptionalPropertyTypesdeclaration/probe tests (source + built) and wires them into CI/linting.
Reviewed changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| tsconfig.tests.json | Excludes test/exact/** from the normal test TS project. |
| tsconfig.exact-public-config-tests.json | Adds an exact-optional TS project used for public surface compile-time probes. |
| package.json | Adds test:exact-public-config and runs it as part of test:declarations. |
| eslint.config.mjs | Adds the new exact-public-config tsconfig to ESLint’s project list. |
| test/functions/issuerAuthorization/authorizeIssuer.test.ts | Adds runtime tests ensuring malformed factory overrides fail before ledger access. |
| test/exact/sourcePublicConfig.types.ts | Adds source-level .d.ts/type-shape probes for exact-optional public contracts. |
| test/exact/builtPublicConfig.types.ts | Adds built (dist) public surface probes to ensure published types stay exact. |
| test/errors/errors.test.ts | Strengthens error-shape expectations (explicit ` |
| test/config/environment.test.ts | Adds assertions for resolved discriminated auth state + Canton config conversion behavior. |
| test/client/OcpClient.test.ts | Updates client authorization tests for atomic factory override + freezing semantics. |
| src/OcpClient.ts | Splits options into clientOptions, makes runtime state explicit, freezes factory coords, and updates issuer authorization flow. |
| src/clientOptions.ts | Introduces exported public client construction option types (deps, presets, env overrides, factory coords). |
| src/environment.ts | Refactors configuration into input vs resolved discriminated unions; adds explicit resolved invariants. |
| src/index.ts | Re-exports clientOptions from the package entrypoint. |
| src/observabilityTypes.ts | Extracts observability-related type declarations into a dedicated module. |
| src/observability.ts | Re-exports observability types from observabilityTypes while keeping implementation here. |
| src/functions/OpenCapTable/issuerAuthorization/types.ts | Switches authorization params to atomic factory?: OcpFactoryCoordinates and updates observability type import. |
| src/functions/OpenCapTable/issuerAuthorization/authorizeIssuer.ts | Updates authorization runtime validation and factory coordinate selection. |
| src/errors/OcpError.ts | Makes cause/classification/context explicit ` |
| src/errors/OcpContractError.ts | Uses contextOrUndefined to omit empty context and makes metadata fields explicit ` |
| src/errors/OcpNetworkError.ts | Uses contextOrUndefined and makes endpoint/status explicit ` |
| src/errors/OcpParseError.ts | Uses contextOrUndefined and makes source explicit ` |
| src/errors/OcpValidationError.ts | Makes expectedType explicit ` |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…e' into codex/exact-public-config-state
…e' into codex/exact-public-config-state # Conflicts: # scripts/check-declarations.ts # src/functions/OpenCapTable/issuerAuthorization/types.ts
…e' into codex/exact-public-config-state
…e' into codex/exact-public-config-state
|
@coderabbitai review |
|
cursor review |
|
@copilot review |
✅ Action performedReview finished.
|
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 567d4df. Configure here.
|
@copilot review Please confirm the exact current head |
|
@copilot review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
src/OcpClient.ts (1)
1030-1047: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
dependencies.environmentin this validation error.
validateInjectedEnvironmentis only used on the injected-dependencies path, so this should reportdependencies.environmentto match the other validation errors and keep path-based filtering consistent.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/OcpClient.ts` around lines 1030 - 1047, Update validateInjectedEnvironment to use the validation path dependencies.environment instead of environment when constructing OcpValidationError, while preserving the existing network mismatch check and error details.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/functions/OpenCapTable/capTable/CapTableBatch.ts`:
- Around line 69-71: Update createUpdateCapTableCommandId to use
crypto.randomUUID() for the fallback command ID instead of combining Date.now()
with Math.random(), while preserving the existing update-captable prefix.
---
Outside diff comments:
In `@src/OcpClient.ts`:
- Around line 1030-1047: Update validateInjectedEnvironment to use the
validation path dependencies.environment instead of environment when
constructing OcpValidationError, while preserving the existing network mismatch
check and error details.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 257ed605-41d8-4a9f-932c-1db631805e2d
📒 Files selected for processing (50)
eslint.config.mjspackage.jsonscripts/check-declarations.tssrc/OcpClient.tssrc/clientOptions.tssrc/environment.tssrc/errors/OcpContractError.tssrc/errors/OcpError.tssrc/errors/OcpNetworkError.tssrc/errors/OcpParseError.tssrc/errors/OcpValidationError.tssrc/functions/OpenCapTable/capTable/CapTableBatch.tssrc/functions/OpenCapTable/capTable/archiveCapTable.tssrc/functions/OpenCapTable/capTable/buildCapTableCommand.tssrc/functions/OpenCapTable/factory/createFactory.tssrc/functions/OpenCapTable/issuerAuthorization/authorizeIssuer.tssrc/functions/OpenCapTable/issuerAuthorization/types.tssrc/functions/OpenCapTable/issuerAuthorization/withdrawAuthorization.tssrc/index.tssrc/observability.tssrc/observabilityTypes.tssrc/types/common.tssrc/utils/commandContext.tssrc/utils/commandParameters.tssrc/utils/environmentConfigKeys.tssrc/utils/exactObject.tssrc/utils/factoryCoordinates.tssrc/utils/observabilityConfig.tstest/batch/CapTableBatch.test.tstest/capTable/archiveCapTable.test.tstest/client/OcpClient.test.tstest/config/environment.test.tstest/declarations/publicApi.types.tstest/declarations/sourceClientConfig.types.tstest/errors/errors.test.tstest/exact/builtPublicConfig.types.tstest/exact/sourcePublicConfig.types.tstest/functions/factory/createFactory.test.tstest/functions/issuerAuthorization/authorizeIssuer.test.tstest/functions/issuerAuthorization/withdrawAuthorization.test.tstest/mocks/fairmint-canton-node-sdk.tstest/observability/observability.test.tstest/publicApi/rootExports.test.tstest/typeContracts/typeAssertions.tstest/types/capTableBatch.types.tstest/types/typeAssertions.types.tstest/utils/factoryCoordinates.test.tstest/validation/generatedDamlBoundary.test.tstsconfig.exact-public-config-tests.jsontsconfig.tests.json
|
Returned to draft: the fresh exact-head CodeRabbit full review identified a valid command-ID collision risk in the |
|
@copilot review Please review exact current head |
Reviewed exact head I also re-checked required workflows in stacked order and both are green:
|
|
@cursor review Please review exact head |
|
@coderabbitai full review Please perform a fresh full review of exact head |
|
If adaptive rate limiting is still in effect for this attempt, that status will be reported once the action completes. ✅ Action performedFull review finished. You're currently rate limited under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. Your next review will be available in 6 minutes. |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 84048b0. Configure here.
|
@coderabbitai full review Retry after the adaptive-limit window. Please perform a fresh full review of unchanged exact head |
|
If adaptive rate limiting is still in effect for this attempt, that status will be reported once the action completes. ✅ Action performedFull review finished. You're currently rate limited under our Fair Usage Limits Policy. Your recent PR review activity is in the 95th percentile or higher among CodeRabbit users, so adaptive limits apply. Your next review will be available in 25 seconds. |
|
@coderabbitai full review Retry after the reported 25-second adaptive-limit window. Please perform the fresh full review of unchanged exact head |
|
If adaptive rate limiting is still in effect for this attempt, that status will be reported once the action completes. ✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/functions/OpenCapTable/capTable/archiveCapTable.ts`:
- Around line 27-36: Widen the public CapTableContractDetails type used by
ArchiveCapTableParams to include contractId, createdEventBlob, and
synchronizerId, matching the fields accepted by snapshotCapTableContractDetails
and cross-checked against capTableContractId. Preserve templateId and make the
added fields’ optionality match the existing supported input shape.
In `@test/functions/issuerAuthorization/authorizeIssuer.test.ts`:
- Around line 1-225: Add a test in the authorizeIssuer factory configuration
suite covering an explicit factory: undefined value. Assert it rejects with the
expected validation error before ledger access, matching the existing
null/incomplete factory validation behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 596a5ad2-027e-4ad5-bb9c-4ea545a4b840
📒 Files selected for processing (50)
eslint.config.mjspackage.jsonscripts/check-declarations.tssrc/OcpClient.tssrc/clientOptions.tssrc/environment.tssrc/errors/OcpContractError.tssrc/errors/OcpError.tssrc/errors/OcpNetworkError.tssrc/errors/OcpParseError.tssrc/errors/OcpValidationError.tssrc/functions/OpenCapTable/capTable/CapTableBatch.tssrc/functions/OpenCapTable/capTable/archiveCapTable.tssrc/functions/OpenCapTable/capTable/buildCapTableCommand.tssrc/functions/OpenCapTable/factory/createFactory.tssrc/functions/OpenCapTable/issuerAuthorization/authorizeIssuer.tssrc/functions/OpenCapTable/issuerAuthorization/types.tssrc/functions/OpenCapTable/issuerAuthorization/withdrawAuthorization.tssrc/index.tssrc/observability.tssrc/observabilityTypes.tssrc/types/common.tssrc/utils/commandContext.tssrc/utils/commandParameters.tssrc/utils/environmentConfigKeys.tssrc/utils/exactObject.tssrc/utils/factoryCoordinates.tssrc/utils/observabilityConfig.tstest/batch/CapTableBatch.test.tstest/capTable/archiveCapTable.test.tstest/client/OcpClient.test.tstest/config/environment.test.tstest/declarations/publicApi.types.tstest/declarations/sourceClientConfig.types.tstest/errors/errors.test.tstest/exact/builtPublicConfig.types.tstest/exact/sourcePublicConfig.types.tstest/functions/factory/createFactory.test.tstest/functions/issuerAuthorization/authorizeIssuer.test.tstest/functions/issuerAuthorization/withdrawAuthorization.test.tstest/mocks/fairmint-canton-node-sdk.tstest/observability/observability.test.tstest/publicApi/rootExports.test.tstest/typeContracts/typeAssertions.tstest/types/capTableBatch.types.tstest/types/typeAssertions.types.tstest/utils/factoryCoordinates.test.tstest/validation/generatedDamlBoundary.test.tstsconfig.exact-public-config-tests.jsontsconfig.tests.json
|
Returned to draft: the fresh exact-head CodeRabbit full review found two valid gaps. I am widening the public cap-table contract-details type to match the already-supported disclosure fields and adding low-level |
|
Review follow-up pushed at exact head |
|
@cursor review Please review exact current head |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 92875cf. Configure here.
|
@coderabbitai full review Please perform a substantive fresh full review of unchanged exact head |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/errors/OcpError.ts`:
- Around line 7-11: Consolidate the repeated diagnostic-context handling by
adding a shared helper alongside contextOrUndefined that merges context and
produces the optional details spread. Update OcpParseError, OcpNetworkError, and
OcpContractError to use this helper instead of independently calling
mergeDiagnosticContext, contextOrUndefined, and conditionally spreading context,
while preserving their existing super(...) details.
In `@src/functions/OpenCapTable/capTable/CapTableBatch.ts`:
- Around line 74-93: Extract the shared native-error/proxy detection from
safeBatchFailureMessage and safeBatchFailureCause into a
resolveNativeError(error): Error | undefined helper. Update both functions to
reuse this helper while preserving their existing message extraction and
fallback behavior, ensuring both consistently identify safe native errors.
In `@src/utils/commandContext.ts`:
- Around line 40-52: Consolidate the explicit-undefined validation in
optionalValue with the existing optionalCommandParameter/explicitUndefined logic
from commandParameters.ts, reusing the shared validation path and
toExactDataValidationError conventions. Remove the duplicated OcpValidationError
construction while preserving rejection of present-but-undefined values and
consistent error text and behavior across both paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6e233608-8423-4f43-9228-e11f85eea49f
📒 Files selected for processing (50)
eslint.config.mjspackage.jsonscripts/check-declarations.tssrc/OcpClient.tssrc/clientOptions.tssrc/environment.tssrc/errors/OcpContractError.tssrc/errors/OcpError.tssrc/errors/OcpNetworkError.tssrc/errors/OcpParseError.tssrc/errors/OcpValidationError.tssrc/functions/OpenCapTable/capTable/CapTableBatch.tssrc/functions/OpenCapTable/capTable/archiveCapTable.tssrc/functions/OpenCapTable/capTable/buildCapTableCommand.tssrc/functions/OpenCapTable/factory/createFactory.tssrc/functions/OpenCapTable/issuerAuthorization/authorizeIssuer.tssrc/functions/OpenCapTable/issuerAuthorization/types.tssrc/functions/OpenCapTable/issuerAuthorization/withdrawAuthorization.tssrc/index.tssrc/observability.tssrc/observabilityTypes.tssrc/types/common.tssrc/utils/commandContext.tssrc/utils/commandParameters.tssrc/utils/environmentConfigKeys.tssrc/utils/exactObject.tssrc/utils/factoryCoordinates.tssrc/utils/observabilityConfig.tstest/batch/CapTableBatch.test.tstest/capTable/archiveCapTable.test.tstest/client/OcpClient.test.tstest/config/environment.test.tstest/declarations/publicApi.types.tstest/declarations/sourceClientConfig.types.tstest/errors/errors.test.tstest/exact/builtPublicConfig.types.tstest/exact/sourcePublicConfig.types.tstest/functions/factory/createFactory.test.tstest/functions/issuerAuthorization/authorizeIssuer.test.tstest/functions/issuerAuthorization/withdrawAuthorization.test.tstest/mocks/fairmint-canton-node-sdk.tstest/observability/observability.test.tstest/publicApi/rootExports.test.tstest/typeContracts/typeAssertions.tstest/types/capTableBatch.types.tstest/types/typeAssertions.types.tstest/utils/factoryCoordinates.test.tstest/validation/generatedDamlBoundary.test.tstsconfig.exact-public-config-tests.jsontsconfig.tests.json
| /** @internal */ | ||
| export function contextOrUndefined(context: OcpErrorContext): OcpErrorContext | undefined { | ||
| return Object.keys(context).length === 0 ? undefined : context; | ||
| } | ||
|
|
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Consider consolidating the repeated "conditional context spread" boilerplate.
OcpParseError, OcpNetworkError, and OcpContractError all now repeat the identical pattern: compute context via mergeDiagnosticContext + contextOrUndefined, then conditionally spread { context } into the super(...) details. Centralizing this here alongside contextOrUndefined would remove the triplicated glue code and give a single place to change the pattern later.
♻️ Proposed consolidating helper
/** `@internal` */
export function contextOrUndefined(context: OcpErrorContext): OcpErrorContext | undefined {
return Object.keys(context).length === 0 ? undefined : context;
}
+
+/** `@internal` */
+export function buildErrorDetails(
+ classification: string,
+ context: OcpErrorContext
+): OcpErrorDetails {
+ const resolvedContext = contextOrUndefined(context);
+ return {
+ classification,
+ ...(resolvedContext !== undefined ? { context: resolvedContext } : {}),
+ };
+}Then each subclass (e.g. OcpNetworkError) simplifies to:
- const context = contextOrUndefined(
- mergeDiagnosticContext(options?.context, { endpoint, statusCode: options?.statusCode })
- );
- super(message, code, options?.cause, {
- classification: options?.classification ?? 'network_error',
- ...(context !== undefined ? { context } : {}),
- });
+ super(message, code, options?.cause, buildErrorDetails(
+ options?.classification ?? 'network_error',
+ mergeDiagnosticContext(options?.context, { endpoint, statusCode: options?.statusCode })
+ ));📝 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.
| /** @internal */ | |
| export function contextOrUndefined(context: OcpErrorContext): OcpErrorContext | undefined { | |
| return Object.keys(context).length === 0 ? undefined : context; | |
| } | |
| /** `@internal` */ | |
| export function contextOrUndefined(context: OcpErrorContext): OcpErrorContext | undefined { | |
| return Object.keys(context).length === 0 ? undefined : context; | |
| } | |
| /** `@internal` */ | |
| export function buildErrorDetails( | |
| classification: string, | |
| context: OcpErrorContext | |
| ): OcpErrorDetails { | |
| const resolvedContext = contextOrUndefined(context); | |
| return { | |
| classification, | |
| ...(resolvedContext !== undefined ? { context: resolvedContext } : {}), | |
| }; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/errors/OcpError.ts` around lines 7 - 11, Consolidate the repeated
diagnostic-context handling by adding a shared helper alongside
contextOrUndefined that merges context and produces the optional details spread.
Update OcpParseError, OcpNetworkError, and OcpContractError to use this helper
instead of independently calling mergeDiagnosticContext, contextOrUndefined, and
conditionally spreading context, while preserving their existing super(...)
details.
| function safeBatchFailureMessage(error: unknown): string { | ||
| const objectLike = (typeof error === 'object' && error !== null) || typeof error === 'function'; | ||
| if (objectLike && !nodeUtilTypes.isProxy(error) && nodeUtilTypes.isNativeError(error)) { | ||
| try { | ||
| const descriptor = Object.getOwnPropertyDescriptor(error, 'message'); | ||
| if (descriptor !== undefined && 'value' in descriptor && typeof descriptor.value === 'string') { | ||
| return toSafeDiagnosticText(descriptor.value); | ||
| } | ||
| } catch { | ||
| // Fall through to the bounded descriptor-safe diagnostic representation. | ||
| } | ||
| } | ||
| return toSafeDiagnosticText(error); | ||
| } | ||
|
|
||
| function safeBatchFailureCause(error: unknown): Error { | ||
| const objectLike = (typeof error === 'object' && error !== null) || typeof error === 'function'; | ||
| if (objectLike && !nodeUtilTypes.isProxy(error) && nodeUtilTypes.isNativeError(error)) return error; | ||
| return new Error(toSafeDiagnosticText(error)); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicated native-error/proxy detection between safeBatchFailureMessage and safeBatchFailureCause.
Both helpers independently recompute the same objectLike && !isProxy && isNativeError condition. Extracting a shared resolveNativeError(error): Error | undefined helper would remove the duplication and guarantee both functions always agree on what counts as a "safe" native error.
♻️ Suggested consolidation
+function resolveNativeError(error: unknown): Error | undefined {
+ const objectLike = (typeof error === 'object' && error !== null) || typeof error === 'function';
+ if (objectLike && !nodeUtilTypes.isProxy(error) && nodeUtilTypes.isNativeError(error)) return error;
+ return undefined;
+}
+
function safeBatchFailureMessage(error: unknown): string {
- const objectLike = (typeof error === 'object' && error !== null) || typeof error === 'function';
- if (objectLike && !nodeUtilTypes.isProxy(error) && nodeUtilTypes.isNativeError(error)) {
+ const nativeError = resolveNativeError(error);
+ if (nativeError !== undefined) {
try {
- const descriptor = Object.getOwnPropertyDescriptor(error, 'message');
+ const descriptor = Object.getOwnPropertyDescriptor(nativeError, 'message');
if (descriptor !== undefined && 'value' in descriptor && typeof descriptor.value === 'string') {
return toSafeDiagnosticText(descriptor.value);
}
} catch {
// Fall through to the bounded descriptor-safe diagnostic representation.
}
}
return toSafeDiagnosticText(error);
}
function safeBatchFailureCause(error: unknown): Error {
- const objectLike = (typeof error === 'object' && error !== null) || typeof error === 'function';
- if (objectLike && !nodeUtilTypes.isProxy(error) && nodeUtilTypes.isNativeError(error)) return error;
- return new Error(toSafeDiagnosticText(error));
+ return resolveNativeError(error) ?? new Error(toSafeDiagnosticText(error));
}📝 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.
| function safeBatchFailureMessage(error: unknown): string { | |
| const objectLike = (typeof error === 'object' && error !== null) || typeof error === 'function'; | |
| if (objectLike && !nodeUtilTypes.isProxy(error) && nodeUtilTypes.isNativeError(error)) { | |
| try { | |
| const descriptor = Object.getOwnPropertyDescriptor(error, 'message'); | |
| if (descriptor !== undefined && 'value' in descriptor && typeof descriptor.value === 'string') { | |
| return toSafeDiagnosticText(descriptor.value); | |
| } | |
| } catch { | |
| // Fall through to the bounded descriptor-safe diagnostic representation. | |
| } | |
| } | |
| return toSafeDiagnosticText(error); | |
| } | |
| function safeBatchFailureCause(error: unknown): Error { | |
| const objectLike = (typeof error === 'object' && error !== null) || typeof error === 'function'; | |
| if (objectLike && !nodeUtilTypes.isProxy(error) && nodeUtilTypes.isNativeError(error)) return error; | |
| return new Error(toSafeDiagnosticText(error)); | |
| } | |
| function resolveNativeError(error: unknown): Error | undefined { | |
| const objectLike = (typeof error === 'object' && error !== null) || typeof error === 'function'; | |
| if (objectLike && !nodeUtilTypes.isProxy(error) && nodeUtilTypes.isNativeError(error)) return error; | |
| return undefined; | |
| } | |
| function safeBatchFailureMessage(error: unknown): string { | |
| const nativeError = resolveNativeError(error); | |
| if (nativeError !== undefined) { | |
| try { | |
| const descriptor = Object.getOwnPropertyDescriptor(nativeError, 'message'); | |
| if (descriptor !== undefined && 'value' in descriptor && typeof descriptor.value === 'string') { | |
| return toSafeDiagnosticText(descriptor.value); | |
| } | |
| } catch { | |
| // Fall through to the bounded descriptor-safe diagnostic representation. | |
| } | |
| } | |
| return toSafeDiagnosticText(error); | |
| } | |
| function safeBatchFailureCause(error: unknown): Error { | |
| return resolveNativeError(error) ?? new Error(toSafeDiagnosticText(error)); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/functions/OpenCapTable/capTable/CapTableBatch.ts` around lines 74 - 93,
Extract the shared native-error/proxy detection from safeBatchFailureMessage and
safeBatchFailureCause into a resolveNativeError(error): Error | undefined
helper. Update both functions to reuse this helper while preserving their
existing message extraction and fallback behavior, ensuring both consistently
identify safe native errors.
| function optionalValue(snapshot: ExactObjectSnapshot, key: string, root: string): unknown { | ||
| if (!snapshot.has(key)) { | ||
| return undefined; | ||
| } | ||
| const value = snapshot.get(key); | ||
| if (value === undefined) { | ||
| throw new OcpValidationError(`${root}.${key}`, `${key} must be omitted rather than set to undefined.`, { | ||
| code: OcpErrorCodes.INVALID_TYPE, | ||
| expectedType: 'defined value or omitted property', | ||
| }); | ||
| } | ||
| return value; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicate "must be omitted rather than undefined" validation logic.
optionalValue here reimplements the same explicit-undefined rejection already present as optionalCommandParameter/explicitUndefined in commandParameters.ts (per graph context). Given the prior consolidation of ExactDataFailure→OcpValidationError mapping into toExactDataValidationError, this analogous "value present but undefined" check is a good candidate for the same treatment to keep error text/behavior from diverging between command-context and command-parameter paths.
♻️ Suggested consolidation
-function optionalValue(snapshot: ExactObjectSnapshot, key: string, root: string): unknown {
- if (!snapshot.has(key)) {
- return undefined;
- }
- const value = snapshot.get(key);
- if (value === undefined) {
- throw new OcpValidationError(`${root}.${key}`, `${key} must be omitted rather than set to undefined.`, {
- code: OcpErrorCodes.INVALID_TYPE,
- expectedType: 'defined value or omitted property',
- });
- }
- return value;
-}
+import { optionalCommandParameter } from './commandParameters';
+const optionalValue = optionalCommandParameter;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/commandContext.ts` around lines 40 - 52, Consolidate the
explicit-undefined validation in optionalValue with the existing
optionalCommandParameter/explicitUndefined logic from commandParameters.ts,
reusing the shared validation path and toExactDataValidationError conventions.
Remove the duplicated OcpValidationError construction while preserving rejection
of present-but-undefined values and consistent error text and behavior across
both paths.
|
Readiness retracted for the DAML-validation boundary cleanup. This PR will retain SDK-owned auth, environment, configuration, and command-context guarantees while removing abandoned stacked ancestry, then be revalidated and freshly reviewed on its exact resulting head before merge. |
Summary
exactOptionalPropertyTypesUpdateCapTablecommand IDs with cryptographic UUIDsValidation
npm run lintnpm run typechecknpm run buildnpm run test:declarationsnpm run test:exact-public-confignpm run test:ci— 74 suites / 4,078 tests passedStack
Depends on #416.
Note
Medium Risk
Touches client bootstrap, environment/auth resolution, and many command entry points; behavior changes mainly around invalid or hostile inputs, but mis-tuned validation could reject previously tolerated configs.
Overview
Tightens how callers configure
OcpClientand Canton environments, with stricter TypeScript unions and runtime guards that only accept plain own-data objects (no proxies, accessors, or explicitundefinedwhere omission is required).Configuration types move to
clientOptionsandenvironment: OAuth2 vs shared-secret are discriminated unions,stagingis added, and resolution/validation now throwsOcpValidationErrorwith structured issues instead of genericError.OcpClient.forStaging, preset helpers, andfromEnvwhitelist allowed keys per entry path and validate injected ledger/validator clients (including matching networks).OcpClientconstruction snapshots dependencies, factory coordinates, and observability; issuer authorization uses a single optionalfactoryobject instead of separate contract/template IDs. SharedinspectExactObject/ command-carrier helpers snapshot cap-table batch, archive, factory, and authorization parameters so later mutation or traps cannot affect submission.Public error and observability shapes are made explicit under
exactOptionalPropertyTypes(e.g.OcpValidationError.receivedValuealways declared asunknown | undefined). Declaration checks and a newtest:exact-public-configcompile step enforce that contract; batchcommandIdgeneration switches to UUIDs, and failure/observability paths avoid unsafe coercion on rejections.Reviewed by Cursor Bugbot for commit 92875cf. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
stagingenvironment preset support.Bug Fixes
undefinedvs omission” behavior.Tests
Chores