Conversation
📝 WalkthroughWalkthroughAdds grant, revoke, and transfer-ownership workflows for DocumentStore contracts (ethers v5/v6), includes ERC‑165 detection with TT fallback, input validation, pre-call static checks, tx option resolution, and extensive unit tests covering success, error, and fallback scenarios. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as Caller
participant Grant as documentStoreGrantRole
participant Check as checkSupportsInterface
participant Contract as Contract
participant TxOpts as getTxOptions
participant Signer as Signer
Caller->>Grant: request grantRole(address, role, account, signer, options)
Grant->>Grant: validate inputs
Grant->>Check: checkSupportsInterface(address, provider)
Check->>Contract: query ERC‑165 interfaces
Contract-->>Check: interface support result
Check-->>Grant: contract type (DocumentStore/Transferable/TT)
Grant->>Contract: callStatic/staticCall.grantRole(role, account)
Contract-->>Grant: pre-check success/failure
Grant->>TxOpts: resolve tx options (chainId, gas)
TxOpts-->>Grant: tx options
Grant->>Signer: send grantRole(role, account, txOptions)
Signer-->>Grant: transaction
Grant-->>Caller: return ContractTransaction
sequenceDiagram
participant Caller as Caller
participant Transfer as documentStoreTransferOwnership
participant GetRole as getRoleString
participant Grant as documentStoreGrantRole
participant Revoke as documentStoreRevokeRole
participant Signer as Signer
Caller->>Transfer: request transferOwnership(address, account, signer, options)
Transfer->>Signer: signer.getAddress()
Transfer->>GetRole: getRoleString(address, "admin")
GetRole-->>Transfer: role string
Transfer->>Grant: documentStoreGrantRole(..., newAccount, signer, options)
Transfer->>Revoke: documentStoreRevokeRole(..., currentOwner, signer, options)
Grant-->>Transfer: grant transaction
Revoke-->>Transfer: revoke transaction
Transfer-->>Caller: { grantTransaction, revokeTransaction }
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts (beta)
No actionable comments were generated in the recent review. 🎉 🧹 Recent nitpick comments
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: 8
🤖 Fix all issues with AI agents
In `@src/__tests__/document-store/grant-role.test.ts`:
- Around line 200-210: The test is asserting the wrong error message; update the
expectation to match the implementation's corrected message that references
grantRole. In the test for grantDocumentStoreRole, change the expected thrown
message from 'Pre-check (callStatic) for issue failed' to the corrected message
(e.g., 'Pre-check (callStatic) for grantRole failed') so it aligns with the
implementation that triggers when mockContract.callStatic.grantRole or
mockContract.grantRole.staticCall rejects; ensure the expectation covers the
exact string produced by grantDocumentStoreRole.
- Around line 68-77: The spies for wallet.getChainId and Provider.getNetwork are
created at describe-evaluation time but cleared by vi.clearAllMocks() in
beforeAll/beforeEach, causing fragile tests; move the wallet instantiation and
vi.spyOn(...) calls into the beforeEach (after clearAllMocks()) so
WalletV5/WalletV6 creation and the vi.spyOn(wallet, 'getChainId') /
vi.spyOn(Provider, 'getNetwork') stubs are re-established for each test,
ensuring code paths that call getChainId/getNetwork (and tests that omit chainId
in options) still have the spies in place.
- Around line 169-182: The test for "Provider missing" always constructs a
WalletV5 signer; change it to mirror revoke-role.test.ts by creating the signer
based on the test's ethersVersion parameter (use WalletV5 when ethersVersion ===
'v5' and WalletV6 when ethersVersion === 'v6') so grantDocumentStoreRole is
exercised for both ethers versions; update the signer variable creation in the
test (the instance passed into grantDocumentStoreRole) to use the conditional
wallet class selection used in revoke-role.test.ts.
- Around line 254-268: The test "should work with different role and account
addresses" is ineffective because differentRole equals the existing mockRole;
update the test to use a genuinely distinct role value so it exercises different
input paths. In the test referencing grantDocumentStoreRole and variables
mockDocumentStoreAddress, mockRole and mockAccount, replace differentRole with a
new hex role string that does not match mockRole (keep differentAccount distinct
from mockAccount as well if needed) so the expectations still assert the call
returns mockTxResponse while actually testing a different role value.
In `@src/document-store/grant-role.ts`:
- Around line 122-125: The catch block in grantRole currently throws a
misleading message ('Pre-check (callStatic) for issue failed'); update the
thrown Error to reference the correct operation (e.g., "Pre-check (callStatic)
for grantRole failed") and include the original error details (or its message)
in the thrown error to aid debugging; locate the catch around the callStatic
pre-check in grantRole and replace the error text accordingly while keeping the
existing console.error(e) logging.
In `@src/document-store/revoke-role.ts`:
- Around line 122-125: The thrown error message in the revokeRole pre-check is
misleading ("for issue"); update the error thrown in the catch block inside
revokeRole (where it currently logs "callStatic failed:" and throws 'Pre-check
(callStatic) for issue failed') to reference revokeRole instead—e.g., throw
'Pre-check (callStatic) for revokeRole failed'—and optionally include the caught
error details in the thrown/logged message to preserve context.
In `@src/document-store/transferOwnership.ts`:
- Around line 17-24: The JSDoc currently says "Revokes a role" but this function
actually transfers ownership; update the comment for
transferOwnershipDocumentStore to accurately describe its behavior: state that
it transfers ownership of a DocumentStore contract (supports both Ethers v5 and
v6 signers), list the three supported store types (DocumentStore ERC‑165,
TransferableDocumentStore ERC‑165, and legacy TT Document Store fallback),
document the documentStoreAddress and newOwner parameters, and note any thrown
errors or return value; keep parameter names (documentStoreAddress, newOwner,
signer) and function name transferOwnershipDocumentStore in the description so
it's clear which function the JSDoc documents.
- Around line 51-65: The code currently starts grantDocumentStoreRole and
revokeDocumentStoreRole in parallel which can consume nonces and cause the owner
to be revoked if the grant fails; change transferOwnership to execute
sequentially by awaiting the result of
grantDocumentStoreRole(documentStoreAddress, roleString, account, signer,
options) first, verify it succeeded (handle/rethrow errors), and only then call
and await revokeDocumentStoreRole(documentStoreAddress, roleString,
ownerAddress, signer, options); update the function’s return type/signature to
return the resolved transaction results (not Promise-wrapped fields) and ensure
errors from the grant abort before the revoke is attempted.
🧹 Nitpick comments (2)
src/document-store/grant-role.ts (1)
23-28: DuplicateIssueOptionsinterface across three files.This same interface is defined identically in
grant-role.ts,revoke-role.ts, andtransferOwnership.ts. Extract it into a shared types file (e.g.,src/document-store/types.ts) and import it from there to keep a single source of truth.src/__tests__/document-store/grant-role.test.ts (1)
59-59:asyncondescribe.eachcallback is unnecessary.The
describe/describe.eachcallback should be synchronous. While it's harmless here (no top-levelawait), it's misleading and could cause subtle issues if someone later adds anawait.Proposed fix
- async ({ Provider, ethersVersion, contractType }) => { + ({ Provider, ethersVersion, contractType }) => {
| let wallet: ethersV5.Wallet | ethersV6.Wallet; | ||
| if (ethersVersion === 'v5') { | ||
| wallet = new WalletV5(PRIVATE_KEY, Provider as any) as ethersV5.Wallet; | ||
| vi.spyOn(wallet, 'getChainId').mockResolvedValue(mockChainId as unknown as number); | ||
| } else { | ||
| wallet = new WalletV6(PRIVATE_KEY, Provider as any); | ||
| vi.spyOn(Provider, 'getNetwork').mockResolvedValue({ | ||
| chainId: mockChainId, | ||
| } as unknown as Network); | ||
| } |
There was a problem hiding this comment.
Spy set at describe-body level is cleared by beforeAll/beforeEach — potential test fragility.
The vi.spyOn(wallet, 'getChainId') (line 71) and vi.spyOn(Provider, 'getNetwork') (line 74) are configured during describe-body evaluation, but vi.clearAllMocks() in both beforeAll (line 84) and beforeEach (line 92) will remove these spies before any test runs. Tests that don't pass chainId in options (e.g., line 134) may fail if the underlying code calls getChainId/getNetwork.
Consider moving the wallet creation and spy setup into beforeEach so they are re-established after each clearAllMocks().
Also applies to: 83-89, 91-103
🤖 Prompt for AI Agents
In `@src/__tests__/document-store/grant-role.test.ts` around lines 68 - 77, The
spies for wallet.getChainId and Provider.getNetwork are created at
describe-evaluation time but cleared by vi.clearAllMocks() in
beforeAll/beforeEach, causing fragile tests; move the wallet instantiation and
vi.spyOn(...) calls into the beforeEach (after clearAllMocks()) so
WalletV5/WalletV6 creation and the vi.spyOn(wallet, 'getChainId') /
vi.spyOn(Provider, 'getNetwork') stubs are re-established for each test,
ensuring code paths that call getChainId/getNetwork (and tests that omit chainId
in options) still have the spies in place.
| } catch (e) { | ||
| console.error('callStatic failed:', e); | ||
| throw new Error('Pre-check (callStatic) for issue failed'); | ||
| } |
There was a problem hiding this comment.
Misleading error message — says "for issue" but this is grantRole.
The error message 'Pre-check (callStatic) for issue failed' appears to be copy-pasted from an issue/revoke document flow. It should reference the actual operation to aid debugging.
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 grantRole failed');
}📝 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.
| } catch (e) { | |
| console.error('callStatic failed:', e); | |
| throw new Error('Pre-check (callStatic) for issue failed'); | |
| } | |
| } catch (e) { | |
| console.error('callStatic failed:', e); | |
| throw new Error('Pre-check (callStatic) for grantRole failed'); | |
| } |
🤖 Prompt for AI Agents
In `@src/document-store/grant-role.ts` around lines 122 - 125, The catch block in
grantRole currently throws a misleading message ('Pre-check (callStatic) for
issue failed'); update the thrown Error to reference the correct operation
(e.g., "Pre-check (callStatic) for grantRole failed") and include the original
error details (or its message) in the thrown error to aid debugging; locate the
catch around the callStatic pre-check in grantRole and replace the error text
accordingly while keeping the existing console.error(e) logging.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/__tests__/document-store/grant-role.test.ts`:
- Around line 214-231: The test for grantDocumentStoreRole wrongly injects
mockContract into the mocked getEthersContractFromProvider instead of the
TT-specific mock; change the mockReturnValue in this test to use
mockTTDocumentStoreContract (the same pattern used in revoke-role.test.ts) so
the TT Document Store path is exercised, and after the test restore the original
mocked implementation of getEthersContractFromProvider to avoid leaking into
later tests.
In `@src/document-store/revoke-role.ts`:
- Around line 23-28: Extract the duplicated IssueOptions interface into a single
shared module (e.g., create src/document-store/types.ts exporting interface
IssueOptions) and replace the local definitions in transferOwnership.ts,
grant-role.ts, revoke-role.ts, and issue.ts with an import from that shared
module; then remove the dead/unreachable error check that references
isDocumentStore, isTransferableDocumentStore, and isTTDocumentStore in
revoke-role.ts (the unreachable block around lines 87–91) so the file no longer
contains that redundant runtime check.
🧹 Nitpick comments (8)
src/__tests__/document-store/grant-role.test.ts (3)
59-59:asynccallback ondescribe.eachis unnecessary and potentially misleading.The
describefactory callback should be synchronous. While this happens to work here (noawaitinside the body, so it runs synchronously), theasynckeyword is misleading —describeignores the returned Promise. This same pattern appears inrevoke-role.test.tsline 59.Proposed fix
- async ({ Provider, ethersVersion, contractType }) => { + ({ Provider, ethersVersion, contractType }) => {
2-2:beforeAllis used but not imported from vitest.Line 2 imports
{ describe, it, expect, beforeEach, vi }butbeforeAll(used at line 74) is not included. It works via vitest globals, but this is inconsistent with the explicit import pattern in this file. The same issue exists inrevoke-role.test.ts.Proposed fix
-import { describe, it, expect, beforeEach, vi } from 'vitest'; +import { describe, it, expect, beforeAll, beforeEach, vi } from 'vitest';Also applies to: 74-74
74-79:beforeAllsets mock forgetEthersContractFromProvider, butbeforeEachcallsvi.clearAllMocks()— mock survives but the pattern is fragile.
vi.clearAllMocks()clears call history but preservesmockReturnValueimplementations, so thebeforeAllmock technically persists. However, test"should fallback to TT Document Store..."(line 216) re-mocksgetEthersContractFromProvidermid-test, and there's no restoration afterward — subsequent tests in the same describe block may use the wrong mock.Consider moving the
getEthersContractFromProvidermock setup intobeforeEachso it's consistently re-established after each test and after eachclearAllMocks()call.Also applies to: 81-103
src/__tests__/document-store/revoke-role.test.ts (1)
2-2: Same minor issues asgrant-role.test.ts: missingbeforeAllimport andasyncdescribe callback.Line 2 is missing the
beforeAllimport, and line 59 uses an unnecessaryasyncon thedescribe.eachcallback. See the corresponding comments ongrant-role.test.tsfor details and proposed fixes.Also applies to: 59-59, 74-74
src/document-store/revoke-role.ts (3)
87-91: Unreachable error — this branch can never execute.When
isTransferableis explicitlytrueorfalse,isDocumentStoreorisTransferableDocumentStoreis set accordingly. WhenisTransferableisundefined, the ERC-165 detection runs and either finds a match or setsisTTDocumentStore = true. In all paths, at least one of the three flags istrueby the time line 87 is reached.This dead code exists in
grant-role.tsas well. Consider removing it or, if kept as a defensive guard, adding a comment explaining it's a safety net.
47-131: Heavy code duplication withgrant-role.ts— nearly identical structure.The entire function body (input validation, interface detection, ABI selection, contract construction, callStatic pre-check, tx options, and final call) is a copy of
grantDocumentStoreRolewith only the method name swapped (grantRole→revokeRole). This pattern will likely grow if more role operations are added (e.g.,renounceRole).Consider extracting a shared helper that encapsulates the common logic (validation, contract type detection, contract instantiation, pre-check, tx options) and accepts the method name as a parameter.
122-125: Original error context is lost in the re-thrown error.The caught error
eis logged toconsole.errorbut not included in the newError. Callers only see "Pre-check (callStatic) for revoke-role failed" without the underlying cause (e.g., revert reason, permission error). Consider chaining the original error.Proposed fix
} catch (e) { console.error('callStatic failed:', e); - throw new Error('Pre-check (callStatic) for revoke-role failed'); + throw new Error('Pre-check (callStatic) for revoke-role failed', { cause: e }); }src/document-store/grant-role.ts (1)
23-28: Same structural issues asrevoke-role.ts— duplicateIssueOptions, unreachable guard, heavy duplication, lost error context.All observations from the
revoke-role.tsreview apply identically here:
- Duplicate
IssueOptions(lines 23–28) — extract to a shared types module.- Unreachable error (lines 87–91) — dead code that can never execute.
- Heavy duplication with
revoke-role.ts— consider a shared helper parameterized by method name.- Lost error context (lines 122–125) — chain the original error with
{ cause: e }.See the detailed comments on
revoke-role.tsfor proposed fixes.Also applies to: 47-131
| it('should fallback to TT Document Store when ERC-165 interfaces not supported', async () => { | ||
| vi.spyOn(coreModule, 'checkSupportsInterface').mockResolvedValue(false); | ||
| const mockContractConstructor = (mockContract: any) => vi.fn(() => mockContract); | ||
| vi.mocked(getEthersContractFromProvider).mockReturnValue( | ||
| mockContractConstructor(mockContract), | ||
| ); | ||
| const result = await grantDocumentStoreRole( | ||
| mockDocumentStoreAddress, | ||
| mockRole, | ||
| mockAccount, | ||
| wallet, | ||
| { | ||
| chainId: mockChainId, | ||
| }, | ||
| ); | ||
| expect(result).toBeDefined(); | ||
| expect(coreModule.checkSupportsInterface).toHaveBeenCalledTimes(2); | ||
| }); |
There was a problem hiding this comment.
TT fallback test within parametrized block uses mockContract instead of mockTTDocumentStoreContract.
Line 218 passes the outer mockContract (DocumentStore or TransferableDocumentStore mock) to the contract constructor mock, but this test intends to verify TT Document Store fallback behavior. Compare with the equivalent test in revoke-role.test.ts (line 219) which correctly uses mockTTDocumentStoreContract.
The test still passes because assertions are weak (toBeDefined() + call count), but it doesn't truly validate the TT contract path. The dedicated TT section (lines 274+) covers this properly, so this is a test accuracy issue rather than a missing-coverage gap.
Proposed fix to match revoke-role.test.ts
vi.spyOn(coreModule, 'checkSupportsInterface').mockResolvedValue(false);
const mockContractConstructor = (mockContract: any) => vi.fn(() => mockContract);
vi.mocked(getEthersContractFromProvider).mockReturnValue(
- mockContractConstructor(mockContract),
+ mockContractConstructor(mockTTDocumentStoreContract),
);Also consider restoring the original mock afterward (as revoke-role does at lines 232–234) to prevent leaking into subsequent tests.
📝 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.
| it('should fallback to TT Document Store when ERC-165 interfaces not supported', async () => { | |
| vi.spyOn(coreModule, 'checkSupportsInterface').mockResolvedValue(false); | |
| const mockContractConstructor = (mockContract: any) => vi.fn(() => mockContract); | |
| vi.mocked(getEthersContractFromProvider).mockReturnValue( | |
| mockContractConstructor(mockContract), | |
| ); | |
| const result = await grantDocumentStoreRole( | |
| mockDocumentStoreAddress, | |
| mockRole, | |
| mockAccount, | |
| wallet, | |
| { | |
| chainId: mockChainId, | |
| }, | |
| ); | |
| expect(result).toBeDefined(); | |
| expect(coreModule.checkSupportsInterface).toHaveBeenCalledTimes(2); | |
| }); | |
| it('should fallback to TT Document Store when ERC-165 interfaces not supported', async () => { | |
| vi.spyOn(coreModule, 'checkSupportsInterface').mockResolvedValue(false); | |
| const mockContractConstructor = (mockContract: any) => vi.fn(() => mockContract); | |
| vi.mocked(getEthersContractFromProvider).mockReturnValue( | |
| mockContractConstructor(mockTTDocumentStoreContract), | |
| ); | |
| const result = await grantDocumentStoreRole( | |
| mockDocumentStoreAddress, | |
| mockRole, | |
| mockAccount, | |
| wallet, | |
| { | |
| chainId: mockChainId, | |
| }, | |
| ); | |
| expect(result).toBeDefined(); | |
| expect(coreModule.checkSupportsInterface).toHaveBeenCalledTimes(2); | |
| }); |
🤖 Prompt for AI Agents
In `@src/__tests__/document-store/grant-role.test.ts` around lines 214 - 231, The
test for grantDocumentStoreRole wrongly injects mockContract into the mocked
getEthersContractFromProvider instead of the TT-specific mock; change the
mockReturnValue in this test to use mockTTDocumentStoreContract (the same
pattern used in revoke-role.test.ts) so the TT Document Store path is exercised,
and after the test restore the original mocked implementation of
getEthersContractFromProvider to avoid leaking into later tests.
| export interface IssueOptions { | ||
| chainId?: CHAIN_ID; | ||
| maxFeePerGas?: GasValue; | ||
| maxPriorityFeePerGas?: GasValue; | ||
| isTransferable?: boolean; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
rg -n 'export interface IssueOptions' --type=tsRepository: TrustVC/trustvc
Length of output: 333
🏁 Script executed:
#!/bin/bash
# Extract IssueOptions interface from each file
echo "=== transferOwnership.ts ==="
sed -n '10,17p' src/document-store/transferOwnership.ts
echo ""
echo "=== grant-role.ts ==="
sed -n '23,30p' src/document-store/grant-role.ts
echo ""
echo "=== revoke-role.ts ==="
sed -n '23,30p' src/document-store/revoke-role.ts
echo ""
echo "=== issue.ts ==="
sed -n '40,47p' src/document-store/issue.tsRepository: TrustVC/trustvc
Length of output: 787
🏁 Script executed:
sed -n '60,95p' src/document-store/revoke-role.tsRepository: TrustVC/trustvc
Length of output: 1255
IssueOptions interface duplicated across 4 files — consolidate to a shared module.
The identical IssueOptions interface is defined in src/document-store/transferOwnership.ts, src/document-store/grant-role.ts, src/document-store/revoke-role.ts, and src/document-store/issue.ts. Extract it to a shared location (e.g., src/document-store/types.ts) and import it in all four modules.
Additionally, remove the unreachable error check at lines 87–91. This code is dead — at least one of isDocumentStore, isTransferableDocumentStore, or isTTDocumentStore will always be true by the time the check executes.
🤖 Prompt for AI Agents
In `@src/document-store/revoke-role.ts` around lines 23 - 28, Extract the
duplicated IssueOptions interface into a single shared module (e.g., create
src/document-store/types.ts exporting interface IssueOptions) and replace the
local definitions in transferOwnership.ts, grant-role.ts, revoke-role.ts, and
issue.ts with an import from that shared module; then remove the
dead/unreachable error check that references isDocumentStore,
isTransferableDocumentStore, and isTTDocumentStore in revoke-role.ts (the
unreachable block around lines 87–91) so the file no longer contains that
redundant runtime check.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/document-store/revoke-role.ts`:
- Around line 1-5: The import uses the wrong ethers v6 type: replace the
imported symbol ContractTransaction (aliased ContractTransactionV6) with
ContractTransactionResponse (aliased e.g. ContractTransactionResponseV6) and
update any return type annotations or variables in revokeRole (and mirror the
same fix in grantRole) to use ContractTransactionResponse instead of
ContractTransaction so signatures match ethers v6 write method returns; adjust
the import line that currently contains "ContractTransaction as
ContractTransactionV6" and any references to ContractTransactionV6 in functions
like revokeRole to the new ContractTransactionResponse alias.
In `@src/document-store/transferOwnership.ts`:
- Around line 30-33: The return type of transferOwnership is incorrect: you
await grantTransaction and revokeTransaction (in the transferOwnership body), so
the signature should not declare Promise<ContractTransactionV5 |
ContractTransactionV6> for those fields. Update the transferOwnership return
type to return the resolved transaction types (e.g., grantTransaction:
ContractTransactionV5 | ContractTransactionV6; revokeTransaction:
ContractTransactionV5 | ContractTransactionV6) and ensure the function returns
the awaited results (the resolved transaction objects) rather than
Promise-wrapped values; keep references to the symbols grantTransaction,
revokeTransaction and the transferOwnership function when making the change.
- Around line 38-39: The call to getRoleString in transferOwnership.ts is
missing the required provider/chainId option and will throw; update the call to
pass the signer.provider (e.g., getRoleString(documentStoreAddress, 'admin', {
provider: signer.provider })) so getRoleString receives a provider; locate the
usage in transferOwnership.ts where ownerAddress and roleString are set and add
the provider option to the getRoleString invocation.
In `@src/document-store/types.ts`:
- Line 1: The import in src/document-store/types.ts uses a non-relative path
("src/utils") which can break without TS path aliases; change the import to a
relative path (e.g., import { CHAIN_ID } from '../utils' or the correct relative
path from document-store to the utils module) and update any other similar
absolute 'src/...' imports in this folder to match existing relative-import
style; then run a quick search for "from 'src/" to ensure consistency.
In `@src/index.ts`:
- Around line 24-25: The new function transferOwnershipDocumentStore is not
exported from the document-store public API; either export it from the
document-store barrel (add transferOwnershipDocumentStore to the exports in the
document-store index) and then import + re-export it alongside
grantDocumentStoreRole and revokeDocumentStoreRole in the top-level index, or if
it is intentionally internal, add a brief comment next to
transferOwnershipDocumentStore explaining why it should not be exported (e.g.,
unstable/internal) so reviewers know this omission is deliberate.
🧹 Nitpick comments (3)
src/document-store/index.ts (1)
1-5:CommandOptionstype is not re-exported — consumers cannot type the options parameter.External consumers calling
grantDocumentStoreRoleorrevokeDocumentStoreRolehave no way to import theCommandOptionstype to properly type theoptionsargument. Consider addingexport type { CommandOptions } from './types';.Proposed fix
export { grantDocumentStoreRole } from './grant-role'; export { deployDocumentStore } from './deploy'; +export type { CommandOptions } from './types';src/document-store/revoke-role.ts (1)
39-118: Massive code duplication withgrant-role.ts.The entire contract-type detection, ABI selection, contract instantiation, static-call pre-check, and tx-options logic is duplicated verbatim across
grant-role.ts,revoke-role.ts, andissue.ts. Only the contract method name differs (grantRole/revokeRole/issue). Consider extracting a shared helper that handles detection, ABI selection, contract creation, and tx options, accepting the method name and args as parameters.src/document-store/transferOwnership.ts (1)
50-56: Dead code: truthy checks on transaction results will never be false.
grantDocumentStoreRoleandrevokeDocumentStoreRoleeither return aContractTransaction(always truthy) or throw an error. Theif (!grantTransactionResult)andif (!revokeTransactionResult)checks will never trigger — the thrown errors from the inner functions already handle failure. Remove these dead checks.Simplified flow
- const grantTransaction = grantDocumentStoreRole( + const grantTransaction = await grantDocumentStoreRole( documentStoreAddress, roleString, account, signer, options, ); - //check if the grant transaction is successful - const grantTransactionResult = await grantTransaction; - if (!grantTransactionResult) { - //add custom error message not proceeding eith the revoke transaction - throw new Error('Grant transaction failed, not proceeding with revoke transaction'); - //return the grant transaction result - } - //call revoke function here - const revokeTransaction = revokeDocumentStoreRole( + + const revokeTransaction = await revokeDocumentStoreRole( documentStoreAddress, roleString, ownerAddress, signer, options, ); - //check if the revoke transaction is successful - const revokeTransactionResult = await revokeTransaction; - if (!revokeTransactionResult) { - throw new Error('Revoke transaction failed'); - //return the revoke transaction result - } + return { grantTransaction, revokeTransaction };Also applies to: 65-70
| import { | ||
| Signer as SignerV6, | ||
| Contract as ContractV6, | ||
| ContractTransaction as ContractTransactionV6, | ||
| } from 'ethersV6'; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check imports in the three files mentioned
echo "=== revoke-role.ts imports ==="
head -10 src/document-store/revoke-role.ts
echo -e "\n=== grant-role.ts imports ==="
head -10 src/document-store/grant-role.ts
echo -e "\n=== issue.ts imports ==="
head -10 src/document-store/issue.tsRepository: TrustVC/trustvc
Length of output: 886
🌐 Web query:
ethers v6 ContractTransactionResponse write method return type
💡 Result:
In ethers v6, a state-changing (“write”) contract method (i.e., nonpayable / payable) returns:
Promise<ContractTransactionResponse>(this is whatBaseContractMethod.send(...)returns) [1][2]
Then:
ContractTransactionResponse.wait(...)returnsPromise<null | ContractTransactionReceipt>[1][2]
Example (TypeScript):
const tx: ethers.ContractTransactionResponse = await contract.setValue(123); // write
const receipt: ethers.ContractTransactionReceipt | null = await tx.wait();Sources: [1] [2]
🏁 Script executed:
# Check function return types in revoke-role.ts
echo "=== revoke-role.ts function signatures using ContractTransactionV6 ==="
cat -n src/document-store/revoke-role.ts | grep -A 5 "ContractTransactionV6"
echo -e "\n=== issue.ts function signatures using ContractTransactionV6 ==="
cat -n src/document-store/issue.ts | grep -A 5 "ContractTransactionV6"Repository: TrustVC/trustvc
Length of output: 2231
Wrong ethers v6 type: ContractTransaction should be ContractTransactionResponse.
In ethers v6, write methods on Contract return ContractTransactionResponse, not ContractTransaction. This file incorrectly imports ContractTransaction on line 4, while src/document-store/issue.ts correctly imports ContractTransactionResponse. The wrong type causes the return type annotation to be inaccurate and will produce type errors for code expecting ContractTransactionResponse methods.
The same issue exists in src/document-store/grant-role.ts (line 4).
Proposed fix for revoke-role.ts
import {
Signer as SignerV6,
Contract as ContractV6,
- ContractTransaction as ContractTransactionV6,
+ ContractTransactionResponse as ContractTransactionV6,
} from 'ethersV6';🤖 Prompt for AI Agents
In `@src/document-store/revoke-role.ts` around lines 1 - 5, The import uses the
wrong ethers v6 type: replace the imported symbol ContractTransaction (aliased
ContractTransactionV6) with ContractTransactionResponse (aliased e.g.
ContractTransactionResponseV6) and update any return type annotations or
variables in revokeRole (and mirror the same fix in grantRole) to use
ContractTransactionResponse instead of ContractTransaction so signatures match
ethers v6 write method returns; adjust the import line that currently contains
"ContractTransaction as ContractTransactionV6" and any references to
ContractTransactionV6 in functions like revokeRole to the new
ContractTransactionResponse alias.
| grantDocumentStoreRole, | ||
| revokeDocumentStoreRole, |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Search for all occurrences of transferOwnershipDocumentStore
rg -n 'transferOwnershipDocumentStore' --type=tsRepository: TrustVC/trustvc
Length of output: 155
🏁 Script executed:
# Check the content of src/index.ts around the mentioned lines
cat -n src/index.ts | sed -n '1,70p'Repository: TrustVC/trustvc
Length of output: 2026
🏁 Script executed:
# Check src/document-store/index.ts for exports
cat -n src/document-store/index.tsRepository: TrustVC/trustvc
Length of output: 550
transferOwnershipDocumentStore is not exported from the public API.
The new transferOwnershipDocumentStore function in src/document-store/transferOwnership.ts is neither exported from src/document-store/index.ts nor re-exported from src/index.ts. If this is intentional (e.g., not ready for public use), add a comment explaining why. Otherwise, export it from src/document-store/index.ts and add it to the imports and re-exports in src/index.ts alongside the other document-store functions.
🤖 Prompt for AI Agents
In `@src/index.ts` around lines 24 - 25, The new function
transferOwnershipDocumentStore is not exported from the document-store public
API; either export it from the document-store barrel (add
transferOwnershipDocumentStore to the exports in the document-store index) and
then import + re-export it alongside grantDocumentStoreRole and
revokeDocumentStoreRole in the top-level index, or if it is intentionally
internal, add a brief comment next to transferOwnershipDocumentStore explaining
why it should not be exported (e.g., unstable/internal) so reviewers know this
omission is deliberate.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/__tests__/document-store/revoke-role.test.ts`:
- Around line 57-59: The describe.each callback is declared async which is a
no-op and can swallow rejected promises; remove the async keyword from the
describe.each callback (the function passed to describe.each in the test titled
'Revoke role with $contractType and ethers version $ethersVersion') and move any
awaited setup/teardown into proper async lifecycle hooks (e.g., beforeAll,
beforeEach, afterAll) or make individual tests (it/test) async so awaits are
honored; ensure any async initialization previously done at describe scope is
relocated to a beforeAll that returns/awaits the promise.
In `@src/document-store/transferOwnership.ts`:
- Around line 30-33: The function transferOwnership currently returns raw
Promise objects (grantTransaction/revokeTransaction) and contains dead
null-checks for documentStoreGrantRole and documentStoreRevokeRole; change it to
await the promises and return the resolved ContractTransaction
(ContractTransactionV5 | ContractTransactionV6) values instead of Promise<...>,
and remove the unreachable if (!result) guards because documentStoreGrantRole
and documentStoreRevokeRole throw on failure; specifically, in transferOwnership
await the calls that produce grantTransaction and revokeTransaction, assign the
resolved results to variables (e.g., grantTx, revokeTx), drop the null-checks
referencing documentStoreGrantRole/documentStoreRevokeRole, and update the
function return type to return the resolved transactions (not Promise<...>).
🧹 Nitpick comments (6)
src/document-store/grant-role.ts (2)
108-111: Original error details are swallowed.The caught error
eis logged to console but not included in the thrown error. Callers lose the root cause (e.g., "AccessControl: account 0x… is missing role 0x…"). Consider chaining the original error.Proposed fix
} catch (e) { console.error('callStatic failed:', e); - throw new Error('Pre-check (callStatic) for grant-role failed'); + throw new Error('Pre-check (callStatic) for grant-role failed', { cause: e }); }
39-118: Near-complete code duplication withrevoke-role.ts.
grant-role.tsandrevoke-role.tsare identical except for the method name (grantRolevsrevokeRole) and error strings. Consider extracting the shared logic (validation, contract-type detection, ABI selection, contract instantiation, pre-check, tx options) into a common helper, with the method name as a parameter.src/document-store/revoke-role.ts (1)
108-111: Original error details are swallowed — same asgrant-role.ts.Consider chaining the original error via
{ cause: e }so callers can inspect the root cause.Proposed fix
} catch (e) { console.error('callStatic failed:', e); - throw new Error('Pre-check (callStatic) for revoke-role failed'); + throw new Error('Pre-check (callStatic) for revoke-role failed', { cause: e }); }src/document-store/transferOwnership.ts (1)
42-58: Noisy and stale comments reduce readability.Lines like
//call the transferOwnership function of the document store contract,//call grant and revoke function here,//add custom error message not proceeding eith the revoke transaction(typo: "eith"), and//return the grant transaction result(after athrow) are low-value or misleading. Clean these up.src/__tests__/document-store/revoke-role.test.ts (2)
74-80:beforeAllsets up the contract mock, butbeforeEachcallsvi.clearAllMocks()— verify this doesn't reset the mock implementation.
beforeAll(line 74) configuresgetEthersContractFromProviderviamockReturnValue, thenbeforeEach(line 83) callsvi.clearAllMocks(). In vitest,clearAllMocksclears recorded call data but should preserve mock implementations — however, this pattern is fragile and easy to break if someone switches toresetAllMocks. Consider moving the contract constructor mock intobeforeEachfor clarity and safety.Also applies to: 82-83
215-235: Fallback test restores mock inside the test body — brittle pattern.Lines 232–234 re-mock
getEthersContractFromProviderback to the non-TT contract at the end of the test. This cleanup is fragile — if the test fails before reaching line 232, subsequent tests in thisdescribeblock could use the wrong mock. ThebeforeEach/afterEachhooks should handle mock restoration.
| ): Promise<{ | ||
| grantTransaction: Promise<ContractTransactionV5 | ContractTransactionV6>; | ||
| revokeTransaction: Promise<ContractTransactionV5 | ContractTransactionV6>; | ||
| }> => { |
There was a problem hiding this comment.
Return type is misleading and the null-checks are dead code.
Two related issues:
-
Return type mismatch: The function returns the raw Promise objects (
grantTransaction,revokeTransaction) rather than the awaited results. This forces callers toawaitalready-resolved promises, which is confusing. Return the resolved values instead. -
Dead null-checks (lines 54, 69):
documentStoreGrantRoleanddocumentStoreRevokeRolethrow on failure — they never return a falsy value. Theseif (!result)guards are unreachable dead code.
Proposed fix
): Promise<{
- grantTransaction: Promise<ContractTransactionV5 | ContractTransactionV6>;
- revokeTransaction: Promise<ContractTransactionV5 | ContractTransactionV6>;
+ grantTransaction: ContractTransactionV5 | ContractTransactionV6;
+ revokeTransaction: ContractTransactionV5 | ContractTransactionV6;
}> => {
if (!documentStoreAddress) throw new Error('Document store address is required');
if (!signer.provider) throw new Error('Provider is required');
if (!account) throw new Error('Account is required');
const ownerAddress = await signer.getAddress();
const roleString = await getRoleString(documentStoreAddress, 'admin', {
provider: signer.provider,
});
- //call the transferOwnership function of the document store contract
- //call grant and revoke function here
- const grantTransaction = documentStoreGrantRole(
+ const grantTransaction = await documentStoreGrantRole(
documentStoreAddress,
roleString,
account,
signer,
options,
);
- //check if the grant transaction is successful
- const grantTransactionResult = await grantTransaction;
- if (!grantTransactionResult) {
- //add custom error message not proceeding eith the revoke transaction
- throw new Error('Grant transaction failed, not proceeding with revoke transaction');
- //return the grant transaction result
- }
- //call revoke function here
- const revokeTransaction = documentStoreRevokeRole(
+
+ const revokeTransaction = await documentStoreRevokeRole(
documentStoreAddress,
roleString,
ownerAddress,
signer,
options,
);
- //check if the revoke transaction is successful
- const revokeTransactionResult = await revokeTransaction;
- if (!revokeTransactionResult) {
- throw new Error('Revoke transaction failed');
- //return the revoke transaction result
- }
+
return { grantTransaction, revokeTransaction };
};Also applies to: 45-73
🤖 Prompt for AI Agents
In `@src/document-store/transferOwnership.ts` around lines 30 - 33, The function
transferOwnership currently returns raw Promise objects
(grantTransaction/revokeTransaction) and contains dead null-checks for
documentStoreGrantRole and documentStoreRevokeRole; change it to await the
promises and return the resolved ContractTransaction (ContractTransactionV5 |
ContractTransactionV6) values instead of Promise<...>, and remove the
unreachable if (!result) guards because documentStoreGrantRole and
documentStoreRevokeRole throw on failure; specifically, in transferOwnership
await the calls that produce grantTransaction and revokeTransaction, assign the
resolved results to variables (e.g., grantTx, revokeTx), drop the null-checks
referencing documentStoreGrantRole/documentStoreRevokeRole, and update the
function return type to return the resolved transactions (not Promise<...>).
|
## [2.8.0](v2.7.0...v2.8.0) (2026-02-13) ### Features * role transfer function ([#129](#129)) ([c600562](c600562))
|
🎉 This PR is included in version 2.8.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
3 participants
Summary
Added comprehensive role management functionality for Document Store contracts, enabling secure access control for document issuance, revocation, and administrative operations.
New Functions Added
Core Role Functions
Supported Contract Types
Summary by CodeRabbit
New Features
Behavior Changes
Tests