Feature/boe v2 - #27
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.
|
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:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
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)
125-154: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAvoid prompting for network selection before falling back to provider-free verification.
verifyW3CDocument()callspromptNetworkSelection()for transferable, obligation, and revokable documents whenever chain-id mapping or provider lookup fails, even though it has a provider-free fallback afterward. In a script, CI, or other non-TTY context, CLI verification will wait for interactive network selection instead of completing. Gate this with an interactive/TTY check or support a--networkoverride before falling back toverifyDocument(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 125 - 154, Update verifyW3CDocument so failed chain-id mapping or provider lookup does not unconditionally call promptNetworkSelection; use an explicit network override when provided, otherwise prompt only in an interactive TTY, and fall back to provider-free verifyDocument(signedVC) in non-interactive contexts.
🧹 Nitpick comments (8)
src/commands/helpers.ts (1)
465-497: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
connectToObligationRegistryinstead of duplicating its logic.
connectToObligationEscrowre-implements the registry connection (Line 472) thatconnectToObligationRegistryalready provides, instead of calling it. This duplicates thenew ethers.Contract(address, TrustVCToken__factory.abi, wallet as any)construction and also skips the (dead, but present) registry validation thatconnectToObligationRegistryperforms, making the two code paths inconsistent.♻️ Proposed refactor
export const connectToObligationEscrow = async ({ tokenId, address, wallet, }: ConnectToObligationEscrowArgs) => { 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 465 - 497, Update connectToObligationEscrow to obtain the registry by calling connectToObligationRegistry with the provided address and wallet, removing its duplicated ethers.Contract construction and local registry setup. Preserve the existing tokenId lookup, escrow validation, connection, success logging, and error propagation behavior..github/workflows/release.yml (1)
46-50: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winRemove
id-token: writeunless OIDC publishing is intentional.The release step uses
NPM_TOKENandNODE_AUTH_TOKENfor npm authentication. The supplied configuration does not show an OIDC or provenance flow.id-token: writeallows steps in this job to request an OIDC token. Keep it only if npm trusted publishing or provenance is configured and required. (github.com)Token-authentication configuration
contents: write issues: write pull-requests: write - id-token: write🤖 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 around lines 46 - 50, Remove the id-token: write permission from the release workflow permissions block, since npm authentication uses NPM_TOKEN and NODE_AUTH_TOKEN. Retain only the permissions required by the release job unless an explicit OIDC trusted-publishing or provenance flow is configured..releaserc.json (1)
2-9: 🔒 Security & Privacy | 🔵 TrivialProtect
betabefore enabling publish-on-push.This configuration makes
betaa prerelease branch, and.github/workflows/release.ymlruns on pushes tobetaat Lines [7]-[10]. Any actor who can push tobetacan publish a beta release. Verify that branch protection requires reviews and status checks and restricts direct pushes. semantic-release recommends protecting release branches. (semantic-release.gitbook.io)🤖 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 @.releaserc.json around lines 2 - 9, Protect the beta branch before relying on the release workflow’s publish-on-push behavior: configure repository branch protection for beta to require pull-request reviews and passing status checks, while restricting direct pushes to authorized actors. Keep the existing beta prerelease settings in the semantic-release configuration unchanged.src/commands/obligation-escrow/status.ts (1)
42-51: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winParallelize the independent status reads.
getObligationRegistryStatus,isObligationRegistryRegistered, andgetObligationEscrowTerminationReasonread the sameopts/wallet/tokenIdand do not depend on each other's result. Running them sequentially adds each call's network round-trip time on top of the others.Use
Promise.allto run the three reads concurrently.⚡ Proposed fix
- 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 42 - 51, Update the status reads in the command’s try block to use Promise.all, invoking getObligationRegistryStatus, isObligationRegistryRegistered, and getObligationEscrowTerminationReason concurrently with the existing opts, wallet, and tokenId arguments. Destructure the results in the same order so the existing registered, status, and reason handling remains unchanged.tests/commands/obligation-escrow/return-to-issuer.test.ts (1)
57-67: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd an assertion on the call arguments passed to
runObligationEscrowTx.This test only checks that
runObligationEscrowTxis called. It doesn't verify what parametersreturnToIssuerHandlerpasses, unlike the sibling tests fordischarge.test.tsandreject.test.ts, which assert onsdkParams. Add a similar check here to catch a regression in parameter mapping.♻️ Proposed addition
expect(runObligationEscrowTx as MockedFunction<any>).toHaveBeenCalled(); + expect(runObligationEscrowTx as MockedFunction<any>).toHaveBeenCalledWith( + expect.objectContaining({ + args: expect.objectContaining({ tokenId: '0x1' }), + }), + );🤖 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 57 - 67, Update the returnToIssuerHandler test to assert the sdkParams passed to runObligationEscrowTx, matching the parameter-mapping assertions in the sibling discharge and reject tests. Verify the expected network, obligationRegistryAddress, tokenId, key, and maxPriorityFeePerGasScale values in addition to confirming the call occurred.src/utils/cli-errors.ts (2)
131-141: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify the unreachable
if (!err)/instanceof Errorbranches.
asEthersError(Line 91-92) treats any object as truthy, including plainErrorinstances (typeof new Error() === 'object'). This makes theif (error instanceof Error) {...}branch insideif (!err) {...}unreachable here, and in the equivalent branches ofisContractCallException(Lines 188-191) anddescribeContractError(Lines 228-230):error instanceof Erroralways implieserroris an object, soasEthersErrornever returnsnullfor it, anderris never falsy in that case.Current output is unaffected because equivalent logic already runs in the
err-truthy path (the message-regex checks at Lines 169-180, and theerr.messagefallback indescribeContractError). Remove the dead branches, or document why they exist, so a future edit to this "Error-only" path does not appear to change behavior when it will not.🤖 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 131 - 141, Remove the unreachable Error-only fallback branches guarded by !err in extractContractRevertLabel, isContractCallException, and describeContractError. Preserve the existing message-regex and err.message fallback behavior in their truthy error paths, without changing output or adding redundant handling.
1-1: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDead
instanceof Errorfallback branches acrosscli-errors.tsandformatting.ts.asEthersErrortreats any object — including plainErrorinstances — as non-null, sincetypeof new Error() === 'object'. This makes everyif (!err) { if (error instanceof Error) {...} }-style special case for plainErrorobjects unreachable, because reachingerror instanceof Erroralready guaranteeserris truthy. Current behavior is unaffected only because equivalent logic is duplicated in theerr-truthy path at each site.
src/utils/cli-errors.ts#L131-141: remove or clarify the unreachableerror instanceof Errorbranch inextractContractRevertLabel, since theerr-truthy message-regex checks (Lines 169-180) already cover it.src/utils/cli-errors.ts#L186-198: remove or clarify the equivalent unreachable branch inisContractCallException(Lines 188-191).src/utils/cli-errors.ts#L228-230: remove or clarify the equivalent unreachable branch indescribeContractError.src/utils/formatting.ts#L37-57: the "Pre-check .* failed:" check (Lines 49-52) is only reached afterisContractCallExceptionalready returnsfalsefor the same error, and produces the same output as falling through to Line 54; simplify or remove it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/utils/cli-errors.ts` at line 1, Remove the unreachable plain-Error fallback branches in extractContractRevertLabel, isContractCallException, and describeContractError, preserving the existing err-truthy handling that already covers Error instances. In formatting.ts, simplify the redundant “Pre-check .* failed:” special case in the relevant formatting function so these errors use the existing fallthrough output.tests/utils/contract-errors.test.ts (1)
1-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a negative test for the
"failed:"fallback regex.Current tests confirm labels are extracted correctly for genuine custom errors (Lines 10-48) and SDK "Pre-check" wrappers (Lines 50-55). Add a test asserting that a generic, non-revert error message (for example,
new Error('Request failed: timeout')with norevert/code/data) is NOT classified as a contract call exception. This guards the fallback heuristic flagged insrc/utils/cli-errors.tsat Lines 169-180.🤖 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 - 56, Add a negative test in the contract error formatting suite using a generic error such as “Request failed: timeout” without revert metadata, and assert isContractCallException returns false. Keep the test focused on preventing the “failed:” fallback heuristic from classifying non-revert errors as contract call exceptions.
🤖 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/ci.yml:
- Around line 3-9: Update the workflow’s on.push trigger to include a branches
filter for main and beta, matching the existing pull_request.branches
configuration; keep push runs limited to those two branches.
In `@README.md`:
- Line 1743: Update the workflow instruction code fence in README.md from an
unspecified language to text, preserving the block’s numbered instructions and
contents.
- Around line 1770-1772: Renumber the “Verify” guide step from 8 to 9, leaving
the preceding “Return to issuer” step and its content unchanged.
- Line 1514: Update the obligation-escrow status description in README.md to
state that the CLI requires wallet or private-key inputs, matching statusHandler
and promptBaseObligationEscrowInputs; do not claim the status operation is
wallet-free.
- Line 1692: Update the TrustVC README link in the “For library / SDK details”
reference to use an anchor that exists in the target repository, or remove/defer
the deep link until the Obligation Registry section is available; preserve the
reference to the Obligation Registry documentation.
In `@src/commands/helpers.ts`:
- Around line 432-454: Remove the unreachable truthiness checks for registry and
escrow instances in connectToObligationRegistry and the corresponding escrow
connection flow. Replace them with actual contract validation, such as
confirming deployed bytecode exists at the supplied address before logging
success and returning the instance; preserve the existing error logging and
rethrow behavior for invalid addresses or connection failures.
In `@src/commands/obligation-escrow/accept-return-to-issuer.ts`:
- Around line 29-51: Update the catch block in acceptReturnedHandler to set
process.exitCode = 1 after logging getErrorMessage(e), or rethrow the error so
the outer handler applies the non-zero exit code consistently. Ensure failed
obligation-return transactions no longer allow the CLI to exit successfully.
In `@src/commands/obligation-escrow/accept.ts`:
- Around line 17-49: Inner transaction handlers swallow failures, so CLI
commands exit successfully after errors. In
src/commands/obligation-escrow/accept.ts lines 17-49, update acceptHandler to
rethrow after logging or set process.exitCode; apply the same change to
dischargeHandler in src/commands/obligation-escrow/discharge.ts lines 17-50 and
rejectHandler in src/commands/obligation-escrow/reject.ts lines 17-49, ensuring
transaction failures produce a non-zero process outcome.
In `@src/commands/obligation-escrow/endorse-transfer-owner.ts`:
- Around line 34-55: Update the catch block in endorseHandler to set a non-zero
process.exitCode after logging getErrorMessage(e), ensuring failed beneficiary
transfers cause the CLI to exit unsuccessfully without changing the existing
success flow.
In `@src/commands/obligation-escrow/reject-return-to-issuer.ts`:
- Around line 29-51: Update the catch block in rejectReturnedHandler to set
process.exitCode to a non-zero value after logging the error with
getErrorMessage(e), ensuring failed restore transactions cause the CLI to exit
unsuccessfully.
In `@src/commands/obligation-escrow/reject-transfer-owner.ts`:
- Around line 29-50: Update the catch block in rejectTransferOwnerHandler to set
process.exitCode to a non-zero value after logging getErrorMessage(e), ensuring
failed beneficiary-transfer rejections cause the CLI to exit unsuccessfully.
In `@src/commands/obligation-escrow/return-to-issuer.ts`:
- Around line 29-50: Update the catch block in returnToIssuerHandler to set
process.exitCode to a non-zero value after logging getErrorMessage(e), ensuring
failed return-to-issuer operations cause the CLI to exit unsuccessfully while
preserving the existing error logging.
In `@src/commands/obligation-escrow/runTx.ts`:
- Around line 44-64: Replace the process.exit(0) branch in the transaction flow
around performDryRunWithConfirmation with a normal cancellation result such as
null, and update the function’s return type accordingly. Modify callers
including acceptReturnedHandler and endorseHandler to detect the cancellation
sentinel before invoking displayTransactionPrice, allowing their existing
cleanup and exit-code handling to run.
In `@src/commands/obligation-escrow/status.ts`:
- Around line 29-59: Factor the repeated prompt-and-command execution and error
handling into one shared helper that logs failures and sets process.exitCode to
1 (or rethrows) so CLI failures are non-zero. Replace the local try/catch
wrappers in status.ts (29-59), nominate-transfer-owner.ts (18-55),
reject-transfer-holder.ts (17-50), reject-transfer-owner-holder.ts (17-50), and
transfer-owner-holder.ts (18-65), covering each file’s handler and
command-specific handler while preserving successful execution and prompt
cancellation behavior.
In `@src/commands/obligation-escrow/transfer-holder.ts`:
- Around line 34-58: Update the catch block in changeHolderHandler to set
process.exitCode to a non-zero value after logging getErrorMessage(e), ensuring
failed holder-transfer transactions cause the CLI to exit unsuccessfully.
In `@src/utils/cli-errors.ts`:
- Around line 169-180: Update extractContractRevertLabel’s wrapped-message
handling so identifiers following “failed:” are accepted only when they match a
known revert name or the expected Solidity custom-error pattern. Preserve valid
“Contract reverted with” labels, but reject generic preprocessing messages such
as “SDK preprocessing failed: badInput” so isContractCallException does not
classify them as contract reverts.
In `@src/utils/obligation-document.ts`:
- Around line 42-52: Validate that the parsed chainId exists in SUPPORTED_CHAINS
before accessing its name in the obligation-document flow. Add a clear
domain-specific error for unsupported chain IDs, while preserving the existing
validation and network assignment behavior for supported chains.
---
Outside diff comments:
In `@src/commands/verify.ts`:
- Around line 125-154: Update verifyW3CDocument so failed chain-id mapping or
provider lookup does not unconditionally call promptNetworkSelection; use an
explicit network override when provided, otherwise prompt only in an interactive
TTY, and fall back to provider-free verifyDocument(signedVC) in non-interactive
contexts.
---
Nitpick comments:
In @.github/workflows/release.yml:
- Around line 46-50: Remove the id-token: write permission from the release
workflow permissions block, since npm authentication uses NPM_TOKEN and
NODE_AUTH_TOKEN. Retain only the permissions required by the release job unless
an explicit OIDC trusted-publishing or provenance flow is configured.
In @.releaserc.json:
- Around line 2-9: Protect the beta branch before relying on the release
workflow’s publish-on-push behavior: configure repository branch protection for
beta to require pull-request reviews and passing status checks, while
restricting direct pushes to authorized actors. Keep the existing beta
prerelease settings in the semantic-release configuration unchanged.
In `@src/commands/helpers.ts`:
- Around line 465-497: Update connectToObligationEscrow to obtain the registry
by calling connectToObligationRegistry with the provided address and wallet,
removing its duplicated ethers.Contract construction and local registry setup.
Preserve the existing tokenId lookup, escrow validation, connection, success
logging, and error propagation behavior.
In `@src/commands/obligation-escrow/status.ts`:
- Around line 42-51: Update the status reads in the command’s try block to use
Promise.all, invoking getObligationRegistryStatus,
isObligationRegistryRegistered, and getObligationEscrowTerminationReason
concurrently with the existing opts, wallet, and tokenId arguments. Destructure
the results in the same order so the existing registered, status, and reason
handling remains unchanged.
In `@src/utils/cli-errors.ts`:
- Around line 131-141: Remove the unreachable Error-only fallback branches
guarded by !err in extractContractRevertLabel, isContractCallException, and
describeContractError. Preserve the existing message-regex and err.message
fallback behavior in their truthy error paths, without changing output or adding
redundant handling.
- Line 1: Remove the unreachable plain-Error fallback branches in
extractContractRevertLabel, isContractCallException, and describeContractError,
preserving the existing err-truthy handling that already covers Error instances.
In formatting.ts, simplify the redundant “Pre-check .* failed:” special case in
the relevant formatting function so these errors use the existing fallthrough
output.
In `@tests/commands/obligation-escrow/return-to-issuer.test.ts`:
- Around line 57-67: Update the returnToIssuerHandler test to assert the
sdkParams passed to runObligationEscrowTx, matching the parameter-mapping
assertions in the sibling discharge and reject tests. Verify the expected
network, obligationRegistryAddress, tokenId, key, and maxPriorityFeePerGasScale
values in addition to confirming the call occurred.
In `@tests/utils/contract-errors.test.ts`:
- Around line 1-56: Add a negative test in the contract error formatting suite
using a generic error such as “Request failed: timeout” without revert metadata,
and assert isContractCallException returns false. Keep the test focused on
preventing the “failed:” fallback heuristic from classifying non-revert errors
as contract call exceptions.
🪄 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: 73fc1030-d2ef-4ca7-b242-d3079305e27d
⛔ 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
| const shouldProceed = await performDryRunWithConfirmation({ | ||
| network, | ||
| getTransactionCallback: async () => { | ||
| const escrow = await connectToObligationEscrow({ | ||
| tokenId, | ||
| address: obligationRegistryAddress, | ||
| wallet, | ||
| }); | ||
| const registry = await connectToObligationRegistry({ | ||
| address: obligationRegistryAddress, | ||
| wallet, | ||
| }); | ||
| const encryptedRemark = validateAndEncryptRemark(remark, encryptionKey); | ||
| const tx = await populate({ escrow, registry }, encryptedRemark); | ||
| return { ...tx, from: await wallet.getAddress() }; | ||
| }, | ||
| }); | ||
|
|
||
| if (!shouldProceed) { | ||
| process.exit(0); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Replace process.exit(0) with a normal return/throw path.
When the user declines the dry run, process.exit(0) terminates the process immediately. This has three effects:
- No message tells the user the transaction was cancelled.
- The caller's
try/catchin every command handler (e.g.,acceptReturnedHandler,endorseHandler) never runs, so any cleanup logic there is skipped. - Unit tests for this cancellation path require mocking
process.exit, or they crash the test runner.
Return a sentinel or throw a distinguishable "cancelled" error instead, and let each caller decide how to react and set its own exit code.
Proposed fix
if (!shouldProceed) {
- process.exit(0);
+ signale.info('Transaction cancelled by user.');
+ return undefined as unknown as TransactionReceipt;
}Adjust the return type/signature and callers accordingly (e.g., return null and have callers check for it before calling displayTransactionPrice).
🤖 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/runTx.ts` around lines 44 - 64, Replace the
process.exit(0) branch in the transaction flow around
performDryRunWithConfirmation with a normal cancellation result such as null,
and update the function’s return type accordingly. Modify callers including
acceptReturnedHandler and endorseHandler to detect the cancellation sentinel
before invoking displayTransactionPrice, allowing their existing cleanup and
exit-code handling to run.
| export const changeHolderHandler = async (args: ObligationEscrowTransferHolderCommand) => { | ||
| try { | ||
| info( | ||
| `Changing holder of obligation ${args.tokenId} on ${args.obligationRegistryAddress} to ${args.newHolder}`, | ||
| ); | ||
| warn('Only the current holder can transfer the holder role.'); | ||
| const transaction = await runObligationEscrowTx({ | ||
| args, | ||
| populate: ({ escrow }, encryptedRemark) => | ||
| escrow.transferHolder.populateTransaction(args.newHolder, encryptedRemark), | ||
| sdk: transferHolderObligationRegistry as any, | ||
| sdkParams: { holderAddress: args.newHolder, remarks: args.remark }, | ||
| }); | ||
| displayTransactionPrice( | ||
| transaction as unknown as TransactionReceiptFees, | ||
| args.network as NetworkCmdName, | ||
| ); | ||
| success(`Holder changed to ${args.newHolder}`); | ||
| info( | ||
| `Find more details at ${getEtherscanAddress({ network: args.network })}/tx/${transaction.hash}`, | ||
| ); | ||
| } catch (e) { | ||
| error(getErrorMessage(e)); | ||
| } | ||
| }; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Set a non-zero exit code on failure.
changeHolderHandler's catch block (Lines 55-57) only logs the error and never sets process.exitCode. A failed holder-transfer transaction still exits the CLI with code 0.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/commands/obligation-escrow/transfer-holder.ts` around lines 34 - 58,
Update the catch block in changeHolderHandler to set process.exitCode to a
non-zero value after logging getErrorMessage(e), ensuring failed holder-transfer
transactions cause the CLI to exit unsuccessfully.
| 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 = err.message.match(/(?:Contract reverted with\s*|failed:\s*)([A-Za-z0-9_]+)/); | ||
| if (wrapped?.[1]) { | ||
| const label = normalizeLabel(wrapped[1]); | ||
| if (label) return label; | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Locate file and relevant symbols"
git ls-files | rg '(^|/)src/utils/(cli-errors|cli-options)\.ts$' || true
echo
echo "Outline cli-errors"
ast-grep outline src/utils/cli-errors.ts --view expanded || true
echo
echo "Relevant cli-errors sections"
sed -n '1,240p' src/utils/cli-errors.ts
echo
echo "Relevant cli-options section"
sed -n '550,610p' src/utils/cli-options.ts
echo
echo "Search for transaction cancellation / proceed anyway logic"
rg -n "Transaction would revert|proceed|isContractCallException|extractContractRevertLabel|KNOWN_REVERT_MESSAGES|REVERT_SELECTOR_TO_NAME" src/utils -SRepository: TrustVC/trustvc-cli
Length of output: 14512
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Behavioral probe for the regex and normalizeLabel behavior from source-derived constants/functions
python3 - <<'PY'
import re
# Copied from the file excerpt and surrounding likely constants in generated search not available here;
# This script is not repository-coding dependent beyond extracting source text if provided.
# Instead, run as one-line in shell? Keeping this as placeholder because requested code needs actual source.
print("placeholder")
PYRepository: TrustVC/trustvc-cli
Length of output: 169
🌐 Web query:
OpenZeppelin custom error names PascalCase documentation
💡 Result:
In the Solidity ecosystem, including projects following OpenZeppelin's conventions, custom errors are named using PascalCase (often referred to as CapWords in the official Solidity Style Guide) [1][2][3]. While OpenZeppelin’s official contribution guidelines do not explicitly define a "PascalCase" rule for custom errors in their primary documentation, they consistently employ this naming convention across their contracts [4]. Their guidelines emphasize the following conventions for custom errors [5][6][7][8]: 1. Domain Prefixing: Errors should include a domain prefix, typically 'ERC' for specification violations or the component name (e.g., 'Governor', 'ECDSA') for internal logic [5][6][7][8]. This helps categorize errors and avoid naming collisions. 2. Avoiding Duplicates: Custom error names must not be declared twice within the library to prevent identifier conflicts during inheritance [5][6][8]. 3. Inclusion of Context: Errors should include relevant arguments (the "offending values") to provide sufficient context for decoding failure reasons [6]. 4. Location Priority: Developers are encouraged to declare errors in the most specific applicable location—starting from the underlying ERC (if defined there), then the interface or library, and finally the implementation or extension [5][6][8]. These practices align with the broader industry standard, such as that referenced by Coinbase's Solidity style guide, which explicitly mandates CapWords (PascalCase) for custom errors [2].
Citations:
- 1: https://docs.soliditylang.org/en/latest/style-guide.html
- 2: https://github.com/coinbase/solidity-style-guide
- 3: https://docs.soliditylang.org/en/v0.8.9/style-guide.html
- 4: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/master/contracts/utils/Errors.sol
- 5: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/HEAD/GUIDELINES.md
- 6: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/f7c16c10/.claude/skills/solidity-style/SKILL.md
- 7: https://github.com/OpenZeppelin/openzeppelin-contracts/blob/f7c16c10/GUIDELINES.md
- 8: https://github.com/OpenZeppelin/openzeppelin-contracts-upgradeable/blob/HEAD/GUIDELINES.md
Reject generic failed: words in extractContractRevertLabel.
extractContractRevertLabel can return any first identifier from message.match(/(?:Contract reverted with\s*|failed:\s*)([A-Za-z0-9_]+)/), and normalizeLabel does not restrict that input. Non-on-chain messages such as "SDK preprocessing failed: badInput" then satisfy isContractCallException, causing performDryRunWithConfirmation to abort with "Transaction would revert" instead of offering the gas-estimation "proceed anyway" path. Require the extracted label to match a known revert name or expected Solidity custom-error pattern before treating it as a revert.
🤖 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 169 - 180, Update
extractContractRevertLabel’s wrapped-message handling so identifiers following
“failed:” are accepted only when they match a known revert name or the expected
Solidity custom-error pattern. Preserve valid “Contract reverted with” labels,
but reject generic preprocessing messages such as “SDK preprocessing failed:
badInput” so isContractCallException does not classify them as contract reverts.
- 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>
1 participant
Summary by CodeRabbit
New Features
Documentation
Bug Fixes
Release