feat(telemetry): add anonymous usage tracking across W3C and OpenAttestation flows#136
Closed
ffeew-dex wants to merge 5 commits into
Closed
feat(telemetry): add anonymous usage tracking across W3C and OpenAttestation flows#136ffeew-dex wants to merge 5 commits into
ffeew-dex wants to merge 5 commits into
Conversation
- Capture anonymous issuance and verification events for credential and document flows so SDK usage can be measured without collecting PII - Persist a hashed random instance identifier and add opt-out controls for users who want telemetry disabled - Document the behavior, export the telemetry helpers, and disable telemetry during tests to keep runs deterministic
- Derive OpenAttestation telemetry from issuer identity proof so V2 and V3 documents report the expected DID method consistently - Derive W3C telemetry from the credential issuer instead of proof or key data so signing and verification emit the same method - Add coverage for issuance and verification telemetry across OA and W3C document variants
Contributor
📝 WalkthroughWalkthroughAdds an opt-in anonymous telemetry subsystem, with env and runtime opt-out, persistent per-install instance ID, non‑blocking timed POSTs to a configurable endpoint, type definitions and tests; integrates telemetry emissions into W3C and OpenAttestation sign/verify/derive flows and documents the feature. Changes
Sequence DiagramsequenceDiagram
participant App as Application
participant Func as Sign/Verify/Derive
participant Tel as Telemetry Module
participant Persist as Persist (fs / localStorage)
participant Remote as Telemetry Endpoint
App->>Func: invoke signW3C / signOA / verify / derive
Func->>Func: perform core operation
Func->>Tel: compute did_method & cryptosuite
Tel-->>Func: did_method
Func->>Tel: emitTelemetry({action_type, document_format, did_method, cryptosuite})
Tel->>Tel: isTelemetryEnabled()?
alt enabled
Tel->>Persist: getInstanceId()
Persist-->>Tel: instance_id
Tel->>Remote: POST /api/v1/telemetry (JSON, timeout 5s)
Remote-->>Tel: response / error
Tel-->>Tel: swallow errors
else disabled
Tel-->>Tel: skip emission
end
Func-->>App: return original result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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 docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (7)
CLAUDE.md (1)
59-59: Consider documenting telemetry behavior and opt-out controls.Line 59 mentions "analytics" under the utils module, which refers to the telemetry feature added in this PR. Since this file guides Claude Code on working with the repository, it would be helpful to document:
- What telemetry tracks (anonymous usage metrics)
- How to disable it (
TRUSTVC_TELEMETRY_DISABLED=1environment variable ordisableTelemetry()API)- That it's documented in the README
This would help developers quickly understand telemetry behavior when working with the codebase.
📝 Suggested addition
Consider adding a "Telemetry" section after the "Testing" section:
## Code Style + +## Telemetry + +The SDK includes optional anonymous usage tracking that emits non-blocking events for issuance and verification operations. Telemetry can be disabled via: +- Environment variable: `TRUSTVC_TELEMETRY_DISABLED=1` +- Programmatic API: `disableTelemetry()` / `enableTelemetry()` + +See README for full details on telemetry behavior and opt-out.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CLAUDE.md` at line 59, Add a short "Telemetry" note to the docs near the utils/ analytics entry describing that telemetry collects anonymous usage metrics, how to opt out via the TRUSTVC_TELEMETRY_DISABLED=1 environment variable or by calling the disableTelemetry() API, and point readers to the README for full details; mention that telemetry is anonymous and configurable and reference the utils/ analytics module and the disableTelemetry() function so developers can find the implementation.vitest.config.ts (1)
28-28: Drop theas anycast on test env object.Line 28 masks type issues and is flagged by static analysis as unnecessary. Prefer a typed object (or a narrow cast like
Record<string, string | undefined>) instead ofany.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vitest.config.ts` at line 28, The env object in vitest config uses an unnecessary `as any` cast; replace it with a properly typed object such as Record<string, string | undefined>. For example, construct a typed variable (e.g., `const testEnv: Record<string, string | undefined> = { ...process.env, TRUSTVC_TELEMETRY_DISABLED: '1' }`) and use `env: testEnv`, or cast process.env narrowly (`...(process.env as Record<string,string|undefined>)`) and remove the `as any` to satisfy static analysis while preserving the TRUSTVC_TELEMETRY_DISABLED entry.README.md (1)
1191-1195: Add language specifier to fenced code block.The
.envexample code block is missing a language specifier, which triggers a markdownlint warning.📝 Suggested fix
Or in a `.env` file: -``` +```env TRUSTVC_TELEMETRY_DISABLED=1</details> <details> <summary>🤖 Prompt for AI Agents</summary>Verify each finding against the current code and only fix it if needed.
In
@README.mdaround lines 1191 - 1195, The fenced code block showing the .env
example (the block containing TRUSTVC_TELEMETRY_DISABLED=1) lacks a language
specifier and triggers markdownlint; update that fenced block to include the env
language specifier (use ```env) so the block becomes a proper env-highlighted
code fence in README.md.</details> </blockquote></details> <details> <summary>src/utils/telemetry/telemetry.ts (3)</summary><blockquote> `92-96`: **Consider using optional chaining for cleaner code.** SonarCloud suggests using optional chaining for the nested property check. <details> <summary>📝 Suggested fix</summary> ```diff function isNodeEnvironment(): boolean { return ( - typeof process !== 'undefined' && process.versions != null && process.versions.node != null + typeof process !== 'undefined' && process.versions?.node != null ); } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/utils/telemetry/telemetry.ts` around lines 92 - 96, The isNodeEnvironment function uses explicit nested null checks for process.versions.node; replace the nested check with optional chaining to simplify the expression while keeping the typeof process !== 'undefined' guard: update the return expression in isNodeEnvironment to use process?.versions?.node != null so the logic is identical but cleaner and less error-prone. ``` </details> --- `98-100`: **Consider using `globalThis.window` for consistency.** SonarCloud suggests preferring `globalThis.window` over `window` for consistency with other global references in this file (e.g., `globalThis.crypto`, `globalThis.fetch`). <details> <summary>📝 Suggested fix</summary> ```diff function isBrowserEnvironment(): boolean { - return typeof window !== 'undefined' && typeof localStorage !== 'undefined'; + return typeof globalThis.window !== 'undefined' && typeof localStorage !== 'undefined'; } ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/utils/telemetry/telemetry.ts` around lines 98 - 100, The isBrowserEnvironment function currently uses raw globals; update it to check globalThis for consistency with other checks (e.g., globalThis.crypto, globalThis.fetch) by replacing references to window and localStorage with globalThis.window and globalThis.localStorage so the function uses typeof globalThis.window !== 'undefined' && typeof globalThis.localStorage !== 'undefined' (refer to the isBrowserEnvironment function to locate the change). ``` </details> --- `157-162`: **Consider using `export ... from` syntax for re-exports.** SonarCloud suggests using the `export ... from` syntax for cleaner re-exports. However, since `isTelemetryEnabled` and `getInstanceId` are defined as regular functions (not exported inline), this would require restructuring. The current approach is functional. <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/utils/telemetry/telemetry.ts` around lines 157 - 162, Change the module to use inline exports instead of a trailing re-export: mark the functions isTelemetryEnabled and getInstanceId with the export keyword at their declarations and export SDK_VERSION where it is defined, then remove the final "export { isTelemetryEnabled, getInstanceId, SDK_VERSION };" line; keep _resetForTesting and the internal symbols (_telemetryOverride, _instanceId) unchanged so tests still call _resetForTesting(). ``` </details> </blockquote></details> <details> <summary>src/utils/telemetry/telemetry.test.ts (1)</summary><blockquote> `1-2`: **Consolidate imports from the same module.** SonarCloud correctly flags that `'./telemetry'` is imported twice. Consolidating into a single import improves readability. <details> <summary>📝 Suggested fix</summary> ```diff -import { emitTelemetry, disableTelemetry, enableTelemetry, extractDidMethod } from './telemetry'; -import { _resetForTesting, isTelemetryEnabled, getInstanceId, SDK_VERSION } from './telemetry'; +import { + emitTelemetry, + disableTelemetry, + enableTelemetry, + extractDidMethod, + _resetForTesting, + isTelemetryEnabled, + getInstanceId, + SDK_VERSION, +} from './telemetry'; ``` </details> <details> <summary>🤖 Prompt for AI Agents</summary> ``` Verify each finding against the current code and only fix it if needed. In `@src/utils/telemetry/telemetry.test.ts` around lines 1 - 2, Consolidate the two imports from './telemetry' into a single import statement that includes all referenced symbols (emitTelemetry, disableTelemetry, enableTelemetry, extractDidMethod, _resetForTesting, isTelemetryEnabled, getInstanceId, SDK_VERSION); update the import lines so there is only one import from './telemetry' that lists these named exports to remove the duplicate import and improve readability. ``` </details> </blockquote></details> </blockquote></details> <details> <summary>🤖 Prompt for all review comments with AI agents</summary>Verify each finding against the current code and only fix it if needed.
Inline comments:
In@src/w3c/derive.ts:
- Around line 21-24: The did_method fallback currently only checks string
credential.issuer and thus misses object-form issuers; update the call site
using extractDidMethod (the expression starting with
credential.proof?.verificationMethod ?? ...) to also support credential.issuer
as an object by extracting credential.issuer.id when issuer is an object
(mirroring the behavior in signW3C/verifyW3CSignature), so that extractDidMethod
receives the verificationMethod or the issuer id string before defaulting to
''/unknown.
Nitpick comments:
In@CLAUDE.md:
- Line 59: Add a short "Telemetry" note to the docs near the utils/ analytics
entry describing that telemetry collects anonymous usage metrics, how to opt out
via the TRUSTVC_TELEMETRY_DISABLED=1 environment variable or by calling the
disableTelemetry() API, and point readers to the README for full details;
mention that telemetry is anonymous and configurable and reference the utils/
analytics module and the disableTelemetry() function so developers can find the
implementation.In
@README.md:
- Around line 1191-1195: The fenced code block showing the .env example (the
block containing TRUSTVC_TELEMETRY_DISABLED=1) lacks a language specifier and
triggers markdownlint; update that fenced block to include the env language
specifier (use ```env) so the block becomes a proper env-highlighted code fence
in README.md.In
@src/utils/telemetry/telemetry.test.ts:
- Around line 1-2: Consolidate the two imports from './telemetry' into a single
import statement that includes all referenced symbols (emitTelemetry,
disableTelemetry, enableTelemetry, extractDidMethod, _resetForTesting,
isTelemetryEnabled, getInstanceId, SDK_VERSION); update the import lines so
there is only one import from './telemetry' that lists these named exports to
remove the duplicate import and improve readability.In
@src/utils/telemetry/telemetry.ts:
- Around line 92-96: The isNodeEnvironment function uses explicit nested null
checks for process.versions.node; replace the nested check with optional
chaining to simplify the expression while keeping the typeof process !==
'undefined' guard: update the return expression in isNodeEnvironment to use
process?.versions?.node != null so the logic is identical but cleaner and less
error-prone.- Around line 98-100: The isBrowserEnvironment function currently uses raw
globals; update it to check globalThis for consistency with other checks (e.g.,
globalThis.crypto, globalThis.fetch) by replacing references to window and
localStorage with globalThis.window and globalThis.localStorage so the function
uses typeof globalThis.window !== 'undefined' && typeof globalThis.localStorage
!== 'undefined' (refer to the isBrowserEnvironment function to locate the
change).- Around line 157-162: Change the module to use inline exports instead of a
trailing re-export: mark the functions isTelemetryEnabled and getInstanceId with
the export keyword at their declarations and export SDK_VERSION where it is
defined, then remove the final "export { isTelemetryEnabled, getInstanceId,
SDK_VERSION };" line; keep _resetForTesting and the internal symbols
(_telemetryOverride, _instanceId) unchanged so tests still call
_resetForTesting().In
@vitest.config.ts:
- Line 28: The env object in vitest config uses an unnecessary
as anycast;
replace it with a properly typed object such as Record<string, string |
undefined>. For example, construct a typed variable (e.g.,const testEnv: Record<string, string | undefined> = { ...process.env, TRUSTVC_TELEMETRY_DISABLED: '1' }) and useenv: testEnv, or cast process.env
narrowly (...(process.env as Record<string,string|undefined>)) and remove the
as anyto satisfy static analysis while preserving the
TRUSTVC_TELEMETRY_DISABLED entry.</details> <details> <summary>🪄 Autofix (Beta)</summary> Fix all unresolved CodeRabbit comments on this PR: - [ ] <!-- {"checkboxId": "4b0d0e0a-96d7-4f10-b296-3a18ea78f0b9"} --> Push a commit to this branch (recommended) - [ ] <!-- {"checkboxId": "ff5b1114-7d8c-49e6-8ac1-43f82af23a33"} --> Create a new PR with the fixes </details> --- <details> <summary>ℹ️ Review info</summary> <details> <summary>⚙️ Run configuration</summary> **Configuration used**: defaults **Review profile**: CHILL **Plan**: Pro **Run ID**: `844ba480-554f-4885-9a3e-5a4c4c65be5c` </details> <details> <summary>📥 Commits</summary> Reviewing files that changed from the base of the PR and between 9233a6dced81a241aa2de72be7e52401506e004f and 09c4544ba9e16de822e78ffb03e1a084ce3ea5fa. </details> <details> <summary>⛔ Files ignored due to path filters (1)</summary> * `package-lock.json` is excluded by `!**/package-lock.json` </details> <details> <summary>📒 Files selected for processing (16)</summary> * `CLAUDE.md` * `README.md` * `src/__tests__/open-attestation/telemetry.test.ts` * `src/__tests__/w3c/telemetry.test.ts` * `src/open-attestation/sign.ts` * `src/open-attestation/verify.ts` * `src/utils/index.ts` * `src/utils/telemetry/index.ts` * `src/utils/telemetry/telemetry.test.ts` * `src/utils/telemetry/telemetry.ts` * `src/utils/telemetry/types.ts` * `src/w3c/derive.ts` * `src/w3c/sign.ts` * `src/w3c/verify.ts` * `tsconfig.json` * `vitest.config.ts` </details> </details> <!-- This is an auto-generated comment by CodeRabbit for review status -->
- move credential and document telemetry mapping into shared helpers so signing, verifying, and deriving report methods and cryptosuites consistently - replace repeated async setup in tests with a shared harness and table-driven cases to reduce duplication and make telemetry assertions more reliable
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/utils/telemetry/telemetry.test.ts (1)
1-3: Consolidate imports from./telemetryand drop redundant Vitest symbol imports.On Lines 2-3, merge duplicated
./telemetryimports into one statement. Also, in practice with globals-enabled Vitest, explicitdescribe/it/expectimports are redundant.Proposed cleanup
-import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; -import { emitTelemetry, disableTelemetry, enableTelemetry, extractDidMethod } from './telemetry'; -import { _resetForTesting, isTelemetryEnabled, getInstanceId, SDK_VERSION } from './telemetry'; +import { afterEach, beforeEach, vi } from 'vitest'; +import { + emitTelemetry, + disableTelemetry, + enableTelemetry, + extractDidMethod, + _resetForTesting, + isTelemetryEnabled, + getInstanceId, + SDK_VERSION, +} from './telemetry';🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/telemetry/telemetry.test.ts` around lines 1 - 3, Replace the two separate imports so that all symbols exported from ./telemetry are imported in one consolidated statement (include emitTelemetry, disableTelemetry, enableTelemetry, extractDidMethod, _resetForTesting, isTelemetryEnabled, getInstanceId, SDK_VERSION) and drop redundant vitest globals (remove explicit describe/it/expect from the vitest import), keeping only the needed vitest symbols (e.g., beforeEach, afterEach, vi) in the vitest import; update the import lines to a single import from './telemetry' and a minimal import from 'vitest'.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/w3c/telemetry.test.ts`:
- Line 1: The test file currently imports Vitest globals explicitly; remove the
import statement that imports describe and it from 'vitest' so the file relies
on the configured Vitest globals instead (locate and delete the import line that
references describe and it). Ensure no other references to that import remain
and run tests to confirm globals are used correctly.
In `@src/open-attestation/telemetry.ts`:
- Around line 20-25: emitOATelemetry currently hardcodes cryptosuite to
SUPPORTED_SIGNING_ALGORITHM.Secp256k1VerificationKey2018 which is incorrect;
update emitOATelemetry to derive the cryptosuite from the OA document structure
by checking V3-style proof.type and V2-style signature.type (handle array or
nested objects and fall back to 'unknown'), then pass that extracted string as
cryptosuite; reference the emitOATelemetry function and use the same
getIdentityProofType helper pattern for locating the proof/signature fields and
defaulting to 'unknown'.
---
Nitpick comments:
In `@src/utils/telemetry/telemetry.test.ts`:
- Around line 1-3: Replace the two separate imports so that all symbols exported
from ./telemetry are imported in one consolidated statement (include
emitTelemetry, disableTelemetry, enableTelemetry, extractDidMethod,
_resetForTesting, isTelemetryEnabled, getInstanceId, SDK_VERSION) and drop
redundant vitest globals (remove explicit describe/it/expect from the vitest
import), keeping only the needed vitest symbols (e.g., beforeEach, afterEach,
vi) in the vitest import; update the import lines to a single import from
'./telemetry' and a minimal import from 'vitest'.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: cdf95d1e-5af6-4a61-a907-3acf99c7a9ce
📒 Files selected for processing (11)
src/__tests__/open-attestation/telemetry.test.tssrc/__tests__/utils/telemetry.tssrc/__tests__/w3c/telemetry.test.tssrc/open-attestation/sign.tssrc/open-attestation/telemetry.tssrc/open-attestation/verify.tssrc/utils/telemetry/telemetry.test.tssrc/w3c/derive.tssrc/w3c/sign.tssrc/w3c/telemetry.tssrc/w3c/verify.ts
✅ Files skipped from review due to trivial changes (2)
- src/open-attestation/sign.ts
- src/tests/open-attestation/telemetry.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/open-attestation/verify.ts
- src/w3c/sign.ts
- src/w3c/verify.ts
- src/w3c/derive.ts
- replace the hardcoded OA cryptosuite value so telemetry reports the actual proof or signature type for v2 and v3 documents - fall back to `unknown` for unsupported documents to keep emitted telemetry consistent instead of sending misleading defaults - extend telemetry tests and helpers to assert cryptosuite extraction alongside DID method reporting
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/utils/telemetry/telemetry.test.ts (2)
77-79: Consider using a clearly syntheticdid:keysample to avoid secret-scan noise.The current value is valid-looking enough to trigger generic key detectors in some scanners. A clearly fake placeholder keeps intent while reducing false positives in CI/security tooling.
Suggested tweak
- input: 'did:key:z6MkrHKzgsahxBLyNAbLQyB1pcWNYC9GmywiWPgkrvntAZcj', + input: 'did:key:z6MkExampleDidKeyForTestsOnly000000000000000000',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/telemetry/telemetry.test.ts` around lines 77 - 79, Replace the realistic-looking DID in the test case named "extracts did:key" in telemetry.test.ts with a clearly synthetic placeholder (for example use an obviously fake fragment like 'did:key:fake' or 'did:key:xxxxxxxx') so CI secret scanners won't flag it; keep the test's expected value ('did:key') and the test logic in the same function unchanged (refer to the test case name "extracts did:key" and the input/expected fields to locate the change).
109-111: Test name is slightly inaccurate vs setup state.
beforeEachsetsTRUSTVC_TELEMETRY_DISABLEDto'', so this case is “empty env value” rather than “not set”. Please rename the test to match what it actually verifies.Suggested tweak
- it('should return true by default when env var is not set', () => { + it('should return true when TRUSTVC_TELEMETRY_DISABLED is empty', () => { expect(isTelemetryEnabled()).toBe(true); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/utils/telemetry/telemetry.test.ts` around lines 109 - 111, Rename the test that currently reads "should return true by default when env var is not set" to accurately reflect the setup where beforeEach sets TRUSTVC_TELEMETRY_DISABLED = '' (empty string); update the test description to something like "should return true when TRUSTVC_TELEMETRY_DISABLED is empty" and keep the assertion calling isTelemetryEnabled() unchanged so the test still verifies behavior for an empty env value.src/__tests__/w3c/telemetry.test.ts (1)
9-51: Add W3C cryptosuite assertions to this telemetry matrix.These tests currently validate only
did_method; a regression in W3Ccryptosuiteextraction would still pass. Consider assertingtelemetry.getLastCryptosuite()per case, similar to OA telemetry tests.♻️ Suggested update
[ { name: 'emits did:web when signing with a string issuer', runOperation: async () => { @@ }, expectedDidMethod: 'did:web', + expectedCryptosuite: 'BbsBlsSignature2020', }, { name: 'emits did:web when verifying with a string issuer', runOperation: () => verifyW3CSignature(W3C_VERIFIABLE_DOCUMENT), expectedDidMethod: 'did:web', + expectedCryptosuite: 'BbsBlsSignature2020', }, @@ runOperation: () => verifyW3CSignature({ @@ }), expectedDidMethod: 'did:key', + expectedCryptosuite: 'BbsBlsSignature2020', }, - ].forEach(({ name, runOperation, expectedDidMethod }) => { + ].forEach(({ name, runOperation, expectedDidMethod, expectedCryptosuite }) => { it(name, async () => { await telemetry.assertDidMethod(runOperation, expectedDidMethod); + expect(telemetry.getLastCryptosuite()).toBe(expectedCryptosuite); }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/w3c/telemetry.test.ts` around lines 9 - 51, The tests only assert did method but not the W3C cryptosuite; update the test matrix entries to include an expectedCryptosuite value for each case and after calling telemetry.assertDidMethod(runOperation, expectedDidMethod) also assert the cryptosuite by calling telemetry.getLastCryptosuite() (or a helper like telemetry.assertCryptosuite if available) and compare it to expectedCryptosuite; modify the objects in the array (and the forEach callback signature) to accept expectedCryptosuite and perform that extra assertion for functions runOperation/verifyW3CSignature so each case verifies both did method and cryptosuite extraction.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/w3c/telemetry.test.ts`:
- Around line 17-24: This test includes an inline privateKeyBase58 literal—move
it into a non-sensitive test fixture and reference it from the test instead:
create a fixture (e.g., export TEST_PRIVATE_KEY_BASE58) under
src/__tests__/fixtures, replace the inline privateKeyBase58 value in the object
(the entry with id 'did:web:trustvc.github.io:did:1#keys-1' and type
VerificationType.Bls12381G2Key2020) with the imported constant, and update the
test imports accordingly to centralize and reuse the test key material.
---
Nitpick comments:
In `@src/__tests__/w3c/telemetry.test.ts`:
- Around line 9-51: The tests only assert did method but not the W3C
cryptosuite; update the test matrix entries to include an expectedCryptosuite
value for each case and after calling telemetry.assertDidMethod(runOperation,
expectedDidMethod) also assert the cryptosuite by calling
telemetry.getLastCryptosuite() (or a helper like telemetry.assertCryptosuite if
available) and compare it to expectedCryptosuite; modify the objects in the
array (and the forEach callback signature) to accept expectedCryptosuite and
perform that extra assertion for functions runOperation/verifyW3CSignature so
each case verifies both did method and cryptosuite extraction.
In `@src/utils/telemetry/telemetry.test.ts`:
- Around line 77-79: Replace the realistic-looking DID in the test case named
"extracts did:key" in telemetry.test.ts with a clearly synthetic placeholder
(for example use an obviously fake fragment like 'did:key:fake' or
'did:key:xxxxxxxx') so CI secret scanners won't flag it; keep the test's
expected value ('did:key') and the test logic in the same function unchanged
(refer to the test case name "extracts did:key" and the input/expected fields to
locate the change).
- Around line 109-111: Rename the test that currently reads "should return true
by default when env var is not set" to accurately reflect the setup where
beforeEach sets TRUSTVC_TELEMETRY_DISABLED = '' (empty string); update the test
description to something like "should return true when
TRUSTVC_TELEMETRY_DISABLED is empty" and keep the assertion calling
isTelemetryEnabled() unchanged so the test still verifies behavior for an empty
env value.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8061f3fd-14eb-4f91-9c0a-825ad8c1e801
📒 Files selected for processing (5)
src/__tests__/open-attestation/telemetry.test.tssrc/__tests__/utils/telemetry.tssrc/__tests__/w3c/telemetry.test.tssrc/open-attestation/telemetry.tssrc/utils/telemetry/telemetry.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/open-attestation/telemetry.ts
- src/tests/utils/telemetry.ts
- move the shared test signing key into a fixture to reduce duplication and keep test setup consistent across suites - extend telemetry coverage to verify cryptosuite reporting alongside did method extraction - simplify placeholder did:key inputs and clarify a telemetry environment test description
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/fixtures/keys.ts`:
- Around line 4-12: The committed raw private key in the TEST_BBS2020_KEY_PAIR
fixture (privateKeyBase58 field) triggers secret scanners; either remove the
hard-coded secret and instead generate the key at test setup (e.g., replace
export TEST_BBS2020_KEY_PAIR with a factory like createTestBbs2020KeyPair() that
derives/loads the key from a test-only generator or env) or add an explicit
scanner allowlist marker scoped to this fixture (e.g., a single-line
scanner-allow comment immediately above TEST_BBS2020_KEY_PAIR referencing the
scanner’s rule/id) so the intent is machine-verifiable; update references to use
the factory or keep the allowlist comment next to the privateKeyBase58 symbol.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2e1611c9-84a0-4832-af2d-81fbba200ac3
📒 Files selected for processing (5)
src/__tests__/core/documentBuilder.test.tssrc/__tests__/fixtures/keys.tssrc/__tests__/w3c/sign.test.tssrc/__tests__/w3c/telemetry.test.tssrc/utils/telemetry/telemetry.test.ts
✅ Files skipped from review due to trivial changes (3)
- src/tests/core/documentBuilder.test.ts
- src/tests/w3c/telemetry.test.ts
- src/utils/telemetry/telemetry.test.ts
Comment on lines
+4
to
+12
| // Dummy/test cryptographic BBS 2020 key pair for local development and CI only. | ||
| export const TEST_BBS2020_KEY_PAIR: PrivateKeyPair = { | ||
| id: 'did:web:trustvc.github.io:did:1#keys-1', | ||
| controller: 'did:web:trustvc.github.io:did:1', | ||
| type: VerificationType.Bls12381G2Key2020, | ||
| publicKeyBase58: | ||
| 'oRfEeWFresvhRtXCkihZbxyoi2JER7gHTJ5psXhHsdCoU1MttRMi3Yp9b9fpjmKh7bMgfWKLESiK2YovRd8KGzJsGuamoAXfqDDVhckxuc9nmsJ84skCSTijKeU4pfAcxeJ', | ||
| privateKeyBase58: '4LDU56PUhA9ZEutnR1qCWQnUhtLtpLu2EHSq4h1o7vtF', | ||
| }; |
Contributor
There was a problem hiding this comment.
Secret-scanner false-positive risk on committed private key fixture.
Line 11 contains raw key material and is already flagged by static analysis. Even for test-only keys, this can create CI/security-noise churn. Please add an explicit scanner allowlist entry scoped to this fixture (or generate key material during test setup) to make intent machine-verifiable.
🧰 Tools
🪛 Betterleaks (1.1.1)
[high] 11-11: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/__tests__/fixtures/keys.ts` around lines 4 - 12, The committed raw
private key in the TEST_BBS2020_KEY_PAIR fixture (privateKeyBase58 field)
triggers secret scanners; either remove the hard-coded secret and instead
generate the key at test setup (e.g., replace export TEST_BBS2020_KEY_PAIR with
a factory like createTestBbs2020KeyPair() that derives/loads the key from a
test-only generator or env) or add an explicit scanner allowlist marker scoped
to this fixture (e.g., a single-line scanner-allow comment immediately above
TEST_BBS2020_KEY_PAIR referencing the scanner’s rule/id) so the intent is
machine-verifiable; update references to use the factory or keep the allowlist
comment next to the privateKeyBase58 symbol.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Add anonymous SDK telemetry for TrustVC issuance and verification flows so the team can understand which document formats, DID methods, and cryptographic suites are being used in practice. This PR also documents the telemetry behavior and opt-out controls.
Changes
disableTelemetry(),enableTelemetry(),emitTelemetry(), andextractDidMethod(), plus environment-based opt-out viaTRUSTVC_TELEMETRY_DISABLEDsign,verify, andderiveflows and OpenAttestationsignandverifyflows, including the follow-up fix to extract the correct DID method / identity proof typenpx vitest --run src/utils/telemetry/telemetry.test.ts src/__tests__/w3c/telemetry.test.ts src/__tests__/open-attestation/telemetry.test.ts(31tests)Issues
N/A
Summary by CodeRabbit
New Features
Documentation
Tests