feat: implement obligation registry functions and enhance document bu… - #155
Merged
Conversation
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds Obligation Registry APIs, BoE credential-status verification and ChangesObligation Registry API and contracts
BoE document verification and signing
Validation and documentation
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 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: 8
🧹 Nitpick comments (2)
src/obligation-registry-functions/deploy.ts (1)
2-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMerge duplicate import from the same module.
SonarCloud flags
'../token-registry-functions/utils'imported twice.♻️ Proposed fix
-import { getTxOptions } from '../token-registry-functions/utils'; -import { getChainIdSafe } from '../token-registry-functions/utils'; +import { getTxOptions, getChainIdSafe } from '../token-registry-functions/utils';🤖 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/deploy.ts` around lines 2 - 3, Combine the duplicate imports from '../token-registry-functions/utils' into a single import declaration containing both getTxOptions and getChainIdSafe.Source: Linters/SAST tools
src/__tests__/e2e/utils.ts (1)
40-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider consolidating with
getVersionedContractFactory/createContractto avoid duplicated ethers-version branching.
getObligationContractFactoryandcreateObligationContractre-implement the same v5/v6 factory/contract-selection logic asgetVersionedContractFactory(Line 6) andcreateContract(Line 24), just with a fixed contract source (v5Contracts). Parameterizing the existing helpers by contract-source instead of adding parallel obligation-specific variants would reduce the number of places that need updating if ethers-version handling changes.♻️ Sketch of a consolidated helper
-export function getObligationContractFactory( - contractName: 'TrustVCToken' | 'ObligationEscrowFactory', - ethersVersion: 'v5' | 'v6', - owner: Signer | ContractRunner, -) { - const Factory = ethersVersion === 'v5' ? ethers.ContractFactory : ethersV6.ContractFactory; - const signer = ethersVersion === 'v5' ? (owner as Signer) : (owner as ContractRunner); - - return new Factory( - v5Contracts[`${contractName}__factory`].abi, - v5Contracts[`${contractName}__factory`].bytecode, - // eslint-disable-next-line `@typescript-eslint/no-explicit-any` - signer as any, - ); -} +export function getObligationContractFactory( + contractName: 'TrustVCToken' | 'ObligationEscrowFactory', + ethersVersion: 'v5' | 'v6', + owner: Signer | ContractRunner, +) { + return getVersionedContractFactory(contractName as any, ethersVersion, 'v5', owner); +}🤖 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/utils.ts` around lines 40 - 68, Consolidate getObligationContractFactory and createObligationContract with the existing getVersionedContractFactory and createContract helpers by parameterizing those helpers to accept the fixed v5Contracts source. Update obligation callers to use the shared helpers, preserve the existing contract names, ABIs, bytecode, and signer behavior, and remove the duplicated ethers-version branching.
🤖 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 774: Update the fenced lifecycle diagram code block in README.md to
specify the text language by changing its opening fence to use text, while
preserving the diagram content and closing fence.
In `@src/__tests__/e2e/obligation-registry-functions/transfer.e2e.test.ts`:
- Around line 76-84: In the transferHolderObligationRegistry rejection
assertions, replace the unavailable rejectedWith matcher with the Hardhat
revertedWith matcher at all three referenced assertions, preserving the existing
expected error message patterns.
In `@src/obligation-registry-functions/mint.ts`:
- Around line 39-52: Replace the duplicated encryption and static-call logic in
src/obligation-registry-functions/mint.ts lines 39-52 with
getEncryptedRemarks(remarks, options.id) and runStaticCall for mint using
signer.provider; apply the same change in
src/obligation-registry-functions/returnToken.ts lines 68-81 for
acceptReturnedObligationRegistry using burn, and lines 113-126 for
rejectReturnedObligationRegistry using restore, preserving the shared helper
behavior across all three sites.
In `@src/obligation-registry-functions/ownerOf.ts`:
- Around line 16-19: In the ownerOf function, replace the unused options
parameter with the module’s underscore-prefixed convention (for example,
_options), remove the void options statement, and preserve the existing API
signature and behavior.
In `@src/obligation-registry-functions/status.ts`:
- Around line 12-54: Clarify the token ID behavior in
getObligationRegistryStatus, isObligationRegistryRegistered, and
getObligationEscrowTerminationReason by documenting that _params.tokenId is
unused and contractOptions.tokenId is the authoritative value for escrow
resolution. Keep the existing function signatures and logic unchanged.
In `@src/obligation-registry-functions/utils.ts`:
- Around line 73-78: Update getEncryptedRemarks so supplied remarks are not
discarded when id is undefined; preserve the existing encrypted 0x-prefixed
result when both remarks and id are present, and define a non-lossy fallback for
remarks without an id instead of returning only '0x'.
- Around line 33-50: Update the tokenId validation in the obligation escrow
resolution path around getObligationEscrowAddress to accept numeric 0 while
rejecting missing or empty token IDs. Replace the truthiness check with explicit
undefined and empty-value handling, preserving validation for genuinely absent
values and the existing resolution flow.
In `@src/verify/fragments/document-status/obligationRecords/utils.ts`:
- Around line 73-90: Update the catch handling around the canonical ownerOf call
to return DOCUMENT_NOT_MINTED only for its expected revert error; rethrow all
other Ethers/provider, network, and address failures so the verifier wrapper
produces ERROR. Preserve the existing CodedError propagation and minted:false
response for the canonical revert.
---
Nitpick comments:
In `@src/__tests__/e2e/utils.ts`:
- Around line 40-68: Consolidate getObligationContractFactory and
createObligationContract with the existing getVersionedContractFactory and
createContract helpers by parameterizing those helpers to accept the fixed
v5Contracts source. Update obligation callers to use the shared helpers,
preserve the existing contract names, ABIs, bytecode, and signer behavior, and
remove the duplicated ethers-version branching.
In `@src/obligation-registry-functions/deploy.ts`:
- Around line 2-3: Combine the duplicate imports from
'../token-registry-functions/utils' into a single import declaration containing
both getTxOptions and getChainIdSafe.
🪄 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: 973dedb2-292e-48c0-b49b-2c1b74a60dac
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (39)
README.mdpackage.jsonsrc/__tests__/e2e/fixtures/sample-boe-credential.tssrc/__tests__/e2e/obligation-registry-functions/fixtures.tssrc/__tests__/e2e/obligation-registry-functions/statusLifecycle.e2e.test.tssrc/__tests__/e2e/obligation-registry-functions/transfer.e2e.test.tssrc/__tests__/e2e/utils.tssrc/__tests__/obligation-registry-functions/deploy.test.tssrc/__tests__/obligation-registry-functions/fixtures.tssrc/__tests__/obligation-registry-functions/lifecycle.test.tssrc/__tests__/obligation-registry-functions/mint.test.tssrc/__tests__/obligation-registry-functions/ownerOf.test.tssrc/__tests__/obligation-registry-functions/rejectTransfers.test.tssrc/__tests__/obligation-registry-functions/returnToken.test.tssrc/__tests__/obligation-registry-functions/status.test.tssrc/__tests__/obligation-registry-functions/transfers.test.tssrc/core/documentBuilder.tssrc/core/endorsement-chain/index.tssrc/core/endorsement-chain/obligation.tssrc/obligation-registry-functions/deploy.tssrc/obligation-registry-functions/index.tssrc/obligation-registry-functions/lifecycle.tssrc/obligation-registry-functions/mint.tssrc/obligation-registry-functions/ownerOf.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/token-registry-v5/contracts.tssrc/utils/documents/index.tssrc/utils/documents/obligation.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
…est.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>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…emarkEscrowMethod
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/verify/fragments/document-status/transferableRecords/transferableRecordVerifier.ts (1)
112-117: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winPreserve
tokenRegistryvalidation for transferable records.The new predicate excludes obligation records but no longer requires a non-empty
tokenRegistry. A malformedTransferableRecordsstatus with neither registry address is therefore selected by this verifier and can reach the token-registry RPC path with an undefined address.Retain the existing
tokenRegistrypresence check alongside the obligation-record exclusion.Proposed fix
credentialStatuses.every( (cs: w3cVC.CredentialStatus) => - cs?.type === TRANSFERABLE_RECORDS_TYPE && !isObligationRecordCredentialStatus(cs), + cs?.type === TRANSFERABLE_RECORDS_TYPE && + typeof cs?.tokenRegistry === 'string' && + cs.tokenRegistry.length > 0 && + !isObligationRecordCredentialStatus(cs), )🤖 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/transferableRecords/transferableRecordVerifier.ts` around lines 112 - 117, Update the credential-status predicate in the transferable-record verifier to require a non-empty tokenRegistry while continuing to exclude obligation-record statuses. Preserve the existing TRANSFERABLE_RECORDS_TYPE check and ensure malformed statuses without a registry are not selected for token-registry RPC processing.src/core/documentBuilder.ts (1)
58-71: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPopulate
tokenIdbefore signing obligation-record credentials.
W3CObligationRecordsConfigandobligationCredentialStatus()omittokenId, butsign()overwrites the document status with this incompletestatusConfig. The downstream verifier constructs the lookup token fromcredentialStatus.tokenId(src/verify/fragments/document-status/obligationRecords/obligationRecordVerifier.ts, Lines 42-42), so credentials built through this API reach verification with0xundefined.Add
tokenIdto the configuration and copy it intostatusConfig, or preserve and require an existing token ID before signing.Proposed fix
export interface W3CObligationRecordsConfig { + tokenId: string; chain: string; chainId: number; obligationRegistry: string; rpcProviderUrl: string; } this.statusConfig = { type: 'TransferableRecords', + tokenId: config.tokenId, tokenNetwork: { chain: config.chain, chainId: config.chainId }, obligationRegistry: config.obligationRegistry, };Also applies to: 174-188
🤖 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/documentBuilder.ts` around lines 58 - 71, Update W3CObligationRecordsConfig and obligationCredentialStatus() to accept a required tokenId, then ensure sign() copies that value into the statusConfig it writes to the credential. Preserve the existing chain, chainId, obligationRegistry, and rpcProviderUrl fields while preventing credentials from being signed with an undefined tokenId.
🧹 Nitpick comments (1)
src/obligation-registry-functions/status.ts (1)
32-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
isObligationRegistryRegisteredsilently loses itsbooleanreturn typing.Unlike its sibling readers (
getObligationRegistryStatus,getObligationEscrowTerminationReason), which explicitly cast their contract-call results,isObligationRegistryRegistereddoesn't cast/annotate its result. Ethers' dynamicContractindex signature types undeclared methods asContractFunction<any>(returnsPromise<any>), soThere is inferred asany— the exported function's return type silently widens frombooleantoanyfor consumers.🧹 Proposed fix
-const isObligationRegistryRegistered = createEscrowViewReader((contract, options) => - contract.isRegistered({ blockTag: options.blockTag }), -); +const isObligationRegistryRegistered = createEscrowViewReader<boolean>(async (contract, options) => + Boolean(await contract.isRegistered({ blockTag: options.blockTag })), +);🤖 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 32 - 46, Update isObligationRegistryRegistered to explicitly preserve a boolean return type, either by annotating the callback/result or casting the contract.isRegistered call to boolean. Keep the existing blockTag option forwarding and reader structure 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 `@README.md`:
- Line 921: Update the README Obligation/BoE example so
obligationCredentialStatus documents credentialStatus.type as ObligationRecords
rather than TransferableRecords, ensuring the example matches the
ObligationRecords verification flow used by verifyDocument.
In `@src/verify/fragments/document-status/obligationRecords/utils.ts`:
- Around line 32-35: Update the chain ID conversion in the expectedChainId
validation flow to reject malformed strings with trailing characters; use strict
numeric conversion such as Number(chainId) while preserving numeric inputs and
the existing finite-value and network.chainId comparison checks.
---
Outside diff comments:
In `@src/core/documentBuilder.ts`:
- Around line 58-71: Update W3CObligationRecordsConfig and
obligationCredentialStatus() to accept a required tokenId, then ensure sign()
copies that value into the statusConfig it writes to the credential. Preserve
the existing chain, chainId, obligationRegistry, and rpcProviderUrl fields while
preventing credentials from being signed with an undefined tokenId.
In
`@src/verify/fragments/document-status/transferableRecords/transferableRecordVerifier.ts`:
- Around line 112-117: Update the credential-status predicate in the
transferable-record verifier to require a non-empty tokenRegistry while
continuing to exclude obligation-record statuses. Preserve the existing
TRANSFERABLE_RECORDS_TYPE check and ensure malformed statuses without a registry
are not selected for token-registry RPC processing.
---
Nitpick comments:
In `@src/obligation-registry-functions/status.ts`:
- Around line 32-46: Update isObligationRegistryRegistered to explicitly
preserve a boolean return type, either by annotating the callback/result or
casting the contract.isRegistered call to boolean. Keep the existing blockTag
option forwarding and reader structure unchanged.
🪄 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: 650fc8f6-2350-4308-b261-86a10438ac4d
📒 Files selected for processing (24)
.github/workflows/tests.ymlREADME.mdsrc/__tests__/core/documentBuilder.test.tssrc/__tests__/core/verify.amoy.test.tssrc/__tests__/core/verify.test.tssrc/__tests__/e2e/obligation-registry-functions/transfer.e2e.test.tssrc/__tests__/obligation-registry-functions/ownerOf.test.tssrc/core/documentBuilder.tssrc/index.tssrc/obligation-registry-functions/deploy.tssrc/obligation-registry-functions/lifecycle.tssrc/obligation-registry-functions/mint.tssrc/obligation-registry-functions/ownerOf.tssrc/obligation-registry-functions/rejectTransfers.tssrc/obligation-registry-functions/returnToken.tssrc/obligation-registry-functions/status.tssrc/obligation-registry-functions/transfer.tssrc/obligation-registry-functions/utils.tssrc/utils/documents/obligation.tssrc/verify/fragments/document-status/obligationRecords/obligationRecordVerifier.tssrc/verify/fragments/document-status/obligationRecords/utils.tssrc/verify/fragments/document-status/obligationRecords/verifierHelpers.tssrc/verify/fragments/document-status/transferableRecords/transferableRecordVerifier.tssrc/verify/fragments/index.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/obligation-registry-functions/deploy.ts
… in obligation registry utility
…date event handling
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/documentBuilder.ts (1)
321-330: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject empty obligation-registry configuration values.
typeof value === 'string'accepts emptychain,obligationRegistry, andrpcProviderUrlvalues. These invalid configurations pass this guard and fail later as misleading network or address errors.Proposed fix
return ( - typeof config?.obligationRegistry === 'string' && - typeof config?.chain === 'string' && + typeof config?.obligationRegistry === 'string' && + config.obligationRegistry.length > 0 && + typeof config?.chain === 'string' && + config.chain.length > 0 && typeof config?.chainId === 'number' && - typeof config?.rpcProviderUrl === 'string' + typeof config?.rpcProviderUrl === 'string' && + config.rpcProviderUrl.length > 0 );🤖 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/documentBuilder.ts` around lines 321 - 330, Update isObligationRecordsConfig to require non-empty obligationRegistry, chain, and rpcProviderUrl strings, while retaining the existing numeric chainId validation. Reject empty values before the configuration is accepted.
🤖 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/__tests__/e2e/obligation-registry-functions/statusLifecycle.e2e.test.ts`:
- Line 5: Update the test setup in statusLifecycle.e2e.test.ts to import Chai
and chai-as-promised, then register the plugin with chai.use(chaiAsPromised)
before any assertions use the .rejected or .rejectedWith matchers.
In `@src/core/documentBuilder.ts`:
- Around line 109-113: Reformat the selectedStatusType union declaration to
match the repository’s Prettier output, preserving all existing string-literal
members and the null initializer while eliminating the ESLint formatting
violation.
In `@src/verify/fragments/document-status/obligationRecords/utils.ts`:
- Around line 101-114: Restrict the catch handling around the obligation status
calls so only the expected ownerOf absence revert is converted through
decodeError into notMintedReason. Do not apply the DOCUMENT_NOT_MINTED fallback
to failures from escrow address, active, or isHoldingToken calls; preserve
CodedError propagation and rethrow unexpected RPC or subsequent-call failures so
verification reports ERROR.
---
Outside diff comments:
In `@src/core/documentBuilder.ts`:
- Around line 321-330: Update isObligationRecordsConfig to require non-empty
obligationRegistry, chain, and rpcProviderUrl strings, while retaining the
existing numeric chainId validation. Reject empty values before the
configuration is accepted.
🪄 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: 744cfbd2-59b6-4c27-891e-773d6802432e
📒 Files selected for processing (19)
README.mdsrc/__tests__/e2e/fixtures/sample-boe-credential.tssrc/__tests__/e2e/obligation-registry-functions/statusLifecycle.e2e.test.tssrc/__tests__/e2e/obligation-registry-functions/transfer.e2e.test.tssrc/__tests__/fixtures/endorsement-chain.tssrc/__tests__/obligation-registry-functions/lifecycle.test.tssrc/__tests__/obligation-registry-functions/mint.test.tssrc/__tests__/obligation-registry-functions/rejectTransfers.test.tssrc/__tests__/obligation-registry-functions/returnToken.test.tssrc/__tests__/obligation-registry-functions/status.test.tssrc/__tests__/obligation-registry-functions/transfers.test.tssrc/__tests__/verify/obligationRecordVerifier.utils.test.tssrc/core/documentBuilder.tssrc/core/endorsement-chain/fetchEscrowTransfer.tssrc/core/endorsement-chain/helpers.tssrc/core/endorsement-chain/types.tssrc/core/endorsement-chain/useEndorsementChain.tssrc/obligation-registry-functions/transfer.tssrc/verify/fragments/document-status/obligationRecords/utils.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/tests/obligation-registry-functions/returnToken.test.ts
- src/tests/obligation-registry-functions/lifecycle.test.ts
- README.md
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
34-34: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winFix the broken
Overviewtable-of-contents link.Line 34 links to
#overview-1, but markdownlint reports no matching heading. Use the anchor generated by theOverviewheading.Suggested fix
- - [Overview](`#overview-1`) + - [Overview](`#overview`)🤖 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 `@README.md` at line 34, Update the Overview table-of-contents entry to reference the anchor generated by the Overview heading instead of the nonexistent `#overview-1` target. Change only the link target in the Overview entry and preserve its displayed text.Source: Linters/SAST tools
🤖 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 930: Align the BoE credential-status contract across
obligationCredentialStatus(), the status reader, and the ObligationRecords
verifier by using the documented ObligationRecords type with obligationRegistry,
or consistently update the implementation and README to use TransferableRecords.
Ensure the verifier’s type check matches the builder’s emitted wire value.
---
Outside diff comments:
In `@README.md`:
- Line 34: Update the Overview table-of-contents entry to reference the anchor
generated by the Overview heading instead of the nonexistent `#overview-1` target.
Change only the link target in the Overview entry and preserve its displayed
text.
🪄 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: c8130ba0-a30c-4291-93fb-4da9cf383318
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
.github/workflows/tests.ymlREADME.mdpackage.jsonsrc/index.tssrc/verify/fragments/document-status/obligationRecords/utils.tssrc/verify/fragments/document-status/transferableRecords/utils.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- .github/workflows/tests.yml
- src/index.ts
- package.json
Contributor
|
@coderabbitai full review |
Contributor
✅ Action performedFull review finished. |
Contributor
✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 56 minutes. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
src/__tests__/obligation-registry-functions/mint.test.ts (1)
48-79: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the final
mintcontract arguments.The tests only verify the
encryptcall. They do not verify thatmintObligationRegistrysends the encrypted value to the registry.Assert the beneficiary, holder, token ID, encrypted remarks, and transaction overrides passed to
mockTrustVCTokenContract.mint. Also assert that the no-remarks case sends'0x'.🤖 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__/obligation-registry-functions/mint.test.ts` around lines 48 - 79, The mintObligationRegistry tests currently verify encryption but not the contract invocation. Extend both “mints with remarks” and “mints without remarks” cases to assert mockTrustVCTokenContract.mint receives the beneficiary, holder, token ID, encrypted remarks, and transaction overrides, using the encrypted value for the remarks case and '0x' when remarks are omitted.src/utils/documents/obligation.ts (1)
1-4: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBreak the barrel-import cycle.
src/utils/documents/obligation.tsimportsTRANSFERABLE_RECORDS_TYPEfromsrc/verify/fragments, andsrc/verify/fragments/index.tsre-exports it from the verifier module. The verifier module importsisObligationRecordCredentialStatusfromsrc/utils/documents/obligation, creating a circular import path across both ESM and CommonJS package entry points. MoveTRANSFERABLE_RECORDS_TYPEto a leaf constants module and import it directly from both modules.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/documents/obligation.ts` around lines 1 - 4, Move TRANSFERABLE_RECORDS_TYPE from the verifier/barrel export path into a leaf constants module, then update obligation.ts and the verifier module to import it directly from that module. Remove the dependency on src/verify/fragments for this constant while preserving existing exports and behavior.
🤖 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/documentBuilder.ts`:
- Around line 173-194: Update obligationCredentialStatus() to reject calls when
credentialStatus() has already configured the builder, using the same
mutual-exclusivity guard and configuration error behavior as credentialStatus().
Preserve the existing validation and status setup when no conflicting
credential-status configuration exists.
- Around line 401-442: Extract the duplicated chain validation from
verifyObligationRegistry() and verifyTokenRegistry() into a shared private
helper, and reuse it in both methods. Also extract their common try/catch
behavior into a private error-wrapping helper that rethrows the
unsupported-registry error while wrapping other failures as the existing network
error; preserve each method’s current validation and error semantics.
In `@src/obligation-registry-functions/utils.ts`:
- Around line 101-119: Update sendTransaction and the related
executeEscrowMethod, createRemarkEscrowMethod, executeRegistryMethod, and public
mint/transfer/returnToken wrappers to expose the correct ethers v5 or v6
transaction response type based on the contract/signer used. Define and reuse a
shared response type or signer-specific overloads, ensuring ethers v6 calls
return ContractTransactionResponse while preserving ethers v5
ContractTransaction behavior.
In `@src/utils/documents/index.ts`:
- Around line 37-39: Update the transferability classification in
isTransferableRecord to evaluate every credential status using the same
TransferableRecords and non-obligation predicate as verifier test(), ensuring
mixed arrays such as transferable plus obligation are not classified as
transferable.
In `@src/utils/documents/obligation.ts`:
- Around line 33-37: Reject empty credential-status arrays in both predicates:
update the credentialStatuses check in src/utils/documents/obligation.ts lines
33-37 and the corresponding transferable-record verifier check in
src/verify/fragments/document-status/transferableRecords/transferableRecordVerifier.ts
lines 112-117 to require a positive length before calling every, while
preserving validation for non-empty statuses.
- Around line 18-23: Update getObligationRecordsCredentialStatus to return
ObligationRecordsCredentialStatus | undefined, preserving undefined when the
input lacks credentialStatus. Ensure the function’s type and any affected
callers reflect that the returned status may be absent.
In
`@src/verify/fragments/document-status/obligationRecords/obligationRecordVerifier.ts`:
- Around line 33-37: Update verify to filter credentialStatuses to supported
obligation-status entries before passing them onward, so unrelated supported
credential-status types do not suppress obligation verification. In test, use
some to return true when any credential status matches an obligation type rather
than requiring every entry to match. Apply the same behavior to the
corresponding logic around the additional referenced section.
- Around line 83-87: Update the invalid-result branch in
obligationRecordVerifier so result.reason is taken from the first entry in
verificationResult that fails ValidTokenRegistryStatus.guard, rather than always
reading index 0. Preserve the existing VALID behavior and assign the selected
invalid status’s reason to the returned result.
In
`@src/verify/fragments/document-status/obligationRecords/obligationRecordVerifier.types.ts`:
- Around line 27-29: Apply the repository’s required Prettier formatting to the
ObligationRecordsVerificationFragment type union, using the formatter’s
multiline layout for the union members so ESLint passes.
---
Nitpick comments:
In `@src/__tests__/obligation-registry-functions/mint.test.ts`:
- Around line 48-79: The mintObligationRegistry tests currently verify
encryption but not the contract invocation. Extend both “mints with remarks” and
“mints without remarks” cases to assert mockTrustVCTokenContract.mint receives
the beneficiary, holder, token ID, encrypted remarks, and transaction overrides,
using the encrypted value for the remarks case and '0x' when remarks are
omitted.
In `@src/utils/documents/obligation.ts`:
- Around line 1-4: Move TRANSFERABLE_RECORDS_TYPE from the verifier/barrel
export path into a leaf constants module, then update obligation.ts and the
verifier module to import it directly from that module. Remove the dependency on
src/verify/fragments for this constant while preserving existing exports and
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: 9dd12c65-7e9b-4dea-a157-bb84de6b90e3
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (52)
.github/workflows/tests.ymlREADME.mdpackage.jsonsrc/__tests__/core/documentBuilder.test.tssrc/__tests__/core/verify.amoy.test.tssrc/__tests__/core/verify.test.tssrc/__tests__/e2e/fixtures/sample-boe-credential.tssrc/__tests__/e2e/obligation-registry-functions/fixtures.tssrc/__tests__/e2e/obligation-registry-functions/statusLifecycle.e2e.test.tssrc/__tests__/e2e/obligation-registry-functions/transfer.e2e.test.tssrc/__tests__/e2e/utils.tssrc/__tests__/fixtures/endorsement-chain.tssrc/__tests__/obligation-registry-functions/deploy.test.tssrc/__tests__/obligation-registry-functions/fixtures.tssrc/__tests__/obligation-registry-functions/lifecycle.test.tssrc/__tests__/obligation-registry-functions/mint.test.tssrc/__tests__/obligation-registry-functions/ownerOf.test.tssrc/__tests__/obligation-registry-functions/rejectTransfers.test.tssrc/__tests__/obligation-registry-functions/returnToken.test.tssrc/__tests__/obligation-registry-functions/status.test.tssrc/__tests__/obligation-registry-functions/transfers.test.tssrc/__tests__/verify/obligationRecordVerifier.utils.test.tssrc/core/documentBuilder.tssrc/core/endorsement-chain/fetchEscrowTransfer.tssrc/core/endorsement-chain/helpers.tssrc/core/endorsement-chain/index.tssrc/core/endorsement-chain/obligation.tssrc/core/endorsement-chain/types.tssrc/core/endorsement-chain/useEndorsementChain.tssrc/index.tssrc/obligation-registry-functions/deploy.tssrc/obligation-registry-functions/index.tssrc/obligation-registry-functions/lifecycle.tssrc/obligation-registry-functions/mint.tssrc/obligation-registry-functions/ownerOf.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/token-registry-v5/contracts.tssrc/utils/documents/index.tssrc/utils/documents/obligation.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/obligationRecords/verifierHelpers.tssrc/verify/fragments/document-status/transferableRecords/transferableRecordVerifier.tssrc/verify/fragments/document-status/transferableRecords/utils.tssrc/verify/fragments/index.tssrc/verify/verify.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…ionRecordVerifier.types.ts Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
rongquan1
approved these changes
Jul 31, 2026
|
🎉 This PR is included in version 2.15.0-beta.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
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
3 participants
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.
…ilder
Summary
What is the background of this pull request?
Changes
Issues
What are the related issues or stories?
Summary by CodeRabbit
New Features
Bug Fixes
Documentation