Feature/boe c2 fixes - #29
Closed
manishdex25 wants to merge 20 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>
|
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:
📝 WalkthroughWalkthroughThis PR adds Obligation Registry deployment and minting, Obligation Escrow lifecycle and transfer commands, BoE verification status reporting, contract error decoding, tests, documentation, and beta release support. ChangesObligation Registry and Escrow CLI
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
🧪 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 redundant error checks for obligationRegistry and tokenId. - Enhanced error message for unsupported chain IDs to provide clearer guidance on using a valid BoE document.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
🧹 Nitpick comments (11)
src/utils/cli-errors.ts (2)
17-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
InvalidTokenTransferToZeroAddressOwnershas a selector but no user-facing message.
REVERT_SELECTOR_TO_NAME['0xbd805e91']maps toInvalidTokenTransferToZeroAddressOwners, butKNOWN_REVERT_MESSAGEShas no entry for that name, unlike every other selector-mapped error. When this selector is decoded,describeContractErrorfalls back to the generic"Contract reverted with InvalidTokenTransferToZeroAddressOwners"instead of actionable guidance.Add a matching entry to
KNOWN_REVERT_MESSAGESfor consistency with the rest of the map.🤖 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/cli-errors.ts` around lines 17 - 75, Add an InvalidTokenTransferToZeroAddressOwners entry to KNOWN_REVERT_MESSAGES with actionable guidance for the zero-address owners transfer error, matching the existing REVERT_SELECTOR_TO_NAME mapping so describeContractError returns a user-facing message instead of the generic fallback.
50-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDecode custom reverts from the contract ABI instead of hardcoding selectors.
REVERT_SELECTOR_TO_NAMEmaps bare 4-byte data values to error names atsrc/utils/cli-errors.ts:151. If a mapped error is deprecated, renamed, or its argument list changes, the CLI can show the wrong message. Use the existing ethers dependency’s ABIInterface.parseError(data)for contract reverts, and keep this table only for selectors whose full ABI signatures are unavailable.🤖 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/cli-errors.ts` around lines 50 - 75, The CLI currently hardcodes contract revert selectors in REVERT_SELECTOR_TO_NAME, which can become stale. Update the revert-decoding logic in cli-errors.ts to use the existing ethers ABI Interface.parseError(data) for selectors covered by the contract ABI, formatting the parsed error name and arguments as appropriate; retain REVERT_SELECTOR_TO_NAME only as a fallback for selectors whose full ABI signatures are unavailable.tests/utils/contract-errors.test.ts (1)
1-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for plain-text, multi-word revert reasons.
None of the current cases pass a multi-word
err.reason(e.g., a standardrequire(condition, "insufficient funds for transfer")revert). This is the scenario that reveals the label-truncation bug flagged insrc/utils/cli-errors.ts(normalizeLabel), where only the first word of the reason would be kept.Add a case asserting that
getErrorMessage/describeContractErrorreturn the full reason text unchanged whenerr.reasoncontains spaces and does not match a known custom-error name.Do you want me to add this test case once the
normalizeLabelfix insrc/utils/cli-errors.tslands?🤖 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 1 - 67, Add a test in the contract revert error formatting suite using an error with a plain-text, multi-word reason such as “insufficient funds for transfer” that is not a known custom-error label. Assert that both getErrorMessage and describeContractError preserve the complete reason text unchanged, covering normalizeLabel without altering the existing custom-error cases.tests/commands/obligation-escrow/discharge.test.ts (1)
79-90: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBoth escrow tests assert
remarks: undefined, which passes trivially.expect.objectContaining({ remarks: undefined })also matches when the key is absent, so neither test proves that the remark flows intosdkParams. Pass a remark in the handler args and assert that value.
tests/commands/obligation-escrow/discharge.test.ts#L79-L90: addremark: 'discharged'to thedischargeHandlerargs and assertremarks: 'discharged'.tests/commands/obligation-escrow/reject.test.ts#L76-L87: addremark: 'rejected'to therejectHandlerargs and assertremarks: '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/obligation-escrow/discharge.test.ts` around lines 79 - 90, Update tests/commands/obligation-escrow/discharge.test.ts lines 79-90 by passing remark: 'discharged' to dischargeHandler and asserting sdkParams.remarks equals 'discharged'. Update tests/commands/obligation-escrow/reject.test.ts lines 76-87 by passing remark: 'rejected' to rejectHandler and asserting sdkParams.remarks equals 'rejected', ensuring both tests verify remark propagation rather than an absent property.src/commands/obligation-escrow/status.ts (2)
40-42: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRun the three reads concurrently.
The three calls are independent read-only queries. Each one performs its own escrow resolution and RPC round trips. Sequential awaits triple the wall-clock latency of the command.
⚡ Proposed refactor
- const status = await getObligationRegistryStatus(opts, wallet, { tokenId }); - const registered = await isObligationRegistryRegistered(opts, wallet, { tokenId }); - const reason = await getObligationEscrowTerminationReason(opts, wallet, { tokenId }); + const [status, registered, reason] = await Promise.all([ + getObligationRegistryStatus(opts, wallet, { tokenId }), + isObligationRegistryRegistered(opts, wallet, { tokenId }), + getObligationEscrowTerminationReason(opts, wallet, { 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/obligation-escrow/status.ts` around lines 40 - 42, Update the status command flow around getObligationRegistryStatus, isObligationRegistryRegistered, and getObligationEscrowTerminationReason to start all three independent reads concurrently and await their results together, preserving the existing variables and downstream behavior.
16-27: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse the SDK reverse mappings for status/reason labels.
ObligationDocumentStatusandObligationEscrowTerminationReasonare exported TypeScript numeric enums, so these maps duplicate labels and require manual updates when the SDK adds values. Derive them from the enum objects instead.🤖 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/status.ts` around lines 16 - 27, Replace the manually maintained STATUS_LABEL and REASON_LABEL mappings with labels derived from the exported numeric enum reverse mappings, ObligationDocumentStatus and ObligationEscrowTerminationReason. Update the status/reason lookup usage as needed while preserving the existing label output for current values and automatically supporting newly added enum members.src/commands/helpers.ts (2)
440-459: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the bytecode check into a shared helper.
connectToObligationRegistryandconnectToObligationEscrowrepeat the same provider check andgetCodevalidation. Extract one helper and reuse it. The message can also state the real cause, which is that no contract exists at the address.♻️ Proposed refactor
+const assertContractDeployed = async ( + wallet: Wallet | HDNodeWallet | ConnectedSigner | Signer, + address: string, + label: string, +): Promise<void> => { + const provider = wallet.provider; + if (!provider) { + throw new Error(`Wallet provider is required to validate the ${label} contract`); + } + const code = await provider.getCode(address); + if (!code || code === '0x') { + throw new Error(`No contract deployed at ${label} address: ${address}`); + } +}; + export const connectToObligationRegistry = async ({ address, wallet, }: ConnectToObligationRegistryArgs) => { try { signale.info(`Connecting to obligation registry at: ${address}`); const registry = new ethers.Contract(address, TrustVCToken__factory.abi, wallet as any); - const provider = wallet.provider; - if (!provider) { - throw new Error('Wallet provider is required to validate the obligation registry contract'); - } - const code = await provider.getCode(address); - if (!code || code === '0x') { - throw new Error(`Failed to connect to obligation registry at address: ${address}`); - } + await assertContractDeployed(wallet, address, 'obligation registry'); signale.success(`Successfully connected to obligation registry`); return 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/commands/helpers.ts` around lines 440 - 459, Extract the provider presence and provider.getCode(address) validation from connectToObligationRegistry into a shared helper, then reuse that helper in both connectToObligationRegistry and connectToObligationEscrow. Preserve the existing rejection behavior and update the missing-contract error to state that no contract exists at the supplied address.
476-487: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winValidate the registry bytecode before you call
ownerOf.If
addressholds no contract,ownerOfrejects with a low-level ethers decode error. The user then sees an opaque message instead of "no contract at address". ReuseconnectToObligationRegistry(or the shared assert helper) first, then resolve the escrow.♻️ Proposed refactor
- 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 476 - 487, Update the registry lookup flow around `registry` and `ownerOf` to validate that `address` contains deployed contract bytecode before calling `ownerOf`. Reuse `connectToObligationRegistry` or the existing shared assertion helper, then resolve and validate the escrow address as before.tests/commands/obligation-registry/mint.test.ts (1)
113-157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd tests for the failure and cancellation paths.
The suite covers only the happy path. Two behaviors in
mint.tsremain untested:
mintObligationTokenwhenmintObligationRegistryrejects. Assert the exit code.mintToObligationRegistrywhenperformDryRunWithConfirmationresolvesfalse. Assert that no transaction is sent.Both paths carry the exit-code defects flagged on
src/commands/obligation-registry/mint.ts.🤖 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 113 - 157, Extend the obligation-registry mint tests with failure and cancellation cases: configure mintObligationRegistry to reject and assert mintObligationToken reports the expected exit code, then configure performDryRunWithConfirmation to resolve false and assert mintToObligationRegistry does not send a transaction. Reuse the existing mocks and symbols in mint.test.ts, covering the corresponding paths in mintObligationToken and mintToObligationRegistry.tests/commands/obligation-registry/deploy.test.ts (1)
94-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a failure-path test for
deployObligationRegistryContract.Add a test where
deployObligationRegistryrejects or a dependency throws. Assert on the resultingprocess.exitCodeor on error propagation. This would surface the exit-code gap flagged insrc/commands/obligation-registry/deploy.ts(lines 146-148).🤖 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 test suite with a failure-path case where deployObligationRegistry rejects or a dependency throws. Assert the resulting process.exitCode or propagated error, targeting the error handling in deployObligationRegistryContract and covering the exit-code behavior referenced in the review.src/commands/obligation-escrow/shared.ts (1)
23-41: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate credential-selection fallback logic.
This block repeats the same
encryptedWalletPath→keyFile→keypriority chain found insrc/commands/obligation-registry/deploy.ts(lines 89-98). Extract a shared helper, for examplewithWalletCredentials(baseResult, { encryptedWalletPath, key, keyFile }), to keep the fallback order consistent as more commands are added.🤖 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/shared.ts` around lines 23 - 41, Extract the credential-priority selection from the shared command result builder into a reusable withWalletCredentials helper, preserving the encryptedWalletPath → keyFile → key order and baseResult fallback. Replace the duplicate chain in the current flow and reuse the helper in the obligation-registry deploy flow so both commands share one consistent implementation.
🤖 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 @.github/workflows/release.yml:
- Line 64: Remove the registry-url configuration from the setup-node step in the
semantic-release workflow so it no longer creates an .npmrc; retain a single
authentication mechanism by using NPM_TOKEN or fully adopting npm Trusted
Publishing, without configuring both.
In `@README.md`:
- Around line 306-309: Remove the duplicate [mint](`#mint`) row from the Token
Registry table in README.md, keeping one primary mint entry and the existing
token-registry mint alternative row.
- Line 1422: Update the `documentverify` prompt label in README.md to insert the
missing separator and use either “Path to BoE / obligation document” or “Path to
document to verify.”
- Line 1675: Update the Node.js version guidance in the README, including the
“Node.js 22+” text and the associated nvm install/use examples, to specify
Node.js 22.19.5+ in alignment with the package.json engine requirement.
In `@src/commands/obligation-escrow/accept-return-to-issuer.ts`:
- Around line 17-25: Ensure prompt-phase failures set a nonzero exit code in the
outer handler catches. In
src/commands/obligation-escrow/accept-return-to-issuer.ts lines 17-25,
src/commands/obligation-escrow/endorse-transfer-owner.ts lines 18-26, and
src/commands/obligation-escrow/return-to-issuer.ts lines 17-25, update each
handler catch to set process.exitCode = 1 after logging the error.
In `@src/commands/obligation-escrow/discharge.ts`:
- Around line 17-25: Replace the duplicated try/catch handlers with
runObligationEscrowCommand: update src/commands/obligation-escrow/discharge.ts
lines 17-25 to pass promptForInputs and dischargeHandler, and
src/commands/obligation-escrow/reject.ts lines 17-25 to pass promptForInputs and
rejectHandler. Remove the manual error logging wrapper so the shared command
runner handles failures and exit codes.
In `@src/commands/obligation-escrow/reject-return-to-issuer.ts`:
- Around line 17-25: Replace the manual outer try/catch in handler with the
shared runObligationEscrowCommand helper, adding its import from ./shared and
passing promptForInputs and rejectReturnedHandler in
src/commands/obligation-escrow/reject-return-to-issuer.ts:17-25. Apply the same
change in src/commands/obligation-escrow/transfer-holder.ts:18-26, passing
promptForInputs and changeHolderHandler; remove the duplicated error handling so
prompt failures set the CLI exit code through the shared helper.
In `@src/commands/obligation-escrow/shared.ts`:
- Around line 48-60: The obligation command handlers must consistently return a
nonzero exit code on prompt or command failure. Keep runObligationEscrowCommand
in src/commands/obligation-escrow/shared.ts as the single wrapper; update
handlers in src/commands/obligation-escrow/accept.ts lines 17-25 and
src/commands/obligation-escrow/reject-transfer-owner.ts lines 17-25 to call it
with their prompt and handler functions, removing duplicated try/catch logic. In
src/commands/obligation-registry/deploy.ts lines 146-148, rethrow the deployment
error or explicitly set process.exitCode = 1 after logging.
In `@src/commands/obligation-registry/deploy.ts`:
- Around line 112-124: Update the dry-run flow around
performDryRunWithConfirmation so getTransactionCallback returns the populated
unsigned transaction for deployObligationRegistry from `@trustvc/trustvc`,
including its actual calldata and value, instead of a zero-data self-transfer.
If that deployment request cannot be populated before confirmation, defer fee
reporting to the broadcast receipt rather than estimating the placeholder
transaction.
In `@src/commands/obligation-registry/mint.ts`:
- Around line 94-96: Update the mint failure handling in mintObligationToken and
the handler catch path to set process.exitCode = 1 after logging the error,
matching the escrow command behavior so failed mints exit non-zero while
preserving the existing error message.
- Around line 126-128: Update the `!shouldProceed` branch after
`performDryRunWithConfirmation` so it does not call `process.exit(0)`. Return or
throw a sentinel that distinguishes cancellation from a definitive dry-run
revert, allowing the caller to choose a nonzero exit status for the revert and
preserving pending output flushing.
In `@src/commands/verify.ts`:
- Around line 149-154: Update verifyW3CDocument’s W3C lookup-failure and
OpenAttestation missing-chainId paths to avoid unconditionally calling
promptNetworkSelection. Prefer the explicit --network option when provided, and
otherwise use a non-interactive fallback that verifies without a provider in
non-TTY environments; retain interactive selection only when appropriate.
In `@src/utils/cli-errors.ts`:
- Around line 100-108: Update normalizeLabel so it only returns a label when the
entire cleaned value matches the intended identifier format, rather than
extracting the first word from free-text messages. Preserve the existing cleanup
and unknown-error filtering, allowing multi-word err.reason values handled by
extractContractRevertLabel to fall through and be surfaced intact by
describeContractError.
- Around line 158-224: Remove the unreachable error-instance branches from
extractContractRevertLabel and isContractCallException, since asEthersError
already returns a non-null object for every Error. Preserve the existing
fallback behavior through the later err.message handling and the remaining
pre-check validation without duplicating the plain Error path.
In `@src/utils/obligation-document.ts`:
- Around line 42-58: Remove the duplicated obligationRegistry and tokenId
validation block in the obligation-document validation flow, ensuring the
remaining if (!tokenId) conditional is properly closed before the chainId
checks. Preserve the existing validation order and error messages for
obligationRegistry, tokenId, chainId, and SUPPORTED_CHAINS.
---
Nitpick comments:
In `@src/commands/helpers.ts`:
- Around line 440-459: Extract the provider presence and
provider.getCode(address) validation from connectToObligationRegistry into a
shared helper, then reuse that helper in both connectToObligationRegistry and
connectToObligationEscrow. Preserve the existing rejection behavior and update
the missing-contract error to state that no contract exists at the supplied
address.
- Around line 476-487: Update the registry lookup flow around `registry` and
`ownerOf` to validate that `address` contains deployed contract bytecode before
calling `ownerOf`. Reuse `connectToObligationRegistry` or the existing shared
assertion helper, then resolve and validate the escrow address as before.
In `@src/commands/obligation-escrow/shared.ts`:
- Around line 23-41: Extract the credential-priority selection from the shared
command result builder into a reusable withWalletCredentials helper, preserving
the encryptedWalletPath → keyFile → key order and baseResult fallback. Replace
the duplicate chain in the current flow and reuse the helper in the
obligation-registry deploy flow so both commands share one consistent
implementation.
In `@src/commands/obligation-escrow/status.ts`:
- Around line 40-42: Update the status command flow around
getObligationRegistryStatus, isObligationRegistryRegistered, and
getObligationEscrowTerminationReason to start all three independent reads
concurrently and await their results together, preserving the existing variables
and downstream behavior.
- Around line 16-27: Replace the manually maintained STATUS_LABEL and
REASON_LABEL mappings with labels derived from the exported numeric enum reverse
mappings, ObligationDocumentStatus and ObligationEscrowTerminationReason. Update
the status/reason lookup usage as needed while preserving the existing label
output for current values and automatically supporting newly added enum members.
In `@src/utils/cli-errors.ts`:
- Around line 17-75: Add an InvalidTokenTransferToZeroAddressOwners entry to
KNOWN_REVERT_MESSAGES with actionable guidance for the zero-address owners
transfer error, matching the existing REVERT_SELECTOR_TO_NAME mapping so
describeContractError returns a user-facing message instead of the generic
fallback.
- Around line 50-75: The CLI currently hardcodes contract revert selectors in
REVERT_SELECTOR_TO_NAME, which can become stale. Update the revert-decoding
logic in cli-errors.ts to use the existing ethers ABI Interface.parseError(data)
for selectors covered by the contract ABI, formatting the parsed error name and
arguments as appropriate; retain REVERT_SELECTOR_TO_NAME only as a fallback for
selectors whose full ABI signatures are unavailable.
In `@tests/commands/obligation-escrow/discharge.test.ts`:
- Around line 79-90: Update tests/commands/obligation-escrow/discharge.test.ts
lines 79-90 by passing remark: 'discharged' to dischargeHandler and asserting
sdkParams.remarks equals 'discharged'. Update
tests/commands/obligation-escrow/reject.test.ts lines 76-87 by passing remark:
'rejected' to rejectHandler and asserting sdkParams.remarks equals 'rejected',
ensuring both tests verify remark propagation rather than an absent property.
In `@tests/commands/obligation-registry/deploy.test.ts`:
- Around line 94-125: Extend the deployObligationRegistryContract test suite
with a failure-path case where deployObligationRegistry rejects or a dependency
throws. Assert the resulting process.exitCode or propagated error, targeting the
error handling in deployObligationRegistryContract and covering the exit-code
behavior referenced in the review.
In `@tests/commands/obligation-registry/mint.test.ts`:
- Around line 113-157: Extend the obligation-registry mint tests with failure
and cancellation cases: configure mintObligationRegistry to reject and assert
mintObligationToken reports the expected exit code, then configure
performDryRunWithConfirmation to resolve false and assert
mintToObligationRegistry does not send a transaction. Reuse the existing mocks
and symbols in mint.test.ts, covering the corresponding paths in
mintObligationToken and mintToObligationRegistry.
In `@tests/utils/contract-errors.test.ts`:
- Around line 1-67: Add a test in the contract revert error formatting suite
using an error with a plain-text, multi-word reason such as “insufficient funds
for transfer” that is not a known custom-error label. Assert that both
getErrorMessage and describeContractError preserve the complete reason text
unchanged, covering normalizeLabel without altering the existing custom-error
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: 415fbd43-034f-4828-a2c6-278ab78a3d17
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (52)
.github/workflows/ci.yml.github/workflows/release.yml.releaserc.jsonREADME.mdpackage.jsonsamples/obligation-credential-subject.sample.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/fixtures/obligation/w3c-obligation-record.jsontests/utils/contract-errors.test.ts
…ests for verify obligation command
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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`:
- Around line 1515-1516: Update the README’s “Who Can Execute” guidance for
return-to-issuer to match the command contract: state that execution uses one
wallet holding both the current holder and beneficiary roles, or consistently
document the separate holder/beneficiary signature flow used by the contract and
tests. Apply the chosen rule to all README references for this command.
🪄 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: d0ffa818-0012-44d2-86c9-c676d45ce9eb
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (7)
README.mdpackage.jsonsrc/commands/obligation-escrow/discharge.tssrc/commands/obligation-escrow/reject.tssrc/commands/obligation-escrow/return-to-issuer.tssrc/utils/obligation-document.tstests/commands/verify.obligation.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/commands/obligation-escrow/discharge.ts
- 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.
This was referenced Aug 3, 2026
Closed
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
New Features
Documentation
Release Updates
Tests