feat: introduce Bill of Exchange (eBOE) status lifecycle#151
feat: introduce Bill of Exchange (eBOE) status lifecycle#151manishdex25 wants to merge 10 commits into
Conversation
|
Warning Review limit reached
Next review available in: 41 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughAdded Token Registry V5 Bill of Exchange status support with ChangesStatus lifecycle
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Document
participant StatusHelper
participant Provider
participant TitleEscrow
Document->>StatusHelper: call accept, reject, discharge, or getStatus
StatusHelper->>Provider: resolve TitleEscrow and detect V5
StatusHelper->>TitleEscrow: read status or perform static pre-check
StatusHelper->>TitleEscrow: submit lifecycle transaction
TitleEscrow-->>Document: return status or transaction result
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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 |
…ng runStatusAction
manishdex25
changed the title
manishdex25
changed the title
manishdex25
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (5)
src/status/types.ts (1)
1-3: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSimplify to a direct re-export.
SonarCloud flags this: the import-then-re-export can be collapsed into a single
export type ... fromstatement.🔧 Proposed fix
-import { ContractOptions, TransactionOptions } from '../token-registry-functions/types'; - -export type { ContractOptions, TransactionOptions }; +export type { ContractOptions, TransactionOptions } from '../token-registry-functions/types';🤖 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/status/types.ts` around lines 1 - 3, Replace the import-then-re-export in the status types module with a single direct type re-export from the token-registry-functions types module, preserving the exported ContractOptions and TransactionOptions symbols.Source: Linters/SAST tools
src/core/documentBuilder.ts (1)
344-353: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the
anycast here —TradeTrustToken__factory.connectis already used with a plain provider elsewhere, so this looks like an unnecessary type suppression. If the v4/v5 factory union is the blocker, narrow that type instead of silencing the checker.🤖 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/core/documentBuilder.ts` around lines 344 - 353, Remove the explicit any cast and its eslint suppression from supportsInterface, passing the provider directly to contractFactory.connect. If the v4/v5 factory union prevents type-checking, narrow the factory type or adjust the method signature so both generated factories accept the plain provider without suppressing the checker.src/__tests__/status/status.test.ts (2)
113-187: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReject/discharge blocks lack parity with accept's parameter-validation tests.
accept(Lines 76-96) tests missing-tokenRegistryAddress, missing-provider, and not-V5 paths, butrejectanddischargeonly cover happy-path/empty-remarks/callStatic-fail. Per the upstream snippets,reject.ts/discharge.tsduplicate the exact same preflight logic asaccept.ts, so a future divergence in any one of them wouldn't be caught here.🤖 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/__tests__/status/status.test.ts` around lines 113 - 187, Add parameter-validation parity tests to the reject and discharge describe blocks, matching the existing accept tests for missing tokenRegistryAddress, missing provider, and non-V5 contract paths. Use the existing reject and discharge helpers and assertions, while preserving their current happy-path, empty-remarks, and callStatic failure coverage.
98-110: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFragile mock cleanup for the callStatic-fail tests.
mockRejectedValuesets a persistent implementation;vi.clearAllMocks()in the nextbeforeEachonly clears call history, not implementations, so the manualmockV5TitleEscrowContract.callStatic.accept = vi.fn();/.staticCall = vi.fn();reassignment is what's actually preventing this rejection from leaking into later tests. Same pattern repeats at Lines 134-146 (reject) and 174-186 (discharge).Using
mockRejectedValueOnceinstead removes the need for this manual bookkeeping and is more resilient to test reordering/parallelization.statusE2eDocument.test.tsalready re-establishes these mocks proactively inbeforeEach, which is a more robust template to follow here too.♻️ Suggested fix (apply the same pattern at the reject/discharge occurrences)
- mockV5TitleEscrowContract.callStatic.accept.mockRejectedValue( - new Error('Simulated failure'), - ); - mockV5TitleEscrowContract.accept.staticCall.mockRejectedValue( - new Error('Simulated failure'), - ); + mockV5TitleEscrowContract.callStatic.accept.mockRejectedValueOnce( + new Error('Simulated failure'), + ); + mockV5TitleEscrowContract.accept.staticCall.mockRejectedValueOnce( + new Error('Simulated failure'), + ); await expect( accept(contractOptions, wallet, { remarks: MOCK_REMARKS }, options), ).rejects.toThrow('Pre-check (callStatic) for accept failed'); - mockV5TitleEscrowContract.callStatic.accept = vi.fn(); - mockV5TitleEscrowContract.accept.staticCall = vi.fn();Regarding the clearAllMocks semantics: "function clearAllMocks(): Vitest · Calls .mockClear() on all spies. This will clear mock history without affecting mock implementations."
🤖 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/__tests__/status/status.test.ts` around lines 98 - 110, Update the failure setup in the accept, reject, and discharge tests to use one-shot rejected implementations via mockRejectedValueOnce for both callStatic and staticCall mocks. Remove the manual mockV5TitleEscrowContract.callStatic.accept/staticCall and corresponding reject/discharge reassignments, while preserving the existing rejection assertions.src/__tests__/status/statusE2eDocument.test.ts (1)
388-406: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDescribe title doesn't match what's tested.
"invalid lifecycle transitions" only asserts that plain ETR functions don't read/mutate
status; it never exercises an actual invalid transition (e.g. discharge fromRejected, or a secondaccepton an already-Acceptedescrow) to verify the terminal-state guarantees the README describes. Consider renaming this block (e.g. "ETR functions are status-unaware") and/or adding cases that configure the mock to reject for an invalid precondition and assert the corresponding error.🤖 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/__tests__/status/statusE2eDocument.test.ts` around lines 388 - 406, Rename the describe block to reflect that it verifies ETR functions are status-unaware, or expand it with genuine invalid-transition cases. If expanding, configure the mock for an invalid precondition such as discharge from Rejected or accepting an already Accepted escrow, then assert the expected error and terminal-state 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 `@README.md`:
- Around line 1265-1291: The README documentation for accept, reject, and
discharge promises client-side role, owner, and status pre-flight checks that
the SDK does not implement. Update the “accept / reject / discharge” section to
describe the actual callStatic/on-chain precondition flow, including the real
failure behavior, unless the implementation is explicitly being changed to add
those checks.
In `@src/__tests__/fixtures/endorsement-chain.ts`:
- Around line 3364-3399: The getLogs fixture entries for StatusAccepted,
StatusRejected, and StatusDischarged use incorrect topic0 hashes. Replace the
three corresponding values in the endorsement-chain fixture with the provided
canonical hashes, preserving their addresses, block ranges, and empty results.
In `@src/status/accept.ts`:
- Around line 29-90: Extract the duplicated address resolution and V5 validation
from accept, reject, and discharge into a shared resolveV5TitleEscrow helper.
Have it validate signer.provider before resolving addresses, preserve the
existing token registry/token ID and version checks, and return the resolved
titleEscrowAddress. Update each action to use the helper, while keeping contract
construction and remark encryption in the respective action flows.
- Around line 39-51: Move the signer.provider validation in the accept flow
before the getTitleEscrowAddress call, while retaining the existing title escrow
and token validation behavior. Update the surrounding logic in the accept
function so getTitleEscrowAddress is only invoked after the provider-required
guard passes.
- Around line 52-61: Guard the remarks encryption in the accept flow before
calling encrypt, ensuring options.id is present whenever remarks are supplied.
Reuse the existing explicit validation style used for titleEscrowAddress and
signer.provider, and throw a clear error for missing options.id; preserve the 0x
fallback when no remarks are provided.
- Line 9: Update the return types of the wrapper and runStatusAction to support
both ethers v5 ContractTransaction and the v6 ContractTransactionResponse
returned by the v6 branch. Use a shared compatibility type or explicit union,
and ensure all related declarations consistently expose that broader Promise
return type.
In `@src/status/discharge.ts`:
- Around line 53-62: Validate that options.id is present before calling encrypt
in the discharge flow, instead of relying on the non-null assertion in the
encryptedRemarks assignment. Reuse the existing validation and error-handling
approach from accept.ts/reject.ts, while preserving the current '0x' behavior
when remarks are absent.
- Around line 40-52: Move the signer.provider validation in the discharge flow
before the getTitleEscrowAddress call, ensuring the provider is checked before
being passed to it. Keep the existing title escrow and token validation behavior
unchanged, and remove the later redundant provider guard.
In `@src/status/reject.ts`:
- Around line 53-62: Validate that options.id is present before calling encrypt
in the reject flow, and use the validated identifier as the encryption key
instead of the non-null assertion in the encryptedRemarks calculation. Preserve
the existing '0x' behavior when remarks are absent and follow the validation
approach used by accept.ts.
- Around line 40-52: Move the signer.provider validation before the
getTitleEscrowAddress call in the reject flow, ensuring the provider guard
executes before signer.provider is passed to getTitleEscrowAddress. Keep the
existing title escrow and token validation behavior unchanged.
In `@src/status/status.ts`:
- Around line 29-41: Move the `signer.provider` validation before the
`getTitleEscrowAddress` call in the status flow, ensuring the provider is
checked before it is passed as an argument. Preserve the existing title escrow,
token registry, and token ID validations, and apply the same ordering in the
corresponding accept, reject, and discharge flows if they duplicate this
pattern.
---
Nitpick comments:
In `@src/__tests__/status/status.test.ts`:
- Around line 113-187: Add parameter-validation parity tests to the reject and
discharge describe blocks, matching the existing accept tests for missing
tokenRegistryAddress, missing provider, and non-V5 contract paths. Use the
existing reject and discharge helpers and assertions, while preserving their
current happy-path, empty-remarks, and callStatic failure coverage.
- Around line 98-110: Update the failure setup in the accept, reject, and
discharge tests to use one-shot rejected implementations via
mockRejectedValueOnce for both callStatic and staticCall mocks. Remove the
manual mockV5TitleEscrowContract.callStatic.accept/staticCall and corresponding
reject/discharge reassignments, while preserving the existing rejection
assertions.
In `@src/__tests__/status/statusE2eDocument.test.ts`:
- Around line 388-406: Rename the describe block to reflect that it verifies ETR
functions are status-unaware, or expand it with genuine invalid-transition
cases. If expanding, configure the mock for an invalid precondition such as
discharge from Rejected or accepting an already Accepted escrow, then assert the
expected error and terminal-state behavior.
In `@src/core/documentBuilder.ts`:
- Around line 344-353: Remove the explicit any cast and its eslint suppression
from supportsInterface, passing the provider directly to
contractFactory.connect. If the v4/v5 factory union prevents type-checking,
narrow the factory type or adjust the method signature so both generated
factories accept the plain provider without suppressing the checker.
In `@src/status/types.ts`:
- Around line 1-3: Replace the import-then-re-export in the status types module
with a single direct type re-export from the token-registry-functions types
module, preserving the exported ContractOptions and TransactionOptions symbols.
🪄 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
Run ID: 56eca595-5d11-4ae3-9daf-117e13628000
📒 Files selected for processing (19)
README.mdsrc/__tests__/core/endorsement-chain-status-events.test.tssrc/__tests__/fixtures/boe-raw-file.jsonsrc/__tests__/fixtures/endorsement-chain.tssrc/__tests__/status/fixtures.tssrc/__tests__/status/status.test.tssrc/__tests__/status/statusE2eDocument.test.tssrc/__tests__/token-registry-functions/fixtures.tssrc/core/documentBuilder.tssrc/core/endorsement-chain/fetchEscrowTransfer.tssrc/core/endorsement-chain/helpers.tssrc/core/endorsement-chain/types.tssrc/index.tssrc/status/accept.tssrc/status/discharge.tssrc/status/index.tssrc/status/reject.tssrc/status/status.tssrc/status/types.ts
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
- Updated README to clarify on-chain preconditions for accept, reject, and discharge functions.
- Deleted the legacy V5 TitleEscrow test file.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/status/runStatusAction.ts (1)
79-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPreserve original error details and remove
console.errorin library code.The catch block swallows the underlying error and throws a generic message, making debugging difficult. Additionally,
console.errorin library code can leak internals and is not controllable by consumers. Include the original error message in the thrown error and drop the console log.♻️ Proposed refactor
try { if (isV6EthersProvider(signer.provider)) { await (titleEscrowContract as ContractV6)[action].staticCall(encryptedRemarks); } else { await (titleEscrowContract as ContractV5).callStatic[action](encryptedRemarks); } } catch (e) { - console.error('callStatic failed:', e); - throw new Error(`Pre-check (callStatic) for ${action} failed`); + throw new Error( + `Pre-check (callStatic) for ${action} failed: ${e instanceof Error ? e.message : String(e)}`, + ); }🤖 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/status/runStatusAction.ts` around lines 79 - 88, Update the catch block around the static call in runStatusAction to remove console.error and rethrow an error that preserves the original failure details alongside the existing action context. Keep the pre-check failure wording while including the caught error message.
🤖 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/__tests__/fixtures/endorsement-chain.ts`:
- Line 3369: The endorsement-chain fixture contains incorrect topic0 hashes for
three status events. Replace the three status-event topic0 values with the real
V5 TitleEscrow ABI hashes, using only the supported events listed in the comment
and preserving the fixture’s existing event structure.
In `@src/status/runStatusAction.ts`:
- Line 64: Update the encryptedRemarks construction in runStatusAction to only
call encrypt when both remarks and options.id are defined; otherwise use the
existing empty “0x” value. Remove the non-null assertion and preserve the
current encryption behavior when a valid ID is available.
- Around line 42-54: Move the signer.provider validation before the
getTitleEscrowAddress call in the centralized status action flow. Ensure the
existing “Provider is required” error is raised before invoking
getTitleEscrowAddress when titleEscrowAddress is absent, while preserving the
token registry, token ID, and title escrow validations.
---
Nitpick comments:
In `@src/status/runStatusAction.ts`:
- Around line 79-88: Update the catch block around the static call in
runStatusAction to remove console.error and rethrow an error that preserves the
original failure details alongside the existing action context. Keep the
pre-check failure wording while including the caught error message.
🪄 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
Run ID: a09cc9d0-6e06-4651-af68-727f9554e43f
📒 Files selected for processing (10)
README.mdsrc/__tests__/core/endorsement-chain-legacy-v5.test.tssrc/__tests__/fixtures/endorsement-chain.tssrc/core/endorsement-chain/useEndorsementChain.tssrc/status/accept.tssrc/status/discharge.tssrc/status/reject.tssrc/status/runStatusAction.tssrc/status/status.tssrc/status/types.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/status/types.ts
- src/status/status.ts
| params: [ | ||
| { | ||
| address: '0x24c9C688cf919D133abB512A41163972dA150f1b', | ||
| topics: ['0x29ab698c427c31fa4320edd82b1a5ed0be38d752324ceb1a7a8658f71ff380ac'], |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for V5 TitleEscrow event definitions =="
rg -n 'event Status(Accepted|Rejected|Discharged)' src/ -A2 \
--glob '!**/*.map' --glob '!node_modules' || echo "No Solidity event defs found"
echo
echo "== Search for status event names in ABIs =="
rg -n '"name":\s*"Status(Accepted|Rejected|Discharged)"' src/ -A5 \
--glob '!**/*.map' --glob '!node_modules' || echo "No ABI event entries found"
echo
echo "== Try computing topic hashes with ethers =="
node -e "
const sigs = [
'StatusAccepted(address,address,uint256,bytes)',
'StatusRejected(address,address,uint256,bytes)',
'StatusDischarged(address,address,uint256,bytes)'
];
try {
const { ethers } = require('ethers');
sigs.forEach(s => console.log(s, '->', ethers.id(s)));
} catch(e) {
console.log('ethers v5 not available:', e.message);
}
" 2>/dev/null || echo 'node/ethers not available'
echo
echo "== Try with Python keccak =="
python3 -c "
try:
from Crypto.Hash import keccak
sigs = [
'StatusAccepted(address,address,uint256,bytes)',
'StatusRejected(address,address,uint256,bytes)',
'StatusDischarged(address,address,uint256,bytes)'
]
for s in sigs:
k = keccak.new(digest_bits=256)
k.update(s.encode())
print(f'{s} -> 0x{k.hexdigest()}')
except ImportError:
print('No keccak library available in Python')
" 2>/dev/null || echo 'python3 not available'
echo
echo "== Current fixture topic hashes =="
rg -n 'topics:' src/__tests__/fixtures/endorsement-chain.ts | tail -10Repository: TrustVC/trustvc
Length of output: 1057
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Locate TitleEscrow-related files =="
fd -i 'TitleEscrow|titleescrow|endorsement|status' .
echo
echo "== Search for event declarations / ABI names repo-wide =="
rg -n 'event\s+Status(Accepted|Rejected|Discharged)|"name"\s*:\s*"Status(Accepted|Rejected|Discharged)"|TitleEscrow' . \
--glob '!**/*.map' --glob '!node_modules' -A3 -B3
echo
echo "== Show likely contract/ABI file tree (top levels only) =="
find . -maxdepth 2 -type f \( -name '*.sol' -o -name '*.json' -o -name '*.ts' \) | sed 's#^\./##' | sort | head -300Repository: TrustVC/trustvc
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Inspect the status-event test =="
sed -n '1,220p' src/__tests__/core/endorsement-chain-status-events.test.ts
echo
echo "== Inspect the fixture around the cited lines =="
sed -n '3348,3405p' src/__tests__/fixtures/endorsement-chain.ts
echo
echo "== Search for the three exact hashes elsewhere =="
rg -n '29ab698c427c31fa4320edd82b1a5ed0be38d752324ceb1a7a8658f71ff380ac|4273e3f9fa628dca95157f3f3e43f55a7b8a8e5a85596670d99f3b2f5d32578b|cde6b978de664fc7c3c45ec359bc419b7e601021747927764218fa9af45238d5' src/ -nRepository: TrustVC/trustvc
Length of output: 6421
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== package.json dependencies relevant to ethers/contracts =="
sed -n '1,240p' package.json | rg -n '"(ethers|ethersV6|`@tradetrust-tt/token-registry-v5`|`@tradetrust-tt/token-registry-v4`)"'
echo
echo "== Can Node resolve ethersV6 / token-registry-v5? =="
node - <<'JS'
for (const mod of ['ethers', 'ethersV6', '`@tradetrust-tt/token-registry-v5/contracts`']) {
try {
const resolved = require.resolve(mod);
console.log(mod + ' => ' + resolved);
} catch (e) {
console.log(mod + ' => NOT RESOLVABLE (' + e.message + ')');
}
}
JS
echo
echo "== Search for event signatures in repo lockfiles / metadata =="
rg -n 'StatusAccepted|StatusRejected|StatusDischarged|TitleEscrow__factory|event' package-lock.json pnpm-lock.yaml yarn.lock src/ --glob '!**/*.map' --max-count 40Repository: TrustVC/trustvc
Length of output: 23244
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const { ethers: ethersV6 } = require('ethersV6');
const { TitleEscrow__factory } = require('`@tradetrust-tt/token-registry-v5/contracts`');
const iface = new ethersV6.Interface(TitleEscrow__factory.abi);
for (const name of ['StatusAccepted', 'StatusRejected', 'StatusDischarged']) {
const fragment = iface.getEvent(name);
console.log(name);
console.log(' signature:', fragment.format());
console.log(' topicHash :', fragment.topicHash);
}
JSRepository: TrustVC/trustvc
Length of output: 837
🏁 Script executed:
#!/bin/bash
set -euo pipefail
node - <<'JS'
const { ethers: ethersV6 } = require('ethersV6');
const { TitleEscrow__factory } = require('`@tradetrust-tt/token-registry-v5/contracts`');
const iface = new ethersV6.Interface(TitleEscrow__factory.abi);
const events = iface.fragments.filter(f => f.type === 'event');
console.log('event count:', events.length);
for (const f of events) {
console.log(f.format(), '=>', f.topicHash);
}
const fixture = new Set([
'0x29ab698c427c31fa4320edd82b1a5ed0be38d752324ceb1a7a8658f71ff380ac',
'0x4273e3f9fa628dca95157f3f3e43f55a7b8a8e5a85596670d99f3b2f5d32578b',
'0xcde6b978de664fc7c3c45ec359bc419b7e601021747927764218fa9af45238d5',
]);
console.log('fixture matches:');
for (const h of fixture) {
const match = events.find(f => f.topicHash === h);
console.log(h, '=>', match ? match.format() : 'NO MATCH');
}
JSRepository: TrustVC/trustvc
Length of output: 1640
Replace these three status-event topic0s with the real V5 ABI hashes. @tradetrust-tt/token-registry-v5 does not expose StatusAccepted, StatusRejected, or StatusDischarged; the V5 TitleEscrow ABI only includes BeneficiaryTransfer, HolderTransfer, Initialized, Nomination, RejectTransferBeneficiary, RejectTransferHolder, RejectTransferOwners, ReturnToIssuer, Shred, and TokenReceived.
🤖 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/__tests__/fixtures/endorsement-chain.ts` at line 3369, The
endorsement-chain fixture contains incorrect topic0 hashes for three status
events. Replace the three status-event topic0 values with the real V5
TitleEscrow ABI hashes, using only the supported events listed in the comment
and preserving the fixture’s existing event structure.
| if (!titleEscrowAddress) { | ||
| if (!tokenRegistryAddress) throw new Error('Token registry address is required'); | ||
| if (!tokenId) throw new Error('Token ID is required'); | ||
| titleEscrowAddress = await getTitleEscrowAddress( | ||
| tokenRegistryAddress, | ||
| tokenId as string, | ||
| signer.provider, | ||
| {}, | ||
| ); | ||
| } | ||
|
|
||
| if (!titleEscrowAddress) throw new Error('Title escrow address is required'); | ||
| if (!signer.provider) throw new Error('Provider is required'); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Move signer.provider null check before getTitleEscrowAddress call.
signer.provider is passed to getTitleEscrowAddress at line 48 before the guard at line 54 validates it. When titleEscrowAddress is absent and the provider is null, the user gets an opaque ethers-internal error instead of the clear "Provider is required" message. This same issue was previously identified and fixed in the individual accept/reject/discharge files but re-appeared when the logic was centralized here.
🔧 Proposed fix
const { chainId, maxFeePerGas, maxPriorityFeePerGas, titleEscrowVersion } = options;
+ if (!signer.provider) throw new Error('Provider is required');
+
if (!titleEscrowAddress) {
if (!tokenRegistryAddress) throw new Error('Token registry address is required');
if (!tokenId) throw new Error('Token ID is required');
titleEscrowAddress = await getTitleEscrowAddress(
tokenRegistryAddress,
tokenId as string,
signer.provider,
{},
);
}
if (!titleEscrowAddress) throw new Error('Title escrow address is required');
- if (!signer.provider) throw new Error('Provider is required');📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!titleEscrowAddress) { | |
| if (!tokenRegistryAddress) throw new Error('Token registry address is required'); | |
| if (!tokenId) throw new Error('Token ID is required'); | |
| titleEscrowAddress = await getTitleEscrowAddress( | |
| tokenRegistryAddress, | |
| tokenId as string, | |
| signer.provider, | |
| {}, | |
| ); | |
| } | |
| if (!titleEscrowAddress) throw new Error('Title escrow address is required'); | |
| if (!signer.provider) throw new Error('Provider is required'); | |
| const { chainId, maxFeePerGas, maxPriorityFeePerGas, titleEscrowVersion } = options; | |
| if (!signer.provider) throw new Error('Provider is required'); | |
| if (!titleEscrowAddress) { | |
| if (!tokenRegistryAddress) throw new Error('Token registry address is required'); | |
| if (!tokenId) throw new Error('Token ID is required'); | |
| titleEscrowAddress = await getTitleEscrowAddress( | |
| tokenRegistryAddress, | |
| tokenId as string, | |
| signer.provider, | |
| {}, | |
| ); | |
| } | |
| if (!titleEscrowAddress) throw new Error('Title escrow address is required'); |
🤖 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/status/runStatusAction.ts` around lines 42 - 54, Move the signer.provider
validation before the getTitleEscrowAddress call in the centralized status
action flow. Ensure the existing “Provider is required” error is raised before
invoking getTitleEscrowAddress when titleEscrowAddress is absent, while
preserving the token registry, token ID, and title escrow validations.
| // eslint-disable-next-line @typescript-eslint/no-explicit-any | ||
| signer as any, | ||
| ); | ||
| const encryptedRemarks = remarks ? `0x${encrypt(remarks, options.id!)}` : '0x'; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Guard options.id before encrypting remarks.
When remarks is provided but options.id is undefined, the non-null assertion options.id! passes undefined to encrypt, which calls key.length on undefined — a TypeError crash. This was previously flagged and fixed in the individual action files but re-appeared here during centralization.
🛡️ Proposed fix
const { remarks } = params;
+ if (remarks && !options.id) throw new Error('`id` is required to encrypt remarks');
const Contract = getEthersContractFromProvider(signer.provider);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const encryptedRemarks = remarks ? `0x${encrypt(remarks, options.id!)}` : '0x'; | |
| const { remarks } = params; | |
| if (remarks && !options.id) throw new Error('`id` is required to encrypt remarks'); | |
| const encryptedRemarks = remarks ? `0x${encrypt(remarks, options.id!)}` : '0x'; |
🤖 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/status/runStatusAction.ts` at line 64, Update the encryptedRemarks
construction in runStatusAction to only call encrypt when both remarks and
options.id are defined; otherwise use the existing empty “0x” value. Remove the
non-null assertion and preserve the current encryption behavior when a valid ID
is available.
- updated testcases of the v4 / v5 and new v5 status changes in endorsment chain
|
1 participant
Summary
What is the background of this pull request?
Changes
Issues
What are the related issues or stories?
Summary by CodeRabbit
New Features
Bug Fixes