feat: add obligation registry and related functions to the project - #153
Closed
manishdex25 wants to merge 25 commits into
Closed
feat: add obligation registry and related functions to the project#153manishdex25 wants to merge 25 commits into
manishdex25 wants to merge 25 commits into
Conversation
Contributor
|
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:
📝 WalkthroughWalkthroughTrustVC adds Obligation Registry (BoE) deployment, lifecycle, verification, document-builder, endorsement-chain, public export, documentation, and Hardhat E2E support alongside existing Token Registry functionality. ChangesObligation Registry API
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 8
🧹 Nitpick comments (27)
src/obligation-registry/index.ts (1)
3-4: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
export * asre-export syntax.♻️ Proposed fix
-import * as contracts from './contracts'; -export { contracts as obligationRegistryContracts }; +export * as obligationRegistryContracts from './contracts';🤖 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/obligation-registry/index.ts` around lines 3 - 4, Update the contracts re-export in the module to use the `export * as obligationRegistryContracts` syntax instead of importing the namespace and re-exporting it. Preserve the existing public export name and behavior.Source: Linters/SAST tools
src/obligation-registry-functions/status.ts (1)
14-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
connectObligationEscrowduplicated across files.Identical to the helper in
rejectTransfers.tsandtransfer.ts. See consolidated comment.🤖 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/obligation-registry-functions/status.ts` around lines 14 - 24, Remove the duplicated connectObligationEscrow helper from status.ts and reuse the shared implementation already used by rejectTransfers.ts and transfer.ts, preserving its provider validation and contract construction behavior.src/obligation-registry-functions/rejectTransfers.ts (2)
56-59: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPre-check failures swallow the original revert reason (x3).
All three functions discard
e's details and throw a generic message. See consolidated comment for the cross-file fix.Also applies to: 83-86, 110-113
🤖 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/obligation-registry-functions/rejectTransfers.ts` around lines 56 - 59, Update the catch blocks in rejectTransferHolder and the corresponding pre-check paths at the other two locations to preserve and propagate the original error details from e instead of throwing only a generic message. Keep the existing console.error context while ensuring each thrown error includes the underlying revert reason.
13-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
resolveObligationEscrowAddress/connectObligationEscrowduplicated across files.Both helpers are identical to the ones in
transfer.ts(andconnectObligationEscrowalso matchesstatus.ts). Move them to./utils.tsand import from there. See consolidated comment.🤖 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/obligation-registry-functions/rejectTransfers.ts` around lines 13 - 35, Move resolveObligationEscrowAddress and connectObligationEscrow from rejectTransfers.ts into the shared ./utils.ts module, reusing the existing implementations from transfer.ts/status.ts as appropriate. Remove the local helper definitions and import both symbols from ./utils.ts, preserving their current validation and contract-connection behavior.src/deploy/obligation-registry.ts (3)
101-111: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
console.warnon missingescrowFactoryAddress.Unconditional console output from a library function may pollute consumer logs. Consider surfacing this via a return field or an injectable logger instead of a hard-coded
console.warn.🤖 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/deploy/obligation-registry.ts` around lines 101 - 111, Replace the hard-coded console.warn in the missing escrowFactoryAddress branch of the obligation registry deployment flow with the library’s existing injectable logger or a returned status field. Preserve fresh ObligationEscrowFactory deployment while avoiding unconditional console output for consumers.
17-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant type alias.
TransactionReceiptis just a copy ofObligationRegistryTransactionReceipt; static analysis flags this as redundant.♻️ Proposed fix
export type ObligationRegistryTransactionReceipt = ContractReceiptV5 | ContractReceiptV6; -type TransactionReceipt = ObligationRegistryTransactionReceipt;And replace all local usages of
TransactionReceiptwithObligationRegistryTransactionReceipt.🤖 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/deploy/obligation-registry.ts` around lines 17 - 18, Remove the redundant TransactionReceipt alias and replace every local TransactionReceipt usage with ObligationRegistryTransactionReceipt. Preserve the existing ContractReceiptV5 | ContractReceiptV6 type definition and behavior.Source: Linters/SAST tools
64-132: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated deploy scaffolding between
deployObligationEscrowFactoryanddeployObligationRegistry.Both functions repeat the identical chainId-resolution →
getTxOptions→getEthersContractFactoryFromProvider→ContractFactoryconstruction sequence, differing only in the factory/bytecode used. Extracting a sharedprepareDeployment(signer, options, factoryArtifact)helper would remove this duplication.🤖 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/deploy/obligation-registry.ts` around lines 64 - 132, Extract the repeated deployment setup from deployObligationEscrowFactory and deployObligationRegistry into a shared prepareDeployment helper accepting the signer, deployment options, and contract artifact. Have the helper perform chain ID resolution, transaction-option creation, provider ContractFactory lookup, and ContractFactory construction, then update both functions to use it while preserving their existing constructor arguments and deployment behavior.src/obligation-registry-functions/transfer.ts (2)
16-38: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
resolveObligationEscrowAddress/connectObligationEscrowduplicated across files.Byte-for-byte identical to the helpers in
rejectTransfers.ts(andconnectObligationEscrowalso matchesstatus.ts). See consolidated comment.🤖 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/obligation-registry-functions/transfer.ts` around lines 16 - 38, Remove the duplicated resolveObligationEscrowAddress and connectObligationEscrow implementations from transfer.ts, and reuse the shared helper definitions already consolidated from rejectTransfers.ts and status.ts. Update imports and call sites as needed while preserving the existing provider, token ID, and escrow address validation behavior.
59-62: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPre-check failures swallow the original revert reason (x4).
Same pattern as the other obligation-registry-functions files. See consolidated comment.
Also applies to: 96-99, 127-130, 162-165
🤖 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/obligation-registry-functions/transfer.ts` around lines 59 - 62, Update each error handler in transfer.ts for the callStatic pre-checks at the indicated locations to preserve and propagate the original error or revert reason instead of replacing it with a generic Error. Keep the existing console.error context, and ensure all four nominate pre-check failure paths expose the underlying failure details.src/obligation-registry-functions/mint.ts (1)
41-44: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPre-check failure swallows the original revert reason.
Same pattern as
discharge.ts— see consolidated comment for the cross-file fix.🤖 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/obligation-registry-functions/mint.ts` around lines 41 - 44, Update the catch block surrounding the mint pre-check callStatic flow in mint.ts to preserve and propagate the original error details, matching the handling used in discharge.ts. Keep the contextual logging and failure behavior, but include the caught error when constructing or throwing the replacement error instead of discarding its revert reason.src/obligation-registry-functions/returnToken.ts (1)
49-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPre-check failures swallow the original revert reason (x3).
Same pattern as the other obligation-registry-functions files; see consolidated comment.
Also applies to: 84-87, 119-122
🤖 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/obligation-registry-functions/returnToken.ts` around lines 49 - 52, Update the catch blocks in returnToIssuer, including the handlers around the pre-checks at all three referenced locations, to preserve and propagate the original error or revert reason instead of replacing it with a generic Error. Retain the failure context in logging while ensuring callers can inspect the underlying exception.src/obligation-registry-functions/discharge.ts (1)
45-48: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPre-check failure swallows the original revert reason.
The caught error
e(likely containing the actual Solidity revert reason) is only logged toconsole.errorand replaced with a generic'Pre-check (callStatic) for discharge failed'message, making failures harder to diagnose for SDK consumers. This exact pattern recurs inmint.ts,rejectTransfers.ts,returnToken.ts, andtransfer.ts(see consolidated comment).🤖 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/obligation-registry-functions/discharge.ts` around lines 45 - 48, Update the callStatic pre-check catch block in discharge.ts and the corresponding blocks in mint.ts, rejectTransfers.ts, returnToken.ts, and transfer.ts to preserve the caught error e when throwing, while retaining the existing pre-check context in the error message. Ensure SDK consumers receive the original Solidity revert reason instead of only the generic failure text.src/verify/fragments/document-status/obligationRecords/utils.ts (1)
13-19: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse optional chaining.
- return error.data && error.data.slice(0, 10) === '0x7e273289'; + return error.data?.slice(0, 10) === '0x7e273289';🤖 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/verify/fragments/document-status/obligationRecords/utils.ts` around lines 13 - 19, Update isNonExistentToken to use optional chaining when accessing error.message and error.data, preserving the existing message check and token-prefix comparison behavior.Source: Linters/SAST tools
src/verify/fragments/document-status/obligationRecords/obligationRecordVerifier.ts (1)
58-58: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify redundant chained condition.
- if (!credentialStatus?.tokenNetwork || !credentialStatus?.tokenNetwork?.chainId) { + if (!credentialStatus?.tokenNetwork?.chainId) {🤖 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/verify/fragments/document-status/obligationRecords/obligationRecordVerifier.ts` at line 58, simplify the condition in the credential status validation around credentialStatus.tokenNetwork and chainId by removing the redundant optional-chaining check while preserving the existing behavior for missing tokenNetwork or chainId.Source: Linters/SAST tools
src/obligation-registry-functions/verify.ts (2)
32-32: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
document as neverbypasses type-checking on the call.Casting
unknowntoneverto satisfyverifyDocument's parameter type hides any real type mismatch rather than surfacing it.🤖 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/obligation-registry-functions/verify.ts` at line 32, Remove the document as never cast in the verifyDocument call within the verification flow. Pass document with its actual type and update verifyDocument’s parameter typing or validate/narrow document beforehand so the call is type-safe without bypassing type-checking.
56-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify with optional chaining.
- if (!fragment || fragment.status !== 'VALID') return null; + if (fragment?.status !== 'VALID') return null;🤖 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/obligation-registry-functions/verify.ts` at line 56, In the validation guard, simplify the fragment status check using optional chaining while preserving the existing null return behavior for missing fragments or non-VALID statuses. Update the condition in the verify flow without changing any surrounding logic.Source: Linters/SAST tools
src/verify/fragments/index.ts (1)
15-18: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer
export … fromfor pure re-exports.-import { - credentialStatusObligationRecordVerifier, - OBLIGATION_RECORDS_NAME, -} from './document-status/obligationRecords/obligationRecordVerifier'; +export { + credentialStatusObligationRecordVerifier, + OBLIGATION_RECORDS_NAME, +} from './document-status/obligationRecords/obligationRecordVerifier';Also applies to: 23-27
🤖 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/verify/fragments/index.ts` around lines 15 - 18, Replace the import-and-re-export pattern for credentialStatusObligationRecordVerifier and OBLIGATION_RECORDS_NAME in the fragments index with a direct export-from statement. Apply the same change to the related re-export block referenced by the comment, preserving the existing exported symbols.Source: Linters/SAST tools
src/core/obligation-endorsement-chain/helpers.ts (3)
85-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMisleading variable name: converts seconds→milliseconds, not the reverse.
msecToSecis multiplied into a seconds-basedblock.timestamp(ethers block timestamps are seconds since epoch), which actually converts seconds to milliseconds. The name implies the opposite conversion and could mislead a future refactor into "fixing" this into a double conversion.✏️ Suggested rename
- const msecToSec = 1000; - const eventTimestamp = (await provider.getBlock(blockNumber))!.timestamp * msecToSec; + const secToMsec = 1000; + const eventTimestamp = (await provider.getBlock(blockNumber))!.timestamp * secToMsec;🤖 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/core/obligation-endorsement-chain/helpers.ts` around lines 85 - 87, Rename the conversion constant msecToSec to accurately describe seconds-to-milliseconds conversion, and update its use in the eventTimestamp calculation while preserving the existing multiplication and returned value.
140-201: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffHigh cognitive complexity flagged by SonarCloud.
getLogsInBlockRangemixes the fast-path fetch, chunk windowing, newest-first/shouldStop scanning, and concurrent-chunk fetching in one function (complexity 19 vs. allowed 15). Consider extracting the windowing and the two scan strategies (early-stop vs. all-chunks) into separate helpers.🤖 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/core/obligation-endorsement-chain/helpers.ts` around lines 140 - 201, Reduce the cognitive complexity of getLogsInBlockRange by extracting block-window construction and the separate newest-first/shouldStop scanning and concurrent all-chunks fetching into focused helpers. Keep the existing fast-path request, range-error fallback, ordering, early-stop behavior, concurrency setting, and flattened results unchanged while making getLogsInBlockRange delegate to those helpers.Source: Linters/SAST tools
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMerged-event type resolution implicitly depends on caller's array concatenation order.
identifyEventTypeFromLogspicks a merged event's type by scanning the grouped events in array order and returning the first match; this is only deterministic today because the sole caller concatenatestransfersbeforestatusEvents.
src/core/obligation-endorsement-chain/helpers.ts#L215-247: make the type-precedence explicit (e.g., check the fixed priority list against the whole group rather than relying on encounter order), independent of input ordering.src/core/obligation-endorsement-chain/useObligationEndorsementChain.ts#L53-53: if the ordering-dependent behavior inidentifyEventTypeFromLogsis intentional, document whytransfersmust precedestatusEventshere so a future reordering doesn't silently change merged event types.🤖 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/core/obligation-endorsement-chain/helpers.ts` at line 1, Update identifyEventTypeFromLogs to resolve merged event types using an explicit fixed-priority check across the entire grouped event set, rather than returning the first match based on input array order. Ensure the result remains deterministic regardless of how transfers and statusEvents are concatenated; only document ordering in useObligationEndorsementChain if preserving encounter-order precedence is intentional.src/core/obligation-endorsement-chain/findObligationMintBlock.ts (2)
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated
ZERO_ADDRESSconstant across two files. Both files independently declare the same zero-address literal; extract it into a shared constants module insrc/core/obligation-endorsement-chain/.
src/core/obligation-endorsement-chain/findObligationMintBlock.ts#L12-12: import the shared constant instead of declaring it locally.src/core/obligation-endorsement-chain/fetchObligationEscrowTransfers.ts#L22-22: import the shared constant instead of declaring it locally.🤖 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/core/obligation-endorsement-chain/findObligationMintBlock.ts` at line 1, Extract the duplicated ZERO_ADDRESS literal into a shared constants module under the obligation-endorsement-chain directory, then update findObligationMintBlock and fetchObligationEscrowTransfers to import and use that shared constant instead of local declarations.
50-177: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffHigh cognitive complexity flagged by SonarCloud.
findObligationMintBlockhandles genesis lookup, escrow scan, mint-Transfer scan, and factory-created fallback in a single function (complexity 17 vs. allowed 15). Consider splitting each fallback tier into a named helper.🤖 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/core/obligation-endorsement-chain/findObligationMintBlock.ts` around lines 50 - 177, Reduce the cognitive complexity of findObligationMintBlock by extracting the genesis lookup, title-escrow log scan, mint Transfer scan, and factory ObligationEscrowCreated fallback into focused named helpers. Keep findObligationMintBlock responsible for orchestration and preserve the existing fallback order, range-error handling, earliest-block selection, and final Unminted Title Escrow error.Source: Linters/SAST tools
src/core/obligation-endorsement-chain/fetchObligationEscrowTransfers.ts (1)
115-119: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMisleading parameter name:
tokenRegistryAddressactually holds the obligation registry address.
classifyEscrowLogs/mapTransferEventtake atokenRegistryAddressparameter, but it's populated fromobligationRegistryAddressat the call site (line 286). This is confusing given the codebase already has a distinct classic "token registry" concept.Also applies to: 204-209, 248-287
🤖 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/core/obligation-endorsement-chain/fetchObligationEscrowTransfers.ts` around lines 115 - 119, Rename the misleading tokenRegistryAddress parameter to obligationRegistryAddress throughout classifyEscrowLogs and mapTransferEvent, including their declarations, calls, and internal references. Preserve the existing value flow from obligationRegistryAddress and do not alter the separate classic token registry concept.src/core/obligation-endorsement-chain/useObligationEndorsementChain.ts (1)
58-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDecrypt failures silently leak raw ciphertext into
remark.On
decryptthrowing, the original (still-encrypted)eventis returned unchanged rather than surfacing the failure, so downstream consumers may render undecrypted hex as if it were the actual remark with no signal that decryption failed.♻️ Suggested fix
try { const remarkHex = event.remark.startsWith('0x') ? event.remark.slice(2) : event.remark; return { ...event, remark: decrypt(remarkHex, keyId) }; } catch { - return event; + return { ...event, remark: '' }; }🤖 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/core/obligation-endorsement-chain/useObligationEndorsementChain.ts` around lines 58 - 68, Update the decrypt failure handling in the chain mapping around useObligationEndorsementChain so a thrown decrypt error does not return the original event with ciphertext in remark. Surface the failure through the established error-handling mechanism, or replace remark with a safe failure value while preserving the event’s other fields; keep successful decryption and empty-remark handling unchanged.src/__tests__/utils/documents/index.test.ts (1)
104-126: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse
toBeUndefined()instead oftoBe(undefined).For better readability and clearer failure messages, prefer the dedicated
toBeUndefined()matcher overtoBe(undefined).♻️ Proposed fix
it('getTokenRegistryAddress - Obligation document returns undefined', () => { const tokenRegistryAddress = getTokenRegistryAddress(W3C_OBLIGATION_RECORD); - expect(tokenRegistryAddress).toBe(undefined); + expect(tokenRegistryAddress).toBeUndefined(); }); it('getTokenRegistryAddress - INVALID OA V2 Transferable Record Document', () => { const tokenRegistryAddress = getTokenRegistryAddress(WRAPPED_DOCUMENT_DID_V2); - expect(tokenRegistryAddress).toBe(undefined); + expect(tokenRegistryAddress).toBeUndefined(); }); }); describe.concurrent('getObligationRegistryAddress', () => { it('getObligationRegistryAddress - VALID W3C VC Obligation Record Document', () => { const obligationRegistryAddress = getObligationRegistryAddress(W3C_OBLIGATION_RECORD); expect(obligationRegistryAddress).toBe('0x71D28767662cB233F887aD2Bb65d048d760bA694'); }); it('getObligationRegistryAddress - Transferable Record returns undefined', () => { const obligationRegistryAddress = getObligationRegistryAddress(W3C_TRANSFERABLE_RECORD); - expect(obligationRegistryAddress).toBe(undefined); + expect(obligationRegistryAddress).toBeUndefined(); }); });🤖 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/__tests__/utils/documents/index.test.ts` around lines 104 - 126, Replace each toBe(undefined) assertion in the getTokenRegistryAddress and getObligationRegistryAddress tests with the dedicated toBeUndefined() matcher, preserving the existing test cases and expected behavior.Source: Linters/SAST tools
src/__tests__/e2e/README.md (1)
57-57: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSpecify a language for the fenced code block.
To prevent markdown linter warnings (like
MD040), consider specifyingtextfor this diagram's code block.♻️ Proposed fix
-``` +```text Deploy factory + registry🤖 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/__tests__/e2e/README.md` at line 57, Specify the fenced code block language in the README diagram by adding the text language identifier to the opening fence, while preserving the diagram content unchanged.Source: Linters/SAST tools
src/__tests__/core/verify.test.ts (1)
818-828: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winWeakened assertions reduce this test's diagnostic value.
Accepting both
VALIDandERRORfor the token-registry and DNS-TXT fragments means the test passes even when the underlying verification fails (e.g., liverpc-amoy.polygon.technologyunreachable), so it now effectively only checks fragment presence. Consider mocking the registry/identity lookups (as the TransferableRecords tests do viavi.spyOn(transferableRecordsUtils, 'isTokenMintedOnRegistry')) and asserting a single expected status, to keep the test deterministic and regression-catching.🤖 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/__tests__/core/verify.test.ts` around lines 818 - 828, Strengthen the assertions in the verification test around OpenAttestationEthereumTokenRegistryStatus and OpenAttestationDnsTxtIdentityProof by mocking their external registry and identity lookups, following the TransferableRecords tests’ isTokenMintedOnRegistry pattern. Configure deterministic successful lookup results and assert the single expected fragment status instead of allowing both VALID and ERROR, while retaining the presence checks.
🤖 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 `@package.json`:
- Line 136: Update the `@tradetrust-tt/token-registry-v5` dependency in
package.json to use a published prerelease version; if a git dependency is
required temporarily, replace the SSH feature-branch URL with an HTTPS URL
pinned to an immutable commit hash.
In `@src/core/documentBuilder.ts`:
- Line 19: Update the import in documentBuilder.ts to use the exported
TR_CONTEXT_URL from `@trustvc/w3c-context` instead of
OBLIGATION_RECORDS_CONTEXT_URL, and update the corresponding test import to
match while preserving existing usage.
In `@src/core/obligation-endorsement-chain/fetchObligationEscrowTransfers.ts`:
- Around line 83-84: Update the block-number validation in the logs loop of
fetchObligationEscrowTransfers so blockNumber 0 is accepted and the error is
thrown only when the value is null or undefined.
In `@src/core/obligation-endorsement-chain/findObligationMintBlock.ts`:
- Around line 27-36: Update resolveFilterTopics to call filter.getTopicFilter()
when that method is available, and read topics from its resolved result before
using the existing direct-topics and await fallbacks. Preserve the undefined
return for rejected or topicless filters, and keep the change scoped to deferred
ethers v6 filters.
In `@src/obligation-registry-functions/accept.ts`:
- Around line 45-48: Preserve the underlying callStatic revert reason in both
acceptObligationRegistry at src/obligation-registry-functions/accept.ts lines
45-48 and rejectObligationRegistry at
src/obligation-registry-functions/reject.ts lines 45-48 by appending
`${e?.message ?? e}` to the thrown Error message while retaining the existing
context and console logging.
In `@src/obligation-registry-functions/rejectTransfers.ts`:
- Around line 13-23: Update resolveObligationEscrowAddress to validate tokenId
by checking for absence rather than falsiness, so numeric tokenId 0 is accepted
while undefined or missing values still throw. Apply the same validation change
in the corresponding tokenId checks in transfer.ts and returnToken.ts.
In `@src/obligation-registry-functions/returnToken.ts`:
- Around line 24-30: Update the escrow resolution in returnToken to accept
tokenId 0 by validating only missing values, and replace the duplicated inline
logic with the existing named escrow-resolution helper used elsewhere. Preserve
the current titleEscrowAddress fast path and required-input errors.
In `@src/obligation-registry-functions/transfer.ts`:
- Around line 16-26: Update resolveObligationEscrowAddress to validate tokenId
as missing only when it is undefined or otherwise absent, not when it equals 0.
Preserve the existing requirement checks for obligationRegistry and
signer.provider, and continue passing valid token IDs to
getObligationEscrowAddress.
---
Nitpick comments:
In `@src/__tests__/core/verify.test.ts`:
- Around line 818-828: Strengthen the assertions in the verification test around
OpenAttestationEthereumTokenRegistryStatus and
OpenAttestationDnsTxtIdentityProof by mocking their external registry and
identity lookups, following the TransferableRecords tests’
isTokenMintedOnRegistry pattern. Configure deterministic successful lookup
results and assert the single expected fragment status instead of allowing both
VALID and ERROR, while retaining the presence checks.
In `@src/__tests__/e2e/README.md`:
- Line 57: Specify the fenced code block language in the README diagram by
adding the text language identifier to the opening fence, while preserving the
diagram content unchanged.
In `@src/__tests__/utils/documents/index.test.ts`:
- Around line 104-126: Replace each toBe(undefined) assertion in the
getTokenRegistryAddress and getObligationRegistryAddress tests with the
dedicated toBeUndefined() matcher, preserving the existing test cases and
expected behavior.
In `@src/core/obligation-endorsement-chain/fetchObligationEscrowTransfers.ts`:
- Around line 115-119: Rename the misleading tokenRegistryAddress parameter to
obligationRegistryAddress throughout classifyEscrowLogs and mapTransferEvent,
including their declarations, calls, and internal references. Preserve the
existing value flow from obligationRegistryAddress and do not alter the separate
classic token registry concept.
In `@src/core/obligation-endorsement-chain/findObligationMintBlock.ts`:
- Line 1: Extract the duplicated ZERO_ADDRESS literal into a shared constants
module under the obligation-endorsement-chain directory, then update
findObligationMintBlock and fetchObligationEscrowTransfers to import and use
that shared constant instead of local declarations.
- Around line 50-177: Reduce the cognitive complexity of findObligationMintBlock
by extracting the genesis lookup, title-escrow log scan, mint Transfer scan, and
factory ObligationEscrowCreated fallback into focused named helpers. Keep
findObligationMintBlock responsible for orchestration and preserve the existing
fallback order, range-error handling, earliest-block selection, and final
Unminted Title Escrow error.
In `@src/core/obligation-endorsement-chain/helpers.ts`:
- Around line 85-87: Rename the conversion constant msecToSec to accurately
describe seconds-to-milliseconds conversion, and update its use in the
eventTimestamp calculation while preserving the existing multiplication and
returned value.
- Around line 140-201: Reduce the cognitive complexity of getLogsInBlockRange by
extracting block-window construction and the separate newest-first/shouldStop
scanning and concurrent all-chunks fetching into focused helpers. Keep the
existing fast-path request, range-error fallback, ordering, early-stop behavior,
concurrency setting, and flattened results unchanged while making
getLogsInBlockRange delegate to those helpers.
- Line 1: Update identifyEventTypeFromLogs to resolve merged event types using
an explicit fixed-priority check across the entire grouped event set, rather
than returning the first match based on input array order. Ensure the result
remains deterministic regardless of how transfers and statusEvents are
concatenated; only document ordering in useObligationEndorsementChain if
preserving encounter-order precedence is intentional.
In `@src/core/obligation-endorsement-chain/useObligationEndorsementChain.ts`:
- Around line 58-68: Update the decrypt failure handling in the chain mapping
around useObligationEndorsementChain so a thrown decrypt error does not return
the original event with ciphertext in remark. Surface the failure through the
established error-handling mechanism, or replace remark with a safe failure
value while preserving the event’s other fields; keep successful decryption and
empty-remark handling unchanged.
In `@src/deploy/obligation-registry.ts`:
- Around line 101-111: Replace the hard-coded console.warn in the missing
escrowFactoryAddress branch of the obligation registry deployment flow with the
library’s existing injectable logger or a returned status field. Preserve fresh
ObligationEscrowFactory deployment while avoiding unconditional console output
for consumers.
- Around line 17-18: Remove the redundant TransactionReceipt alias and replace
every local TransactionReceipt usage with ObligationRegistryTransactionReceipt.
Preserve the existing ContractReceiptV5 | ContractReceiptV6 type definition and
behavior.
- Around line 64-132: Extract the repeated deployment setup from
deployObligationEscrowFactory and deployObligationRegistry into a shared
prepareDeployment helper accepting the signer, deployment options, and contract
artifact. Have the helper perform chain ID resolution, transaction-option
creation, provider ContractFactory lookup, and ContractFactory construction,
then update both functions to use it while preserving their existing constructor
arguments and deployment behavior.
In `@src/obligation-registry-functions/discharge.ts`:
- Around line 45-48: Update the callStatic pre-check catch block in discharge.ts
and the corresponding blocks in mint.ts, rejectTransfers.ts, returnToken.ts, and
transfer.ts to preserve the caught error e when throwing, while retaining the
existing pre-check context in the error message. Ensure SDK consumers receive
the original Solidity revert reason instead of only the generic failure text.
In `@src/obligation-registry-functions/mint.ts`:
- Around line 41-44: Update the catch block surrounding the mint pre-check
callStatic flow in mint.ts to preserve and propagate the original error details,
matching the handling used in discharge.ts. Keep the contextual logging and
failure behavior, but include the caught error when constructing or throwing the
replacement error instead of discarding its revert reason.
In `@src/obligation-registry-functions/rejectTransfers.ts`:
- Around line 56-59: Update the catch blocks in rejectTransferHolder and the
corresponding pre-check paths at the other two locations to preserve and
propagate the original error details from e instead of throwing only a generic
message. Keep the existing console.error context while ensuring each thrown
error includes the underlying revert reason.
- Around line 13-35: Move resolveObligationEscrowAddress and
connectObligationEscrow from rejectTransfers.ts into the shared ./utils.ts
module, reusing the existing implementations from transfer.ts/status.ts as
appropriate. Remove the local helper definitions and import both symbols from
./utils.ts, preserving their current validation and contract-connection
behavior.
In `@src/obligation-registry-functions/returnToken.ts`:
- Around line 49-52: Update the catch blocks in returnToIssuer, including the
handlers around the pre-checks at all three referenced locations, to preserve
and propagate the original error or revert reason instead of replacing it with a
generic Error. Retain the failure context in logging while ensuring callers can
inspect the underlying exception.
In `@src/obligation-registry-functions/status.ts`:
- Around line 14-24: Remove the duplicated connectObligationEscrow helper from
status.ts and reuse the shared implementation already used by rejectTransfers.ts
and transfer.ts, preserving its provider validation and contract construction
behavior.
In `@src/obligation-registry-functions/transfer.ts`:
- Around line 16-38: Remove the duplicated resolveObligationEscrowAddress and
connectObligationEscrow implementations from transfer.ts, and reuse the shared
helper definitions already consolidated from rejectTransfers.ts and status.ts.
Update imports and call sites as needed while preserving the existing provider,
token ID, and escrow address validation behavior.
- Around line 59-62: Update each error handler in transfer.ts for the callStatic
pre-checks at the indicated locations to preserve and propagate the original
error or revert reason instead of replacing it with a generic Error. Keep the
existing console.error context, and ensure all four nominate pre-check failure
paths expose the underlying failure details.
In `@src/obligation-registry-functions/verify.ts`:
- Line 32: Remove the document as never cast in the verifyDocument call within
the verification flow. Pass document with its actual type and update
verifyDocument’s parameter typing or validate/narrow document beforehand so the
call is type-safe without bypassing type-checking.
- Line 56: In the validation guard, simplify the fragment status check using
optional chaining while preserving the existing null return behavior for missing
fragments or non-VALID statuses. Update the condition in the verify flow without
changing any surrounding logic.
In `@src/obligation-registry/index.ts`:
- Around line 3-4: Update the contracts re-export in the module to use the
`export * as obligationRegistryContracts` syntax instead of importing the
namespace and re-exporting it. Preserve the existing public export name and
behavior.
In
`@src/verify/fragments/document-status/obligationRecords/obligationRecordVerifier.ts`:
- Line 58: simplify the condition in the credential status validation around
credentialStatus.tokenNetwork and chainId by removing the redundant
optional-chaining check while preserving the existing behavior for missing
tokenNetwork or chainId.
In `@src/verify/fragments/document-status/obligationRecords/utils.ts`:
- Around line 13-19: Update isNonExistentToken to use optional chaining when
accessing error.message and error.data, preserving the existing message check
and token-prefix comparison behavior.
In `@src/verify/fragments/index.ts`:
- Around line 15-18: Replace the import-and-re-export pattern for
credentialStatusObligationRecordVerifier and OBLIGATION_RECORDS_NAME in the
fragments index with a direct export-from statement. Apply the same change to
the related re-export block referenced by the comment, preserving the existing
exported symbols.
🪄 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: bfcf49f6-dcbc-48e4-9f67-cecbcff1e329
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (54)
README.mdpackage.jsonsrc/__tests__/core/documentBuilder.test.tssrc/__tests__/core/verify.amoy.test.tssrc/__tests__/core/verify.pol.test.tssrc/__tests__/core/verify.test.tssrc/__tests__/e2e/README.mdsrc/__tests__/e2e/fixtures.tssrc/__tests__/e2e/obligation-registry-functions/endorsementChain.e2e.test.tssrc/__tests__/e2e/obligation-registry-functions/rejectTransfer.e2e.test.tssrc/__tests__/e2e/obligation-registry-functions/returnToken.e2e.test.tssrc/__tests__/e2e/obligation-registry-functions/statusLifecycle.e2e.test.tssrc/__tests__/e2e/obligation-registry-functions/transfer.e2e.test.tssrc/__tests__/e2e/obligationUtils.tssrc/__tests__/e2e/token-registry-functions/returnToken.e2e.test.tssrc/__tests__/fixtures/endorsement-chain.tssrc/__tests__/obligation-registry-functions/verify.test.tssrc/__tests__/utils/documents/index.test.tssrc/__tests__/verify/obligationRecordVerifier.test.tssrc/core/documentBuilder.tssrc/core/index.tssrc/core/obligation-endorsement-chain/fetchObligationEscrowTransfers.tssrc/core/obligation-endorsement-chain/findObligationMintBlock.tssrc/core/obligation-endorsement-chain/helpers.tssrc/core/obligation-endorsement-chain/index.tssrc/core/obligation-endorsement-chain/retrieveObligationEndorsementChain.tssrc/core/obligation-endorsement-chain/types.tssrc/core/obligation-endorsement-chain/useObligationEndorsementChain.tssrc/deploy/obligation-registry.tssrc/index.tssrc/obligation-registry-functions/accept.tssrc/obligation-registry-functions/discharge.tssrc/obligation-registry-functions/index.tssrc/obligation-registry-functions/mint.tssrc/obligation-registry-functions/reject.tssrc/obligation-registry-functions/rejectTransfers.tssrc/obligation-registry-functions/returnToken.tssrc/obligation-registry-functions/status.tssrc/obligation-registry-functions/transfer.tssrc/obligation-registry-functions/types.tssrc/obligation-registry-functions/utils.tssrc/obligation-registry-functions/verify.tssrc/obligation-registry/contracts.tssrc/obligation-registry/index.tssrc/obligation-registry/roleHash.tssrc/obligation-registry/supportInterfaceIds.tssrc/obligation-registry/utils.tssrc/utils/documents/index.tssrc/verify/fragments/document-status/obligationRecords/obligationRecordVerifier.tssrc/verify/fragments/document-status/obligationRecords/obligationRecordVerifier.types.tssrc/verify/fragments/document-status/obligationRecords/utils.tssrc/verify/fragments/document-status/transferableRecords/transferableRecordVerifier.tssrc/verify/fragments/index.tssrc/verify/verify.ts
…ication and token management - Updated AMOY_RPC_URL to provide a fallback URL when INFURA_API_KEY is not set.
…nsfers.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…to feature/boe-v-trustvc
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/verify/fragments/document-status/obligationRecords/utils.ts (1)
14-14: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valuePrefer optional chaining.
You can use optional chaining to make this condition more concise and remove the
Booleanwrapper, as the strict equality operator natively evaluates to a boolean.♻️ Proposed refactor
- const hasNonExistentSelector = Boolean(error.data && error.data.slice(0, 10) === '0x7e273289'); + const hasNonExistentSelector = error.data?.slice(0, 10) === '0x7e273289';🤖 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/verify/fragments/document-status/obligationRecords/utils.ts` at line 14, Update the hasNonExistentSelector assignment to use optional chaining when accessing error.data and compare the sliced value directly to the selector, removing the unnecessary Boolean wrapper while preserving the boolean result.Source: Linters/SAST tools
src/obligation-registry-functions/returnToken.ts (1)
26-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove redundant
signer.providerchecks.These manual
if (!signer.provider)checks are redundant because the utility functions they immediately call (resolveObligationEscrowAddressandconnectTrustVCToken) already enforce this exact validation internally. Removing them reduces boilerplate.
src/obligation-registry-functions/returnToken.ts#L26-L27: Removeif (!signer.provider) throw new Error('Provider is required');.src/obligation-registry-functions/returnToken.ts#L45-L46: Remove the redundant provider check.src/obligation-registry-functions/returnToken.ts#L65-L66: Remove the redundant provider check.src/obligation-registry-functions/rejectTransfers.ts#L23-L24: Remove the redundant provider check.src/obligation-registry-functions/transfer.ts#L22-L23: Remove the redundant provider check.src/obligation-registry-functions/mint.ts#L18-L18: Remove the redundant provider check.🤖 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/obligation-registry-functions/returnToken.ts` around lines 26 - 27, Remove the redundant signer.provider validation before the utility calls in returnToken.ts at lines 26-27, 45-46, and 65-66, rejectTransfers.ts at lines 23-24, transfer.ts at lines 22-23, and mint.ts at line 18. Preserve the existing calls to resolveObligationEscrowAddress and connectTrustVCToken, which provide the required validation.
🤖 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/obligation-registry-functions/lifecycle.ts`:
- Around line 34-42: Replace the manual provider/registry checks and direct
getObligationEscrowAddress calls with resolveObligationEscrowAddress({
obligationRegistry, tokenId }, signer), preserving the existing escrowAddress
flow and importing the resolver instead. Apply this in
src/obligation-registry-functions/lifecycle.ts lines 34-42 and
src/obligation-registry-functions/status.ts lines 18-26.
In `@src/obligation-registry-functions/utils.ts`:
- Around line 104-107: Update the catch block surrounding the callStatic
pre-check to include the original error message in the thrown Error while
retaining the existing precheck context. Use the caught error from the
callStatic failure and preserve the existing console logging behavior.
---
Nitpick comments:
In `@src/obligation-registry-functions/returnToken.ts`:
- Around line 26-27: Remove the redundant signer.provider validation before the
utility calls in returnToken.ts at lines 26-27, 45-46, and 65-66,
rejectTransfers.ts at lines 23-24, transfer.ts at lines 22-23, and mint.ts at
line 18. Preserve the existing calls to resolveObligationEscrowAddress and
connectTrustVCToken, which provide the required validation.
In `@src/verify/fragments/document-status/obligationRecords/utils.ts`:
- Line 14: Update the hasNonExistentSelector assignment to use optional chaining
when accessing error.data and compare the sliced value directly to the selector,
removing the unnecessary Boolean wrapper while preserving the boolean result.
🪄 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: b23a3728-8724-49c0-bf41-b2e26900cd8d
📒 Files selected for processing (18)
src/__tests__/core/verify.amoy.test.tssrc/__tests__/e2e/obligation-registry-functions/returnToken.e2e.test.tssrc/__tests__/verify/obligationRecordUtils.test.tssrc/__tests__/verify/obligationRecordVerifier.test.tssrc/core/obligation-endorsement-chain/fetchObligationEscrowTransfers.tssrc/core/obligation-endorsement-chain/findObligationMintBlock.tssrc/core/obligation-endorsement-chain/helpers.tssrc/obligation-registry-functions/index.tssrc/obligation-registry-functions/lifecycle.tssrc/obligation-registry-functions/mint.tssrc/obligation-registry-functions/rejectTransfers.tssrc/obligation-registry-functions/returnToken.tssrc/obligation-registry-functions/status.tssrc/obligation-registry-functions/transfer.tssrc/obligation-registry-functions/types.tssrc/obligation-registry-functions/utils.tssrc/verify/fragments/document-status/obligationRecords/obligationRecordVerifier.tssrc/verify/fragments/document-status/obligationRecords/utils.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- src/tests/core/verify.amoy.test.ts
- src/core/obligation-endorsement-chain/fetchObligationEscrowTransfers.ts
- src/verify/fragments/document-status/obligationRecords/obligationRecordVerifier.ts
- src/obligation-registry-functions/types.ts
- src/tests/e2e/obligation-registry-functions/returnToken.e2e.test.ts
- src/core/obligation-endorsement-chain/helpers.ts
- src/core/obligation-endorsement-chain/findObligationMintBlock.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…to feature/boe-v-trustvc
- Replaced dynamic AMOY_RPC_URL with a static URL in documentBuilder tests.
Contributor
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 `@README.md`:
- Line 758: Update the fenced code block in README.md around the ASCII diagram
to specify the text language, changing the opening fence to use text while
preserving the diagram content and closing fence.
In `@src/__tests__/core/documentBuilder.test.ts`:
- Around line 140-142: Investigate the obligationRecords credentialStatus
implementation used by documentBuilder and replace the undefined context value
with the correct obligation-records context URL, ensuring the required constant
is properly defined or imported. Preserve the expected URL in the serialized
JSON-LD `@context` and update the test only if needed to reflect the corrected
implementation.
🪄 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: 0c188bdd-455e-40d2-9ee6-78e6490cd1d2
📒 Files selected for processing (6)
README.mdsrc/__tests__/core/documentBuilder.test.tssrc/__tests__/core/verify.test.tssrc/__tests__/e2e/obligation-registry-functions/returnToken.e2e.test.tssrc/obligation-registry-functions/lifecycle.tssrc/obligation-registry-functions/utils.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/obligation-registry-functions/lifecycle.ts
- src/tests/core/verify.test.ts
- src/tests/e2e/obligation-registry-functions/returnToken.e2e.test.ts
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
src/verify-obligation/fragments/document-status/obligationRecords/obligationRecordVerifier.ts (1)
73-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant optional-chain check.
!credentialStatus?.tokenNetwork || !credentialStatus?.tokenNetwork?.chainIdis redundant — iftokenNetworkis missing,?.chainIdalready evaluates toundefined.Proposed simplification
- if (!credentialStatus?.tokenNetwork || !credentialStatus?.tokenNetwork?.chainId) { + if (!credentialStatus?.tokenNetwork?.chainId) {🤖 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/verify-obligation/fragments/document-status/obligationRecords/obligationRecordVerifier.ts` at line 73, Update the condition in the obligation record verifier to remove the redundant credentialStatus.tokenNetwork existence check, relying on the optional chain through tokenNetwork.chainId to cover missing tokenNetwork while preserving the existing falsy chainId behavior.Source: Linters/SAST tools
src/verify-obligation/fragments/document-status/w3cCredentialStatus.ts (1)
68-114: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify nested ternaries for status-code/message selection.
The triple-nested ternaries (lines 99-114) determining
code/codeString/messageare hard to follow and easy to get wrong on future edits; static analysis flags this three times. Alsopurposes(line 68) could be aSetfor cheaper membership checks.Proposed refactor
- reason: { - code: hasRevocationAndSuspension - ? W3CCredentialStatusCode.DOCUMENT_REVOKED_AND_SUSPENDED - : hasRevocation - ? W3CCredentialStatusCode.DOCUMENT_REVOKED - : W3CCredentialStatusCode.DOCUMENT_SUSPENDED, - codeString: hasRevocationAndSuspension - ? 'REVOKED_AND_SUSPENDED' - : hasRevocation - ? 'REVOKED' - : 'SUSPENDED', - message: hasRevocationAndSuspension - ? 'Document has been revoked and suspended.' - : hasRevocation - ? 'Document has been revoked.' - : 'Document has been suspended.', - }, + reason: getRevocationReason(hasRevocation, hasSuspension),with a small helper
getRevocationReason(hasRevocation, hasSuspension)returning the appropriate{code, codeString, message}triple.🤖 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/verify-obligation/fragments/document-status/w3cCredentialStatus.ts` around lines 68 - 114, Refactor the status-reason selection in the verification result flow to use a small helper such as getRevocationReason(hasRevocation, hasSuspension), returning the code, codeString, and message for each revocation/suspension combination instead of nested ternaries. Update purposes to a Set and use membership checks while preserving the existing precedence and returned status behavior.Source: Linters/SAST tools
src/core/verifyObligation.ts (1)
31-32: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSilent default RPC provider when neither
providernorrpcProviderUrlis supplied.
new ethers.providers.JsonRpcProvider(options?.rpcProviderUrl)withrpcProviderUrlundefined connects to ethers' default local endpoint rather than failing fast on missing configuration. This can hide misconfiguration when a document actually needs on-chain lookups.Proposed fix
const provider = - options?.provider || new ethers.providers.JsonRpcProvider(options?.rpcProviderUrl); + options?.provider ?? + (options?.rpcProviderUrl + ? new ethers.providers.JsonRpcProvider(options.rpcProviderUrl) + : (() => { + throw new Error('Either `provider` or `rpcProviderUrl` must be supplied.'); + })());🤖 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/core/verifyObligation.ts` around lines 31 - 32, Update the provider initialization in verifyObligation so it fails fast when neither options.provider nor options.rpcProviderUrl is supplied, instead of constructing JsonRpcProvider with an undefined URL. Preserve the existing custom provider and configured rpcProviderUrl paths, and make the missing-configuration failure explicit before any on-chain lookup.
🤖 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/verify-obligation/fragments/document-status/obligationRecords/utils.ts`:
- Around line 59-70: Update isTokenMintedOnObligationRegistry to accept the
credential’s declared chain ID, query the provider network, and reject
mismatches before calling ownerOf or returning a minted status. Pass
credentialStatus.tokenNetwork.chainId from the caller and preserve escrow
enrichment only for responses validated against that chain.
In `@src/verify-obligation/fragments/issuer-identity/w3cIssuerIdentity.ts`:
- Around line 6-28: Update checkDidResolve to stop converting all resolution
exceptions into false: preserve false only for confirmed unresolved or invalid
DID results, and propagate network, timeout, malformed-response, and other
operational errors. Update verify() to catch propagated resolution errors and
return ERROR, while retaining INVALID for a false result.
---
Nitpick comments:
In `@src/core/verifyObligation.ts`:
- Around line 31-32: Update the provider initialization in verifyObligation so
it fails fast when neither options.provider nor options.rpcProviderUrl is
supplied, instead of constructing JsonRpcProvider with an undefined URL.
Preserve the existing custom provider and configured rpcProviderUrl paths, and
make the missing-configuration failure explicit before any on-chain lookup.
In
`@src/verify-obligation/fragments/document-status/obligationRecords/obligationRecordVerifier.ts`:
- Line 73: Update the condition in the obligation record verifier to remove the
redundant credentialStatus.tokenNetwork existence check, relying on the optional
chain through tokenNetwork.chainId to cover missing tokenNetwork while
preserving the existing falsy chainId behavior.
In `@src/verify-obligation/fragments/document-status/w3cCredentialStatus.ts`:
- Around line 68-114: Refactor the status-reason selection in the verification
result flow to use a small helper such as getRevocationReason(hasRevocation,
hasSuspension), returning the code, codeString, and message for each
revocation/suspension combination instead of nested ternaries. Update purposes
to a Set and use membership checks while preserving the existing precedence and
returned status 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 297324cd-a132-4f9c-902b-be99789756a0
📒 Files selected for processing (23)
README.mdsrc/__tests__/core/verify.test.tssrc/__tests__/obligation-registry-functions/verify.test.tssrc/__tests__/verify-obligation/obligationRecordUtils.test.tssrc/__tests__/verify-obligation/obligationRecordVerifier.test.tssrc/core/documentBuilder.tssrc/core/index.tssrc/core/verifyObligation.tssrc/index.tssrc/obligation-registry-functions/verify.tssrc/verify-obligation/fragments/document-integrity/bbs2023W3CSignatureIntegrity.tssrc/verify-obligation/fragments/document-integrity/ecdsaW3CSignatureIntegrity.tssrc/verify-obligation/fragments/document-integrity/w3cModernSignatureIntegrityFactory.tssrc/verify-obligation/fragments/document-integrity/w3cSignatureIntegrity.tssrc/verify-obligation/fragments/document-status/obligationRecords/obligationRecordVerifier.tssrc/verify-obligation/fragments/document-status/obligationRecords/obligationRecordVerifier.types.tssrc/verify-obligation/fragments/document-status/obligationRecords/utils.tssrc/verify-obligation/fragments/document-status/w3cCredentialStatus.tssrc/verify-obligation/fragments/document-status/w3cEmptyCredentialStatus/index.tssrc/verify-obligation/fragments/index.tssrc/verify-obligation/fragments/issuer-identity/w3cIssuerIdentity.tssrc/verify-obligation/index.tssrc/verify-obligation/verify.ts
💤 Files with no reviewable changes (1)
- src/tests/core/verify.test.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- src/core/index.ts
- src/index.ts
- src/obligation-registry-functions/verify.ts
- src/core/documentBuilder.ts
…es for ethers v6 compatibility
…tor existing tests
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
src/core/obligationDocumentBuilder.ts (3)
237-243: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the nested ternary for readability (SonarCloud).
♻️ Suggested fix
private buildContext(context: string | string[]): string[] { - const arrayContext = Array.isArray(context) ? context : context ? [context] : []; + let arrayContext: string[]; + if (Array.isArray(context)) { + arrayContext = context; + } else { + arrayContext = context ? [context] : []; + } if (arrayContext.includes(VC_V1_URL)) {🤖 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/core/obligationDocumentBuilder.ts` around lines 237 - 243, Update buildContext so the nested ternary used to initialize arrayContext is replaced with a clear equivalent conditional flow, preserving the existing handling for array, truthy single-value, and empty contexts.Source: Linters/SAST tools
258-269: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueSequential awaits can be parallelized.
isV4SupportedandisV5Supportedare independent network calls; running them viaPromise.allreduces round-trip latency.♻️ Suggested fix
- const isV4Supported = await this.supportsInterface( - v4Contracts.TradeTrustToken__factory, - constantsV4.contractInterfaceId.TradeTrustTokenMintable, - provider, - ); - const isV5Supported = await this.supportsInterface( - v5Contracts.TradeTrustToken__factory, - constantsV5.contractInterfaceId.TradeTrustTokenMintable, - provider, - ); + const [isV4Supported, isV5Supported] = await Promise.all([ + this.supportsInterface( + v4Contracts.TradeTrustToken__factory, + constantsV4.contractInterfaceId.TradeTrustTokenMintable, + provider, + ), + this.supportsInterface( + v5Contracts.TradeTrustToken__factory, + constantsV5.contractInterfaceId.TradeTrustTokenMintable, + provider, + ), + ]);🤖 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/core/obligationDocumentBuilder.ts` around lines 258 - 269, Parallelize the independent interface checks in the try block of the obligation document builder by starting both supportsInterface calls together and awaiting them with Promise.all, while preserving the existing provider, factories, interface IDs, and isV4Supported/isV5Supported result assignments.
42-51: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMark never-reassigned members
readonly.
documentType(Line 44) andrequiredFields(Line 48) are only ever set once (declaration/constructor) and never reassigned elsewhere in the class — safe to markreadonly.♻️ Suggested fix
- private documentType: string = 'w3c'; + private readonly documentType: string = 'w3c'; private selectedStatusType: 'obligationRecords' | 'verifiableDocument' | null = null; private statusConfig: Partial<CredentialStatus> = {}; private rpcProviderUrl: string; - private requiredFields: string[] = ['credentialSubject']; + private readonly requiredFields: string[] = ['credentialSubject'];🤖 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/core/obligationDocumentBuilder.ts` around lines 42 - 51, Mark the never-reassigned documentType and requiredFields members in ObligationDocumentBuilder as readonly, preserving their existing initialization and all other class behavior.Source: Linters/SAST tools
src/__tests__/core/documentBuilder.obligation.test.ts (1)
13-25: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the error branches of
credentialStatus().Only the obligation-records happy path is tested. Given
credentialStatus()also throws on mixed obligation/verifiable config and on missing required fields, and the class distinguishes averifiableDocumentbranch, a couple more tests would meaningfully increase confidence in this new builder.🤖 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/__tests__/core/documentBuilder.obligation.test.ts` around lines 13 - 25, The documentBuilder obligation tests only cover the successful obligation-records path. Add tests for credentialStatus() that assert errors are thrown for mixed obligation/verifiable configuration and for missing required fields, and cover the verifiableDocument branch with its expected behavior. Keep the existing happy-path test unchanged.
🤖 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/core/obligationDocumentBuilder.ts`:
- Around line 272-281: Update the catch handling in the obligation document
builder to preserve and rethrow configuration errors from supportsInterface,
including “Configuration Error: Missing obligationRegistry for interface check.”
Keep the existing special handling for the unsupported registry version, but
avoid rewriting known configuration failures as generic network errors; retain
the network error conversion only for genuine verification failures.
- Around line 284-298: The supportsInterface method currently passes an ethers
v5 provider to both registry factories, masking the mismatch with a never cast.
Use the existing ethersV6 provider when contractFactory is the v5
TradeTrustToken factory, while retaining the ethers v5 provider for the v4
factory and preserving the existing registry address validation.
- Around line 251-256: Update verifyObligationRegistry to query the network from
this.rpcProviderUrl via the configured RPC provider and compare its chain ID
with the credential’s tokenNetwork.chainId before performing interface
validation. Reject mismatches with an explicit error, while preserving the
existing SUPPORTED_CHAINS validation for unsupported chain IDs.
---
Nitpick comments:
In `@src/__tests__/core/documentBuilder.obligation.test.ts`:
- Around line 13-25: The documentBuilder obligation tests only cover the
successful obligation-records path. Add tests for credentialStatus() that assert
errors are thrown for mixed obligation/verifiable configuration and for missing
required fields, and cover the verifiableDocument branch with its expected
behavior. Keep the existing happy-path test unchanged.
In `@src/core/obligationDocumentBuilder.ts`:
- Around line 237-243: Update buildContext so the nested ternary used to
initialize arrayContext is replaced with a clear equivalent conditional flow,
preserving the existing handling for array, truthy single-value, and empty
contexts.
- Around line 258-269: Parallelize the independent interface checks in the try
block of the obligation document builder by starting both supportsInterface
calls together and awaiting them with Promise.all, while preserving the existing
provider, factories, interface IDs, and isV4Supported/isV5Supported result
assignments.
- Around line 42-51: Mark the never-reassigned documentType and requiredFields
members in ObligationDocumentBuilder as readonly, preserving their existing
initialization and all other class 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0675280b-e8d4-4147-acfe-81c33ec5d734
📒 Files selected for processing (12)
README.mdsrc/__tests__/core/documentBuilder.obligation.test.tssrc/__tests__/core/verify.rpc-resilience.test.tssrc/__tests__/e2e/README.mdsrc/__tests__/e2e/obligation-registry-functions/endorsementChain.e2e.test.tssrc/__tests__/e2e/obligation-registry-functions/rejectTransfer.e2e.test.tssrc/__tests__/e2e/obligation-registry-functions/returnToken.e2e.test.tssrc/__tests__/e2e/obligation-registry-functions/statusLifecycle.e2e.test.tssrc/__tests__/e2e/obligation-registry-functions/transfer.e2e.test.tssrc/__tests__/utils/documents/obligation.test.tssrc/core/index.tssrc/core/obligationDocumentBuilder.ts
🚧 Files skipped from review as they are similar to previous changes (8)
- src/core/index.ts
- src/tests/e2e/README.md
- src/tests/e2e/obligation-registry-functions/returnToken.e2e.test.ts
- src/tests/e2e/obligation-registry-functions/endorsementChain.e2e.test.ts
- src/tests/e2e/obligation-registry-functions/rejectTransfer.e2e.test.ts
- src/tests/e2e/obligation-registry-functions/transfer.e2e.test.ts
- src/tests/e2e/obligation-registry-functions/statusLifecycle.e2e.test.ts
- README.md
…e related documentation
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
This was referenced Jul 30, 2026
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.
capabilities.
Summary
What is the background of this pull request?
Changes
Issues
What are the related issues or stories?
Summary by CodeRabbit
New Features
Documentation
Tests