Feature/boe c2 fixes - #29
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
| with: | ||
| node-version: 22.x | ||
| cache: 'npm' | ||
| registry-url: 'https://registry.npmjs.org/' |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
if rg -n 'registry-url|NODE_AUTH_TOKEN' .github/workflows/release.yml; then
echo "Remove setup-node npm authentication for semantic-release." >&2
exit 1
fiRepository: TrustVC/trustvc-cli
Length of output: 328
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "## release workflow around setup-node"
sed -n '50,85p' .github/workflows/release.yml || true
echo
echo "## workflow trigger and dependencies"
sed -n '1,45p' .github/workflows/release.yml || true
echo
echo "## package type and scripts"
python3 - <<'PY'
import json
from pathlib import Path
p=Path('package.json')
if p.exists():
data=json.loads(p.read_text())
for k in ['name','type','scripts','devDependencies','dependencies']:
if k in data:
print(f"{k}:", data[k])
PYRepository: TrustVC/trustvc-cli
Length of output: 2992
Remove setup-node npm authentication for semantic-release.
setup-node creates an .npmrc when registry-url is set, and semantic-release npm authentication can conflict with that file. Use NPM_TOKEN for token authentication, or switch fully to npm Trusted Publishing, but do not configure both mechanisms.
Proposed fix
with:
node-version: 22.x
cache: 'npm'
- registry-url: 'https://registry.npmjs.org/'
env:
GITHUB_TOKEN: ${{ secrets.GH_TOKEN }}
NPM_TOKEN: ${{ secrets.NPM_TOKEN }}
- NODE_AUTH_TOKEN: ${{ secrets.NPM_TOKEN }}🤖 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 @.github/workflows/release.yml at 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.
| | **Token Registry** | `[mint](#mint)` | Mint tokens to blockchain registries | | ||
| | | `[token-registry deploy](#token-registry-deploy)` | Deploy token registry contracts | | ||
| | | `[mint](#mint)` | Mint tokens to blockchain registries | | ||
| | | `token-registry mint` | Alternative: `mint` | |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Remove the duplicate mint entry.
The table lists [mint] twice and then repeats token-registry mint as an alternative. Keep one primary mint row and one alternative row.
🤖 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` around lines 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.
|
|
||
| **Interactive Prompts:** | ||
|
|
||
| - Path to BoE / obligation documentverify |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the documentverify prompt label.
The prompt concatenates document and verify. Change it to Path to BoE / obligation document or Path to document to verify.
🤖 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 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.”
| ### Before you start | ||
|
|
||
| 1. **Install the CLI** — `npm install -g @trustvc/trustvc-cli` or `npx @trustvc/trustvc-cli <command>` | ||
| 2. **Node.js 22+** |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
jq -r '.engines.node' package.json
rg -n 'Node\.js 22\+|nvm (install|use) 22' README.mdRepository: TrustVC/trustvc-cli
Length of output: 338
Document the declared Node.js engine floor.
package.json requires Node.js >=22.19.5, but the README and the nvm example say Node.js 22+. Update the Node version guidance and nvm install 22 / nvm use 22 to Node.js 22.19.5+.
🤖 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 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.
Source: MCP tools
| export const handler = async (): Promise<void> => { | ||
| try { | ||
| const answers = await promptForInputs(); | ||
| if (!answers) return; | ||
| await acceptReturnedHandler(answers); | ||
| } catch (err: unknown) { | ||
| error(err instanceof Error ? err.message : String(err)); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Outer handler catch never sets process.exitCode in three obligation-escrow commands. In each file, the outer handler try/catch logs the error from promptForInputs() but does not set process.exitCode. Each command's inner handler already sets process.exitCode = 1 on its own failures, so a prompt-phase failure (document verification, wallet selection) silently exits with code 0 while an execution-phase failure exits with code 1. Automation or CI scripts relying on the exit code cannot reliably detect prompt-phase failures.
src/commands/obligation-escrow/accept-return-to-issuer.ts#L17-L25: addprocess.exitCode = 1;inside the outer catch block ofhandler.src/commands/obligation-escrow/endorse-transfer-owner.ts#L18-L26: addprocess.exitCode = 1;inside the outer catch block ofhandler.src/commands/obligation-escrow/return-to-issuer.ts#L17-L25: addprocess.exitCode = 1;inside the outer catch block ofhandler.
📍 Affects 3 files
src/commands/obligation-escrow/accept-return-to-issuer.ts#L17-L25(this comment)src/commands/obligation-escrow/endorse-transfer-owner.ts#L18-L26src/commands/obligation-escrow/return-to-issuer.ts#L17-L25
🤖 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/accept-return-to-issuer.ts` around lines 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.
| if (!shouldProceed) { | ||
| process.exit(0); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Do not exit with code 0 when the dry run reports a revert.
performDryRunWithConfirmation returns false in two cases: the user cancels, and the gas estimation hits a definitive contract revert (see src/utils/cli-options.ts Lines 575-583). This code maps both cases to process.exit(0), so a would-revert mint reports success. process.exit also terminates the process before pending output flushes.
Return a sentinel or throw instead, and let the caller decide the exit code.
🐛 Proposed fix
- if (!shouldProceed) {
- process.exit(0);
- }
+ if (!shouldProceed) {
+ throw new Error('Mint transaction was not submitted.');
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!shouldProceed) { | |
| process.exit(0); | |
| } | |
| if (!shouldProceed) { | |
| throw new Error('Mint transaction was not submitted.'); | |
| } |
🤖 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/mint.ts` around lines 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.
| const networkName = await promptNetworkSelection(); | ||
| const provider = getSupportedNetwork(networkName).provider() as unknown as V5Provider; | ||
| if (provider) { | ||
| return await CaptureConsoleWarnAsync(() => verifyDocument(signedVC, { provider })); | ||
| } | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether the verify command exposes a --network style flag or TTY guard
rg -n --type=ts -C5 "promptNetworkSelection|isTTY|--network|builder" src/commands/verify.tsRepository: TrustVC/trustvc-cli
Length of output: 1335
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '--- verify command outline ---\n'
ast-grep outline src/commands/verify.ts --view compact || true
printf '\n--- relevant verify.ts sections ---\n'
sed -n '1,220p' src/commands/verify.ts | cat -n
printf '\n--- promptNetworkSelection definition/usages ---\n'
rg -n --type=ts -C8 "function promptNetworkSelection|const promptNetworkSelection|promptNetworkSelection\\(" src
printf '\n--- CLI option definitions for verify ---\n'
rg -n --type=ts -C6 "\"verify\"|name:\\s*['\"]verify|command.*verify|builder:*|options:*|--network|network" srcRepository: TrustVC/trustvc-cli
Length of output: 248
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- verify command outline ---'
ast-grep outline src/commands/verify.ts --view compact || true
echo
echo '--- relevant verify.ts sections ---'
sed -n '1,220p' src/commands/verify.ts | cat -n
echo
echo '--- promptNetworkSelection definition/usages ---'
rg -n --type=ts -C8 'function promptNetworkSelection|const promptNetworkSelection|promptNetworkSelection\(' src
echo
echo '--- CLI option definitions for verify ---'
rg -n --type=ts -C6 '["'"'"']verify["'"'"']|name:\s*["'"'"']verify|command.*verify|builder:*|options:*|--network|network' srcRepository: TrustVC/trustvc-cli
Length of output: 50376
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- src/commands/verify.ts lines 1-220 ---'
sed -n '1,220p' src/commands/verify.ts | cat -n
echo
echo '--- all verify command definitions/options in src ---'
rg -n --type=ts -C8 'command\(["'"'"'"]verify|command:"verify"|name:\s*["'"'"'"]verify|builder:|builder\s*[\(:]|--network|network|promptNetworkSelection' src/commands src | head -n 220
echo
echo '--- cli-options promptNetworkSelection definition ---'
file_candidate="$(rg -l 'promptNetworkSelection\s*['"'"']|function\s+promptNetworkSelection|const\s+promptNetworkSelection' src | head -n 1 || true)"
if [ -n "${file_candidate:-}" ]; then
echo "candidate: $file_candidate"
sed -n '130,190p' "$file_candidate" | cat -n
fiRepository: TrustVC/trustvc-cli
Length of output: 27258
Avoid an unconditional interactive network prompt in verify runs.
In verifyW3CDocument, both the W3C automatic chain lookup failure path and the OpenAttestation missing chainId path call promptNetworkSelection() without a --network fallback. promptNetworkSelection() uses an interactive select(), while non-TTY environments need either an explicit network option or a silent fallback to verify without a provider.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/verify.ts` around lines 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.
| const normalizeLabel = (raw: string): string | undefined => { | ||
| const cleaned = raw.trim().replace(/\(\)$/, ''); | ||
| if (!cleaned || UNKNOWN_CUSTOM_ERROR.test(cleaned) || cleaned === '(unknown custom error)') { | ||
| return undefined; | ||
| } | ||
| // Drop leading junk like ": OwnerHolderMustDiffer" or parentheses wrappers | ||
| const ident = cleaned.match(/([A-Za-z][A-Za-z0-9_]*)/); | ||
| return ident?.[1]; | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Multi-word revert reasons get truncated to their first word.
normalizeLabel extracts an identifier with an unanchored regex (cleaned.match(/([A-Za-z][A-Za-z0-9_]*)/)), returning only the first matched word segment. extractContractRevertLabel calls this directly on err.revert?.name ?? err.errorName (Line 172) and on err.reason (Line 179) and returns as soon as it gets a truthy result, without the isKnownRevertName/SOLIDITY_CUSTOM_ERROR_NAME validation that acceptFailedSuffixLabel applies elsewhere in this file.
For a standard require(condition, "message") revert, ethers' CallExceptionError.reason is the full decoded message and commonly contains spaces (for example "insufficient funds for transfer"). With the current logic:
normalizeLabel("insufficient funds for transfer")returns just"insufficient".extractContractRevertLabelreturns"insufficient"as the label.describeContractErrorthen emits"Contract reverted with insufficient", discarding the rest of the actual revert message.
This affects any plain-text require revert across every obligation registry/escrow command that routes through getErrorMessage/describeContractError, not just the known custom errors this file targets.
Require the identifier to consume the whole cleaned string before treating it as a label, so free-text reasons fall through to the err.reason fallback in describeContractError (Line 246-248) and surface intact.
🐛 Proposed fix
const normalizeLabel = (raw: string): string | undefined => {
const cleaned = raw.trim().replace(/\(\)$/, '');
if (!cleaned || UNKNOWN_CUSTOM_ERROR.test(cleaned) || cleaned === '(unknown custom error)') {
return undefined;
}
- // Drop leading junk like ": OwnerHolderMustDiffer" or parentheses wrappers
- const ident = cleaned.match(/([A-Za-z][A-Za-z0-9_]*)/);
- return ident?.[1];
+ // Drop leading junk like ": OwnerHolderMustDiffer" or parentheses wrappers.
+ // Require the whole remaining string to be a single identifier so plain-text
+ // require() reasons (e.g. "insufficient funds for transfer") are rejected
+ // instead of being truncated to their first word.
+ const ident = cleaned.match(/^:?\s*([A-Za-z][A-Za-z0-9_]*)$/);
+ return ident?.[1];
};Also applies to: 172-181
🤖 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 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.
| export function extractContractRevertLabel(error: unknown): string | undefined { | ||
| const err = asEthersError(error); | ||
| if (!err) { | ||
| if (error instanceof Error) { | ||
| const fromExec = error.message.match(/execution reverted:\s*([A-Za-z0-9_]+)/); | ||
| if (fromExec?.[1]) { | ||
| const label = normalizeLabel(fromExec[1]); | ||
| if (label) return label; | ||
| } | ||
| return labelFromWrappedMessage(error.message); | ||
| } | ||
| return undefined; | ||
| } | ||
|
|
||
| const revertName = err.revert?.name ?? err.errorName; | ||
| if (typeof revertName === 'string') { | ||
| const label = normalizeLabel(revertName); | ||
| if (label) return label; | ||
| } | ||
|
|
||
| if (typeof err.reason === 'string') { | ||
| const label = normalizeLabel(err.reason); | ||
| if (label) return label; | ||
| } | ||
|
|
||
| // Prefer selector decoding before shortMessage — gas estimate often only has | ||
| // "execution reverted (unknown custom error)" + data: 0x7e288225 | ||
| const fromData = labelFromRevertData(err); | ||
| if (fromData) return fromData; | ||
|
|
||
| if (typeof err.shortMessage === 'string' && err.shortMessage.trim()) { | ||
| if (!UNKNOWN_CUSTOM_ERROR.test(err.shortMessage)) { | ||
| const match = err.shortMessage.match(/execution reverted(?::\s*)?(.+)?/i); | ||
| if (match?.[1]) { | ||
| const label = normalizeLabel(match[1]); | ||
| if (label) return label; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| if (typeof err.message === 'string') { | ||
| const custom = err.message.match(/execution reverted:\s*([A-Za-z0-9_]+)/); | ||
| if (custom?.[1]) { | ||
| const label = normalizeLabel(custom[1]); | ||
| if (label) return label; | ||
| } | ||
| const wrapped = labelFromWrappedMessage(err.message); | ||
| if (wrapped) return wrapped; | ||
| } | ||
|
|
||
| return undefined; | ||
| } | ||
|
|
||
| /** True when the failure is a definitive on-chain revert (not a transient gas/RPC issue). */ | ||
| export function isContractCallException(error: unknown): boolean { | ||
| const err = asEthersError(error); | ||
| if (!err) { | ||
| if (error instanceof Error && /Pre-check .* failed:/i.test(error.message)) { | ||
| return Boolean(extractContractRevertLabel(error)); | ||
| } | ||
| return false; | ||
| } | ||
| if (err.code === 'CALL_EXCEPTION') return true; | ||
| if (err.revert?.name || err.errorName) return true; | ||
| if (labelFromRevertData(err)) return true; | ||
| return Boolean(extractContractRevertLabel(error)); | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Dead branch: asEthersError never returns null for Error instances.
asEthersError treats any value with typeof value === 'object' as non-null, and typeof (new Error(...)) === 'object' is always true. As a result, error instanceof Error can only be true in exactly the cases where asEthersError(error) already returned non-null. This makes the if (!err) { if (error instanceof Error) {...} } blocks in extractContractRevertLabel (Lines 161-168) and isContractCallException (Lines 215-217) unreachable for every possible input: any value satisfying instanceof Error also satisfies the "object" type guard, and any value failing instanceof Error also fails that inner check.
Current tests still pass because equivalent logic is duplicated later via the err.message handling (Lines 198-206), but this duplication means the two code paths can silently diverge in the future if one is edited without the other.
Remove the dead branches, or make asEthersError return null for plain Error instances that lack ethers-specific fields so the intended fallback path actually executes.
🤖 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 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.
| if (!obligationRegistry) { | ||
| throw new Error('Document does not contain a valid obligationRegistry address'); | ||
| } | ||
| if (!tokenId) { | ||
| throw new Error('Document does not contain a valid token ID'); | ||
| if (!obligationRegistry) { | ||
| throw new Error('Document does not contain a valid obligationRegistry address'); | ||
| } | ||
| if (!tokenId) { | ||
| throw new Error('Document does not contain a valid token ID'); | ||
| } | ||
| if (!chainId) { | ||
| throw new Error('Document does not contain a valid chain ID'); | ||
| } | ||
| if (!SUPPORTED_CHAINS[chainId]) { | ||
| throw new Error(`Document references unsupported chain ID: ${chainId}`); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Fix duplicated, unbalanced conditional blocks causing a syntax error.
Lines 45-51 duplicate lines 42-44 and re-open if (!tokenId) without closing the first one. This produces unbalanced braces. The CI pipeline logs confirm an ESLint parsing error "'}' expected" at line 77, which is the direct result of this malformed block.
🐛 Proposed fix to remove the duplicated block
if (!obligationRegistry) {
throw new Error('Document does not contain a valid obligationRegistry address');
}
if (!tokenId) {
throw new Error('Document does not contain a valid token ID');
- if (!obligationRegistry) {
- throw new Error('Document does not contain a valid obligationRegistry address');
- }
- if (!tokenId) {
- throw new Error('Document does not contain a valid token ID');
}
if (!chainId) {
throw new Error('Document does not contain a valid chain ID');
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!obligationRegistry) { | |
| throw new Error('Document does not contain a valid obligationRegistry address'); | |
| } | |
| if (!tokenId) { | |
| throw new Error('Document does not contain a valid token ID'); | |
| if (!obligationRegistry) { | |
| throw new Error('Document does not contain a valid obligationRegistry address'); | |
| } | |
| if (!tokenId) { | |
| throw new Error('Document does not contain a valid token ID'); | |
| } | |
| if (!chainId) { | |
| throw new Error('Document does not contain a valid chain ID'); | |
| } | |
| if (!SUPPORTED_CHAINS[chainId]) { | |
| throw new Error(`Document references unsupported chain ID: ${chainId}`); | |
| } | |
| if (!obligationRegistry) { | |
| throw new Error('Document does not contain a valid obligationRegistry address'); | |
| } | |
| if (!tokenId) { | |
| throw new Error('Document does not contain a valid token ID'); | |
| } | |
| if (!chainId) { | |
| throw new Error('Document does not contain a valid chain ID'); | |
| } | |
| if (!SUPPORTED_CHAINS[chainId]) { | |
| throw new Error(`Document references unsupported chain ID: ${chainId}`); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/utils/obligation-document.ts` around lines 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.
Source: Pipeline failures
1 participant
Summary by CodeRabbit
New Features
Documentation
Release