Feature/boe integration testing - #31
Merged
Merged
Conversation
- Introduced new commands for managing BoE obligations, including accept, reject, transfer, and return functionalities. - Added types for obligation registry and escrow commands to enhance type safety and clarity. - Implemented shared utilities for prompting user inputs across obligation escrow commands. - Enhanced transaction handling with detailed logging and error management for better user experience.
…dentialSubject fields for improved clarity and accuracy
…evelopment instructions
- Updated .releaserc.json to include a new branch 'beta-boe' for beta prereleases. - Modified release.yml to trigger workflows on pushes to the 'beta-boe' branch.
- Bumped version to 1.1.0 in package.json and package-lock.json. - Updated @trustvc/trustvc dependency to version 2.15.0-beta.3. - Added new dependencies including @account-abstraction/contracts and @digitalbazaar/ecdsa-rdfc-2019-cryptosuite. - Improved README for clarity on obligation escrow commands.
…andling - Clarified the process for minting BoE token IDs, emphasizing the need to sign with `w3c-sign` before minting. - Improved instructions for using the Obligation Registry address in documents. - Enhanced error messages in CLI to provide clearer guidance on contract call exceptions. - Updated deployment logs to include instructions for using the Obligation Registry address.
- Renamed the 'beta-boe' branch to 'beta' in .releaserc.json for clarity. - Updated CI workflow to include checks for pushes and pull requests to 'main' and 'beta' branches, as well as feature branches. - Enhanced release workflow permissions and added npm registry URL for publishing. - Adjusted npm publish settings in the release configuration to ensure proper deployment.
- Improved formatting for better readability, including consistent use of markdown syntax. - Clarified instructions for using the Obligation Registry and related commands. - Removed unnecessary whitespace to enhance overall document cleanliness.
…scrow commands - Changed @trustvc/trustvc dependency to a local file reference for development. - Enhanced error handling in various obligation escrow commands to ensure proper exit codes on transaction failures. - Refactored command handlers to streamline transaction execution and error logging. - Updated README to clarify command usage and improve overall documentation quality.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
- Removed redundant error checks for obligationRegistry and tokenId. - Enhanced error message for unsupported chain IDs to provide clearer guidance on using a valid BoE document.
…ests for verify obligation command
- Refactored command handlers for various obligation escrow actions to utilize a shared execution function, reducing code duplication. - Improved error handling to ensure proper exit codes on transaction failures. - Updated README to reflect changes in command usage and prerequisites for Node.js version.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
- Removed redundant error handling for mint transaction submission. - Updated error handling to return null instead of throwing an error when the transaction should not proceed.
- Removed the explicit registry URL from the release workflow. - Cleaned up environment variable definitions for NPM token usage.
…andling in command files - Introduced a new JSON file for the driving licence credential schema. - Improved error handling in various command files to ensure proper exit codes on transaction failures. - Updated README for better command usage clarity.
- Updated input prompts in various obligation escrow command files to return null on cancellation, improving user experience. - Enhanced error handling in command handlers to ensure proper exit codes on transaction failures. - Clarified roles and responsibilities in the README for executing commands related to obligation escrow.
- Added checks to ensure proper exit codes when `shouldProceed` is null in various document store and title escrow command files. - Updated README to reflect the new structure of command files for better clarity.
- Added decrypted.json to .gitignore to prevent tracking of sensitive files. - Removed the decrypted.json file as it is no longer needed. - Updated README to clarify command usage and improve overall documentation structure.
- Removed redundant checks for `shouldProceed` being null in various command files to streamline exit logic. - Updated README to reflect changes in command verification processes for improved clarity.
- Simplified CLI error helpers by removing outdated constants and functions related to known revert messages and selectors. - Updated comments for clarity and conciseness, enhancing the overall readability of the error handling logic.
|
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:
📝 WalkthroughWalkthroughThe PR adds Obligation Registry deployment and minting, Obligation Escrow lifecycle and transfer commands, obligation-document extraction, network-aware verification, beta release configuration, tests, and expanded documentation. ChangesObligation Registry and minting
Escrow workflows
Verification and release support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant CLI
participant Document
participant Registry
participant Escrow
participant Blockchain
CLI->>Document: verify and extract obligation metadata
CLI->>Registry: deploy or mint obligation
CLI->>Escrow: execute lifecycle or transfer transaction
Registry->>Blockchain: submit registry transaction
Escrow->>Blockchain: submit escrow transaction
Blockchain-->>CLI: return transaction receipt and status
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
- Streamlined input handling in various command files to ensure consistent null returns on user cancellation. - Enhanced error response mechanisms to provide clearer exit codes on transaction failures. - Updated README for improved documentation on command usage and error handling practices.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
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)
200-229: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAlign
verifyOpenAttestationDocumentfallback handling withverifyW3CDocument.
verifyOpenAttestationDocumentonly callsresolveFallbackProviderwhenchainIdis missing (Line 214). IfchainIdis present but unsupported,getSupportedNetworkNameFromIdthrows inside thetryblock at Line 220, thecatchat Line 224 only logs a warning, and execution falls through to the no-provider fallback at Line 229.resolveFallbackProvideris never called in this case, so--networkcannot rescue verification for a document with an unrecognized chain ID.verifyW3CDocumentdoes not have this gap: it callsresolveFallbackProviderunconditionally after any chain-resolution failure (Lines 179-194).A second effect of the current structure: when
!chainIdandresolveFallbackProviderreturnsundefined(non-interactive session, no--networkgiven), execution still falls into thetryblock withchainIdstill falsy.getSupportedNetworkNameFromId(Number(chainId))is guaranteed to throw ("Unsupported chain id NaN"), producing a second, confusing warning after the already-logged non-interactive warning.Restructure so any chain-resolution failure (missing or unsupported chainId) falls back to
resolveFallbackProviderexactly once, matchingverifyW3CDocument.🐛 Proposed fix
- // If chainId is not found, prefer --network / TTY prompt / no-provider fallback - if (requiresNetwork && !chainId) { - const provider = await resolveFallbackProvider(options.network); - if (provider) return await verifyDocument(signedVC, { provider }); - } - try { const chainName = getSupportedNetworkNameFromId(Number(chainId)); const network = getSupportedNetwork(chainName); const provider = network.provider() as unknown as V5Provider; if (provider) return await verifyDocument(signedVC, { provider }); } catch (err: unknown) { signale.warn(`${err instanceof Error ? err.message : String(err)}`); } + // Chain lookup failed (missing or unsupported chainId): prefer --network / TTY prompt / no-provider fallback + const fallbackProvider = await resolveFallbackProvider(options.network); + if (fallbackProvider) return await verifyDocument(signedVC, { provider: fallbackProvider }); + // Fallback: Verify without provider return await verifyDocument(signedVC);This also removes the untested "unsupported chainId with
--networkoverride" path fromverify.test.ts; add a test case for it once fixed.🤖 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 200 - 229, Restructure verifyOpenAttestationDocument so chain resolution failures, including a missing or unsupported chainId, invoke resolveFallbackProvider(options.network) exactly once before the no-provider fallback. Avoid entering getSupportedNetworkNameFromId with a falsy chainId when no fallback provider is available, while preserving direct verification for non-network documents and provider-based verification for supported chains. Add coverage in verify.test.ts for an unsupported chainId rescued by the --network override.
🧹 Nitpick comments (17)
tests/fixtures/obligation/w3c-obligation-record-sepolia.json (1)
47-50: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
chainIdtype differs from the sibling fixture.This fixture stores
tokenNetwork.chainIdas a number (11155111).w3c-obligation-record.json'stokenNetwork.chainIdis a string ("80002"). Verification code inverify.tstolerates both viaNumber(chainId), but the inconsistency across fixtures obscures the actual schema expected for real documents. Align the type across obligation fixtures to match the real-world credential schema.🤖 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/fixtures/obligation/w3c-obligation-record-sepolia.json` around lines 47 - 50, Update tokenNetwork.chainId in the Sepolia obligation fixture to use the same string type as the sibling obligation fixture, preserving the real-world credential schema consistently across fixtures.tests/commands/verify.obligation.sepolia.test.ts (1)
78-92: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard
process.env.SEPOLIA_RPCcleanup withtry/finally.If
getSupportedNetwork(NetworkCmdName.Sepolia).provider()throws,delete process.env.SEPOLIA_RPCnever runs and may leak into later tests in the same process.♻️ Proposed fix
process.env.SEPOLIA_RPC = customRpc; - const { getSupportedNetwork, NetworkCmdName } = await import('../../src/utils/networks'); - const { JsonRpcProvider } = await import('ethers'); - const provider = getSupportedNetwork(NetworkCmdName.Sepolia).provider(); - - delete process.env.SEPOLIA_RPC; - - expect(provider).toBeInstanceOf(JsonRpcProvider); - expect((provider as any)._getConnection?.().url ?? (provider as any).connection?.url).toBe( - customRpc, - ); + try { + const { getSupportedNetwork, NetworkCmdName } = await import('../../src/utils/networks'); + const { JsonRpcProvider } = await import('ethers'); + const provider = getSupportedNetwork(NetworkCmdName.Sepolia).provider(); + + expect(provider).toBeInstanceOf(JsonRpcProvider); + expect((provider as any)._getConnection?.().url ?? (provider as any).connection?.url).toBe( + customRpc, + ); + } finally { + delete process.env.SEPOLIA_RPC; + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/commands/verify.obligation.sepolia.test.ts` around lines 78 - 92, Update the SEPOLIA_RPC override test around getSupportedNetwork(...).provider() to wrap the environment-variable setup and provider assertions in try/finally, deleting process.env.SEPOLIA_RPC in the finally block so cleanup always occurs when provider creation throws.src/commands/helpers.ts (2)
467-487: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
connectToObligationRegistryinsideconnectToObligationEscrow.Lines 473-474 duplicate the registry connection logic and its log message. Call the existing helper to keep one connection path.
♻️ 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 });🤖 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 467 - 487, Update connectToObligationEscrow to obtain the registry by calling the existing connectToObligationRegistry helper, removing the duplicated ethers.Contract construction and connection log while preserving the subsequent ownerOf lookup and validation flow.
436-449: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnnotate the return type and drop the unreachable falsy check.
new ethers.Contract(...)never returns a falsy value. It throws for an invalid address. Theif (!registry)branch is therefore unreachable. The helper also returns an untypedethers.Contract, so callers such assrc/commands/obligation-registry/mint.tsLine 116 accessregistry.mint.populateTransactionwith no type checking.connectToTitleEscrowat Line 128 annotates its return type; follow the same pattern here and inconnectToObligationEscrowat Line 488.♻️ Proposed refactor
export const connectToObligationRegistry = async ({ address, wallet, -}: ConnectToObligationRegistryArgs) => { +}: ConnectToObligationRegistryArgs): Promise<InstanceType<typeof TrustVCToken__factory>> => { try { signale.info(`Connecting to obligation registry at: ${address}`); const registry = new ethers.Contract(address, TrustVCToken__factory.abi, wallet as any); - if (!registry) { - const error = `Failed to connect to obligation registry at address: ${address}`; - signale.error(error); - throw new Error(error); - } signale.success(`Successfully connected to obligation registry`); - return registry; + return registry as unknown as InstanceType<typeof TrustVCToken__factory>;🤖 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 436 - 449, Update connectToObligationRegistry and connectToObligationEscrow to declare the same explicit ethers.Contract return type used by connectToTitleEscrow, preserving typed access for callers such as registry.mint.populateTransaction. Remove the unreachable !registry check and its associated error handling from both helpers while retaining their existing connection success and exception behavior.src/commands/obligation-registry/deploy.ts (1)
111-160: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse one network lookup for the chain ID.
Line 111 reads
supportedNetwork[network as NetworkCmdName].networkIdonly to log it at Line 160. Line 146 reads the same value throughgetSupportedNetwork(network). Keep a single lookup.♻️ Proposed refactor
const wallet = await getWalletOrSigner({ network, ...rest }); - const chainId = supportedNetwork[network as NetworkCmdName].networkId; + const chainId = getSupportedNetwork(network).networkId;Then reuse
chainIdat Line 146 and drop the now-unusedsupportedNetworkimport.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/obligation-registry/deploy.ts` around lines 111 - 160, Reuse the existing chainId variable when constructing the deployObligationRegistry options instead of calling getSupportedNetwork(network) again. Remove the now-unused supportedNetwork import while preserving the existing chain ID logging and deployment behavior.tests/commands/obligation-registry/mint.test.ts (2)
43-53: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant document-extraction mocks.
src/utilsis mocked at Line 64 andextractObligationDocumentInfois stubbed at Line 72. TheisObligationRecord,getObligationRegistryAddress,getTokenId, andgetChainIdmocks are therefore unreachable in this suite. Keep onlymintObligationRegistryto make the test intent clear.🤖 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 43 - 53, Update the `@trustvc/trustvc` mock in the test setup to retain only the mintObligationRegistry mock; remove the unreachable isObligationRecord, getObligationRegistryAddress, getTokenId, and getChainId stubs while preserving the actual module imports.
86-87: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInvoke the dry-run callback and cover the gas-estimation branch.
The mocked
performDryRunWithConfirmationresolvestruewithout callinggetTransactionCallback. The mockedconnectToObligationRegistry,validateAndEncryptRemark, andmint.populateTransactionat Lines 55-62 therefore never run.canEstimateGasPriceis fixed tofalse, sosrc/commands/obligation-registry/mint.tsLines 131-145 stay untested.tests/commands/obligation-escrow/reject-return-to-issuer.test.tsLines 82-85 shows the pattern that executes the callback.💚 Proposed change
- performDryRunWithConfirmation: vi.fn().mockResolvedValue(true), + performDryRunWithConfirmation: vi.fn(async ({ getTransactionCallback }) => { + await getTransactionCallback(); + return true; + }),Add a second case that sets
canEstimateGasPricetotrueand asserts thatmaxFeePerGasreachesmintObligationRegistry.🤖 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 86 - 87, Update the dry-run mock in the mint command tests so performDryRunWithConfirmation invokes the provided getTransactionCallback, allowing the mocked connectToObligationRegistry, validateAndEncryptRemark, and mint.populateTransaction paths to execute. Add a second test case with canEstimateGasPrice returning true and assert that the calculated maxFeePerGas is passed to mintObligationRegistry.tests/commands/obligation-registry/deploy.test.ts (1)
94-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the
escrowFactoryAddressdry-run branch and assert the SDK options.Both tests omit
escrowFactoryAddress, so theethers.ContractFactorydry-run path atsrc/commands/obligation-registry/deploy.tsLines 117-132 never runs. The mockedperformDryRunWithConfirmationat Line 62 also never invokesgetTransactionCallback, unlike the escrow tests. Line 112 only asserts thatdeployObligationRegistrywas called; it does not assert thechainIdandescrowFactoryAddressoptions. Add a case that suppliesescrowFactoryAddress, executes the callback, and asserts the fourth-argument options object.💚 Proposed additional assertion
expect(deployMock).toHaveBeenCalled(); + expect(deployMock).toHaveBeenCalledWith( + 'My BoE Registry', + 'BOE', + expect.anything(), + expect.objectContaining({ chainId: 80002 }), + ); expect(address).toBe('0xRegistry');🤖 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, Add a test case in deployObligationRegistryContract coverage that provides escrowFactoryAddress, invokes the mocked performDryRunWithConfirmation getTransactionCallback, and exercises the ethers.ContractFactory dry-run branch. Assert deployObligationRegistry receives the expected fourth-argument options, including chainId and escrowFactoryAddress, while preserving the existing deployment assertions.src/commands/obligation-escrow/shared.ts (1)
33-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the credential-selection branches.
The three sequential
ifblocks each spreadbaseResultand cast the result. Use a single conditional spread to reduce repetition.♻️ Proposed simplification
- if (encryptedWalletPath) { - return { ...baseResult, encryptedWalletPath } as BaseObligationEscrowCommand; - } - if (keyFile) { - return { ...baseResult, keyFile } as BaseObligationEscrowCommand; - } - if (key) { - return { ...baseResult, key } as BaseObligationEscrowCommand; - } - return baseResult as BaseObligationEscrowCommand; + const credential = encryptedWalletPath + ? { encryptedWalletPath } + : keyFile + ? { keyFile } + : key + ? { key } + : {}; + return { ...baseResult, ...credential } as BaseObligationEscrowCommand;🤖 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 33 - 42, Update the credential-selection logic in the shared command result construction to use one conditional spread for encryptedWalletPath, keyFile, and key instead of three repeated if blocks. Preserve the existing precedence order and BaseObligationEscrowCommand cast while retaining baseResult when no credential is provided.src/commands/obligation-escrow/accept.ts (2)
22-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExtract a shared runner for the repeated transaction-command boilerplate.
Six obligation-escrow command handlers repeat the same sequence: log an
infomessage, callrunObligationEscrowTxwith apopulate/sdk/sdkParamstriple cast withas any, calldisplayTransactionPrice, logsuccess, then log the Etherscan URL. The shared root cause is the absence of a helper that wraps this sequence, leaving each file to copy-paste it and independently bypass SDK typing withas any.
src/commands/obligation-escrow/accept.ts#L22-L42: extract the info/tx/price/success/info sequence into a shared helper (e.g.,executeObligationEscrowAction({ args, populate, sdk, sdkParams, startMessage, successMessage })), and drop the redundant local try/catch noted separately for this file.src/commands/obligation-escrow/discharge.ts#L22-L39: replace the handler body with a call to the shared helper, passingdischarge-specific messages andpopulate/sdk/sdkParams.src/commands/obligation-escrow/reject.ts#L21-L37: replace the handler body with a call to the shared helper, passingreject-specific messages andpopulate/sdk/sdkParams.src/commands/obligation-escrow/reject-return-to-issuer.ts#L21-L39: replace the handler body with a call to the shared helper, passingreject-return-to-issuer-specific messages and the registry-sidepopulate/sdk/sdkParams.src/commands/obligation-escrow/reject-transfer-owner.ts#L21-L38: replace the handler body with a call to the shared helper, passingreject-transfer-owner-specific messages andpopulate/sdk/sdkParams.src/commands/obligation-escrow/return-to-issuer.ts#L21-L38: replace the handler body with a call to the shared helper, passingreturn-to-issuer-specific messages andpopulate/sdk/sdkParams.A single shared helper also gives one place to properly type the
sdkparameter (for example, a union of the specific obligation-registry SDK function signatures) instead of repeatingas anyat each call site.🤖 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.ts` around lines 22 - 42, Extract the repeated transaction flow from acceptHandler into a shared executeObligationEscrowAction helper that handles start logging, runObligationEscrowTx, transaction pricing, success logging, and the Etherscan URL, while properly typing sdk instead of using as any; remove acceptHandler’s local try/catch and delegate with its accept-specific values. Apply the same delegation in src/commands/obligation-escrow/discharge.ts lines 22-39, reject.ts lines 21-37, reject-return-to-issuer.ts lines 21-39, reject-transfer-owner.ts lines 21-38, and return-to-issuer.ts lines 21-38, preserving each command’s messages and populate/sdk/sdkParams configuration.
22-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftDuplicated transaction-command boilerplate (deferred to consolidated comment).
This handler repeats the same info/tx/displayTransactionPrice/success/info shape and
as anySDK cast found indischarge.ts,reject.ts,reject-return-to-issuer.ts,reject-transfer-owner.ts, andreturn-to-issuer.ts. See the consolidated comment at the end of this review for a shared extraction proposal.🤖 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.ts` around lines 22 - 42, Extract the repeated obligation escrow transaction flow from acceptHandler and the related handlers into a shared helper, including status messages, displayTransactionPrice, transaction-link output, and error handling. Update acceptHandler, discharge, reject, reject-return-to-issuer, reject-transfer-owner, and return-to-issuer to use the helper, preserving each command’s transaction population and success wording while centralizing the SDK invocation and avoiding repeated as any casts.src/commands/obligation-escrow/transfer-holder.ts (1)
23-23: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant prompt description.
promptAddressbuilds the message asEnter the address of the ${role} (${description}):(src/utils/cli-options.ts:488-520). The current arguments produce "Enter the address of the new holder (new holder):". Omit the description, or use a distinct one.♻️ Proposed change
- const newHolder = await promptAddress('new holder', 'new holder'); + const newHolder = await promptAddress('new holder');🤖 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` at line 23, Update the promptAddress call assigning newHolder to remove the redundant second “new holder” description, allowing the prompt to display only the role-based message or a distinct description.tests/commands/obligation-escrow/accept.test.ts (2)
32-38: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe
@trustvc/trustvcmock is unused in these tests. Each test also mocks../../../src/commands/obligation-escrow/runTx, so the SDK function is passed as a value and never invoked. Eachvi.importActual('@trustvc/trustvc')still loads the full SDK, which adds test startup cost without adding coverage. Remove the block in each file, or extract the shared mock scaffolding (signale, utils, runTx) into one test helper and import it.
tests/commands/obligation-escrow/accept.test.ts#L32-L38: remove theacceptObligationRegistrymock block.tests/commands/obligation-escrow/discharge.test.ts#L35-L41: remove thedischargeObligationRegistrymock block.tests/commands/obligation-escrow/endorse-transfer-owner.test.ts#L35-L41: remove thetransferBeneficiaryObligationRegistrymock block.tests/commands/obligation-escrow/reject.test.ts#L32-L38: remove therejectObligationRegistrymock block.tests/commands/obligation-escrow/reject-transfer-owner.test.ts#L35-L41: remove therejectTransferBeneficiaryObligationRegistrymock block.tests/commands/obligation-escrow/return-to-issuer.test.ts#L32-L38: remove thereturnToIssuerObligationRegistrymock block.🤖 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/accept.test.ts` around lines 32 - 38, Remove the unused SDK mock blocks from tests/commands/obligation-escrow/accept.test.ts (32-38), discharge.test.ts (35-41), endorse-transfer-owner.test.ts (35-41), reject.test.ts (32-38), reject-transfer-owner.test.ts (35-41), and return-to-issuer.test.ts (32-38), including their vi.importActual calls and operation-specific mock functions; retain the existing runTx, signale, and utility mocks.
74-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winHandler tests assert only that the runner was called. Both tests would pass if the handler passed the wrong SDK function or the wrong
sdkParamskeys, which is the main contract these handlers own.tests/commands/obligation-escrow/reject.test.tsandtests/commands/obligation-escrow/discharge.test.tsshow the stronger pattern.
tests/commands/obligation-escrow/accept.test.ts#L74-L84: replacetoHaveBeenCalled()withtoHaveBeenCalledWith(expect.objectContaining({ sdk: expect.any(Function), sdkParams: expect.objectContaining({ remarks: undefined }) })).tests/commands/obligation-escrow/return-to-issuer.test.ts#L57-L67: replacetoHaveBeenCalled()with an equivalenttoHaveBeenCalledWithassertion on thesdkParamspayload forreturnToIssuerObligationRegistry.🤖 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/accept.test.ts` around lines 74 - 84, Strengthen the handler assertions so they verify the SDK callback and required parameter payload, not merely invocation. In tests/commands/obligation-escrow/accept.test.ts lines 74-84, assert runObligationEscrowTx was called with an object containing any function as sdk and sdkParams containing remarks: undefined; in tests/commands/obligation-escrow/return-to-issuer.test.ts lines 57-67, replace the call-only assertion with an equivalent toHaveBeenCalledWith objectContaining assertion covering the returnToIssuerObligationRegistry sdkParams payload.src/commands/obligation-escrow/runTx.ts (3)
69-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueCollapse the duplicated SDK invocation.
Both branches call
sdkwith the samecontractOptions,wallet, andsdkParams. Only the gas fields differ. Build the transaction options first, then callsdkonce.♻️ Proposed refactor
- let transaction; - - if (canEstimateGasPrice(network)) { - if (!wallet.provider) { - throw new Error('Provider is required for gas estimation'); - } - const gasFees = await getGasFees({ provider: wallet.provider, ...rest }); - transaction = await sdk(contractOptions, wallet, sdkParams, { - chainId: networkId as unknown as CHAIN_ID, - maxFeePerGas: gasFees.maxFeePerGas?.toString(), - maxPriorityFeePerGas: gasFees.maxPriorityFeePerGas?.toString(), - id: encryptionKey, - }); - } else { - transaction = await sdk(contractOptions, wallet, sdkParams, { - chainId: networkId as unknown as CHAIN_ID, - id: encryptionKey, - }); - } + const txOptions: Record<string, unknown> = { + chainId: networkId as unknown as CHAIN_ID, + id: encryptionKey, + }; + + if (canEstimateGasPrice(network)) { + if (!wallet.provider) { + throw new Error('Provider is required for gas estimation'); + } + const gasFees = await getGasFees({ provider: wallet.provider, ...rest }); + txOptions.maxFeePerGas = gasFees.maxFeePerGas?.toString(); + txOptions.maxPriorityFeePerGas = gasFees.maxPriorityFeePerGas?.toString(); + } + + const transaction = await sdk(contractOptions, wallet, sdkParams, txOptions);🤖 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 69 - 85, Refactor the branching around sdk in the gas-estimation flow to construct shared transaction options once, adding maxFeePerGas and maxPriorityFeePerGas only when canEstimateGasPrice(network) is true. Preserve the provider validation and gas-fee retrieval, then invoke sdk once with the shared contractOptions, wallet, sdkParams, chainId, encryptionKey, and conditionally populated gas fields.
18-28: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffConsider typing the contracts and wallet parameters.
escrow,registry, andwalletareany. Every call site then addssdk: ... as any. Typing these against the ethers contract types returned byconnectToObligationEscrowandconnectToObligationRegistry, and against the signer type returned bygetWalletOrSigner, would catch method and parameter mistakes in thepopulatecallbacks at compile time.🤖 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 18 - 28, Replace the any types in PopulateFn and SdkCall with the ethers contract types returned by connectToObligationEscrow and connectToObligationRegistry, and use the signer type returned by getWalletOrSigner for wallet. Update related call sites to pass these typed values without sdk as any casts, while preserving the existing transaction behavior.
62-64: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid
process.exit(0)inside the shared runner.
process.exit(0)terminates the process immediately from a library-level helper. Pending stdout writes, and any cleanup in callers, can be skipped. It also makes the helper hard to test, because the test process would exit.
performDryRunWithConfirmationalready models cancellation as a normal return value (src/utils/cli-options.ts:539-588). Propagate cancellation instead of exiting, and let the command layer decide the exit code.♻️ Proposed refactor
- if (!shouldProceed) { - process.exit(0); - } + if (!shouldProceed) { + throw new TransactionCancelledError(); + }Define the error next to the runner and handle it in
runObligationEscrowCommand(or in each handler) so cancellation is not reported as a failure:export class TransactionCancelledError extends Error { constructor() { super('Transaction cancelled by user.'); this.name = 'TransactionCancelledError'; } }🤖 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 62 - 64, Replace process.exit(0) in the shared runner with propagation of cancellation, using a TransactionCancelledError defined alongside the runner or an equivalent normal cancellation result. Update runObligationEscrowCommand or the relevant command handlers to catch this cancellation and return a successful exit outcome without reporting it as a failure, while preserving performDryRunWithConfirmation’s cancellation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@package.json`:
- Line 38: Update the `@trustvc/trustvc` dependency in package.json and
package-lock.json so the stable main release uses a stable TrustVC version; keep
the beta version only on the beta release line.
In `@README.md`:
- Around line 1737-1741: Update the BoE guide’s “Install the CLI” examples to
use the beta package or an explicit BoE-compatible version instead of
unversioned `@trustvc/trustvc-cli` installs. Keep both the global npm and npx
usage examples, ensuring neither resolves the stable latest release.
In `@src/commands/helpers.ts`:
- Around line 16-22: Update the v5Contracts destructuring in the helpers module
to remove the unavailable ObligationEscrow__factory and use a correctly
generated or imported factory from the compiled obligation contract artifacts
wherever its ABI is needed. Keep the other v5 contract factories unchanged and
ensure the later ABI access resolves at connection time.
In `@src/commands/obligation-escrow/transfer-owner-holder.ts`:
- Around line 28-53: Wrap the body of transferOwnersHandler in try/catch and
report failures with error(getErrorMessage(e)), importing error from signale and
reusing the existing error-message utility. Apply the equivalent try/catch and
error reporting to rejectTransferOwnersHandler in
src/commands/obligation-escrow/reject-transfer-owner-holder.ts at lines 21-38;
update transferOwnersHandler in
src/commands/obligation-escrow/transfer-owner-holder.ts at lines 28-53.
In `@src/commands/obligation-registry/deploy.ts`:
- Around line 163-165: Ensure failed obligation-registry commands return
non-zero exit status: in src/commands/obligation-registry/deploy.ts lines
163-165, rethrow after logging in deployObligationRegistryContract and have the
handler at line 30 set a non-zero exit code; apply the same rethrow change in
mintObligationToken at src/commands/obligation-registry/mint.ts lines 94-96 and
set a non-zero exit code in its handler catch at line 34.
---
Outside diff comments:
In `@src/commands/verify.ts`:
- Around line 200-229: Restructure verifyOpenAttestationDocument so chain
resolution failures, including a missing or unsupported chainId, invoke
resolveFallbackProvider(options.network) exactly once before the no-provider
fallback. Avoid entering getSupportedNetworkNameFromId with a falsy chainId when
no fallback provider is available, while preserving direct verification for
non-network documents and provider-based verification for supported chains. Add
coverage in verify.test.ts for an unsupported chainId rescued by the --network
override.
---
Nitpick comments:
In `@src/commands/helpers.ts`:
- Around line 467-487: Update connectToObligationEscrow to obtain the registry
by calling the existing connectToObligationRegistry helper, removing the
duplicated ethers.Contract construction and connection log while preserving the
subsequent ownerOf lookup and validation flow.
- Around line 436-449: Update connectToObligationRegistry and
connectToObligationEscrow to declare the same explicit ethers.Contract return
type used by connectToTitleEscrow, preserving typed access for callers such as
registry.mint.populateTransaction. Remove the unreachable !registry check and
its associated error handling from both helpers while retaining their existing
connection success and exception behavior.
In `@src/commands/obligation-escrow/accept.ts`:
- Around line 22-42: Extract the repeated transaction flow from acceptHandler
into a shared executeObligationEscrowAction helper that handles start logging,
runObligationEscrowTx, transaction pricing, success logging, and the Etherscan
URL, while properly typing sdk instead of using as any; remove acceptHandler’s
local try/catch and delegate with its accept-specific values. Apply the same
delegation in src/commands/obligation-escrow/discharge.ts lines 22-39, reject.ts
lines 21-37, reject-return-to-issuer.ts lines 21-39, reject-transfer-owner.ts
lines 21-38, and return-to-issuer.ts lines 21-38, preserving each command’s
messages and populate/sdk/sdkParams configuration.
- Around line 22-42: Extract the repeated obligation escrow transaction flow
from acceptHandler and the related handlers into a shared helper, including
status messages, displayTransactionPrice, transaction-link output, and error
handling. Update acceptHandler, discharge, reject, reject-return-to-issuer,
reject-transfer-owner, and return-to-issuer to use the helper, preserving each
command’s transaction population and success wording while centralizing the SDK
invocation and avoiding repeated as any casts.
In `@src/commands/obligation-escrow/runTx.ts`:
- Around line 69-85: Refactor the branching around sdk in the gas-estimation
flow to construct shared transaction options once, adding maxFeePerGas and
maxPriorityFeePerGas only when canEstimateGasPrice(network) is true. Preserve
the provider validation and gas-fee retrieval, then invoke sdk once with the
shared contractOptions, wallet, sdkParams, chainId, encryptionKey, and
conditionally populated gas fields.
- Around line 18-28: Replace the any types in PopulateFn and SdkCall with the
ethers contract types returned by connectToObligationEscrow and
connectToObligationRegistry, and use the signer type returned by
getWalletOrSigner for wallet. Update related call sites to pass these typed
values without sdk as any casts, while preserving the existing transaction
behavior.
- Around line 62-64: Replace process.exit(0) in the shared runner with
propagation of cancellation, using a TransactionCancelledError defined alongside
the runner or an equivalent normal cancellation result. Update
runObligationEscrowCommand or the relevant command handlers to catch this
cancellation and return a successful exit outcome without reporting it as a
failure, while preserving performDryRunWithConfirmation’s cancellation behavior.
In `@src/commands/obligation-escrow/shared.ts`:
- Around line 33-42: Update the credential-selection logic in the shared command
result construction to use one conditional spread for encryptedWalletPath,
keyFile, and key instead of three repeated if blocks. Preserve the existing
precedence order and BaseObligationEscrowCommand cast while retaining baseResult
when no credential is provided.
In `@src/commands/obligation-escrow/transfer-holder.ts`:
- Line 23: Update the promptAddress call assigning newHolder to remove the
redundant second “new holder” description, allowing the prompt to display only
the role-based message or a distinct description.
In `@src/commands/obligation-registry/deploy.ts`:
- Around line 111-160: Reuse the existing chainId variable when constructing the
deployObligationRegistry options instead of calling getSupportedNetwork(network)
again. Remove the now-unused supportedNetwork import while preserving the
existing chain ID logging and deployment behavior.
In `@tests/commands/obligation-escrow/accept.test.ts`:
- Around line 32-38: Remove the unused SDK mock blocks from
tests/commands/obligation-escrow/accept.test.ts (32-38), discharge.test.ts
(35-41), endorse-transfer-owner.test.ts (35-41), reject.test.ts (32-38),
reject-transfer-owner.test.ts (35-41), and return-to-issuer.test.ts (32-38),
including their vi.importActual calls and operation-specific mock functions;
retain the existing runTx, signale, and utility mocks.
- Around line 74-84: Strengthen the handler assertions so they verify the SDK
callback and required parameter payload, not merely invocation. In
tests/commands/obligation-escrow/accept.test.ts lines 74-84, assert
runObligationEscrowTx was called with an object containing any function as sdk
and sdkParams containing remarks: undefined; in
tests/commands/obligation-escrow/return-to-issuer.test.ts lines 57-67, replace
the call-only assertion with an equivalent toHaveBeenCalledWith objectContaining
assertion covering the returnToIssuerObligationRegistry sdkParams payload.
In `@tests/commands/obligation-registry/deploy.test.ts`:
- Around line 94-125: Add a test case in deployObligationRegistryContract
coverage that provides escrowFactoryAddress, invokes the mocked
performDryRunWithConfirmation getTransactionCallback, and exercises the
ethers.ContractFactory dry-run branch. Assert deployObligationRegistry receives
the expected fourth-argument options, including chainId and
escrowFactoryAddress, while preserving the existing deployment assertions.
In `@tests/commands/obligation-registry/mint.test.ts`:
- Around line 43-53: Update the `@trustvc/trustvc` mock in the test setup to
retain only the mintObligationRegistry mock; remove the unreachable
isObligationRecord, getObligationRegistryAddress, getTokenId, and getChainId
stubs while preserving the actual module imports.
- Around line 86-87: Update the dry-run mock in the mint command tests so
performDryRunWithConfirmation invokes the provided getTransactionCallback,
allowing the mocked connectToObligationRegistry, validateAndEncryptRemark, and
mint.populateTransaction paths to execute. Add a second test case with
canEstimateGasPrice returning true and assert that the calculated maxFeePerGas
is passed to mintObligationRegistry.
In `@tests/commands/verify.obligation.sepolia.test.ts`:
- Around line 78-92: Update the SEPOLIA_RPC override test around
getSupportedNetwork(...).provider() to wrap the environment-variable setup and
provider assertions in try/finally, deleting process.env.SEPOLIA_RPC in the
finally block so cleanup always occurs when provider creation throws.
In `@tests/fixtures/obligation/w3c-obligation-record-sepolia.json`:
- Around line 47-50: Update tokenNetwork.chainId in the Sepolia obligation
fixture to use the same string type as the sibling obligation fixture,
preserving the real-world credential schema consistently across fixtures.
🪄 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: f503d95d-9af6-4251-a533-f9ade1b3d2a8
⛔ 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.gitignore.releaserc.jsonREADME.mdpackage.jsonsrc/commands/helpers.tssrc/commands/obligation-escrow/accept-return-to-issuer.tssrc/commands/obligation-escrow/accept.tssrc/commands/obligation-escrow/discharge.tssrc/commands/obligation-escrow/endorse-transfer-owner.tssrc/commands/obligation-escrow/index.tssrc/commands/obligation-escrow/nominate-transfer-owner.tssrc/commands/obligation-escrow/reject-return-to-issuer.tssrc/commands/obligation-escrow/reject-transfer-holder.tssrc/commands/obligation-escrow/reject-transfer-owner-holder.tssrc/commands/obligation-escrow/reject-transfer-owner.tssrc/commands/obligation-escrow/reject.tssrc/commands/obligation-escrow/return-to-issuer.tssrc/commands/obligation-escrow/runTx.tssrc/commands/obligation-escrow/shared.tssrc/commands/obligation-escrow/status.tssrc/commands/obligation-escrow/transfer-holder.tssrc/commands/obligation-escrow/transfer-owner-holder.tssrc/commands/obligation-registry/deploy.tssrc/commands/obligation-registry/index.tssrc/commands/obligation-registry/mint.tssrc/commands/verify.tssrc/types.tssrc/utils/index.tssrc/utils/obligation-document.tstests/commands/obligation-escrow/accept-return-to-issuer.test.tstests/commands/obligation-escrow/accept.test.tstests/commands/obligation-escrow/discharge.test.tstests/commands/obligation-escrow/endorse-transfer-owner.test.tstests/commands/obligation-escrow/nominate-transfer-owner.test.tstests/commands/obligation-escrow/reject-return-to-issuer.test.tstests/commands/obligation-escrow/reject-transfer-holder.test.tstests/commands/obligation-escrow/reject-transfer-owner-holder.test.tstests/commands/obligation-escrow/reject-transfer-owner.test.tstests/commands/obligation-escrow/reject.test.tstests/commands/obligation-escrow/return-to-issuer.test.tstests/commands/obligation-escrow/status.test.tstests/commands/obligation-escrow/transfer-holder.test.tstests/commands/obligation-escrow/transfer-owner-holder.test.tstests/commands/obligation-registry/deploy.test.tstests/commands/obligation-registry/mint.test.tstests/commands/verify.obligation.sepolia.test.tstests/commands/verify.obligation.test.tstests/commands/verify.test.tstests/fixtures/obligation/w3c-obligation-record-sepolia.jsontests/fixtures/obligation/w3c-obligation-record.json
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
README.md (1)
345-347: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winFix the Obligation / BoE command links.
These entries link to
#obligation-escrow-transfer-holder, so readers reach the wrong command section. Link each command to its matching heading, or add a dedicated aggregate heading for the wildcard reject entry.Also applies to: 351-351
🤖 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 345 - 347, Update the command links in the README command table for obligation-escrow nominate-transfer-owner, endorse-transfer-owner, and transfer-owner-holder so each points to its matching command heading instead of the shared obligation-escrow-transfer-holder anchor; ensure any wildcard reject entry also targets its corresponding heading or a dedicated aggregate heading.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@README.md`:
- Line 1737: Update the installation examples in the README around the CLI
installation entry to remove nested backticks, using separate code spans for
each complete npm and npx command while preserving the `@beta` tag.
---
Outside diff comments:
In `@README.md`:
- Around line 345-347: Update the command links in the README command table for
obligation-escrow nominate-transfer-owner, endorse-transfer-owner, and
transfer-owner-holder so each points to its matching command heading instead of
the shared obligation-escrow-transfer-holder anchor; ensure any wildcard reject
entry also targets its corresponding heading or a dedicated aggregate heading.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
- Introduced `toSdkSigner` utility to standardize signer handling across obligation registry and escrow commands. - Updated various command files to utilize `toSdkSigner`, enhancing consistency in signer usage. - Cleaned up unnecessary code and improved type definitions in utility files for better clarity and maintainability. - Enhanced test cases to ensure proper mocking and input handling in obligation escrow commands.
- Implemented `toSdkSigner` utility across obligation registry and escrow commands to standardize signer usage. - Updated command implementations to utilize the new utility, enhancing consistency and maintainability. - Cleaned up type definitions in utility files for improved clarity. - Adjusted test cases to ensure proper mocking and input handling in obligation escrow commands.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/utils/wallet.ts (1)
39-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winKeep the SDK escape hatch private.
SdkSigneris exported asany. This lets callers bypass type checking outsidetoSdkSigner. If the TrustVC SDK exposes a signer type, use it here and keep the cast insidetoSdkSigner. Otherwise, keep this escape-hatch type local and narrow it to this boundary. Verify the SDK declaration before changing the alias.🤖 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/wallet.ts` around lines 39 - 42, The exported SdkSigner alias in the wallet signer boundary exposes an any-based escape hatch to callers. Verify whether the TrustVC SDK provides a signer type and use that type if available; otherwise make the escape-hatch type private and narrow it to toSdkSigner, keeping any casting contained within that function rather than exporting it.
🤖 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.
Nitpick comments:
In `@src/utils/wallet.ts`:
- Around line 39-42: The exported SdkSigner alias in the wallet signer boundary
exposes an any-based escape hatch to callers. Verify whether the TrustVC SDK
provides a signer type and use that type if available; otherwise make the
escape-hatch type private and narrow it to toSdkSigner, keeping any casting
contained within that function rather than exporting it.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cd509d8d-ddcb-4d15-8b2e-dcd5df04f05b
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (19)
src/commands/obligation-escrow/status.tssrc/commands/obligation-registry/deploy.tssrc/commands/obligation-registry/mint.tssrc/utils/cli-options.tssrc/utils/wallet.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.ts
🚧 Files skipped from review as they are similar to previous changes (17)
- tests/commands/obligation-escrow/transfer-holder.test.ts
- tests/commands/obligation-escrow/reject-transfer-owner-holder.test.ts
- tests/commands/obligation-escrow/reject-return-to-issuer.test.ts
- tests/commands/obligation-escrow/reject-transfer-owner.test.ts
- tests/commands/obligation-escrow/nominate-transfer-owner.test.ts
- src/commands/obligation-escrow/status.ts
- tests/commands/obligation-escrow/transfer-owner-holder.test.ts
- tests/commands/obligation-escrow/accept-return-to-issuer.test.ts
- tests/commands/obligation-escrow/status.test.ts
- tests/commands/obligation-escrow/discharge.test.ts
- tests/commands/obligation-escrow/reject-transfer-holder.test.ts
- tests/commands/obligation-escrow/return-to-issuer.test.ts
- tests/commands/obligation-escrow/endorse-transfer-owner.test.ts
- src/commands/obligation-registry/mint.ts
- tests/commands/obligation-escrow/accept.test.ts
- src/commands/obligation-registry/deploy.ts
- tests/commands/obligation-escrow/reject.test.ts
Contributor
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (2)
README.md (1)
1737-1737: 📐 Maintainability & Code Quality | 🟡 MinorFix the malformed Markdown in the beta install step.
The nested code spans trigger MD038 and do not render as two valid copyable commands. Use separate code spans around each complete command.
Proposed fix
-1. **Install the CLI** — `npm install -g `@trustvc/trustvc-cli`@beta` or `npx `@trustvc/trustvc-cli`@beta <command>` +1. **Install the CLI** — `npm install -g `@trustvc/trustvc-cli`@beta` or `npx `@trustvc/trustvc-cli`@beta <command>`🤖 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 1737, Fix the beta CLI installation step in the README by removing the nested Markdown code spans and wrapping each complete install command separately, so both commands render correctly and remain copyable.Source: Linters/SAST tools
package.json (1)
38-38: 🗄️ Data Integrity & Integration | 🟠 MajorKeep the stable release channel on a stable TrustVC dependency.
.releaserc.jsonstill declares bothmainandbeta, so both channels consume this manifest. With@trustvc/trustvcpinned to2.16.0-beta.1, amainrelease publishes a stable CLI that depends on a beta SDK.Keep a stable SDK version on
main, or preventmainfrom publishing until this dependency is promoted. Updatepackage-lock.jsonwith the selected manifest.#!/usr/bin/env bash set -euo pipefail printf '%s\n' '--- package.json ---' jq -r '.dependencies["`@trustvc/trustvc`"]' package.json printf '%s\n' '--- package-lock.json ---' jq -r '.packages[""].dependencies["`@trustvc/trustvc`"] // .dependencies["`@trustvc/trustvc`"]' package-lock.json printf '%s\n' '--- release branches ---' jq -c '.branches' .releaserc.json printf '%s\n' '--- release workflow ---' sed -n '1,80p' .github/workflows/release.yml🤖 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 `@package.json` at line 38, Keep the stable main release channel from consuming the beta TrustVC SDK by replacing the `@trustvc/trustvc` dependency in package.json with the selected stable version, then regenerate package-lock.json so its corresponding dependency entries match the manifest. Preserve beta release support without allowing main to publish with a beta SDK.
🧹 Nitpick comments (9)
src/utils/wallet.ts (2)
30-31: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
CliSignercollapses toSigner.
Wallet,HDNodeWallet, andConnectedSignerall extend or intersectSigner, so the union is equivalent toSigneralone. The alias adds no narrowing. Keep the alias name for readability, but drop the redundant members.♻️ Proposed change
/** Wallet/signer shapes produced by this CLI (`ethers@6`). */ -export type CliSigner = Signer | Wallet | ConnectedSigner | HDNodeWallet; +export type CliSigner = Signer;🤖 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/wallet.ts` around lines 30 - 31, Update the CliSigner type alias to retain only Signer, removing the redundant Wallet, ConnectedSigner, and HDNodeWallet members while preserving the alias name and its existing readability comment.
33-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse the SDK’s signer type instead of
SdkSigner = any.
toSdkSignererases signer type checking at every TrustVC call site, including obligation-registry mint/deploy and escrow status/registration calls. Import theethersV6Signertype that@trustvc/trustvcuses and typeSdkSigneras that, so the cast remains only inside the SDK boundary instead of allowing arbitrary values everywhere.🤖 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/wallet.ts` around lines 33 - 42, Replace the `SdkSigner = any` alias with the `Signer` type exported by the SDK’s `ethersV6` namespace, and update `toSdkSigner` to return that type. Keep the compatibility cast confined to `toSdkSigner` so TrustVC call sites, including obligation-registry and escrow operations, retain signer type checking.src/commands/obligation-registry/deploy.ts (1)
112-112: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueResolve the network once.
Line 112 reads
supportedNetwork[network as NetworkCmdName].networkIdand Line 151 readsgetSupportedNetwork(network).networkIdfor the same value. Use one accessor for both.supportedNetwork[...]also throws aTypeErrorifnetworkis not a known key, whilegetSupportedNetworkcentralizes that lookup.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/obligation-registry/deploy.ts` at line 112, Update the network ID lookup in the deployment flow to use getSupportedNetwork(network).networkId consistently, including the assignment currently indexing supportedNetwork directly. Reuse this accessor for both lookups so unknown networks follow the centralized validation behavior.src/commands/helpers.ts (1)
467-487: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
connectToObligationRegistryinsideconnectToObligationEscrow.Lines 473-474 duplicate the registry connection that
connectToObligationRegistryalready performs. Call the existing helper to keep one construction path and consistent logging.♻️ Proposed refactor
try { - signale.info(`Connecting to obligation registry at: ${address}`); - const registry = new ethers.Contract(address, TrustVCToken__factory.abi, wallet as any); + const registry = await connectToObligationRegistry({ address, wallet }); signale.info(`Fetching obligation escrow address for tokenId: ${tokenId}`);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/commands/helpers.ts` around lines 467 - 487, Update connectToObligationEscrow to obtain the registry by calling the existing connectToObligationRegistry helper with the provided address and wallet, removing its duplicated ethers.Contract construction and connection log while preserving the subsequent ownerOf lookup and validation flow.src/utils/obligation-document.ts (1)
13-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueType the
documentparameter instead ofany.
extractObligationDocumentInfoaccepts and returnsany. The TrustVC helpersisObligationRecord,getObligationRegistryAddress, andgetTokenIddeclare their own input types. Use those types sodocument.idat Line 58 is checked at compile time.🤖 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 13 - 21, Update extractObligationDocumentInfo to replace the document any type with the TrustVC helper input type used by isObligationRecord, getObligationRegistryAddress, and getTokenId, and apply the corresponding concrete type to the returned document field. Ensure document.id is compile-time checked while preserving the existing helper calls and behavior.tests/commands/obligation-registry/deploy.test.ts (1)
94-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the dry-run branch and the failure path.
Every test sets
confirmtofalse, soescrowFactoryAddressstaysundefinedand the block atsrc/commands/obligation-registry/deploy.tsLines 117-133 never runs. That block builds anethers.ContractFactoryfromv5Contracts.TrustVCToken__factoryand is the most failure-prone part of the command. Add one test that suppliesescrowFactoryAddressand assertsperformDryRunWithConfirmationis called. Add one test that rejectsdeployObligationRegistryand asserts the error is logged.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/commands/obligation-registry/deploy.test.ts` around lines 94 - 125, Extend the deployObligationRegistryContract tests to cover the escrowFactoryAddress branch by supplying that option and asserting performDryRunWithConfirmation is called, and add a failure test where deployObligationRegistry rejects and the error is logged. Keep the existing successful deployment assertions unchanged.tests/commands/obligation-registry/mint.test.ts (1)
86-93: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCover the gas-estimation branch and add direct tests for
extractObligationDocumentInfo.
canEstimateGasPriceis mocked tofalse, sosrc/commands/obligation-registry/mint.tsLines 132-146 never run. That branch passesmaxFeePerGasandmaxPriorityFeePerGasto the SDK. Add a test withcanEstimateGasPricereturningtrueandgetGasFeesmocked.
extractObligationDocumentInfois also mocked here, so its validation branches insrc/utils/obligation-document.tshave no coverage. Add a separate unit test file for the non-obligation-record rejection, the unsupported chain ID rejection, and the missingdocument.idrejection.🤖 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 86 - 93, Expand the tests around the mint command to cover the gas-estimation path by overriding canEstimateGasPrice to return true, mocking getGasFees, and asserting the SDK receives maxFeePerGas and maxPriorityFeePerGas. Add a separate unit-test file for extractObligationDocumentInfo covering non-obligation records, unsupported chain IDs, and missing document.id validation failures.tests/commands/obligation-escrow/status.test.ts (1)
42-51: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the no-wallet status contract.
The test preserves the actual
getWalletOrSignerexport. It does not assert that the provider-only status path avoids wallet access. MockgetWalletOrSignerand assert that it is not called. This prevents a read-only status request from gaining a signing-key dependency.Proposed test change
vi.mock('../../../src/utils', async (importOriginal) => { const actual = await importOriginal<typeof import('../../../src/utils')>(); return { ...actual, + getWalletOrSigner: vi.fn(), getSupportedNetwork: vi.fn().mockReturnValue({ provider: () => ({ mock: 'provider' }), networkId: 80002, }), }; }); it('reads status via network provider without a wallet', async () => { const trustvc = await import('`@trustvc/trustvc`'); + const utils = await import('../../../src/utils'); await statusHandler({ network: NetworkCmdName.Amoy, obligationRegistryAddress: '0xRegistry', tokenId: '0x1', }); + expect(utils.getWalletOrSigner).not.toHaveBeenCalled();Also applies to: 59-73
🤖 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/status.test.ts` around lines 42 - 51, Update the status tests using the utils mock to replace getWalletOrSigner with a spy/mock, then assert it is not called when exercising the provider-only no-wallet status path. Preserve the existing getSupportedNetwork and getErrorMessage mocks, and apply the assertion to both relevant test cases.tests/commands/obligation-escrow/accept.test.ts (1)
52-54: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftTest the transaction configuration at the command boundary.
runObligationEscrowTxconnects contracts, encrypts remarks, builds dry-run transactions, and submits SDK calls. Replacing it with a resolved mock lets these tests pass when a handler selects the wrong SDK function, builds the wrongpopulatecallback, or drops a transaction field.Use the real runner with mocked contracts, as the returned-title tests do. Otherwise, invoke and assert the exact runner options with concrete input values.
tests/commands/obligation-escrow/accept.test.ts#L52-L54: Assert the command-specificsdk,populate, andsdkParams.tests/commands/obligation-escrow/discharge.test.ts#L55-L57: Pass a non-emptyremarkand assert its exactsdkParams.remarksvalue.tests/commands/obligation-escrow/reject-transfer-owner-holder.test.ts#L55-L57: Pass a non-emptyremarkand assert its exact transaction mapping.tests/commands/obligation-escrow/return-to-issuer.test.ts#L40-L42: Assert the return-specific SDK call and transaction population.tests/commands/obligation-escrow/transfer-holder.test.ts#L54-L56: Assert holder and remark mapping through transaction population and SDK submission.tests/commands/obligation-escrow/transfer-owner-holder.test.ts#L54-L56: Assert both addresses and the remark through transaction population and SDK submission.🤖 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/accept.test.ts` around lines 52 - 54, Replace the resolved runObligationEscrowTx mock with the real runner and mocked contracts, or invoke it while asserting exact transaction options. In tests/commands/obligation-escrow/accept.test.ts:52-54, verify command-specific sdk, populate, and sdkParams; in tests/commands/obligation-escrow/discharge.test.ts:55-57 and tests/commands/obligation-escrow/reject-transfer-owner-holder.test.ts:55-57, use non-empty remarks and assert their exact mapping; in tests/commands/obligation-escrow/return-to-issuer.test.ts:40-42, verify return-specific SDK and population; in tests/commands/obligation-escrow/transfer-holder.test.ts:54-56 and tests/commands/obligation-escrow/transfer-owner-holder.test.ts:54-56, assert holder/address and remark mapping through population and SDK submission.
🤖 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 217-246: Update the BoE Quick Start installation instructions near
the “Obligation Registry & Escrow (BoE)” examples to install the beta CLI
explicitly with the `@beta` dist-tag, and ensure the documentation states that the
beta package must be published before using these commands. Keep the general
unversioned installation guidance unchanged.
In `@src/commands/obligation-escrow/accept-return-to-issuer.ts`:
- Around line 41-43: Ensure every obligation escrow command catch block sets a
nonzero exit status after logging the error: update the catches in
accept-return-to-issuer.ts (41-43), endorse-transfer-owner.ts (45-47),
nominate-transfer-owner.ts (45-47), reject-transfer-holder.ts (40-42),
transfer-holder.ts (48-50), and the catch block in shared.ts. Preserve the
existing error logging and set process.exitCode to 1 in each location.
In `@src/commands/obligation-registry/deploy.ts`:
- Around line 72-79: Normalize the result of promptAddress in the reuseFactory
branch so an empty string becomes undefined before assigning
escrowFactoryAddress. Preserve valid addresses unchanged, ensuring
deployObligationRegistry receives the option as omitted rather than an empty
string.
In `@src/commands/verify.ts`:
- Around line 200-202: Update verifyOpenAttestationDocument so every
OpenAttestation chain-resolution failure, including
getSupportedNetworkNameFromId() exceptions and a missing result from
network.provider(), resolves the provider using the supplied --network override
before verifying without a provider. Preserve the existing no-provider fallback
only when no override can be resolved, and add coverage for a non-empty
unsupported chain ID with an explicit network override.
In `@tests/commands/obligation-escrow/reject-transfer-owner.test.ts`:
- Around line 43-53: Export a vi.fn() stub for runObligationEscrowCommand in
both shared-module mocks:
tests/commands/obligation-escrow/reject-transfer-owner.test.ts lines 43-53 and
tests/commands/obligation-escrow/reject.test.ts lines 40-50. Keep the existing
promptBaseObligationEscrowInputs mock and add the missing named export so each
command module can be evaluated.
In `@tests/commands/verify.test.ts`:
- Around line 244-245: Restore each test’s original process-global state rather
than hardcoding cleanup values: in tests/commands/verify.test.ts lines 244-245,
capture the original process.stdin.isTTY value or property descriptor before
mutation and restore it in afterEach; in
tests/commands/verify.obligation.sepolia.test.ts lines 78-86, capture the
existing SEPOLIA_RPC value and restore it in a finally block that covers import
and provider-creation failures.
---
Duplicate comments:
In `@package.json`:
- Line 38: Keep the stable main release channel from consuming the beta TrustVC
SDK by replacing the `@trustvc/trustvc` dependency in package.json with the
selected stable version, then regenerate package-lock.json so its corresponding
dependency entries match the manifest. Preserve beta release support without
allowing main to publish with a beta SDK.
In `@README.md`:
- Line 1737: Fix the beta CLI installation step in the README by removing the
nested Markdown code spans and wrapping each complete install command
separately, so both commands render correctly and remain copyable.
---
Nitpick comments:
In `@src/commands/helpers.ts`:
- Around line 467-487: Update connectToObligationEscrow to obtain the registry
by calling the existing connectToObligationRegistry helper with the provided
address and wallet, removing its duplicated ethers.Contract construction and
connection log while preserving the subsequent ownerOf lookup and validation
flow.
In `@src/commands/obligation-registry/deploy.ts`:
- Line 112: Update the network ID lookup in the deployment flow to use
getSupportedNetwork(network).networkId consistently, including the assignment
currently indexing supportedNetwork directly. Reuse this accessor for both
lookups so unknown networks follow the centralized validation behavior.
In `@src/utils/obligation-document.ts`:
- Around line 13-21: Update extractObligationDocumentInfo to replace the
document any type with the TrustVC helper input type used by isObligationRecord,
getObligationRegistryAddress, and getTokenId, and apply the corresponding
concrete type to the returned document field. Ensure document.id is compile-time
checked while preserving the existing helper calls and behavior.
In `@src/utils/wallet.ts`:
- Around line 30-31: Update the CliSigner type alias to retain only Signer,
removing the redundant Wallet, ConnectedSigner, and HDNodeWallet members while
preserving the alias name and its existing readability comment.
- Around line 33-42: Replace the `SdkSigner = any` alias with the `Signer` type
exported by the SDK’s `ethersV6` namespace, and update `toSdkSigner` to return
that type. Keep the compatibility cast confined to `toSdkSigner` so TrustVC call
sites, including obligation-registry and escrow operations, retain signer type
checking.
In `@tests/commands/obligation-escrow/accept.test.ts`:
- Around line 52-54: Replace the resolved runObligationEscrowTx mock with the
real runner and mocked contracts, or invoke it while asserting exact transaction
options. In tests/commands/obligation-escrow/accept.test.ts:52-54, verify
command-specific sdk, populate, and sdkParams; in
tests/commands/obligation-escrow/discharge.test.ts:55-57 and
tests/commands/obligation-escrow/reject-transfer-owner-holder.test.ts:55-57, use
non-empty remarks and assert their exact mapping; in
tests/commands/obligation-escrow/return-to-issuer.test.ts:40-42, verify
return-specific SDK and population; in
tests/commands/obligation-escrow/transfer-holder.test.ts:54-56 and
tests/commands/obligation-escrow/transfer-owner-holder.test.ts:54-56, assert
holder/address and remark mapping through population and SDK submission.
In `@tests/commands/obligation-escrow/status.test.ts`:
- Around line 42-51: Update the status tests using the utils mock to replace
getWalletOrSigner with a spy/mock, then assert it is not called when exercising
the provider-only no-wallet status path. Preserve the existing
getSupportedNetwork and getErrorMessage mocks, and apply the assertion to both
relevant test cases.
In `@tests/commands/obligation-registry/deploy.test.ts`:
- Around line 94-125: Extend the deployObligationRegistryContract tests to cover
the escrowFactoryAddress branch by supplying that option and asserting
performDryRunWithConfirmation is called, and add a failure test where
deployObligationRegistry rejects and the error is logged. Keep the existing
successful deployment assertions unchanged.
In `@tests/commands/obligation-registry/mint.test.ts`:
- Around line 86-93: Expand the tests around the mint command to cover the
gas-estimation path by overriding canEstimateGasPrice to return true, mocking
getGasFees, and asserting the SDK receives maxFeePerGas and
maxPriorityFeePerGas. Add a separate unit-test file for
extractObligationDocumentInfo covering non-obligation records, unsupported chain
IDs, and missing document.id validation failures.
🪄 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: 0d914582-6711-489c-9c5a-46f60f3f20e5
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (54)
.github/workflows/ci.yml.github/workflows/release.yml.gitignore.releaserc.jsonREADME.mdpackage.jsonsrc/commands/helpers.tssrc/commands/obligation-escrow/accept-return-to-issuer.tssrc/commands/obligation-escrow/accept.tssrc/commands/obligation-escrow/discharge.tssrc/commands/obligation-escrow/endorse-transfer-owner.tssrc/commands/obligation-escrow/index.tssrc/commands/obligation-escrow/nominate-transfer-owner.tssrc/commands/obligation-escrow/reject-return-to-issuer.tssrc/commands/obligation-escrow/reject-transfer-holder.tssrc/commands/obligation-escrow/reject-transfer-owner-holder.tssrc/commands/obligation-escrow/reject-transfer-owner.tssrc/commands/obligation-escrow/reject.tssrc/commands/obligation-escrow/return-to-issuer.tssrc/commands/obligation-escrow/runTx.tssrc/commands/obligation-escrow/shared.tssrc/commands/obligation-escrow/status.tssrc/commands/obligation-escrow/transfer-holder.tssrc/commands/obligation-escrow/transfer-owner-holder.tssrc/commands/obligation-registry/deploy.tssrc/commands/obligation-registry/index.tssrc/commands/obligation-registry/mint.tssrc/commands/verify.tssrc/types.tssrc/utils/cli-options.tssrc/utils/index.tssrc/utils/obligation-document.tssrc/utils/wallet.tstests/commands/obligation-escrow/accept-return-to-issuer.test.tstests/commands/obligation-escrow/accept.test.tstests/commands/obligation-escrow/discharge.test.tstests/commands/obligation-escrow/endorse-transfer-owner.test.tstests/commands/obligation-escrow/nominate-transfer-owner.test.tstests/commands/obligation-escrow/reject-return-to-issuer.test.tstests/commands/obligation-escrow/reject-transfer-holder.test.tstests/commands/obligation-escrow/reject-transfer-owner-holder.test.tstests/commands/obligation-escrow/reject-transfer-owner.test.tstests/commands/obligation-escrow/reject.test.tstests/commands/obligation-escrow/return-to-issuer.test.tstests/commands/obligation-escrow/status.test.tstests/commands/obligation-escrow/transfer-holder.test.tstests/commands/obligation-escrow/transfer-owner-holder.test.tstests/commands/obligation-registry/deploy.test.tstests/commands/obligation-registry/mint.test.tstests/commands/verify.obligation.sepolia.test.tstests/commands/verify.obligation.test.tstests/commands/verify.test.tstests/fixtures/obligation/w3c-obligation-record-sepolia.jsontests/fixtures/obligation/w3c-obligation-record.json
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
…ands - Added instructions for using BoE commands with the `@beta` dist-tag in the README. - Updated obligation escrow command handlers to ensure consistent exit codes on errors. - Improved error handling in `accept-return-to-issuer`, `endorse-transfer-owner`, `nominate-transfer-owner`, `reject-transfer-holder`, and `transfer-holder` commands. - Enhanced tests for the verify command to handle unsupported chain IDs and ensure proper provider usage.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Contributor
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
package.json (1)
38-38: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAlign the beta release channel with the package manifest and README.
The beta channel is separate from
main, but the shared package manifest and documentation assume the default release path. This can publish a stable CLI with a prerelease SDK and can direct users to a CLI that does not contain the documented BoE commands.
package.json#L38-L38: use a stable@trustvc/trustvcdependency formain, or use a branch-specific release manifest.README.md#L217-L246: publish the BoE Quick Start only after the default channel contains the documented commands.README.md#L1737-L1741: keep the unversioned installation examples only when they resolve the BoE-capable CLI.🤖 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 `@package.json` at line 38, Align the release manifest and documentation across all affected sites: in package.json lines 38-38, use the stable `@trustvc/trustvc` dependency for main or provide a branch-specific release manifest; in README.md lines 217-246, publish the BoE Quick Start only once the default channel includes the documented commands; and in README.md lines 1737-1741, retain unversioned installation examples only when they resolve to the BoE-capable CLI.
🧹 Nitpick comments (5)
tests/commands/obligation-registry/deploy.test.ts (1)
94-125: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a case that supplies
escrowFactoryAddress.This test omits
escrowFactoryAddress, sodeployObligationRegistryContracttakes the branch that skips the dry run. Theethers.ContractFactorypath at Lines 117-133 ofsrc/commands/obligation-registry/deploy.tsstays uncovered, including thev5Contracts.TrustVCToken__factory.abiand.bytecodeaccess. A second case with a factory address would catch a missing factory export before release.🤖 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, The deployObligationRegistryContract tests currently cover only the no-factory branch. Add a second test case that supplies escrowFactoryAddress, exercises the ethers.ContractFactory deployment path, and mocks or asserts the required v5Contracts.TrustVCToken__factory ABI and bytecode access while preserving the existing SDK deployment assertions.tests/commands/obligation-registry/mint.test.ts (1)
87-87: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider a case with
canEstimateGasPricereturning true.The mock forces
canEstimateGasPricetofalse, so the gas-estimation branch inmintToObligationRegistrynever runs. That branch readswallet.provider, callsgetGasFees, and passesmaxFeePerGasandmaxPriorityFeePerGastomintObligationRegistry. A second case would cover 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 `@tests/commands/obligation-registry/mint.test.ts` at line 87, Extend the mintToObligationRegistry tests with a case where canEstimateGasPrice returns true, using a wallet mock with provider support and stubbing getGasFees. Assert that mintObligationRegistry receives the returned maxFeePerGas and maxPriorityFeePerGas values, while preserving the existing false-path coverage.src/utils/wallet.ts (1)
30-42: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deriving
SdkSignerfrom the SDK signature instead ofany.
SdkSigner = anyremoves all checking at every SDK call site. A derived type keeps the intended cast narrow and still avoids the duplicate-ethers-package assignability error. Also note thatCliSignercollapses toSigner, becauseWallet,HDNodeWallet, andConnectedSignerall extendSigner. Keep the alias if it documents intent, but it adds no type constraint.♻️ Suggested typing
-// eslint-disable-next-line `@typescript-eslint/no-explicit-any` -export type SdkSigner = any; +import type { mintObligationRegistry } from '`@trustvc/trustvc`'; + +/** Signer parameter type expected by the TrustVC SDK entry points. */ +export type SdkSigner = Parameters<typeof mintObligationRegistry>[1]; -export const toSdkSigner = (signer: CliSigner | Signer): SdkSigner => signer; +export const toSdkSigner = (signer: CliSigner | Signer): SdkSigner => signer as SdkSigner;🤖 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/wallet.ts` around lines 30 - 42, Replace the unconstrained any alias in SdkSigner with a type derived from the relevant TrustVC SDK method signature, preserving the narrow cast needed at SDK boundaries while retaining compile-time checking at call sites. Update toSdkSigner to use that derived alias, and keep CliSigner only as documentation if desired since its concrete signer unions collapse to Signer.src/types.ts (1)
117-123: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMake obligation escrow command types include
keyFile.
promptBaseObligationEscrowInputsreturnskeyFile, butBaseObligationEscrowCommandonly composesPartial<WalletOption> | Partial<PrivateKeyOption>;Partial<PrivateKeyOption>allowskeyFilewhenkeyis absent, so this cast should not hide an excess property. Use the tighter mint-style type here to avoid passingkeyFilethroughgetWalletOrSignerwhen the command contract may be changed later.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/types.ts` around lines 117 - 123, Update BaseObligationEscrowCommand to use the same tighter wallet/signing option type as the mint command, explicitly including keyFile alongside the supported signer fields. Ensure promptBaseObligationEscrowInputs’ keyFile is part of the command contract and remains accepted by getWalletOrSigner without relying on a broad union or hidden excess-property cast.tests/commands/obligation-escrow/accept.test.ts (1)
76-86: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the complete handler-to-runner configuration.
Each listed test mocks
runObligationEscrowTx. The current assertions can pass when a handler selects the wrong SDK function or callback. Assert the exactsdkreference and complete command-specificsdkParams. Assertpopulatebehavior with contract stubs when it selects a contract method.
tests/commands/obligation-escrow/accept.test.ts#L76-L86: Assert the accept SDK function and all accept parameters.tests/commands/obligation-escrow/discharge.test.ts#L79-L99: Assert the discharge SDK function and complete no-remark configuration.tests/commands/obligation-escrow/reject-transfer-owner-holder.test.ts#L79-L93: Assert the rejection SDK function and full transfer-rejection parameters.tests/commands/obligation-escrow/return-to-issuer.test.ts#L59-L69: Assert the return SDK function and token parameters.tests/commands/obligation-escrow/transfer-holder.test.ts#L79-L96: Assert the holder-transfer SDK function and full holder parameters.tests/commands/obligation-escrow/transfer-owner-holder.test.ts#L83-L102: Assert the owner-and-holder transfer SDK function and both address parameters.🤖 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/accept.test.ts` around lines 76 - 86, Strengthen the handler-to-runner assertions: in tests/commands/obligation-escrow/accept.test.ts:76-86, assert the exact accept SDK function and complete sdkParams; in tests/commands/obligation-escrow/discharge.test.ts:79-99, assert the discharge SDK function and complete no-remark configuration; in tests/commands/obligation-escrow/reject-transfer-owner-holder.test.ts:79-93, assert the rejection SDK function and full transfer-rejection parameters; in tests/commands/obligation-escrow/return-to-issuer.test.ts:59-69, assert the return SDK function and token parameters; in tests/commands/obligation-escrow/transfer-holder.test.ts:79-96, assert the holder-transfer SDK function and full holder parameters; and in tests/commands/obligation-escrow/transfer-owner-holder.test.ts:83-102, assert the owner-and-holder transfer SDK function and both address parameters. Where the selected runner uses populate, stub the contract method and assert its behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/commands/obligation-escrow/accept.ts`:
- Around line 22-41: Remove the try/catch from acceptHandler so errors from
runObligationEscrowTx and subsequent transaction handling propagate to
runObligationEscrowCommand. Keep the existing success and transaction-display
behavior unchanged, allowing the shared wrapper to report failures and set
process.exitCode = 1.
In `@src/commands/obligation-escrow/status.ts`:
- Around line 56-69: Update statusHandler to construct a real read-only Ethers
runner with VoidSigner using ZeroAddress and the existing provider, then pass
that signer through toObligationRegistryStatus, isObligationRegistryRegistered,
and getObligationEscrowTerminationReason via toSdkSigner. Add an RPC-backed test
covering these statusHandler view calls.
In `@src/utils/obligation-document.ts`:
- Around line 51-57: Update the network resolution in the obligation-document
flow around SUPPORTED_CHAINS and the network variable so external chain names
are translated to the corresponding NetworkCmdName keys before use. Ensure
values passed to supportedNetwork[] and getSupportedNetwork() use command names
such as pol rather than aliases like matic, while preserving the
unsupported-chain validation.
---
Duplicate comments:
In `@package.json`:
- Line 38: Align the release manifest and documentation across all affected
sites: in package.json lines 38-38, use the stable `@trustvc/trustvc` dependency
for main or provide a branch-specific release manifest; in README.md lines
217-246, publish the BoE Quick Start only once the default channel includes the
documented commands; and in README.md lines 1737-1741, retain unversioned
installation examples only when they resolve to the BoE-capable CLI.
---
Nitpick comments:
In `@src/types.ts`:
- Around line 117-123: Update BaseObligationEscrowCommand to use the same
tighter wallet/signing option type as the mint command, explicitly including
keyFile alongside the supported signer fields. Ensure
promptBaseObligationEscrowInputs’ keyFile is part of the command contract and
remains accepted by getWalletOrSigner without relying on a broad union or hidden
excess-property cast.
In `@src/utils/wallet.ts`:
- Around line 30-42: Replace the unconstrained any alias in SdkSigner with a
type derived from the relevant TrustVC SDK method signature, preserving the
narrow cast needed at SDK boundaries while retaining compile-time checking at
call sites. Update toSdkSigner to use that derived alias, and keep CliSigner
only as documentation if desired since its concrete signer unions collapse to
Signer.
In `@tests/commands/obligation-escrow/accept.test.ts`:
- Around line 76-86: Strengthen the handler-to-runner assertions: in
tests/commands/obligation-escrow/accept.test.ts:76-86, assert the exact accept
SDK function and complete sdkParams; in
tests/commands/obligation-escrow/discharge.test.ts:79-99, assert the discharge
SDK function and complete no-remark configuration; in
tests/commands/obligation-escrow/reject-transfer-owner-holder.test.ts:79-93,
assert the rejection SDK function and full transfer-rejection parameters; in
tests/commands/obligation-escrow/return-to-issuer.test.ts:59-69, assert the
return SDK function and token parameters; in
tests/commands/obligation-escrow/transfer-holder.test.ts:79-96, assert the
holder-transfer SDK function and full holder parameters; and in
tests/commands/obligation-escrow/transfer-owner-holder.test.ts:83-102, assert
the owner-and-holder transfer SDK function and both address parameters. Where
the selected runner uses populate, stub the contract method and assert its
behavior.
In `@tests/commands/obligation-registry/deploy.test.ts`:
- Around line 94-125: The deployObligationRegistryContract tests currently cover
only the no-factory branch. Add a second test case that supplies
escrowFactoryAddress, exercises the ethers.ContractFactory deployment path, and
mocks or asserts the required v5Contracts.TrustVCToken__factory ABI and bytecode
access while preserving the existing SDK deployment assertions.
In `@tests/commands/obligation-registry/mint.test.ts`:
- Line 87: Extend the mintToObligationRegistry tests with a case where
canEstimateGasPrice returns true, using a wallet mock with provider support and
stubbing getGasFees. Assert that mintObligationRegistry receives the returned
maxFeePerGas and maxPriorityFeePerGas values, while preserving the existing
false-path coverage.
🪄 Autofix
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: a7e8f5c9-9690-431e-992e-d51c9d375699
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (54)
.github/workflows/ci.yml.github/workflows/release.yml.gitignore.releaserc.jsonREADME.mdpackage.jsonsrc/commands/helpers.tssrc/commands/obligation-escrow/accept-return-to-issuer.tssrc/commands/obligation-escrow/accept.tssrc/commands/obligation-escrow/discharge.tssrc/commands/obligation-escrow/endorse-transfer-owner.tssrc/commands/obligation-escrow/index.tssrc/commands/obligation-escrow/nominate-transfer-owner.tssrc/commands/obligation-escrow/reject-return-to-issuer.tssrc/commands/obligation-escrow/reject-transfer-holder.tssrc/commands/obligation-escrow/reject-transfer-owner-holder.tssrc/commands/obligation-escrow/reject-transfer-owner.tssrc/commands/obligation-escrow/reject.tssrc/commands/obligation-escrow/return-to-issuer.tssrc/commands/obligation-escrow/runTx.tssrc/commands/obligation-escrow/shared.tssrc/commands/obligation-escrow/status.tssrc/commands/obligation-escrow/transfer-holder.tssrc/commands/obligation-escrow/transfer-owner-holder.tssrc/commands/obligation-registry/deploy.tssrc/commands/obligation-registry/index.tssrc/commands/obligation-registry/mint.tssrc/commands/verify.tssrc/types.tssrc/utils/cli-options.tssrc/utils/index.tssrc/utils/obligation-document.tssrc/utils/wallet.tstests/commands/obligation-escrow/accept-return-to-issuer.test.tstests/commands/obligation-escrow/accept.test.tstests/commands/obligation-escrow/discharge.test.tstests/commands/obligation-escrow/endorse-transfer-owner.test.tstests/commands/obligation-escrow/nominate-transfer-owner.test.tstests/commands/obligation-escrow/reject-return-to-issuer.test.tstests/commands/obligation-escrow/reject-transfer-holder.test.tstests/commands/obligation-escrow/reject-transfer-owner-holder.test.tstests/commands/obligation-escrow/reject-transfer-owner.test.tstests/commands/obligation-escrow/reject.test.tstests/commands/obligation-escrow/return-to-issuer.test.tstests/commands/obligation-escrow/status.test.tstests/commands/obligation-escrow/transfer-holder.test.tstests/commands/obligation-escrow/transfer-owner-holder.test.tstests/commands/obligation-registry/deploy.test.tstests/commands/obligation-registry/mint.test.tstests/commands/verify.obligation.sepolia.test.tstests/commands/verify.obligation.test.tstests/commands/verify.test.tstests/fixtures/obligation/w3c-obligation-record-sepolia.jsontests/fixtures/obligation/w3c-obligation-record.json
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
- Replaced `Signer` import with `VoidSigner` and `ZeroAddress` in `status.ts` for improved clarity. - Enhanced `extractObligationDocumentInfo` function in `obligation-document.ts` by adding `getSupportedNetworkNameFromId` to map library chain names to NetworkCmdName keys.
rongquan1
approved these changes
Aug 5, 2026
tradetrustimda
pushed a commit
that referenced
this pull request
Aug 5, 2026
# [1.2.0-beta.1](v1.1.0...v1.2.0-beta.1) (2026-08-05) ### Features * boe and integration testing ([#31](#31)) ([feb0829](feb0829))
|
🎉 This PR is included in version 1.2.0-beta.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.2.0-beta.2 🎉 The release is available on: Your semantic-release bot 📦🚀 |
tradetrustimda
pushed a commit
that referenced
this pull request
Aug 12, 2026
# [1.3.0-beta.1](v1.2.0...v1.3.0-beta.1) (2026-08-12) ### Bug Fixes * update W3C obligation record with new issuer and restructure credentialSubject fields for improved clarity and accuracy ([0c0bfed](0c0bfed)) ### Features * add obligation escrow commands and types ([f7c0c78](f7c0c78)) * boe and integration testing ([#31](#31)) ([feb0829](feb0829))
|
🎉 This PR is included in version 1.3.0-beta.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Chores