Feature/boe cli beta - #30
Closed
manishdex25 wants to merge 27 commits into
Closed
Conversation
- Introduced new commands for managing BoE obligations, including accept, reject, transfer, and return functionalities. - Added types for obligation registry and escrow commands to enhance type safety and clarity. - Implemented shared utilities for prompting user inputs across obligation escrow commands. - Enhanced transaction handling with detailed logging and error management for better user experience.
…dentialSubject fields for improved clarity and accuracy
…evelopment instructions
- Updated .releaserc.json to include a new branch 'beta-boe' for beta prereleases. - Modified release.yml to trigger workflows on pushes to the 'beta-boe' branch.
- Bumped version to 1.1.0 in package.json and package-lock.json. - Updated @trustvc/trustvc dependency to version 2.15.0-beta.3. - Added new dependencies including @account-abstraction/contracts and @digitalbazaar/ecdsa-rdfc-2019-cryptosuite. - Improved README for clarity on obligation escrow commands.
…andling - Clarified the process for minting BoE token IDs, emphasizing the need to sign with `w3c-sign` before minting. - Improved instructions for using the Obligation Registry address in documents. - Enhanced error messages in CLI to provide clearer guidance on contract call exceptions. - Updated deployment logs to include instructions for using the Obligation Registry address.
- Renamed the 'beta-boe' branch to 'beta' in .releaserc.json for clarity. - Updated CI workflow to include checks for pushes and pull requests to 'main' and 'beta' branches, as well as feature branches. - Enhanced release workflow permissions and added npm registry URL for publishing. - Adjusted npm publish settings in the release configuration to ensure proper deployment.
- Improved formatting for better readability, including consistent use of markdown syntax. - Clarified instructions for using the Obligation Registry and related commands. - Removed unnecessary whitespace to enhance overall document cleanliness.
…scrow commands - Changed @trustvc/trustvc dependency to a local file reference for development. - Enhanced error handling in various obligation escrow commands to ensure proper exit codes on transaction failures. - Refactored command handlers to streamline transaction execution and error logging. - Updated README to clarify command usage and improve overall documentation quality.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
- Removed redundant error checks for obligationRegistry and tokenId. - Enhanced error message for unsupported chain IDs to provide clearer guidance on using a valid BoE document.
…ests for verify obligation command
- Refactored command handlers for various obligation escrow actions to utilize a shared execution function, reducing code duplication. - Improved error handling to ensure proper exit codes on transaction failures. - Updated README to reflect changes in command usage and prerequisites for Node.js version.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
- Removed redundant error handling for mint transaction submission. - Updated error handling to return null instead of throwing an error when the transaction should not proceed.
|
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:
📝 WalkthroughWalkthroughChangesObligation Registry and Escrow CLI
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Operator
participant CLI
participant TrustVC
participant Blockchain
Operator->>CLI: provide BoE document and wallet inputs
CLI->>TrustVC: extract obligation data and prepare transaction
TrustVC->>Blockchain: dry run and submit registry or escrow operation
Blockchain-->>CLI: return receipt and status data
CLI-->>Operator: display fees, result, and explorer link
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
- Removed the explicit registry URL from the release workflow. - Cleaned up environment variable definitions for NPM token usage.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/commands/verify.ts (1)
195-225: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winOA verification skips the
--networkfallback when chain lookup fails butchainIdis present.
resolveFallbackProviderruns only insideif (requiresNetwork && !chainId)(lines 208-212). IfchainIdis present butgetSupportedNetworkNameFromId/getSupportedNetwork/.provider()fails inside thetryblock (lines 214-221), the function falls straight toreturn await verifyDocument(signedVC)(line 224) without ever consultingoptions.networkor prompting. Compare this toverifyW3CDocument(lines 174-192), which always callsresolveFallbackProviderafter its own lookup attempt fails, regardless of whetherchainIdwas present. This means a user who passes--networkto rescue an OA obligation/transferable/revokable document with an unsupported embedded chain gets silently verified without a provider instead.Additionally, when
chainIdis absent andresolveFallbackProvideralready returns no provider (non-interactive, no--network), execution still falls into thetryblock with a falsychainId, producing a second, confusing warning from thecatchon top of the "non-interactive" warning already logged.Restructure to mirror the W3C flow: attempt the chain-embedded lookup first, then always fall back to
resolveFallbackProviderif that attempt didn't yield a provider.🐛 Proposed fix to align OA fallback with the W3C flow
// If the document is not transferable or revokable, verify directly if (!requiresNetwork) return await verifyDocument(signedVC); - // If chainId is not found, prefer --network / TTY prompt / no-provider fallback - if (requiresNetwork && !chainId) { - const provider = await resolveFallbackProvider(options.network); - if (provider) return await verifyDocument(signedVC, { provider }); - } - try { const chainName = getSupportedNetworkNameFromId(Number(chainId)); const network = getSupportedNetwork(chainName); const provider = network.provider() as unknown as V5Provider; if (provider) return await verifyDocument(signedVC, { provider }); } catch (err: unknown) { signale.warn(`${err instanceof Error ? err.message : String(err)}`); } + // Prefer --network / TTY prompt / no-provider fallback + const fallbackProvider = await resolveFallbackProvider(options.network); + if (fallbackProvider) return await verifyDocument(signedVC, { provider: fallbackProvider }); + // Fallback: Verify without provider return await verifyDocument(signedVC);🤖 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/commands/verify.ts` around lines 195 - 225, Restructure verifyOpenAttestationDocument so the embedded chain lookup is attempted only when chainId is present, and resolveFallbackProvider(options.network) is always called when that lookup does not produce a provider, including unsupported or absent chain IDs. Return verifyDocument with the fallback provider when available, otherwise proceed directly to the providerless verification without re-entering the embedded lookup or emitting a duplicate warning.
🧹 Nitpick comments (10)
tests/commands/verify.obligation.test.ts (1)
118-130: 📐 Maintainability & Code Quality | 🔵 TrivialHardcoded enum values reduce test resilience to SDK changes.
The assertion at line 127 hardcodes
status=2 terminationReason=2instead of interpolatingObligationDocumentStatus.RejectedandObligationEscrowTerminationReason.Rejected, which are already imported and used elsewhere in this file (lines 60-61). If the SDK changes these numeric values, this assertion silently passes or fails without reflecting the actual enum semantics being tested.♻️ Proposed fix to reference the enum members directly
expect(signale.default.info).toHaveBeenCalledWith( - 'Obligation document status: registry=0xRegistry status=2 terminationReason=2', + `Obligation document status: registry=0xRegistry status=${ObligationDocumentStatus.Rejected} terminationReason=${ObligationEscrowTerminationReason.Rejected}`, );🤖 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 `@tests/commands/verify.obligation.test.ts` around lines 118 - 130, Update the obligation status assertion in the shredded BoE test within describe('verify') to interpolate ObligationDocumentStatus.Rejected and ObligationEscrowTerminationReason.Rejected instead of hardcoded numeric values, reusing the existing imports while preserving the expected log message structure.tests/utils/contract-errors.test.ts (1)
57-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a regression test for single-word non-revert reasons.
The current negative tests use multi-word text only. A single-word reason such as
timeoutcurrently passesnormalizeLabeland is reported as a contract revert. See the related comment onsrc/utils/cli-errors.tslines 100-111. Add the case so the fix stays covered.💚 Proposed test
it('rejects generic failed: suffixes that are not contract reverts', () => { const preprocessing = new Error('SDK preprocessing failed: badInput'); expect(extractContractRevertLabel(preprocessing)).toBeUndefined(); expect(isContractCallException(preprocessing)).toBe(false); }); + + it('does not treat single-word transient reasons as contract reverts', () => { + const transient = Object.assign(new Error('timeout'), { reason: 'timeout' }); + expect(extractContractRevertLabel(transient)).toBeUndefined(); + expect(isContractCallException(transient)).toBe(false); + });🤖 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 `@tests/utils/contract-errors.test.ts` around lines 57 - 76, Add a regression case in the contract-error tests covering a single-word non-revert reason such as “timeout”; assert extractContractRevertLabel returns undefined and the error is not classified as a contract call exception, matching the existing multi-word negative cases.src/commands/helpers.ts (1)
470-507: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
connectToObligationRegistryinsideconnectToObligationEscrow.
connectToObligationEscrowbuilds its own registry contract instance.src/commands/obligation-escrow/runTx.ts(lines 47-55) then callsconnectToObligationRegistryfor the same address, so the registry contract is constructed twice per transaction. Calling the existing helper removes the duplicate construction and gives the escrow path the same registry bytecode validation.♻️ Proposed refactor
try { - signale.info(`Connecting to obligation registry at: ${address}`); - const registry = new ethers.Contract(address, TrustVCToken__factory.abi, wallet as any); + const registry = await connectToObligationRegistry({ address, wallet }); signale.info(`Fetching obligation escrow address for tokenId: ${tokenId}`);🤖 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/commands/helpers.ts` around lines 470 - 507, Update connectToObligationEscrow to obtain the registry through the existing connectToObligationRegistry helper instead of constructing a new ethers.Contract directly. Preserve the existing ownerOf lookup and escrow validation flow, passing the address and wallet required by the helper so registry bytecode validation and construction are reused.tests/commands/obligation-registry/mint.test.ts (1)
64-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd direct unit tests for
extractObligationDocumentInfo.This suite mocks
extractObligationDocumentInfo, so the extractor insrc/utils/obligation-document.tsis never executed. The extractor drives the registry address, the token ID, the network, and the remark encryption key for every registry and escrow command. No test file for it appears in this cohort. Add cases for the non-obligation document rejection, the unsupported chain ID, and the missingdocument.idpath.
Do you want me to generate the test file?🤖 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 `@tests/commands/obligation-registry/mint.test.ts` around lines 64 - 93, Add a dedicated unit-test suite for extractObligationDocumentInfo in src/utils/obligation-document.ts instead of relying on the mocked extractor in mint.test.ts. Cover rejection of non-obligation documents, unsupported chain IDs, and documents missing document.id, while asserting the expected errors or failure behavior for each case.src/commands/obligation-registry/deploy.ts (2)
112-112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueResolve the chain ID through one helper.
Line 112 indexes
supportedNetworkdirectly, and line 151 callsgetSupportedNetwork(network)for the same value. Ifnetworkis not a key ofsupportedNetwork, line 112 throws aTypeErroron.networkId.chainIdis used only for the log at line 165. UsegetSupportedNetworkin both places.♻️ Proposed refactor
- const chainId = supportedNetwork[network as NetworkCmdName].networkId; + const chainId = getSupportedNetwork(network).networkId;🤖 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/commands/obligation-registry/deploy.ts` at line 112, Update the chainId initialization in the deploy flow to use getSupportedNetwork(network), matching the existing lookup near the later deployment logic instead of indexing supportedNetwork directly. Preserve the chain ID used by the log while ensuring unsupported network names are handled consistently through the helper.
30-33: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHandler catch blocks bypass
getErrorMessage. Both command handlers printerr.messagedirectly. This PR adds contract-revert decoding ingetErrorMessage, so these two paths report raw ethers text such asexecution reverted (unknown custom error)instead of the actionable message.getErrorMessageis already imported in both files.
src/commands/obligation-registry/deploy.ts#L30-L33: replaceerror(err instanceof Error ? err.message : String(err))witherror(getErrorMessage(err)).src/commands/obligation-registry/mint.ts#L34-L37: apply the same replacement.🤖 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/commands/obligation-registry/deploy.ts` around lines 30 - 33, Update the catch handler in src/commands/obligation-registry/deploy.ts lines 30-33 to pass the caught error to the existing getErrorMessage helper before calling error. Apply the same change in src/commands/obligation-registry/mint.ts lines 34-37, replacing the direct err.message/String formatting while preserving the existing exit-code behavior.tests/commands/obligation-registry/deploy.test.ts (1)
94-125: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd coverage for the dry-run branches.
The suite only covers the success path with no factory address. Two behaviors added in this PR remain untested: the skip of the dry run when
escrowFactoryAddressis absent, and thenullresult that must setprocess.exitCode = 1and returnnull. Add cases for both.🤖 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 `@tests/commands/obligation-registry/deploy.test.ts` around lines 94 - 125, Extend the deployObligationRegistryContract tests with cases covering both dry-run branches: verify the dry run is skipped when escrowFactoryAddress is absent, and verify a null dry-run result sets process.exitCode to 1 and returns null. Reuse the existing SDK and logging mocks, and assert the relevant dry-run invocation and exit-code behavior.src/commands/obligation-escrow/reject-transfer-owner-holder.ts (1)
21-39: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueTwo escrow handlers skip the cohort's try/catch error-handling pattern.
rejectTransferOwnersHandlerandtransferOwnersHandlerdon't wrap their body in try/catch or log viagetErrorMessage, unlikeacceptReturnedHandler,endorseHandler, andchangeHolderHandlerin the same cohort. The outerrunObligationEscrowCommandwrapper still catches unhandled errors in the productionhandlerpath (persrc/commands/obligation-escrow/shared.ts), so this is not a functional break today. It is an inconsistency that risks diverging further as more escrow commands are added, and it means calling these handlers directly (as the corresponding test files do) bypasses error handling entirely.
src/commands/obligation-escrow/reject-transfer-owner-holder.ts#L21-L39: wrap the handler body in try/catch, log witherror(getErrorMessage(e)), and setprocess.exitCode = 1on failure, matching sibling handlers.src/commands/obligation-escrow/transfer-owner-holder.ts#L28-L54: apply the same try/catch pattern.🤖 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/commands/obligation-escrow/reject-transfer-owner-holder.ts` around lines 21 - 39, Update rejectTransferOwnersHandler in src/commands/obligation-escrow/reject-transfer-owner-holder.ts (lines 21-39) and transferOwnersHandler in src/commands/obligation-escrow/transfer-owner-holder.ts (lines 28-54) to wrap their existing logic in try/catch; on failure, log getErrorMessage(e) via error and set process.exitCode = 1, matching the sibling escrow handlers.src/types.ts (1)
125-137: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEscrow command type names don't match the commands that use them.
ObligationEscrowEndorseTransferOfOwnersCommand(defined withnewHolder/newOwner) sounds like it belongs to an "endorse transfer owner" command, buttransfer-owner-holder.tsconsumes it.ObligationEscrowNominateBeneficiaryCommand(defined withnewBeneficiaryonly) sounds "nominate"-specific, butendorse-transfer-owner.tsconsumes it instead. Both types are structurally correct for their handlers today, so this does not break functionality. It does create confusion for anyone reading the command-to-type mapping, and increases the risk of a future contributor picking the wrong type when adding or modifying an escrow command.
src/types.ts#L125-L137: renameObligationEscrowEndorseTransferOfOwnersCommandandObligationEscrowNominateBeneficiaryCommandto names that reflect their actual field shape rather than a specific command (for exampleObligationEscrowTransferOwnersCommandfor thenewHolder+newOwnershape, andObligationEscrowNewBeneficiaryCommandfor thenewBeneficiary-only shape, shared by bothnominateandendorsecommands).src/commands/obligation-escrow/endorse-transfer-owner.ts#L1-L25: update the import and type annotations to the renamed type.src/commands/obligation-escrow/transfer-owner-holder.ts#L1-L26: update the import and type annotations to the renamed type.🤖 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/types.ts` around lines 125 - 137, Rename the two escrow command types in src/types.ts#L125-L137 to reflect their field shapes: use a shared transfer-owners name for the newHolder/newOwner shape and a new-beneficiary name for the newBeneficiary-only shape. Update imports and type annotations in src/commands/obligation-escrow/endorse-transfer-owner.ts#L1-L25 and src/commands/obligation-escrow/transfer-owner-holder.ts#L1-L26 to reference the renamed types, preserving their existing structural usage.tests/commands/obligation-escrow/return-to-issuer.test.ts (1)
54-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign test coverage with sibling test files.
This test only asserts
runObligationEscrowTxwas called, without checking thepopulate/sdkParamsarguments. Sibling tests (reject.test.ts,reject-transfer-owner.test.ts) assert onsdkParams, and other sibling tests also coverpromptForInputs. Add an equivalentpromptForInputstest and ansdkParams/populateassertion here for parity.🤖 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 `@tests/commands/obligation-escrow/return-to-issuer.test.ts` around lines 54 - 68, The returnToIssuerHandler tests should match sibling coverage by adding a promptForInputs behavior test and expanding the runObligationEscrowTx assertion to verify the expected populate/sdkParams arguments. Use the existing test patterns and mocks from reject.test.ts and reject-transfer-owner.test.ts, updating the return-to-issuer suite without changing production code.
🤖 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 38: Update the `@trustvc/trustvc` dependency in package.json from the beta
version to a stable TrustVC release before the first main release, or configure
the dependency separately for beta and main branches so the stable
semantic-release path cannot include the beta SDK.
In `@README.md`:
- Around line 1669-1686: Correct the README project tree entry for verify.ts to
reference src/commands/verify.ts rather than src/commands/w3c/. Keep verify.ts
as a direct child of src/commands and adjust the surrounding tree branch
characters to match the corrected hierarchy.
- Around line 345-351: Update the Obligation/BoE command table in README.md so
every command row links to its corresponding detailed heading below, including
the grouped reject-transfer-* row linking to the shared transfer section.
Preserve the existing command names and descriptions while adding links
consistently with the README’s established anchor format.
- Around line 1409-1560: Update the README documentation for obligation-escrow
accept-return-to-issuer and reject-return-to-issuer to state that the connected
issuer wallet must have the required obligation registry role: accepter for
burn/shred and restorer for restoring the BoE to escrow. Keep the existing
wallet/private-key prompt documentation unchanged.
In `@src/commands/obligation-escrow/index.ts`:
- Around line 8-9: Update the builder function’s commandDir configuration to
load only valid command modules, excluding helper files such as runTx.ts and
shared.ts while retaining the intended obligation-escrow subcommands.
In `@src/commands/obligation-escrow/nominate-transfer-owner.ts`:
- Around line 26-44: Wrap the transaction flow in nominateHandler with
try/catch, including the initial logging and runObligationEscrowTx call.
Preserve the existing success and early-return behavior, and mirror
acceptHandler’s catch path by formatting/logging the failure and setting
process.exitCode to 1.
In `@src/commands/obligation-escrow/reject-transfer-holder.ts`:
- Around line 21-39: Wrap the transaction execution flow in
rejectTransferHolderHandler with the same try/catch error handling used by
acceptHandler and nominateHandler. Handle errors from runObligationEscrowTx and
the subsequent transaction reporting consistently with those handlers, while
preserving the existing success and early-return behavior.
In `@src/commands/obligation-escrow/shared.ts`:
- Around line 48-60: Remove the redundant local try/catch blocks from
dischargeHandler in src/commands/obligation-escrow/discharge.ts#L23-L46,
rejectReturnedHandler in
src/commands/obligation-escrow/reject-return-to-issuer.ts#L22-L46,
rejectTransferOwnerHandler in
src/commands/obligation-escrow/reject-transfer-owner.ts#L22-L45, rejectHandler
in src/commands/obligation-escrow/reject.ts#L22-L44, and returnToIssuerHandler
in src/commands/obligation-escrow/return-to-issuer.ts#L22-L45, allowing errors
to propagate to runObligationEscrowCommand. Keep
src/commands/obligation-escrow/shared.ts#L48-L60 unchanged as the sole shared
error handler.
- Around line 15-42: Update promptBaseObligationEscrowInputs to catch
cancellation rejections from promptAndReadDocument and promptWalletSelection,
returning the falsy result expected by runObligationEscrowCommand instead of
propagating the error. Preserve the existing successful prompt flow and input
construction, while allowing non-cancellation errors to retain their current
handling.
In `@src/commands/verify.ts`:
- Around line 136-152: Validate networkOverride in resolveFallbackProvider
before calling getSupportedNetwork, ensuring it matches a supportedNetwork key;
when invalid, throw an actionable error that identifies the bad network and
explains how to select a valid one. Preserve the existing provider resolution
for valid overrides and the interactive/non-interactive fallback behavior.
In `@src/utils/cli-errors.ts`:
- Around line 100-111: Update normalizeLabel to accept only PascalCase Solidity
custom-error identifiers, reusing the existing SOLIDITY_CUSTOM_ERROR_NAME
pattern rather than the current broad identifier regex. Preserve trimming and
rejection of unknown labels, while ensuring lowercase or otherwise
non-PascalCase single-word reasons return undefined.
In `@src/utils/cli-options.ts`:
- Around line 538-547: Update callers of performDryRunWithConfirmation,
including the document-store and token-registry deploy/issue/mint commands, to
check shouldProceed === null before the existing cancellation branch and exit
with failure status for definitive dry-run reverts. Preserve process.exit(0) for
user cancellation, and keep successful execution unchanged.
In `@src/utils/obligation-document.ts`:
- Line 58: Remove the `'N/A'` fallback from documentId in the obligation
document creation flow. Validate that document.id is present and throw an error
when it is missing; otherwise return the actual document.id so downstream
encryptionKey consumers and SDK options never use a shared placeholder key.
---
Outside diff comments:
In `@src/commands/verify.ts`:
- Around line 195-225: Restructure verifyOpenAttestationDocument so the embedded
chain lookup is attempted only when chainId is present, and
resolveFallbackProvider(options.network) is always called when that lookup does
not produce a provider, including unsupported or absent chain IDs. Return
verifyDocument with the fallback provider when available, otherwise proceed
directly to the providerless verification without re-entering the embedded
lookup or emitting a duplicate warning.
---
Nitpick comments:
In `@src/commands/helpers.ts`:
- Around line 470-507: Update connectToObligationEscrow to obtain the registry
through the existing connectToObligationRegistry helper instead of constructing
a new ethers.Contract directly. Preserve the existing ownerOf lookup and escrow
validation flow, passing the address and wallet required by the helper so
registry bytecode validation and construction are reused.
In `@src/commands/obligation-escrow/reject-transfer-owner-holder.ts`:
- Around line 21-39: Update rejectTransferOwnersHandler in
src/commands/obligation-escrow/reject-transfer-owner-holder.ts (lines 21-39) and
transferOwnersHandler in src/commands/obligation-escrow/transfer-owner-holder.ts
(lines 28-54) to wrap their existing logic in try/catch; on failure, log
getErrorMessage(e) via error and set process.exitCode = 1, matching the sibling
escrow handlers.
In `@src/commands/obligation-registry/deploy.ts`:
- Line 112: Update the chainId initialization in the deploy flow to use
getSupportedNetwork(network), matching the existing lookup near the later
deployment logic instead of indexing supportedNetwork directly. Preserve the
chain ID used by the log while ensuring unsupported network names are handled
consistently through the helper.
- Around line 30-33: Update the catch handler in
src/commands/obligation-registry/deploy.ts lines 30-33 to pass the caught error
to the existing getErrorMessage helper before calling error. Apply the same
change in src/commands/obligation-registry/mint.ts lines 34-37, replacing the
direct err.message/String formatting while preserving the existing exit-code
behavior.
In `@src/types.ts`:
- Around line 125-137: Rename the two escrow command types in
src/types.ts#L125-L137 to reflect their field shapes: use a shared
transfer-owners name for the newHolder/newOwner shape and a new-beneficiary name
for the newBeneficiary-only shape. Update imports and type annotations in
src/commands/obligation-escrow/endorse-transfer-owner.ts#L1-L25 and
src/commands/obligation-escrow/transfer-owner-holder.ts#L1-L26 to reference the
renamed types, preserving their existing structural usage.
In `@tests/commands/obligation-escrow/return-to-issuer.test.ts`:
- Around line 54-68: The returnToIssuerHandler tests should match sibling
coverage by adding a promptForInputs behavior test and expanding the
runObligationEscrowTx assertion to verify the expected populate/sdkParams
arguments. Use the existing test patterns and mocks from reject.test.ts and
reject-transfer-owner.test.ts, updating the return-to-issuer suite without
changing production code.
In `@tests/commands/obligation-registry/deploy.test.ts`:
- Around line 94-125: Extend the deployObligationRegistryContract tests with
cases covering both dry-run branches: verify the dry run is skipped when
escrowFactoryAddress is absent, and verify a null dry-run result sets
process.exitCode to 1 and returns null. Reuse the existing SDK and logging
mocks, and assert the relevant dry-run invocation and exit-code behavior.
In `@tests/commands/obligation-registry/mint.test.ts`:
- Around line 64-93: Add a dedicated unit-test suite for
extractObligationDocumentInfo in src/utils/obligation-document.ts instead of
relying on the mocked extractor in mint.test.ts. Cover rejection of
non-obligation documents, unsupported chain IDs, and documents missing
document.id, while asserting the expected errors or failure behavior for each
case.
In `@tests/commands/verify.obligation.test.ts`:
- Around line 118-130: Update the obligation status assertion in the shredded
BoE test within describe('verify') to interpolate
ObligationDocumentStatus.Rejected and ObligationEscrowTerminationReason.Rejected
instead of hardcoded numeric values, reusing the existing imports while
preserving the expected log message structure.
In `@tests/utils/contract-errors.test.ts`:
- Around line 57-76: Add a regression case in the contract-error tests covering
a single-word non-revert reason such as “timeout”; assert
extractContractRevertLabel returns undefined and the error is not classified as
a contract call exception, matching the existing multi-word negative cases.
🪄 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: 5e16ace7-b22d-4630-a022-6a46761b52db
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (53)
.github/workflows/ci.yml.github/workflows/release.yml.releaserc.jsonREADME.mdpackage.jsonsrc/commands/helpers.tssrc/commands/obligation-escrow/accept-return-to-issuer.tssrc/commands/obligation-escrow/accept.tssrc/commands/obligation-escrow/discharge.tssrc/commands/obligation-escrow/endorse-transfer-owner.tssrc/commands/obligation-escrow/index.tssrc/commands/obligation-escrow/nominate-transfer-owner.tssrc/commands/obligation-escrow/reject-return-to-issuer.tssrc/commands/obligation-escrow/reject-transfer-holder.tssrc/commands/obligation-escrow/reject-transfer-owner-holder.tssrc/commands/obligation-escrow/reject-transfer-owner.tssrc/commands/obligation-escrow/reject.tssrc/commands/obligation-escrow/return-to-issuer.tssrc/commands/obligation-escrow/runTx.tssrc/commands/obligation-escrow/shared.tssrc/commands/obligation-escrow/status.tssrc/commands/obligation-escrow/transfer-holder.tssrc/commands/obligation-escrow/transfer-owner-holder.tssrc/commands/obligation-registry/deploy.tssrc/commands/obligation-registry/index.tssrc/commands/obligation-registry/mint.tssrc/commands/verify.tssrc/types.tssrc/utils/cli-errors.tssrc/utils/cli-options.tssrc/utils/formatting.tssrc/utils/index.tssrc/utils/obligation-document.tstests/commands/obligation-escrow/accept-return-to-issuer.test.tstests/commands/obligation-escrow/accept.test.tstests/commands/obligation-escrow/discharge.test.tstests/commands/obligation-escrow/endorse-transfer-owner.test.tstests/commands/obligation-escrow/nominate-transfer-owner.test.tstests/commands/obligation-escrow/reject-return-to-issuer.test.tstests/commands/obligation-escrow/reject-transfer-holder.test.tstests/commands/obligation-escrow/reject-transfer-owner-holder.test.tstests/commands/obligation-escrow/reject-transfer-owner.test.tstests/commands/obligation-escrow/reject.test.tstests/commands/obligation-escrow/return-to-issuer.test.tstests/commands/obligation-escrow/status.test.tstests/commands/obligation-escrow/transfer-holder.test.tstests/commands/obligation-escrow/transfer-owner-holder.test.tstests/commands/obligation-registry/deploy.test.tstests/commands/obligation-registry/mint.test.tstests/commands/verify.obligation.test.tstests/commands/verify.test.tstests/fixtures/obligation/w3c-obligation-record.jsontests/utils/contract-errors.test.ts
…andling in command files - Introduced a new JSON file for the driving licence credential schema. - Improved error handling in various command files to ensure proper exit codes on transaction failures. - Updated README for better command usage clarity.
- Updated input prompts in various obligation escrow command files to return null on cancellation, improving user experience. - Enhanced error handling in command handlers to ensure proper exit codes on transaction failures. - Clarified roles and responsibilities in the README for executing commands related to obligation escrow.
- Added checks to ensure proper exit codes when `shouldProceed` is null in various document store and title escrow command files. - Updated README to reflect the new structure of command files for better clarity.
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 `@decrypted.json`:
- Line 1: Update the decrypted credential fixture used by verification to
represent a signed or wrapped credential: generate it through the
signing/wrapping pipeline so it contains the expected W3C proof or
OpenAttestation data/signature shape. Alternatively, if this fixture
intentionally remains plaintext, change the related verification assertion to
expect verifyDocumentSignature() to fail.
In `@README.md`:
- Line 1693: Update the verify.ts project-tree entry to describe verification of
W3C, OA, ETR, and BoE documents, matching the unified trustvc verify behavior
documented above.
🪄 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: 8ce3d8ef-1bf9-44aa-986a-a7af527e8cdc
📒 Files selected for processing (36)
.github/workflows/release.ymlREADME.mddecrypted.jsonsrc/commands/document-store/deploy.tssrc/commands/document-store/grant-role.tssrc/commands/document-store/issue.tssrc/commands/document-store/revoke-role.tssrc/commands/document-store/revoke.tssrc/commands/obligation-escrow/discharge.tssrc/commands/obligation-escrow/endorse-transfer-owner.tssrc/commands/obligation-escrow/index.tssrc/commands/obligation-escrow/nominate-transfer-owner.tssrc/commands/obligation-escrow/reject-return-to-issuer.tssrc/commands/obligation-escrow/reject-transfer-holder.tssrc/commands/obligation-escrow/reject-transfer-owner.tssrc/commands/obligation-escrow/reject.tssrc/commands/obligation-escrow/return-to-issuer.tssrc/commands/obligation-escrow/shared.tssrc/commands/obligation-escrow/transfer-holder.tssrc/commands/obligation-escrow/transfer-owner-holder.tssrc/commands/title-escrow/accept-return-to-issuer.tssrc/commands/title-escrow/endorse-transfer-owner.tssrc/commands/title-escrow/nominate-transfer-owner.tssrc/commands/title-escrow/reject-return-to-issuer.tssrc/commands/title-escrow/reject-transfer-holder.tssrc/commands/title-escrow/reject-transfer-owner-holder.tssrc/commands/title-escrow/reject-transfer-owner.tssrc/commands/title-escrow/return-to-issuer.tssrc/commands/title-escrow/transfer-holder.tssrc/commands/title-escrow/transfer-owner-holder.tssrc/commands/token-registry/deploy.tssrc/commands/token-registry/mint.tssrc/commands/verify.tssrc/utils/cli-errors.tssrc/utils/obligation-document.tstests/utils/contract-errors.test.ts
💤 Files with no reviewable changes (1)
- .github/workflows/release.yml
🚧 Files skipped from review as they are similar to previous changes (10)
- src/commands/obligation-escrow/reject-return-to-issuer.ts
- src/commands/obligation-escrow/index.ts
- src/utils/obligation-document.ts
- tests/utils/contract-errors.test.ts
- src/commands/obligation-escrow/return-to-issuer.ts
- src/utils/cli-errors.ts
- src/commands/obligation-escrow/nominate-transfer-owner.ts
- src/commands/obligation-escrow/reject.ts
- src/commands/obligation-escrow/endorse-transfer-owner.ts
- src/commands/verify.ts
- Added decrypted.json to .gitignore to prevent tracking of sensitive files. - Removed the decrypted.json file as it is no longer needed. - Updated README to clarify command usage and improve overall documentation structure.
- Removed redundant checks for `shouldProceed` being null in various command files to streamline exit logic. - Updated README to reflect changes in command verification processes for improved clarity.
- Simplified CLI error helpers by removing outdated constants and functions related to known revert messages and selectors. - Updated comments for clarity and conciseness, enhancing the overall readability of the error handling logic.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit