Conversation
📝 WalkthroughWalkthroughAdds a Document Store feature set: deploy, issue, revoke, role resolution, ERC‑165 interface detection with TT fallback, ethers v5/v6 compatibility, ABIs/interface IDs, public exports, tests/fixtures, README docs, and adds dependency Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client/App
participant DS as DocumentStore\nModule
participant Provider as Ethers\nProvider
participant Contract as Contract\nInstance
participant Chain as Blockchain
Client->>DS: documentStoreIssue(address, hash, signer)
activate DS
DS->>Provider: determine signer/version (v5 or v6)
Provider-->>DS: version info
alt auto-detect contract type
DS->>Contract: checkSupportsInterface(IDocumentStore)
Contract-->>DS: supports? / not?
alt not supported
DS->>Contract: checkSupportsInterface(ITransferableDocumentStore)
Contract-->>DS: supports? / not?
alt none supported
DS->>DS: select TT Document Store ABI
else
DS->>DS: select TransferableDocumentStore ABI
end
else
DS->>DS: select DocumentStore ABI
end
else
DS->>DS: use explicit contract type
end
DS->>Contract: pre-check (callStatic/staticCall).issue(hash)
Contract-->>DS: pre-check result
alt pre-check passed
DS->>DS: build tx options (gas/chainId)
DS->>Contract: send transaction: issue(hash)
Contract->>Chain: broadcast tx
Chain-->>Contract: tx receipt
Contract-->>DS: receipt
DS-->>Client: return transaction/receipt
else
DS-->>Client: throw pre-check error
end
deactivate DS
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Fix all issues with AI agents
In `@package.json`:
- Line 124: The dependency "@trustvc/document-store" in package.json is invalid;
update package.json to remove or replace this entry: either delete the
"@trustvc/document-store": "^1.0.2" line if unused, or change it to the correct
package name (e.g. "@tradetrust-tt/document-store" or the canonical TradeTrust
package) and run npm install to verify; ensure any import/require references in
the codebase (search for "@trustvc/document-store") are updated to the new
package identifier as well.
In `@src/__tests__/document-store/deploy.test.ts`:
- Line 32: Remove the leftover debug console.log by deleting the statement that
logs "result" (the console.log('result', result) call) from the test in
deploy.test.ts; if needed, replace it with or strengthen an assertion that
checks the expected properties of the variable result so the test verifies
behavior rather than printing debug output.
In `@src/__tests__/document-store/fixtures.ts`:
- Around line 6-8: The mock address constants (MOCK_DOCUMENT_STORE_ADDRESS,
MOCK_TRANSFERABLE_DOCUMENT_STORE_ADDRESS, MOCK_TT_DOCUMENT_STORE_ADDRESS) use
non-hex strings and should be replaced with valid 0x-prefixed 40-hex-char
Ethereum addresses (preferably EIP-55 checksummed) so code paths that validate
addresses (e.g., ethers.utils.getAddress) won’t fail; update the three constants
to valid checksum addresses (or derive them via ethers.utils.getAddress in
tests) while keeping the same constant names.
In `@src/__tests__/document-store/issue.test.ts`:
- Around line 56-58: The describe.each callback is declared async which is
incorrect because describe/describe.each must use a synchronous factory
function; remove the async keyword from the describe.each callback (the function
that currently takes { Provider, ethersVersion, contractType }) and, if any
awaited setup exists, move that asynchronous work into beforeAll/beforeEach or
individual it blocks (refer to the describe.each usage and any
beforeAll/beforeEach/it inside the same block to relocate awaits).
- Around line 82-90: The test file is missing beforeAll in the vitest import
list; update the top import that currently pulls in "describe, it, expect,
beforeEach, vi" to also include "beforeAll" so the file consistently uses
explicit imports (e.g., change the import to "describe, it, expect, beforeEach,
beforeAll, vi"); no other code changes are needed—just add beforeAll to the
import statement so the beforeAll hook is explicitly imported.
- Around line 229-252: The failing tests call documentStoreIssue which
internally calls getEthersContractFromProvider, but vi.clearAllMocks() in
beforeEach clears that mock for the tests at "should handle invalid document
hash format gracefully" and "should handle already issued document";
re-establish the mock in both tests by creating a mockContractConstructor that
returns mockContract and then calling
vi.mocked(getEthersContractFromProvider).mockReturnValue(mockContractConstructor(mockContract))
so documentStoreIssue receives the expected mockContract when it calls
getEthersContractFromProvider(signer.provider).
In `@src/__tests__/document-store/revoke.test.ts`:
- Around line 56-58: The describe.each callback is declared async which is
incorrect because describe must register synchronously; remove the async
modifier from the describe.each callback and move any asynchronous setup/awaits
inside that block into proper lifecycle hooks (e.g., beforeAll, beforeEach, or
inside individual it/test callbacks). Locate the describe.each usage that
iterates over providers and the callback function (the one referencing Provider,
ethersVersion, contractType) and ensure it is synchronous, relocating any awaits
to hooks like beforeAll/afterAll or to the tests themselves.
- Around line 82-102: The beforeAll sets a mock for
getEthersContractFromProvider which is then wiped by beforeEach's
vi.clearAllMocks; either move the getEthersContractFromProvider mock setup into
beforeEach after the vi.clearAllMocks call (so the mock is re-established each
test) or remove the vi.clearAllMocks from beforeEach and replace it with
targeted mock resets (e.g., mockContract.mockReset or specific .mockReset on
getEthersContractFromProvider) to preserve the getEthersContractFromProvider
mock; update references in the test setup where mockContract.callStatic.revoke
and mockContract.revoke.staticCall are set so they run after clearing/resetting
mocks.
In `@src/document-store/deploy.ts`:
- Line 58: Remove the debug console.log calls that were left in production:
delete the console.log('isv6', isV6) and the other console.log statements around
that area (any remaining console.log uses in this deploy logic) and either
replace them with the project's structured logger (e.g., processLogger.debug or
a configured logger) or remove them entirely; search for occurrences of
console.log in this file and update the code paths that reference isV6 and
related deployment variables to use the proper logging API (or no logging) so no
debug console statements remain.
- Line 75: deploymentTransaction() can return null so calling .wait() unguarded
may throw; update the code around the call in deploy.ts to check the result of
contract.deploymentTransaction() before calling .wait() — store the transaction
in a local (e.g., const tx = contract.deploymentTransaction()), if tx is null
handle it (throw a descriptive error or log and return) and only call await
tx.wait() when tx is non-null, assigning the result to receipt; ensure any
downstream use of receipt handles the error path accordingly.
In `@src/document-store/document-store-roles.ts`:
- Around line 18-23: The code can dereference SUPPORTED_CHAINS[chainId].name
when both options.provider and options.chainId are missing; in getRoleString,
check for a valid provider first and only access SUPPORTED_CHAINS when chainId
is defined and points to a valid entry. If chainId is missing or invalid, either
use a safe default (e.g., DEFAULT_CHAIN_ID) or throw a clear error; then call
utils.getProvider({ network: SUPPORTED_CHAINS[chainId].name }) (or with the
default) so you never read .name on undefined. Ensure the guards involve the
symbols options, chainId, provider, SUPPORTED_CHAINS, and utils.getProvider to
locate and fix the logic.
In `@src/document-store/index.ts`:
- Around line 3-4: The exports revokeDocumentStoreRole and
grantDocumentStoreRole in src/document-store/index.ts reference missing modules
('./revoke-role' and './grant-role') and break the build; remove these two
export lines from index.ts (or alternatively add the corresponding module files
implementing revokeDocumentStoreRole and grantDocumentStoreRole) so the package
no longer imports non-existent modules—ensure any code that depended on those
symbols is updated or guarded until the modules are added.
In `@src/document-store/revoke.ts`:
- Around line 47-52: The public API for documentStoreRevoke is inconsistent with
documentStoreIssue: change the documentStoreRevoke function signature to
(documentStoreAddress: string, documentHash: string, signer: SignerV5 |
SignerV6, options: RevokeOptions = {}) to match documentStoreIssue; update all
internal uses within this module (and any exports) to pass documentHash before
signer and adjust any parameter destructuring or calls to functions like
getDocumentStoreContract or revoke on the contract accordingly so runtime calls
remain correct.
🧹 Nitpick comments (11)
src/__tests__/document-store/fixtures.ts (1)
126-259: Consider extracting a factory function for mock contracts to reduce duplication.The three mock contract objects (
mockDocumentStoreContract,mockTransferableDocumentStoreContract,mockTTDocumentStoreContract) are nearly identical, differing only in the string prefix of the resolved tx hash and thenamereturn value. A helper function likecreateMockContract(prefix: string, name: string)would eliminate ~130 lines of repetition.src/document-store/deploy.ts (1)
42-50: Input validation could be more robust forowneraddress.The function validates that
owneris truthy but does not check if it's a valid Ethereum address. An invalid address would only fail later during deployment with a less descriptive error. Consider validating withethers.utils.isAddress(owner).src/document-store/tt-document-store-abi.ts (2)
7-9: Stale/misleading TODO comments should be cleaned up.Line 8's TODO says "Replace with the actual full ABI" and line 9 says "Minimal ABI for issue function" — but the ABI below already contains ~20 functions, events, and a constructor. Similarly, line 232's comment says "Add other functions from the ABI here (name, isIssued, isRevoked, etc.)" but those functions are already present (lines 143, 160, 186). These stale comments are misleading and suggest the code is incomplete when it may not be.
1-5: Doc comment contradicts ABI contents re:supportsInterface.The file header (line 3) states this is "a legacy document store contract that doesn't support ERC-165 supportsInterface," yet the ABI includes a
supportsInterface(bytes4)entry (lines 218-224). If the contract truly doesn't support ERC-165, having it in the ABI is confusing. If it does have the function but just returnsfalsefor known interface IDs, the doc comment should clarify that distinction.Also applies to: 218-224
src/document-store/document-store-roles.ts (1)
13-42: Consider narrowingroleparameter type to the known role literals.The
roleparameter isstring, but the only valid values are'admin' | 'issuer' | 'revoker'. Using a union type (or at leasttypeof rolesList[number]) would give callers compile-time feedback instead of a runtime'Invalid role'error.src/document-store/revoke.ts (2)
85-89: Unreachable dead code — this condition can never be true.When
isTransferableis provided (notundefined), one ofisDocumentStoreorisTransferableDocumentStoreis alwaystrue(lines 59-60). WhenisTransferableisundefined, the detection block (lines 64-83) setsisTTDocumentStore = trueif both arefalse. Therefore, the condition!isDocumentStore && !isTransferableDocumentStore && !isTTDocumentStoreon line 85 is unreachable.This isn't harmful, but it's worth noting for clarity. Same applies to the identical block in
issue.ts(line 85).
47-131: Significant code duplication withissue.ts.The entire body of
documentStoreRevokeis nearly identical todocumentStoreIssue— the contract type detection, ABI selection, contract instantiation, callStatic pre-check, and tx execution logic are all the same with onlyissue→revokesubstituted. Consider extracting shared logic (e.g., contract detection + instantiation, pre-check execution) into a common helper to reduce duplication.src/__tests__/document-store/issue.test.ts (1)
106-118: Inconsistent argument order indocumentStoreIssuecalls — blank lines between arguments.Throughout the test, calls to
documentStoreIssuehave blank lines inserted between arguments (e.g., line 109 betweenmockDocumentStoreAddressandmockDocumentHash). This is just a formatting/readability nit but is unusual and inconsistent. Consider removing the blank lines for cleaner call sites.src/document-store/issue.ts (2)
121-124:console.errorin library code may be undesirable for consumers.Logging the raw error to
console.errorin a library function creates noise that consumers can't control. Consider either removing it, or re-throwing the original error as thecauseso consumers can inspect it if needed.Proposed fix
} catch (e) { - console.error('callStatic failed:', e); - throw new Error('Pre-check (callStatic) for issue failed'); + throw new Error('Pre-check (callStatic) for issue failed', { cause: e }); }Same applies to
revoke.tsline 122.
95-103: When both interfaces are supported,TransferableDocumentStoresilently wins.If a contract supports both
IDocumentStoreandITransferableDocumentStore(which is the case for TransferableDocumentStore contracts that inherit DocumentStore), line 99 checksisTransferableDocumentStorefirst. This is likely the correct behavior, but adding a brief comment here would make the priority explicit for future maintainers.src/document-store/index.ts (1)
1-10: Missing exports for role utilities (getRoleString,rolesList) fromdocument-store-roles.ts.The barrel file exports role mutation functions (
grantDocumentStoreRole,revokeDocumentStoreRole) but doesn't export the role resolution utilities (getRoleString,rolesList) from./document-store-roles. If these are intended to be part of the public API, add the re-export.
| "@tradetrust-tt/token-registry-v5": "npm:@tradetrust-tt/token-registry@^5.5.0", | ||
| "@tradetrust-tt/tradetrust": "^6.10.2", | ||
| "@tradetrust-tt/tt-verify": "^9.6.0", | ||
| "@trustvc/document-store": "^1.0.2", |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
@trustvc/document-store npm package
💡 Result:
There doesn’t appear to be an npm package published as @trustvc/document-store (no npm listing turns up for that exact name). Instead, the “Document Store” smart-contract bindings/library used in the TrustVC/TradeTrust ecosystem is published as:
@tradetrust-tt/document-store(current TradeTrust package) — install with:and deploy vianpm i `@tradetrust-tt/document-store`deployAndWait(...). [1]
Related/legacy alternatives you may see referenced:
@govtechsg/document-store(older OpenAttestation-era package). [2]@trustvc/trustvc(TrustVC umbrella library; TradeTrust docs commonly instruct installing this for document workflows, but it’s not a standalone@trustvc/document-storepackage). [3][4]
Sources:
[1] (npmjs.com)
[2] (npmjs.com)
[3] (npmjs.com)
[4] (github.com)
Citations:
- 1: https://www.npmjs.com/package/%40tradetrust-tt%2Fdocument-store?utm_source=openai
- 2: https://www.npmjs.com/package/%40govtechsg/document-store?utm_source=openai
- 3: https://www.npmjs.com/package/%40trustvc/trustvc?utm_source=openai
- 4: https://github.com/TrustVC/trustvc?utm_source=openai
🏁 Script executed:
# Check package.json for ethers setup and current dependencies
cat -n package.json | head -150Repository: TrustVC/trustvc
Length of output: 6595
Remove or replace @trustvc/document-store — package does not exist on npm.
The package @trustvc/document-store@^1.0.2 does not exist in the npm registry. npm install will fail on this dependency. Either remove it if unused, or replace with the correct package name (likely @tradetrust-tt/document-store based on TradeTrust documentation).
🤖 Prompt for AI Agents
In `@package.json` at line 124, The dependency "@trustvc/document-store" in
package.json is invalid; update package.json to remove or replace this entry:
either delete the "@trustvc/document-store": "^1.0.2" line if unused, or change
it to the correct package name (e.g. "@tradetrust-tt/document-store" or the
canonical TradeTrust package) and run npm install to verify; ensure any
import/require references in the codebase (search for "@trustvc/document-store")
are updated to the new package identifier as well.
| export const MOCK_DOCUMENT_STORE_ADDRESS = '0xDocumentStoreContract'; | ||
| export const MOCK_TRANSFERABLE_DOCUMENT_STORE_ADDRESS = '0xTransferableDocumentStoreContract'; | ||
| export const MOCK_TT_DOCUMENT_STORE_ADDRESS = '0xTTDocumentStoreContract'; |
There was a problem hiding this comment.
Mock addresses are not valid Ethereum addresses.
'0xDocumentStoreContract' and similar are not valid hex addresses (wrong length, non-hex characters). While these work for simple mock assertions, they may cause issues if any code path validates address format (e.g., ethers.utils.getAddress). Consider using properly checksummed 40-hex-char addresses for more robust mocking.
🤖 Prompt for AI Agents
In `@src/__tests__/document-store/fixtures.ts` around lines 6 - 8, The mock
address constants (MOCK_DOCUMENT_STORE_ADDRESS,
MOCK_TRANSFERABLE_DOCUMENT_STORE_ADDRESS, MOCK_TT_DOCUMENT_STORE_ADDRESS) use
non-hex strings and should be replaced with valid 0x-prefixed 40-hex-char
Ethereum addresses (preferably EIP-55 checksummed) so code paths that validate
addresses (e.g., ethers.utils.getAddress) won’t fail; update the three constants
to valid checksum addresses (or derive them via ethers.utils.getAddress in
tests) while keeping the same constant names.
| console.log('before ContractFactory'); | ||
| const contract = await ContractFactory.deploy(storeName, owner, txOptions); | ||
| console.log('after ContractFactory'); | ||
| const receipt = await contract.deploymentTransaction().wait(); |
There was a problem hiding this comment.
Potential null-pointer dereference: deploymentTransaction() can return null.
In ethers v6, deploymentTransaction() returns ContractTransactionResponse | null. If it returns null, calling .wait() on it will throw a runtime error. Add a null guard.
Proposed fix
- const receipt = await contract.deploymentTransaction().wait();
+ const deployTx = contract.deploymentTransaction();
+ if (!deployTx) throw new Error('Deployment transaction not found');
+ const receipt = await deployTx.wait();📝 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 receipt = await contract.deploymentTransaction().wait(); | |
| const deployTx = contract.deploymentTransaction(); | |
| if (!deployTx) throw new Error('Deployment transaction not found'); | |
| const receipt = await deployTx.wait(); |
🤖 Prompt for AI Agents
In `@src/document-store/deploy.ts` at line 75, deploymentTransaction() can return
null so calling .wait() unguarded may throw; update the code around the call in
deploy.ts to check the result of contract.deploymentTransaction() before calling
.wait() — store the transaction in a local (e.g., const tx =
contract.deploymentTransaction()), if tx is null handle it (throw a descriptive
error or log and return) and only call await tx.wait() when tx is non-null,
assigning the result to receipt; ensure any downstream use of receipt handles
the error path accordingly.
| const { chainId } = options || {}; | ||
| let provider = options?.provider; | ||
|
|
||
| if (!provider) { | ||
| provider = utils.getProvider({ network: SUPPORTED_CHAINS[chainId].name }); | ||
| } |
There was a problem hiding this comment.
Null-pointer dereference when both provider and chainId are missing.
If getRoleString is called without options, or with options that omit both provider and chainId, then chainId is undefined. SUPPORTED_CHAINS[undefined] evaluates to undefined, and accessing .name on it will throw a TypeError at runtime with no helpful message.
Add a guard before accessing the chain config:
🐛 Proposed fix
if (!provider) {
+ if (!chainId) throw new Error('Either provider or chainId must be provided');
provider = utils.getProvider({ network: SUPPORTED_CHAINS[chainId].name });
}🤖 Prompt for AI Agents
In `@src/document-store/document-store-roles.ts` around lines 18 - 23, The code
can dereference SUPPORTED_CHAINS[chainId].name when both options.provider and
options.chainId are missing; in getRoleString, check for a valid provider first
and only access SUPPORTED_CHAINS when chainId is defined and points to a valid
entry. If chainId is missing or invalid, either use a safe default (e.g.,
DEFAULT_CHAIN_ID) or throw a clear error; then call utils.getProvider({ network:
SUPPORTED_CHAINS[chainId].name }) (or with the default) so you never read .name
on undefined. Ensure the guards involve the symbols options, chainId, provider,
SUPPORTED_CHAINS, and utils.getProvider to locate and fix the logic.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/__tests__/document-store/fixtures.ts`:
- Around line 71-72: The test fixtures list has grantRole and revokeRole with
reversed parameter order; update the fixture entries for the functions named
grantRole and revokeRole (in the fixtures array) so their format strings match
the actual ABI/OpenZeppelin signature: use "grantRole(bytes32 role, address
account)" and "revokeRole(bytes32 role, address account)" (i.e., bytes32 first,
address second) to mirror the definitions in tt-document-store-abi.ts and avoid
signature mismatches in tests.
🧹 Nitpick comments (3)
src/__tests__/document-store/fixtures.ts (3)
76-121: Constructors should not return values — use plain objects orvi.fn()instead.Biome correctly flags that returning from a constructor overrides
this, which is a confusing anti-pattern. Since these are mocks and don't need class semantics, replace the classes with plain object factories.Proposed fix (example for DocumentStore__factory)
- DocumentStore__factory: class { - static abi = ['constructor(string storeName, address owner)']; - static bytecode = '0x60006000'; - constructor() { - return { - deploy: vi.fn(() => ({ - waitForDeployment: vi.fn().mockResolvedValue(undefined), - target: '0xDeployedDocumentStoreAddress', - deploymentTransaction: vi.fn(() => ({ hash: 'deploy_tx_hash' })), - deployTransaction: { - wait: vi.fn().mockResolvedValue({}), - hash: 'deploy_tx_hash', - }, - address: '0xDeployedDocumentStoreAddress', - })), - }; - } - }, + DocumentStore__factory: Object.assign( + vi.fn(() => ({ + deploy: vi.fn(() => ({ + waitForDeployment: vi.fn().mockResolvedValue(undefined), + target: '0xDeployedDocumentStoreAddress', + deploymentTransaction: vi.fn(() => ({ hash: 'deploy_tx_hash' })), + deployTransaction: { + wait: vi.fn().mockResolvedValue({}), + hash: 'deploy_tx_hash', + }, + address: '0xDeployedDocumentStoreAddress', + })), + })), + { + abi: ['constructor(string storeName, address owner)'], + bytecode: '0x60006000', + }, + ),Apply the same pattern to
TransferableDocumentStore__factory.
125-130: SharedcallStaticFxnreference across all mock contracts risks test pollution.All three mock contracts (
mockDocumentStoreContract,mockTransferableDocumentStoreContract,mockTTDocumentStoreContract) share the samecallStaticFxnobject. If a test resets, reconfigures, or asserts call counts on one contract'scallStatic, it silently affects the others.Create separate instances per contract:
Proposed fix
-const callStaticFxn = { - issue: vi.fn(), - revoke: vi.fn(), - grantRole: vi.fn(), - revokeRole: vi.fn(), -}; +const createCallStaticFxn = () => ({ + issue: vi.fn(), + revoke: vi.fn(), + grantRole: vi.fn(), + revokeRole: vi.fn(), +});Then use
callStatic: createCallStaticFxn()in each mock contract.
123-124: Same shared-reference concern applies topromiseResolveTrue/promiseResolveFalse.
isIssued,isRevoked, andisActiveacross all three contracts point to the samevi.fn()instances. Call counts and mock state will leak between contracts. Consider creating per-contract instances (e.g., via a factory function) for the same reason ascallStaticFxn.Also applies to: 165-167, 205-207, 245-247
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/document-store/deploy.ts`:
- Around line 83-86: The call to contract.deployTransaction.wait() can return
undefined in ethers v5; add a null guard after awaiting wait() in the deploy
function (the section using ContractFactory.deploy and
contract.deployTransaction.wait) so that if receipt is undefined you handle it
(e.g., throw a descriptive error or retry) instead of returning undefined—ensure
you reference the local variable receipt and update callers if necessary to
expect an error rather than a possibly undefined receipt.
🧹 Nitpick comments (1)
src/document-store/deploy.ts (1)
42-50: Consider validatingowneras a proper address.Currently the check is only
!owner, which would pass any non-empty string. A malformed address would still reach the contract deployment and fail with an obscure on-chain error. Consider usingethers.utils.isAddress(owner)(v5) orethers.isAddress(owner)(v6) for an early, descriptive validation.
| const contract = await ContractFactory.deploy(storeName, owner, txOptions); | ||
| const receipt = await contract.deployTransaction.wait(); | ||
|
|
||
| return receipt; |
There was a problem hiding this comment.
Add a null guard on the v5 wait() result.
In ethers v5, wait() can also return undefined if the transaction is dropped/replaced. For consistency and safety, guard the receipt here too.
Proposed fix
const contract = await ContractFactory.deploy(storeName, owner, txOptions);
const receipt = await contract.deployTransaction.wait();
-
+ if (!receipt) throw new Error('Deployment transaction receipt not found');
return receipt;📝 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 contract = await ContractFactory.deploy(storeName, owner, txOptions); | |
| const receipt = await contract.deployTransaction.wait(); | |
| return receipt; | |
| const contract = await ContractFactory.deploy(storeName, owner, txOptions); | |
| const receipt = await contract.deployTransaction.wait(); | |
| if (!receipt) throw new Error('Deployment transaction receipt not found'); | |
| return receipt; |
🤖 Prompt for AI Agents
In `@src/document-store/deploy.ts` around lines 83 - 86, The call to
contract.deployTransaction.wait() can return undefined in ethers v5; add a null
guard after awaiting wait() in the deploy function (the section using
ContractFactory.deploy and contract.deployTransaction.wait) so that if receipt
is undefined you handle it (e.g., throw a descriptive error or retry) instead of
returning undefined—ensure you reference the local variable receipt and update
callers if necessary to expect an error rather than a possibly undefined
receipt.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/__tests__/document-store/issue.test.ts`:
- Around line 87-99: The beforeEach lacks re-mocking
getEthersContractFromProvider so new Contract(...) returns an empty mock and the
mocked methods on mockContract (e.g., mockContract.callStatic.issue and
mockContract.issue.staticCall) are never present; in the beforeEach (near the
existing vi.spyOn and mockContract mocks) restore/mock
getEthersContractFromProvider to return a constructor that yields mockContract
(so new Contract(...) returns mockContract) — use the same approach as the
"Contract Type Detection" setup to ensure documentStoreIssue calls the mocked
methods on mockContract.
In `@src/__tests__/document-store/revoke.test.ts`:
- Around line 196-213: The test for fallback to TT Document Store is mocking
getEthersContractFromProvider to return the standard mockContract instead of
mockTTDocumentStoreContract, so update the mock in the test for
documentStoreRevoke when coreModule.checkSupportsInterface resolves false to
return mockTTDocumentStoreContract (or ensure the mock returns a constructor
that creates mockTTDocumentStoreContract) and add an assertion that verifies
TT-specific behavior—e.g., that getEthersContractFromProvider was called with
TT_DOCUMENT_STORE_ABI or that the result matches the
mockTTDocumentStoreContract's expected value; reference checkSupportsInterface,
documentStoreRevoke, getEthersContractFromProvider, mockTTDocumentStoreContract,
and TT_DOCUMENT_STORE_ABI to locate and change the mocks and assertions.
🧹 Nitpick comments (12)
src/document-store/deploy.ts (2)
42-47: No validation onowneraddress format.
owneris only checked for truthiness. An invalid address (e.g.,"hello") will pass validation but cause a revert on-chain during deployment. Consider validating it as a proper Ethereum address usingethers.utils.isAddress(v5) orethers.isAddress(v6) to fail fast with a clear message.
88-93: Error wrapping discards the original stack trace.
throw new Error(...)with the message fromeloses the original error's stack. Consider usingcauseto preserve it for debugging.Proposed fix
throw new Error( `Failed to deploy DocumentStore: ${e instanceof Error ? e.message : String(e)}`, + { cause: e }, );src/__tests__/document-store/fixtures.ts (1)
76-111: Constructors returning values are a code smell flagged by Biome.The
DocumentStore__factoryandTransferableDocumentStore__factorymock classes have constructors thatreturnan object, which overridesthis. While this works for mocking, it's fragile and flagged by static analysis (lint/correctness/noConstructorReturn). Consider using plain factory functions or objects with a mock constructor instead.Proposed refactor using factory functions
- DocumentStore__factory: class { - static abi = ['constructor(string storeName, address owner)']; - static bytecode = '0x60006000'; - constructor() { - return { - deploy: vi.fn(() => ({ - ... - })), - }; - } - }, + DocumentStore__factory: { + abi: ['constructor(string storeName, address owner)'], + bytecode: '0x60006000', + },Note: The actual
ContractFactoryis mocked at the ethers level (lines 10-48), so the@trustvc/document-storemock only needs to provideabiandbytecodeas static properties—the constructor-based deploy mock is already handled by the ethersContractFactorymock.src/__tests__/document-store/deploy.test.ts (3)
19-37: Test relies on shape-based provider detection viaany-typed objects.The v6 deployment tests construct inline mock objects (
const providerV6: any = {...}) that mimic the structural checks inisV6EthersProvider(havingproviderproperty, no_isProvider). This is fine for unit tests but note that the localproviderV6variable on line 20 shadows the importedproviderV6from fixtures (line 6), which could cause confusion during maintenance.Consider renaming the local variable to something like
mockProviderV6to distinguish it from the fixture import.
39-61: Redundantvi.spyOn— the mock object already hasgetNetworkas avi.fn().Line 48 spies on
providerV6.getNetwork, butproviderV6(the local mock on line 40) already hasgetNetwork: vi.fn().mockResolvedValue(...). The subsequentvi.spyOnreplaces it with the same value, adding no coverage.
9-116: Tests only verify the happy-path receipt shape; no assertion on contract factory selection.All four deployment tests assert the same receipt shape. There's no verification that
DocumentStore__factoryvsTransferableDocumentStore__factorywas actually used based on theisTransferableflag. Consider adding assertions (e.g., spy on factory constructor calls) to confirm the correct factory is selected for each variant.src/__tests__/document-store/issue.test.ts (1)
67-76: Wallet and spy setup indescribe.eachbody runs during test registration — fragile pattern.The wallet creation and
vi.spyOncalls (lines 68–76) execute synchronously whendescribe.eachregisters the test suite, not inside abeforeEach. Whilevi.clearAllMocks()preserves spy implementations, this means:
- The same wallet instance is shared across all tests in the variant.
- Any test that mutates the wallet or its provider could affect subsequent tests.
Moving wallet creation into
beforeEachwould improve test isolation.src/document-store/revoke.ts (3)
121-124: Error wrapping in callStatic catch discards the original error context.The original error
eis logged viaconsole.errorbut not attached to the re-thrown error. Consumers won't have the root cause in the error chain. Preserve it withcause.Proposed fix
} catch (e) { - console.error('callStatic failed:', e); - throw new Error('Pre-check (callStatic) for revoke failed'); + throw new Error('Pre-check (callStatic) for revoke failed', { cause: e }); }
59-61: Implicit boolean coercion ofisTransferable(which can beundefined) may be confusing.
let isDocumentStore = !isTransferable;evaluates totruewhenisTransferableisundefined, which happens to be correct here since the variables are overwritten in the detection block. But the intent isn't immediately clear — a reader might wonder ifundefinedwas meant to default to "DocumentStore". A brief comment would help.
53-55: No format validation ondocumentStoreAddressordocumentHash.Like
deploy.ts, bothdocumentStoreAddressanddocumentHashare only checked for truthiness. Invalid formats (e.g., non-hex strings, wrong length) will pass validation and only fail on-chain. FordocumentHash, a basic regex check for0x+ 64 hex chars, and fordocumentStoreAddress,isAddress(), would give early, clear errors.src/__tests__/document-store/revoke.test.ts (2)
82-88: Remove commented-outbeforeAllblock.This dead code is left over from the previous review fix. It adds noise and should be cleaned up.
Proposed fix
- // beforeAll(() => { - // vi.clearAllMocks(); - // const mockContractConstructor = (mockContract: any) => vi.fn(() => mockContract); - // vi.mocked(getEthersContractFromProvider).mockReturnValue( - // mockContractConstructor(mockContract), - // ); - // }); -
67-76: Wallet and spy setup in describe body — same isolation concern as issue.test.ts.Same pattern as the issue tests: wallet instances and spies are created during test registration. Consider moving to
beforeEachfor better test isolation across test cases in the same variant.
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/__tests__/document-store/issue.test.ts`:
- Around line 204-227: The test's fallback path is mocking the wrong contract
and using a weak assertion: update the mocked getEthersContractFromProvider to
return the TT ABI mock (use mockTTDocumentStoreContract instead of mockContract)
so documentStoreIssue exercises the TT Document Store fallback, then replace
expect(result).toBeDefined() with a strict equality check matching the expected
issued response (same shape/value used in the dedicated TT fallback tests), and
keep/verify coreModule.checkSupportsInterface call counts as before; ensure you
reference getEthersContractFromProvider, mockTTDocumentStoreContract,
documentStoreIssue, and coreModule.checkSupportsInterface when making the
changes.
🧹 Nitpick comments (2)
src/__tests__/document-store/issue.test.ts (2)
82-104: Fragile mock lifecycle:beforeAllsetup +beforeEachclearAllMocksrelies on subtle Vitest behavior.
vi.clearAllMocks()inbeforeEach(line 93) clears call history but preserves themockReturnValueset inbeforeAll(line 87). This works, but the coupling is implicit and fragile — switching tovi.resetAllMocks()or enablingmockReset: truein Vitest config would silently break all tests in this block. Consider re-establishing thegetEthersContractFromProvidermock insidebeforeEachinstead, matching the pattern used in theContract Type DetectionandTT Document Store (Fallback)blocks (lines 274-277, 374-377).Suggested consolidation into beforeEach
- beforeAll(() => { - // Clear any existing mocks first - vi.clearAllMocks(); - const mockContractConstructor = (mockContract: any) => vi.fn(() => mockContract); - // Only set up the mock if it hasn't been set up yet - vi.mocked(getEthersContractFromProvider).mockReturnValue( - mockContractConstructor(mockContract), - ); - }); - beforeEach(() => { vi.clearAllMocks(); + const mockContractConstructor = (mc: any) => vi.fn(() => mc); + vi.mocked(getEthersContractFromProvider).mockReturnValue( + mockContractConstructor(mockContract), + ); vi.spyOn(coreModule, 'checkSupportsInterface').mockImplementation(
67-76: Wallet and provider spy setup in describe body risks test pollution.
vi.spyOn(wallet, 'getChainId')(line 70) andvi.spyOn(Provider, 'getNetwork')(line 73) are executed during describe-body evaluation rather than inside a lifecycle hook. This means:
- The spy on
providerV6(a shared fixture) is never properly restored between parameterized runs, sinceclearAllMocks()preserves spy wiring.- If a test mutates the wallet instance, state leaks to subsequent tests.
Moving wallet creation and spy setup into
beforeEachwould improve isolation and align with the pattern in the non-parameterized describe blocks.
| it('should fallback to TT Document Store when ERC-165 interfaces not supported', async () => { | ||
| // When checkSupportsInterface returns false, it should fallback to TT Document Store | ||
| vi.spyOn(coreModule, 'checkSupportsInterface').mockResolvedValue(false); | ||
|
|
||
| // Mock the contract to use TT Document Store | ||
| const mockContractConstructor = (mockContract: any) => vi.fn(() => mockContract); | ||
| vi.mocked(getEthersContractFromProvider).mockReturnValue( | ||
| mockContractConstructor(mockContract), | ||
| ); | ||
|
|
||
| const result = await documentStoreIssue( | ||
| mockDocumentStoreAddress, | ||
|
|
||
| mockDocumentHash, | ||
| wallet, | ||
| { | ||
| chainId: mockChainId, | ||
| }, | ||
| ); | ||
|
|
||
| // Should successfully issue using TT Document Store as fallback | ||
| expect(result).toBeDefined(); | ||
| expect(coreModule.checkSupportsInterface).toHaveBeenCalledTimes(2); | ||
| }); |
There was a problem hiding this comment.
Weak assertion and wrong mock contract in fallback test.
This test verifies TT Document Store fallback but:
- Line 211: Re-mocks
getEthersContractFromProviderto returnmockContract(DocumentStore or TransferableDocumentStore), notmockTTDocumentStoreContract. The fallback path should use the TT ABI, but the test can't verify this because the mock contract doesn't change. - Line 225:
expect(result).toBeDefined()is a weak assertion compared totoEqual(...)used elsewhere. It passes for any truthy value.
The dedicated TT fallback block (lines 366–495) properly uses mockTTDocumentStoreContract and asserts exact values. Consider aligning this test or removing it in favor of the dedicated block.
🤖 Prompt for AI Agents
In `@src/__tests__/document-store/issue.test.ts` around lines 204 - 227, The
test's fallback path is mocking the wrong contract and using a weak assertion:
update the mocked getEthersContractFromProvider to return the TT ABI mock (use
mockTTDocumentStoreContract instead of mockContract) so documentStoreIssue
exercises the TT Document Store fallback, then replace
expect(result).toBeDefined() with a strict equality check matching the expected
issued response (same shape/value used in the dedicated TT fallback tests), and
keep/verify coreModule.checkSupportsInterface call counts as before; ensure you
reference getEthersContractFromProvider, mockTTDocumentStoreContract,
documentStoreIssue, and coreModule.checkSupportsInterface when making the
changes.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/document-store/revoke.ts`:
- Around line 112-124: The pre-check callStatic invocation for ethers v5 is
using bracket signature lookup which doesn't match the test mock; update the v5
branch to call (documentStoreContract as
ContractV5).callStatic.revoke(documentHash) instead of using
callStatic['revoke(bytes32)'], and in the catch block include the original error
when re-throwing (e.g., attach or wrap the caught error) so the thrown Error
preserves the underlying error details; refer to isV6EthersProvider,
documentStoreContract, ContractV5, and callStatic.revoke to locate the change.
🧹 Nitpick comments (1)
src/document-store/revoke.ts (1)
85-89: Unreachable dead code — the error at Line 86 can never be thrown.When
isTransferableis defined (true/false), one ofisDocumentStore/isTransferableDocumentStoreis alwaystrue(Lines 59-60). WhenisTransferableisundefined, Lines 80-82 setisTTDocumentStore = trueas a fallback if both interface checks fail. Thus the condition on Line 85 is never satisfied.Consider removing it or converting it to a debug assertion if you want to keep it as a safety net.
| // Check callStatic (dry run) to ensure transaction will succeed | ||
| try { | ||
| const isV6 = isV6EthersProvider(signer.provider); | ||
|
|
||
| if (isV6) { | ||
| await (documentStoreContract as ContractV6)['revoke(bytes32)'].staticCall(documentHash); | ||
| } else { | ||
| await (documentStoreContract as ContractV5).callStatic['revoke(bytes32)'](documentHash); | ||
| } | ||
| } catch (e) { | ||
| console.error('callStatic failed:', e); | ||
| throw new Error('Pre-check (callStatic) for revoke failed'); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find test files for revoke.ts and locate fixtures
fd "revoke" --type f -e "ts" -e "test" | head -20Repository: TrustVC/trustvc
Length of output: 131
🏁 Script executed:
# Find issue.ts and related document-store test files
fd "issue\." --type f | grep -E "document-store|test"Repository: TrustVC/trustvc
Length of output: 129
🏁 Script executed:
# Look for test fixtures and mock setups
find . -name "*fixture*" -o -name "*mock*" | grep -E "document-store|test" | head -20Repository: TrustVC/trustvc
Length of output: 248
🏁 Script executed:
# Search for callStatic usage patterns in the codebase
rg "callStatic\[" --type ts -B 3 -A 3Repository: TrustVC/trustvc
Length of output: 1120
🏁 Script executed:
# Read the revoke test file to see test structure and failures
wc -l src/__tests__/document-store/revoke.test.tsRepository: TrustVC/trustvc
Length of output: 106
🏁 Script executed:
# Read the fixtures file to understand mock setup
wc -l src/__tests__/document-store/fixtures.tsRepository: TrustVC/trustvc
Length of output: 103
🏁 Script executed:
# Read issue.ts to compare callStatic pattern
wc -l src/document-store/issue.tsRepository: TrustVC/trustvc
Length of output: 90
🏁 Script executed:
# Read the revoke test file
cat -n src/__tests__/document-store/revoke.test.tsRepository: TrustVC/trustvc
Length of output: 15885
🏁 Script executed:
# Read the fixtures file
cat -n src/__tests__/document-store/fixtures.tsRepository: TrustVC/trustvc
Length of output: 10214
🏁 Script executed:
# Read issue.ts for comparison
cat -n src/document-store/issue.tsRepository: TrustVC/trustvc
Length of output: 6016
🏁 Script executed:
# Verify exact v5 pattern by checking if issue and revoke have different signatures
rg "callStatic\.(revoke|issue)" src/document-store/ -B 2 -A 2Repository: TrustVC/trustvc
Length of output: 1107
🏁 Script executed:
# Check if there are any overloads of revoke() function that would require signature notation
rg "revoke\(" src/document-store/revoke.ts -B 3 -A 3Repository: TrustVC/trustvc
Length of output: 689
🏁 Script executed:
# Compare the actual transaction calls (non-callStatic) to understand the pattern
rg "\['revoke\(bytes32\)\'\]|\['issue\(bytes32\)\'\]" src/document-store/ -B 1 -A 1Repository: TrustVC/trustvc
Length of output: 193
🏁 Script executed:
# Compare the actual transaction calls (non-callStatic) to understand the pattern
rg "revoke\(bytes32\)" src/document-store/ -B 1 -A 1Repository: TrustVC/trustvc
Length of output: 636
🏁 Script executed:
# Check the DocumentStore ABI/interface to see if revoke has multiple overloads
rg "revoke" src/__tests__/document-store/fixtures.ts -B 2 -A 2Repository: TrustVC/trustvc
Length of output: 1764
🏁 Script executed:
# Check if there's a pattern difference - does issue have overloads?
cat src/__tests__/document-store/fixtures.ts | grep -A 10 "documentStoreFxnFragments"Repository: TrustVC/trustvc
Length of output: 1011
🏁 Script executed:
# Check if issue() function also uses signature notation anywhere
cat src/document-store/issue.ts | grep -E "\['issue|callStatic"Repository: TrustVC/trustvc
Length of output: 382
🏁 Script executed:
# Verify the actual test execution - check if tests currently pass or fail
# Look for any CI/test status information
git log --oneline -20 -- "src/__tests__/document-store/revoke.test.ts"Repository: TrustVC/trustvc
Length of output: 101
Fix ethers v5 callStatic invocation — revoke.ts uses inconsistent bracket notation.
The code at line 119 uses callStatic['revoke(bytes32)'] but should use callStatic.revoke() to match the actual mock setup and be consistent with issue.ts (line 119). There is only one revoke overload, so signature notation is not needed. The bracket notation with full signature 'revoke(bytes32)' creates a property lookup mismatch: the test fixtures mock .callStatic.revoke (simple name), not .callStatic['revoke(bytes32)'] (signature key).
Change line 119 from:
await (documentStoreContract as ContractV5).callStatic['revoke(bytes32)'](documentHash);
to:
await (documentStoreContract as ContractV5).callStatic.revoke(documentHash);
Additionally, preserve the original error when re-throwing at line 123:
} catch (e) {
- console.error('callStatic failed:', e);
- throw new Error('Pre-check (callStatic) for revoke failed');
+ throw new Error(`Pre-check (callStatic) for revoke failed: ${e instanceof Error ? e.message : e}`);
}🧰 Tools
🪛 GitHub Actions: CI
[error] 123-123: Pre-check (callStatic) for revoke failed. documentStoreContract.callStatic.revoke(bytes32) is not a function.
🪛 GitHub Check: Tests / Run Tests (20.x)
[failure] 123-123: src/tests/document-store/revoke.test.ts > Revoke Document > Revoke Document with 'DocumentStore' and ethers version 'v5' > should revoke document without chainId option
Error: Pre-check (callStatic) for revoke failed
❯ Module.documentStoreRevoke src/document-store/revoke.ts:123:11
❯ src/tests/document-store/revoke.test.ts:137:30
[failure] 123-123: src/tests/document-store/revoke.test.ts > Revoke Document > Revoke Document with 'DocumentStore' and ethers version 'v5' > should revoke document without chainId option
Error: Pre-check (callStatic) for revoke failed
❯ Module.documentStoreRevoke src/document-store/revoke.ts:123:11
❯ src/tests/document-store/revoke.test.ts:137:30
[failure] 123-123: src/tests/document-store/revoke.test.ts > Revoke Document > Revoke Document with 'DocumentStore' and ethers version 'v5' > should revoke document with explicit contract type
Error: Pre-check (callStatic) for revoke failed
❯ Module.documentStoreRevoke src/document-store/revoke.ts:123:11
❯ src/tests/document-store/revoke.test.ts:123:30
[failure] 123-123: src/tests/document-store/revoke.test.ts > Revoke Document > Revoke Document with 'DocumentStore' and ethers version 'v5' > should revoke document with explicit contract type
Error: Pre-check (callStatic) for revoke failed
❯ Module.documentStoreRevoke src/document-store/revoke.ts:123:11
❯ src/tests/document-store/revoke.test.ts:123:30
[failure] 123-123: src/tests/document-store/revoke.test.ts > Revoke Document > Revoke Document with 'DocumentStore' and ethers version 'v5' > should revoke document with explicit contract type
Error: Pre-check (callStatic) for revoke failed
❯ Module.documentStoreRevoke src/document-store/revoke.ts:123:11
❯ src/tests/document-store/revoke.test.ts:123:30
[failure] 123-123: src/tests/document-store/revoke.test.ts > Revoke Document > Revoke Document with 'DocumentStore' and ethers version 'v5' > should revoke document with explicit contract type
Error: Pre-check (callStatic) for revoke failed
❯ Module.documentStoreRevoke src/document-store/revoke.ts:123:11
❯ src/tests/document-store/revoke.test.ts:123:30
[failure] 123-123: src/tests/document-store/revoke.test.ts > Revoke Document > Revoke Document with 'DocumentStore' and ethers version 'v5' > should revoke document hash successfully
Error: Pre-check (callStatic) for revoke failed
❯ Module.documentStoreRevoke src/document-store/revoke.ts:123:11
❯ src/tests/document-store/revoke.test.ts:109:24
[failure] 123-123: src/tests/document-store/revoke.test.ts > Revoke Document > Revoke Document with 'DocumentStore' and ethers version 'v5' > should revoke document hash successfully
Error: Pre-check (callStatic) for revoke failed
❯ Module.documentStoreRevoke src/document-store/revoke.ts:123:11
❯ src/tests/document-store/revoke.test.ts:109:24
[failure] 123-123: src/tests/document-store/revoke.test.ts > Revoke Document > Revoke Document with 'DocumentStore' and ethers version 'v5' > should revoke document hash successfully
Error: Pre-check (callStatic) for revoke failed
❯ Module.documentStoreRevoke src/document-store/revoke.ts:123:11
❯ src/tests/document-store/revoke.test.ts:109:24
[failure] 123-123: src/tests/document-store/revoke.test.ts > Revoke Document > Revoke Document with 'DocumentStore' and ethers version 'v5' > should revoke document hash successfully
Error: Pre-check (callStatic) for revoke failed
❯ Module.documentStoreRevoke src/document-store/revoke.ts:123:11
❯ src/tests/document-store/revoke.test.ts:109:24
🪛 GitHub Check: Tests / Run Tests (22.x)
[failure] 123-123: src/tests/document-store/revoke.test.ts > Revoke Document > Revoke Document with 'DocumentStore' and ethers version 'v5' > should revoke document without chainId option
Error: Pre-check (callStatic) for revoke failed
❯ Module.documentStoreRevoke src/document-store/revoke.ts:123:11
❯ src/tests/document-store/revoke.test.ts:137:30
[failure] 123-123: src/tests/document-store/revoke.test.ts > Revoke Document > Revoke Document with 'DocumentStore' and ethers version 'v5' > should revoke document without chainId option
Error: Pre-check (callStatic) for revoke failed
❯ Module.documentStoreRevoke src/document-store/revoke.ts:123:11
❯ src/tests/document-store/revoke.test.ts:137:30
[failure] 123-123: src/tests/document-store/revoke.test.ts > Revoke Document > Revoke Document with 'DocumentStore' and ethers version 'v5' > should revoke document with explicit contract type
Error: Pre-check (callStatic) for revoke failed
❯ Module.documentStoreRevoke src/document-store/revoke.ts:123:11
❯ src/tests/document-store/revoke.test.ts:123:30
[failure] 123-123: src/tests/document-store/revoke.test.ts > Revoke Document > Revoke Document with 'DocumentStore' and ethers version 'v5' > should revoke document with explicit contract type
Error: Pre-check (callStatic) for revoke failed
❯ Module.documentStoreRevoke src/document-store/revoke.ts:123:11
❯ src/tests/document-store/revoke.test.ts:123:30
[failure] 123-123: src/tests/document-store/revoke.test.ts > Revoke Document > Revoke Document with 'DocumentStore' and ethers version 'v5' > should revoke document with explicit contract type
Error: Pre-check (callStatic) for revoke failed
❯ Module.documentStoreRevoke src/document-store/revoke.ts:123:11
❯ src/tests/document-store/revoke.test.ts:123:30
[failure] 123-123: src/tests/document-store/revoke.test.ts > Revoke Document > Revoke Document with 'DocumentStore' and ethers version 'v5' > should revoke document with explicit contract type
Error: Pre-check (callStatic) for revoke failed
❯ Module.documentStoreRevoke src/document-store/revoke.ts:123:11
❯ src/tests/document-store/revoke.test.ts:123:30
[failure] 123-123: src/tests/document-store/revoke.test.ts > Revoke Document > Revoke Document with 'DocumentStore' and ethers version 'v5' > should revoke document hash successfully
Error: Pre-check (callStatic) for revoke failed
❯ Module.documentStoreRevoke src/document-store/revoke.ts:123:11
❯ src/tests/document-store/revoke.test.ts:109:24
[failure] 123-123: src/tests/document-store/revoke.test.ts > Revoke Document > Revoke Document with 'DocumentStore' and ethers version 'v5' > should revoke document hash successfully
Error: Pre-check (callStatic) for revoke failed
❯ Module.documentStoreRevoke src/document-store/revoke.ts:123:11
❯ src/tests/document-store/revoke.test.ts:109:24
[failure] 123-123: src/tests/document-store/revoke.test.ts > Revoke Document > Revoke Document with 'DocumentStore' and ethers version 'v5' > should revoke document hash successfully
Error: Pre-check (callStatic) for revoke failed
❯ Module.documentStoreRevoke src/document-store/revoke.ts:123:11
❯ src/tests/document-store/revoke.test.ts:109:24
[failure] 123-123: src/tests/document-store/revoke.test.ts > Revoke Document > Revoke Document with 'DocumentStore' and ethers version 'v5' > should revoke document hash successfully
Error: Pre-check (callStatic) for revoke failed
❯ Module.documentStoreRevoke src/document-store/revoke.ts:123:11
❯ src/tests/document-store/revoke.test.ts:109:24
🤖 Prompt for AI Agents
In `@src/document-store/revoke.ts` around lines 112 - 124, The pre-check
callStatic invocation for ethers v5 is using bracket signature lookup which
doesn't match the test mock; update the v5 branch to call (documentStoreContract
as ContractV5).callStatic.revoke(documentHash) instead of using
callStatic['revoke(bytes32)'], and in the catch block include the original error
when re-throwing (e.g., attach or wrap the caught error) so the thrown Error
preserves the underlying error details; refer to isV6EthersProvider,
documentStoreContract, ContractV5, and callStatic.revoke to locate the change.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/__tests__/document-store/issue.test.ts`:
- Around line 175-183: The test 'should throw when provider is missing' always
constructs a v5 wallet (new WalletV5(...)) so it doesn't cover the v6 path;
update the test to branch on ethersVersion (or the test parameter) and create
the providerless signer appropriately (use WalletV5 for ethersVersion === 'v5'
and the v6 Wallet constructor/class for ethersVersion === 'v6'), then call
documentStoreIssue(mockDocumentStoreAddress, mockDocumentHash,
signerWithoutProvider, { chainId: mockChainId }) and assert it rejects with
'Provider is required'.
🧹 Nitpick comments (2)
src/__tests__/document-store/issue.test.ts (2)
67-76: Wallet creation and spy setup outside lifecycle hooks is fragile.Placing
walletinitialization andvi.spyOncalls in thedescribebody (rather than inbeforeAllorbeforeEach) works becausevi.clearAllMocks()preserves implementations, but this pattern is atypical and can silently break if someone switches tovi.resetAllMocks()orvi.restoreAllMocks()later. Consider moving this setup intobeforeAllalongside the existing mock setup at lines 82–90.
246-256: Test relies on execution order for mock state — consider re-establishing the mock explicitly.Unlike the sibling test at line 229, this test does not re-set
getEthersContractFromProvider. It works only because test 229 (which runs first) restores the mock after the fallback test at line 204 overrides it. Adding the mock setup here would make this test self-contained and resilient to reordering.Proposed fix
it('should handle already issued document', async () => { + const mockContractConstructor = (mockContract: any) => vi.fn(() => mockContract); + vi.mocked(getEthersContractFromProvider).mockReturnValue( + mockContractConstructor(mockContract), + ); mockContract.callStatic.issue.mockRejectedValue(new Error('Document already issued'));
| it('should throw when provider is missing', async () => { | ||
| const signerWithoutProvider = new WalletV5('0x'.padEnd(66, '1')); | ||
|
|
||
| await expect( | ||
| documentStoreIssue(mockDocumentStoreAddress, mockDocumentHash, signerWithoutProvider, { | ||
| chainId: mockChainId, | ||
| }), | ||
| ).rejects.toThrow('Provider is required'); | ||
| }); |
There was a problem hiding this comment.
"Missing provider" test always uses a v5 wallet, even in v6-parameterized runs.
new WalletV5('0x'.padEnd(66, '1')) is hard-coded regardless of ethersVersion. For v6 iterations this doesn't exercise the v6 signer-without-provider path. Consider branching on ethersVersion to create the appropriate providerless wallet.
Proposed fix
it('should throw when provider is missing', async () => {
- const signerWithoutProvider = new WalletV5('0x'.padEnd(66, '1'));
+ const signerWithoutProvider = ethersVersion === 'v5'
+ ? new WalletV5('0x'.padEnd(66, '1'))
+ : new WalletV6('0x'.padEnd(66, '1'));
await expect(
documentStoreIssue(mockDocumentStoreAddress, mockDocumentHash, signerWithoutProvider, {🤖 Prompt for AI Agents
In `@src/__tests__/document-store/issue.test.ts` around lines 175 - 183, The test
'should throw when provider is missing' always constructs a v5 wallet (new
WalletV5(...)) so it doesn't cover the v6 path; update the test to branch on
ethersVersion (or the test parameter) and create the providerless signer
appropriately (use WalletV5 for ethersVersion === 'v5' and the v6 Wallet
constructor/class for ethersVersion === 'v6'), then call
documentStoreIssue(mockDocumentStoreAddress, mockDocumentHash,
signerWithoutProvider, { chainId: mockChainId }) and assert it rejects with
'Provider is required'.
|
## [2.7.0](v2.6.1...v2.7.0) (2026-02-12) ### Features * deploy issue revoke functions ([#128](#128)) ([ba89e9c](ba89e9c))
|
🎉 This PR is included in version 2.7.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
3 participants
PR Summary: Add Document Store Functions
Overview
This PR adds comprehensive document store functionality to the TrustVC library, enabling deployment, issuance, revocation, and role management of document stores on blockchain networks.
Key Features Added
📋 Document Store Deployment
deployDocumentStore()function supporting both ethers v5 and v6📄 Document Management
issueDocument()- Issue documents to the storerevokeDocument()- Revoke issued documents🔐 Role Management
grantRole()- Grant roles to addressesrevokeRole()- Revoke roles from addresses🧪 Comprehensive Testing
Technical Implementation
Files Added:
src/document-store/deploy.ts- Deployment functionalitysrc/document-store/issue.ts- Document issuancesrc/document-store/revoke.ts- Document revocationsrc/document-store/document-store-roles.ts- Role definitionssrc/document-store/contract-interfaces.ts- Contract interfacessrc/document-store/index.ts- Module exportsKey Technical Details:
Breaking Changes
None - this is a new feature addition.
Testing
This implementation provides a complete document store management solution for blockchain applications requiring secure document issuance and management capabilities.
Summary by CodeRabbit
New Features
Documentation
Tests
Chores