Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughMoves DocumentStore deploy implementation from Changes
Sequence DiagramsequenceDiagram
participant User
participant SDK
participant Validator
participant Detector
participant Utils
participant Deployer
participant Factory
participant Signer
participant Chain
rect rgba(200,200,255,0.5)
User->>SDK: deployTokenRegistry(name,symbol,signer,opts)
end
SDK->>Validator: validate inputs (name,symbol,addresses)
SDK->>Detector: detect signer version & chainId
Detector-->>SDK: signerV5|signerV6, chainId
SDK->>Utils: resolve txOptions & default addresses
alt quick-start
SDK->>Deployer: connect & encode init params
Deployer->>Signer: send deploy tx (deploy(params, txOptions))
Signer->>Chain: broadcast tx
Chain-->>Signer: return receipt
else standalone
SDK->>Factory: create ContractFactory (v5/v6)
Factory->>Signer: deploy implementation (constructor args, txOptions)
Signer->>Chain: broadcast tx
Chain-->>Signer: return txHash/receipt
SDK->>Signer: waitFor deployment receipt (v5/v6 specific)
end
SDK-->>User: return contract address & receipt
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (4)
src/token-registry-functions/types.ts (1)
116-117:Partial<PrivateKeyOption>neutralizes the mutual exclusivity constraint.
PrivateKeyOptionuses a discriminated union withneverto enforce that only one ofkeyorkeyFileis provided. Wrapping it inPartial<>makes both fields optional (including thenever-typed ones becomingundefined), which allows bothkeyandkeyFileto be set simultaneously.If the intent is to preserve mutual exclusivity, use
PrivateKeyOptiondirectly instead ofPartial<PrivateKeyOption>.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/token-registry-functions/types.ts` around lines 116 - 117, The type NetworkAndWalletSignerOption currently uses Partial<PrivateKeyOption>, which defeats the discriminated-union mutual-exclusivity enforced by PrivateKeyOption; replace Partial<PrivateKeyOption> with PrivateKeyOption (keeping NetworkOption & (Partial<WalletOption> | PrivateKeyOption)) so the discriminant and never fields in PrivateKeyOption remain effective and prevent both key and keyFile being set simultaneously; update any related imports or references if needed to reflect the stronger type.src/deploy/token-registry.ts (1)
188-241: Standalone mode lacks error handling around deployment — inconsistent withdeployDocumentStore.The
deployDocumentStorefunction wraps deployment in atry/catchwith a descriptive error message (lines 128–153 indocument-store.ts). The standalone path here has no similar protection. A failed deployment will propagate a raw ethers error.Consider wrapping for consistency, or at minimum document the different error-handling strategy.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/deploy/token-registry.ts` around lines 188 - 241, Wrap the standalone deployment block in a try/catch similar to deployDocumentStore: surround the creation and deploy calls for tokenFactory/ContractFactoryV5/ContractFactoryV6 (references: getEthersContractFactoryFromProvider, tokenFactory, TradeTrustToken__factory, isV6EthersProvider, getTxOptions, deploy, deploymentTransaction, deployTransaction) with a try, catch the thrown error, and rethrow a new Error that includes a clear contextual message like "Failed to deploy Token Registry" plus the original error message/details to preserve diagnostics; ensure both v5 and v6 branches are inside the try so any failure is caught and logged-consistently.src/token-registry-functions/utils.ts (1)
8-26:getTxOptionsparameter types don't reflect actual usage —chainIdand gas values can beundefined.Callers (e.g.,
document-store.tsline 108,token-registry.tsline 220) pass potentially-undefinedchainIdand gas values. The function handles this at runtime (lines 15-16 fallback forchainId, line 25 check for gas values), but the signature declares them as required. This creates a type-safety gap that will silently passundefinedthrough unless strict null checks are enforced.Consider making these parameters explicitly optional to match actual usage.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/token-registry-functions/utils.ts` around lines 8 - 26, The function getTxOptions currently accepts required parameters but treats chainId, maxFeePerGas and maxPriorityFeePerGas as possibly undefined; change its signature to make chainId?: CHAIN_ID, maxFeePerGas?: GasValue and maxPriorityFeePerGas?: GasValue (keeping signer: SignerV6 | Signer required), update any internal casts/usages (e.g., the getChainIdSafe fallback and SUPPORTED_CHAINS lookup) to rely on the now-optional types, and adjust any callers or their passed arguments if they assume non-null inputs so the type system reflects actual runtime behavior.src/__tests__/token-registry-functions/deploy.test.ts (1)
101-103: Remove commented-out code.These leftover commented-out import statements (also at lines 175-177) appear to be development artifacts. They add noise without value.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/token-registry-functions/deploy.test.ts` around lines 101 - 103, Remove the leftover commented-out import statements referencing getEthersContractFactoryFromProvider and isV6EthersProvider (the lines importing from '../../utils/ethers/index.js') in the test file; simply delete those commented lines at both occurrences (near the shown diff and the other instance around lines 175-177) to eliminate noise and keep the test file clean.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/token-registry-functions/deploy.test.ts`:
- Around line 371-408: The test fails because getChainIdSafe is not mocked and
the test still passes chainId so auto-detection in deployTokenRegistry is never
exercised; update the test fixtures to export a mocked getChainIdSafe (e.g., add
getChainIdSafe: vi.fn() to the vi.mock('../../token-registry-functions/utils',
...) setup in fixtures.ts) and change the test in deploy.test.ts to call
deployTokenRegistry without passing the chainId option (remove { chainId:
mockChainId } so deployTokenRegistry triggers the !chainId branch and the mocked
getChainIdSafe can be used).
In `@src/__tests__/token-registry-functions/fixtures.ts`:
- Around line 20-34: The failing CI is due to the mock for the utils module
missing getChainIdSafe, so tests call the real function and then try to use
.mockResolvedValue; add a mocked getChainIdSafe to the vi.mock return object
(alongside getDefaultContractAddress, isSupportedTitleEscrowFactory,
isValidAddress) as vi.fn().mockResolvedValue(<number>)—use the expected test
chain id (e.g., 31337 or 1) so deploy.test.ts can call
getChainIdSafe.mockResolvedValue without error.
In `@src/deploy/document-store.ts`:
- Line 137: Call to contract.deploymentTransaction().wait() lacks a null guard;
change the call to mirror the token-registry pattern by using optional chaining
or an explicit null check before awaiting the transaction (e.g., use
contract.deploymentTransaction()?.wait() or check the result of
contract.deploymentTransaction() and handle null), updating the deployment logic
that uses contract.deploymentTransaction() so it safely handles the null |
ContractTransactionResponse signature.
In `@src/deploy/token-registry.ts`:
- Around line 228-230: The call to contract.deploymentTransaction()?.wait() can
yield undefined if deploymentTransaction() returns null/undefined, violating the
declared Promise<TransactionReceipt> return type; update the function (around
contract.deploymentTransaction and its usage of wait) to explicitly check the
result of contract.deploymentTransaction(), throw or return a rejected Promise
with a clear error if it's null/undefined, and only call .wait() on a non-null
transaction so the function always resolves/rejects with a TransactionReceipt or
an error.
- Around line 154-169: In deployTokenRegistry add an explicit guard that
validates signer.provider before it's used: check if signer.provider is falsy
(similar to deployDocumentStore's check) and throw a clear error (e.g., "Signer
has no provider, cannot deploy TokenRegistry") before calling
getEthersContractFromProvider; this ensures
getEthersContractFromProvider(signer.provider) and subsequent usage in
getEthersContractFromProvider, deployerContract creation and
TDocDeployer__factory interactions are safe.
- Around line 186-187: The quick-start branch is returning the raw
ContractTransaction/ContractTransactionResponse from
deployerContract.deploy(...) instead of a TransactionReceipt; change it to
capture the deploy call result and await its receipt: call
deployerContract.deploy(tokenRegistryImplAddress, initParam, txOptions) into a
variable, then if that result has a wait method (handle both ethers v5/v6
shapes) call await result.wait() and return that receipt; if the result is
already a receipt, return it as-is. Ensure you reference
deployerContract.deploy, tokenRegistryImplAddress, initParam, and txOptions when
implementing this change.
In `@src/token-registry-functions/ownerOf.ts`:
- Around line 47-68: The code instantiates contracts with new Contract(...) and
declares tradeTrustTokenContract with if (isV5TT) / else if (isV4TT) leaving a
possible uninitialized variable; change the second branch to else to ensure
tradeTrustTokenContract is always assigned, and stop using new Contract(...) —
instead use the factories' .connect(...) (e.g.
v5Contracts.TradeTrustToken__factory.connect(tokenRegistryAddress, signer) and
v4Contracts.TradeTrustToken__factory.connect(...)) or update the
getEthersContractFromProvider test fixture to return a real Contract
constructor; ensure the resulting tradeTrustTokenContract has an ownerOf method
before calling ownerOf(tokenId).
In `@src/token-registry-functions/types.ts`:
- Around line 119-128: Remove the unused type declaration
DeployTokenRegistryCommand from the types file: locate the exported type named
DeployTokenRegistryCommand and delete its entire definition; then run a
project-wide search for DeployTokenRegistryCommand to confirm no references
remain and update any barrel exports or export lists if it was re-exported
elsewhere (remove it from exports if present) so the codebase and build stay
clean.
In `@src/token-registry-functions/utils.ts`:
- Around line 64-85: The function isSupportedTitleEscrowFactory currently
accepts an optional provider but immediately passes it to
getEthersContractFromProvider (via provider) which will throw if undefined;
either make provider required in the function signature or add an explicit early
guard at the start of isSupportedTitleEscrowFactory that checks provider !==
undefined and throws a clear error (e.g. "provider is required for
isSupportedTitleEscrowFactory") before calling getEthersContractFromProvider, so
titleEscrowFactoryContract and subsequent calls (implementation,
supportsInterface) never receive an undefined provider.
---
Nitpick comments:
In `@src/__tests__/token-registry-functions/deploy.test.ts`:
- Around line 101-103: Remove the leftover commented-out import statements
referencing getEthersContractFactoryFromProvider and isV6EthersProvider (the
lines importing from '../../utils/ethers/index.js') in the test file; simply
delete those commented lines at both occurrences (near the shown diff and the
other instance around lines 175-177) to eliminate noise and keep the test file
clean.
In `@src/deploy/token-registry.ts`:
- Around line 188-241: Wrap the standalone deployment block in a try/catch
similar to deployDocumentStore: surround the creation and deploy calls for
tokenFactory/ContractFactoryV5/ContractFactoryV6 (references:
getEthersContractFactoryFromProvider, tokenFactory, TradeTrustToken__factory,
isV6EthersProvider, getTxOptions, deploy, deploymentTransaction,
deployTransaction) with a try, catch the thrown error, and rethrow a new Error
that includes a clear contextual message like "Failed to deploy Token Registry"
plus the original error message/details to preserve diagnostics; ensure both v5
and v6 branches are inside the try so any failure is caught and
logged-consistently.
In `@src/token-registry-functions/types.ts`:
- Around line 116-117: The type NetworkAndWalletSignerOption currently uses
Partial<PrivateKeyOption>, which defeats the discriminated-union
mutual-exclusivity enforced by PrivateKeyOption; replace
Partial<PrivateKeyOption> with PrivateKeyOption (keeping NetworkOption &
(Partial<WalletOption> | PrivateKeyOption)) so the discriminant and never fields
in PrivateKeyOption remain effective and prevent both key and keyFile being set
simultaneously; update any related imports or references if needed to reflect
the stronger type.
In `@src/token-registry-functions/utils.ts`:
- Around line 8-26: The function getTxOptions currently accepts required
parameters but treats chainId, maxFeePerGas and maxPriorityFeePerGas as possibly
undefined; change its signature to make chainId?: CHAIN_ID, maxFeePerGas?:
GasValue and maxPriorityFeePerGas?: GasValue (keeping signer: SignerV6 | Signer
required), update any internal casts/usages (e.g., the getChainIdSafe fallback
and SUPPORTED_CHAINS lookup) to rely on the now-optional types, and adjust any
callers or their passed arguments if they assume non-null inputs so the type
system reflects actual runtime behavior.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/__tests__/token-registry-functions/ownerOf.test.ts (1)
47-47: Remove dead commented-out code.Line 47 and lines 70–72 are leftover code that serves no documentation purpose. If the disabled
afterEachwas intentionally suppressed to avoid destroying module-level spies, add an inline comment explaining why rather than leaving the commented-out block.Also applies to: 70-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/token-registry-functions/ownerOf.test.ts` at line 47, Remove the dead commented-out code: delete the commented line initializing mockContract and the commented-out afterEach block in ownerOf.test.ts (the disabled "let mockContract = isV5TT ? mockV5TradeTrustTokenContract : mockV4TradeTrustTokenContract;" and the commented afterEach that was left to preserve module-level spies). If the afterEach was intentionally suppressed, replace the commented block with a concise inline comment explaining why module-level spies must not be destroyed (reference the afterEach and mockContract symbols so reviewers understand the rationale).src/__tests__/token-registry-functions/fixtures.ts (1)
24-35: Prefer a static default forgetChainIdSafeinstead of delegating to the real function.Using
vi.fn().mockImplementation(original.getChainIdSafe)makes the mock's default behavior call the real function, which internally queries the provider's network. This works only because theproviderV5.getNetworkspy is active (lines 370–373), but creates a hidden coupling: any test using a signer not connected toproviderV5, or any scenario where that spy is cleared/restored, will causegetChainIdSafeto attempt a real network call.A static default is simpler and unconditionally safe:
♻️ Proposed refactor
- getChainIdSafe: vi.fn().mockImplementation(original.getChainIdSafe), + getChainIdSafe: vi.fn().mockResolvedValue(1),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/token-registry-functions/fixtures.ts` around lines 24 - 35, Replace the mock that delegates to the real function so getChainIdSafe no longer calls into the provider; specifically, replace vi.fn().mockImplementation(original.getChainIdSafe) with a static mock that returns a fixed chain id (e.g., vi.fn().mockResolvedValue(1) or vi.fn().mockReturnValue(1) depending on whether getChainIdSafe is async) so tests are not coupled to providerV5.getNetwork or a real network call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/token-registry-functions/fixtures.ts`:
- Around line 368-373: The module-level spy on providerV5.getNetwork is fragile
and persists across tests; move the vi.spyOn(providerV5,
'getNetwork').mockResolvedValue(...) out of module scope and install it inside a
test lifecycle hook (e.g., beforeEach or beforeAll) in the consuming test files
so the mock implementation is re-applied for each test and survives mock
restoration/clear calls; reference the providerV5 constant and its getNetwork
method when locating where to remove the module-level spy and add the
lifecycle-scoped spy.
- Around line 40-45: MockContractConstructor currently routes by abi ===
'TradeTrustToken' which is identical for both factories
(TradeTrustToken__factory.abi), causing all addresses to resolve to
mockV5TradeTrustTokenContract; change the routing to use the address only (e.g.,
check address === MOCK_V5_ADDRESS to pick mockV5TradeTrustTokenContract and
otherwise return mockV4TradeTrustTokenContract) or alternatively give the v4/v5
factory abis unique strings and route by those; update MockContractConstructor
accordingly so MOCK_V4_ADDRESS maps to mockV4TradeTrustTokenContract and
MOCK_V5_ADDRESS maps to mockV5TradeTrustTokenContract.
---
Nitpick comments:
In `@src/__tests__/token-registry-functions/fixtures.ts`:
- Around line 24-35: Replace the mock that delegates to the real function so
getChainIdSafe no longer calls into the provider; specifically, replace
vi.fn().mockImplementation(original.getChainIdSafe) with a static mock that
returns a fixed chain id (e.g., vi.fn().mockResolvedValue(1) or
vi.fn().mockReturnValue(1) depending on whether getChainIdSafe is async) so
tests are not coupled to providerV5.getNetwork or a real network call.
In `@src/__tests__/token-registry-functions/ownerOf.test.ts`:
- Line 47: Remove the dead commented-out code: delete the commented line
initializing mockContract and the commented-out afterEach block in
ownerOf.test.ts (the disabled "let mockContract = isV5TT ?
mockV5TradeTrustTokenContract : mockV4TradeTrustTokenContract;" and the
commented afterEach that was left to preserve module-level spies). If the
afterEach was intentionally suppressed, replace the commented block with a
concise inline comment explaining why module-level spies must not be destroyed
(reference the afterEach and mockContract symbols so reviewers understand the
rationale).
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/token-registry-functions/types.ts (2)
116-117:Partial<PrivateKeyOption>is redundant and misleading.Both branches of
PrivateKeyOptionalready have only optional fields, soPartial<>applied to the union is a no-op. OnlyPartial<WalletOption>serves a real purpose (makingencryptedWalletPathoptional).♻️ Proposed cleanup
export type NetworkAndWalletSignerOption = NetworkOption & - (Partial<WalletOption> | Partial<PrivateKeyOption>); + (Partial<WalletOption> | PrivateKeyOption);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/token-registry-functions/types.ts` around lines 116 - 117, NetworkAndWalletSignerOption currently uses Partial<PrivateKeyOption> which is redundant because PrivateKeyOption's fields are already optional; remove Partial<> and simplify the union so NetworkAndWalletSignerOption = NetworkOption & (Partial<WalletOption> | PrivateKeyOption) (keeping Partial<WalletOption> to allow encryptedWalletPath to be optional) and update any references to NetworkAndWalletSignerOption to match the simplified type; ensure imports/types for NetworkOption, WalletOption, and PrivateKeyOption remain unchanged.
5-9:GasPriceScaleis asymmetric andGasOption.dryRunshould be optional.Two related concerns:
GasPriceScaleexposesmaxPriorityFeePerGasScalebut has no correspondingmaxFeePerGasScale. Since EIP-1559 involves bothmaxFeePerGasandmaxPriorityFeePerGas, please confirm this asymmetry is intentional (e.g. only the tip is user-scaled, base fee is network-driven).
dryRun: booleanonGasOptionis required, which forces every caller to explicitly passdryRun: false. As an opt-in flag it should be optional.♻️ Proposed fix for `dryRun` optionality
export interface GasOption extends GasPriceScale { - dryRun: boolean; + dryRun?: boolean; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/token-registry-functions/types.ts` around lines 5 - 9, The GasPriceScale interface currently only exposes maxPriorityFeePerGasScale—decide whether this asymmetry is intentional; if not, add a corresponding maxFeePerGasScale property to GasPriceScale (or document/confirm why only the tip is user-scaled). Also make GasOption.dryRun optional by changing its declaration on the GasOption interface (so callers aren’t forced to pass dryRun: false); update any related types/usages of GasOption to handle the optional boolean.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/token-registry-functions/types.ts`:
- Around line 116-117: NetworkAndWalletSignerOption currently uses
Partial<PrivateKeyOption> which is redundant because PrivateKeyOption's fields
are already optional; remove Partial<> and simplify the union so
NetworkAndWalletSignerOption = NetworkOption & (Partial<WalletOption> |
PrivateKeyOption) (keeping Partial<WalletOption> to allow encryptedWalletPath to
be optional) and update any references to NetworkAndWalletSignerOption to match
the simplified type; ensure imports/types for NetworkOption, WalletOption, and
PrivateKeyOption remain unchanged.
- Around line 5-9: The GasPriceScale interface currently only exposes
maxPriorityFeePerGasScale—decide whether this asymmetry is intentional; if not,
add a corresponding maxFeePerGasScale property to GasPriceScale (or
document/confirm why only the tip is user-scaled). Also make GasOption.dryRun
optional by changing its declaration on the GasOption interface (so callers
aren’t forced to pass dryRun: false); update any related types/usages of
GasOption to handle the optional boolean.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/document-store/transferOwnership.ts (1)
53-58: Null guards on transaction results are dead code.
documentStoreGrantRoleanddocumentStoreRevokeRoleeither return a truthy transaction object or throw — they never return a falsy value. Theif (!grantTransactionResult)andif (!revokeTransactionResult)branches can therefore never be entered.♻️ Proposed cleanup
- const grantTransactionResult = await grantTransaction; - if (!grantTransactionResult) { - throw new Error('Grant transaction failed, not proceeding with revoke transaction'); - } + await grantTransaction; const revokeTransaction = documentStoreRevokeRole( documentStoreAddress, roleString, ownerAddress, signer, options, ); - const revokeTransactionResult = await revokeTransaction; - if (!revokeTransactionResult) { - throw new Error('Revoke transaction failed'); - } + await revokeTransaction;Also applies to: 68-72
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/document-store/transferOwnership.ts` around lines 53 - 58, The null-check branches around the awaited transaction results are dead code because documentStoreGrantRole and documentStoreRevokeRole either return a transaction object or throw; remove the unreachable guards and their error throws in transferOwnership.ts: delete the if (!grantTransactionResult) { ... } block (referring to grantTransactionResult and the accompanying error throw) and likewise remove the if (!revokeTransactionResult) { ... } block that checks revokeTransactionResult; keep the awaits (const grantTransactionResult = await grantTransaction; const revokeTransactionResult = await revokeTransaction;) and proceed using those results or let thrown errors propagate.src/document-store/grant-role.ts (1)
108-111: Original error is swallowed — consider chaining it ascause.The caught error
eis logged but discarded when the genericErroris thrown, which makes root-cause debugging harder in production (only the log survives if the caller catches and re-throws).♻️ Proposed fix: chain the original error
- 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 });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/document-store/grant-role.ts` around lines 108 - 111, The catch block in grant-role.ts currently logs the caught error `e` and throws a new generic Error('Pre-check (callStatic) for grant-role failed') which discards the original error; update the catch to rethrow the new Error while chaining the original error as its cause (use the Error constructor's options or equivalent) so callers can inspect the root cause (preserve or keep the console.error line if desired), referencing the existing thrown message and the caught variable `e`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/document-store/grant-role.ts`:
- Around line 108-111: The catch block in grant-role.ts currently logs the
caught error `e` and throws a new generic Error('Pre-check (callStatic) for
grant-role failed') which discards the original error; update the catch to
rethrow the new Error while chaining the original error as its cause (use the
Error constructor's options or equivalent) so callers can inspect the root cause
(preserve or keep the console.error line if desired), referencing the existing
thrown message and the caught variable `e`.
In `@src/document-store/transferOwnership.ts`:
- Around line 53-58: The null-check branches around the awaited transaction
results are dead code because documentStoreGrantRole and documentStoreRevokeRole
either return a transaction object or throw; remove the unreachable guards and
their error throws in transferOwnership.ts: delete the if
(!grantTransactionResult) { ... } block (referring to grantTransactionResult and
the accompanying error throw) and likewise remove the if
(!revokeTransactionResult) { ... } block that checks revokeTransactionResult;
keep the awaits (const grantTransactionResult = await grantTransaction; const
revokeTransactionResult = await revokeTransaction;) and proceed using those
results or let thrown errors propagate.
Contributor
Author
|
@coderabbitai review |
Contributor
✅ Actions performedReview triggered.
|
Contributor
Author
|
@CodeRabbit full review |
Contributor
✅ Actions performedFull review triggered. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (7)
src/__tests__/token-registry-functions/deploy.test.ts (1)
371-408:⚠️ Potential issue | 🟡 MinorChain-ID auto-detect test still passes a chainId.
Line 405 supplies
chainId, so the auto-detect branch (andgetChainIdSafe) never runs. Consider omittingchainIdand assertinggetChainIdSafeis called; also ensure the utils mock exports a mockedgetChainIdSafe.✅ Suggested adjustment
- await deployTokenRegistry(mockRegistryName, mockRegistrySymbol, wallet, { - chainId: mockChainId, - }); + await deployTokenRegistry(mockRegistryName, mockRegistrySymbol, wallet, {}); + expect(getChainIdSafe).toHaveBeenCalled();#!/bin/bash # Verify getChainIdSafe is mocked in fixtures and inspect the chain-id test. rg -n "getChainIdSafe" src/__tests__/token-registry-functions/fixtures.ts -C2 rg -n "auto-detect chain ID" src/__tests__/token-registry-functions/deploy.test.ts -C3 rg -n "chainId:\s*mockChainId" src/__tests__/token-registry-functions/deploy.test.ts -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/token-registry-functions/deploy.test.ts` around lines 371 - 408, The test currently passes a chainId into deployTokenRegistry so the auto-detect branch never runs; remove the chainId from the deployTokenRegistry call in the "should auto-detect chain ID when not provided" test and instead assert that getChainIdSafe was invoked (mock and spy the utils export getChainIdSafe) and that deployTokenRegistry still resolves; ensure getChainIdSafe is properly exported and mocked in your test fixtures (search for getChainIdSafe in fixtures and add a vi.fn() mock if missing) so the test verifies auto-detection behavior for deployTokenRegistry.src/__tests__/token-registry-functions/fixtures.ts (2)
368-373: Avoid module-scopegetNetworkspy.Same as prior review: move the spy into a test lifecycle hook to avoid fragile cross-test state.
♻️ Suggested change
export const providerV5 = new ethersV5.providers.JsonRpcProvider(); -// Mock the getNetwork method for v5 provider -vi.spyOn(providerV5, 'getNetwork').mockResolvedValue({ - name: 'mainnet', - chainId: 1, -});Then add a
beforeEach/beforeAllspy in the consuming tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/token-registry-functions/fixtures.ts` around lines 368 - 373, The module-level spy on providerV5.getNetwork is creating cross-test state; remove the vi.spyOn call from the top-level in fixtures.ts and instead set up and restore the spy inside the test lifecycle of consuming tests (e.g., add a beforeEach or beforeAll that does vi.spyOn(providerV5, 'getNetwork').mockResolvedValue({ name: 'mainnet', chainId: 1 }) and an afterEach/afterAll that restores it via vi.restoreAllMocks() or spy.mockRestore(); keep the providerV5 instance export unchanged but move any mocking into the tests that import it.
40-45:⚠️ Potential issue | 🟡 MinorRoute MockContractConstructor by address only.
Same issue as prior review: both ABIs are identical, so routing by ABI collapses v4 into v5.
🔧 Proposed fix
- const isV5 = address === MOCK_V5_ADDRESS || abi === 'TradeTrustToken'; + const isV5 = address === MOCK_V5_ADDRESS;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/token-registry-functions/fixtures.ts` around lines 40 - 45, The mock factory MockContractConstructor currently selects v5 vs v4 by checking address OR abi, which collapses v4 into v5 because ABIs are identical; change the selection logic in MockContractConstructor to route solely by address (e.g., compare address to MOCK_V5_ADDRESS and return mockV5TradeTrustTokenContract, otherwise return mockV4TradeTrustTokenContract) and remove the abi === 'TradeTrustToken' check so mockV4TradeTrustTokenContract is reachable.src/deploy/document-store.ts (1)
131-137:⚠️ Potential issue | 🟡 MinorGuard
deploymentTransaction()before.wait().Same as prior review: add a null guard to keep the return type safe.
🔒 Proposed fix
- return await contract.deploymentTransaction().wait(); + const deployTx = contract.deploymentTransaction(); + if (!deployTx) throw new Error('Deployment transaction receipt not available'); + return await deployTx.wait();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/deploy/document-store.ts` around lines 131 - 137, The code calls contract.deploymentTransaction().wait() without guarding for a null/undefined deployment transaction; in the deploy path using ContractFactoryV6 (symbols: contractFactory, ContractFactoryV6, deploy, contract.deploymentTransaction, .wait) add a null check: retrieve the result of contract.deploymentTransaction(), ensure it is not null/undefined before calling .wait(), and handle the null case by returning a safe fallback (e.g., null or a rejected Promise) or throwing a descriptive error so the function's return type remains correct and type-safe.src/deploy/token-registry.ts (3)
220-228:⚠️ Potential issue | 🟡 MinorGuard
deploymentTransaction()before.wait().Same as prior review: add a null guard to keep return type consistent.
🔒 Proposed fix
- return await contract.deploymentTransaction().wait(); + const deployTx = contract.deploymentTransaction(); + if (!deployTx) throw new Error('Deployment transaction receipt not available'); + return await deployTx.wait();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/deploy/token-registry.ts` around lines 220 - 228, The code calls deploymentTransaction().wait() on the contract returned by (tokenFactory as ContractFactoryV6).deploy(...); guard against a null/undefined deploymentTransaction to keep the function's return type consistent: after calling deploy in the isV6 branch (symbols: isV6, tokenFactory, ContractFactoryV6, deploy, contract, deploymentTransaction, wait), check if contract.deploymentTransaction() is truthy and only call .wait() when present, otherwise return the appropriate empty/undefined/default value consistent with the non-v6 branch.
184-185:⚠️ Potential issue | 🔴 CriticalQuick-start should return a mined receipt.
Same as prior review:
deployerContract.deploy(...)returns a tx response, not a receipt. Await.wait()to fulfill the documented return type.🧾 Proposed fix
- return await deployerContract.deploy(tokenRegistryImplAddress, initParam, txOptions); + const tx = await deployerContract.deploy(tokenRegistryImplAddress, initParam, txOptions); + return await tx.wait();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/deploy/token-registry.ts` around lines 184 - 185, The current return from deployerContract.deploy(tokenRegistryImplAddress, initParam, txOptions) is a transaction response, not a mined receipt; change the implementation to await the deploy call into a variable (e.g., tx = await deployerContract.deploy(...)) and then await tx.wait() and return that receipt so the function returns a mined receipt as documented.
121-165:⚠️ Potential issue | 🟠 MajorAdd a
signer.providerguard before use.Same issue as prior review:
signer.providercan be undefined, causing failures ingetEthersContractFromProviderandisSupportedTitleEscrowFactory.🔒 Proposed fix
export const deployTokenRegistry = async ( registryName: string, registrySymbol: string, signer: SignerV5 | SignerV6, options: DeployOptions = {}, ): Promise<TransactionReceipt> => { + if (!signer.provider) throw new Error('Provider is required'); // Extract gas options🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/deploy/token-registry.ts` around lines 121 - 165, Add a guard to ensure signer.provider is defined before any use: check signer.provider at the top of the function (before calling getEthersContractFromProvider and isSupportedTitleEscrowFactory) and either throw a clear error (e.g., "Signer has no provider; cannot deploy in quick-start mode") or obtain/require a provider parameter; then use signer.provider only after this check when calling getEthersContractFromProvider and isSupportedTitleEscrowFactory to avoid undefined access.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/deploy/token-registry.ts`:
- Around line 115-120: The function deployTokenRegistry destructures properties
from the options parameter but doesn't provide a default, causing a crash if
callers omit options; update the deployTokenRegistry signature to default
options to an empty object (options = {}) so destructuring inside the function
is safe and existing callers that pass nothing continue to work; ensure you
update any JSDoc/type annotations if present for DeployOptions to reflect
optionality.
---
Duplicate comments:
In `@src/__tests__/token-registry-functions/deploy.test.ts`:
- Around line 371-408: The test currently passes a chainId into
deployTokenRegistry so the auto-detect branch never runs; remove the chainId
from the deployTokenRegistry call in the "should auto-detect chain ID when not
provided" test and instead assert that getChainIdSafe was invoked (mock and spy
the utils export getChainIdSafe) and that deployTokenRegistry still resolves;
ensure getChainIdSafe is properly exported and mocked in your test fixtures
(search for getChainIdSafe in fixtures and add a vi.fn() mock if missing) so the
test verifies auto-detection behavior for deployTokenRegistry.
In `@src/__tests__/token-registry-functions/fixtures.ts`:
- Around line 368-373: The module-level spy on providerV5.getNetwork is creating
cross-test state; remove the vi.spyOn call from the top-level in fixtures.ts and
instead set up and restore the spy inside the test lifecycle of consuming tests
(e.g., add a beforeEach or beforeAll that does vi.spyOn(providerV5,
'getNetwork').mockResolvedValue({ name: 'mainnet', chainId: 1 }) and an
afterEach/afterAll that restores it via vi.restoreAllMocks() or
spy.mockRestore(); keep the providerV5 instance export unchanged but move any
mocking into the tests that import it.
- Around line 40-45: The mock factory MockContractConstructor currently selects
v5 vs v4 by checking address OR abi, which collapses v4 into v5 because ABIs are
identical; change the selection logic in MockContractConstructor to route solely
by address (e.g., compare address to MOCK_V5_ADDRESS and return
mockV5TradeTrustTokenContract, otherwise return mockV4TradeTrustTokenContract)
and remove the abi === 'TradeTrustToken' check so mockV4TradeTrustTokenContract
is reachable.
In `@src/deploy/document-store.ts`:
- Around line 131-137: The code calls contract.deploymentTransaction().wait()
without guarding for a null/undefined deployment transaction; in the deploy path
using ContractFactoryV6 (symbols: contractFactory, ContractFactoryV6, deploy,
contract.deploymentTransaction, .wait) add a null check: retrieve the result of
contract.deploymentTransaction(), ensure it is not null/undefined before calling
.wait(), and handle the null case by returning a safe fallback (e.g., null or a
rejected Promise) or throwing a descriptive error so the function's return type
remains correct and type-safe.
In `@src/deploy/token-registry.ts`:
- Around line 220-228: The code calls deploymentTransaction().wait() on the
contract returned by (tokenFactory as ContractFactoryV6).deploy(...); guard
against a null/undefined deploymentTransaction to keep the function's return
type consistent: after calling deploy in the isV6 branch (symbols: isV6,
tokenFactory, ContractFactoryV6, deploy, contract, deploymentTransaction, wait),
check if contract.deploymentTransaction() is truthy and only call .wait() when
present, otherwise return the appropriate empty/undefined/default value
consistent with the non-v6 branch.
- Around line 184-185: The current return from
deployerContract.deploy(tokenRegistryImplAddress, initParam, txOptions) is a
transaction response, not a mined receipt; change the implementation to await
the deploy call into a variable (e.g., tx = await deployerContract.deploy(...))
and then await tx.wait() and return that receipt so the function returns a mined
receipt as documented.
- Around line 121-165: Add a guard to ensure signer.provider is defined before
any use: check signer.provider at the top of the function (before calling
getEthersContractFromProvider and isSupportedTitleEscrowFactory) and either
throw a clear error (e.g., "Signer has no provider; cannot deploy in quick-start
mode") or obtain/require a provider parameter; then use signer.provider only
after this check when calling getEthersContractFromProvider and
isSupportedTitleEscrowFactory to avoid undefined access.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (17)
src/__tests__/document-store/deploy.test.tssrc/__tests__/token-registry-functions/deploy.test.tssrc/__tests__/token-registry-functions/fixtures.tssrc/__tests__/token-registry-functions/ownerOf.test.tssrc/__tests__/token-registry-functions/transfers.test.tssrc/deploy/document-store.tssrc/deploy/token-registry.tssrc/document-store/deploy.tssrc/document-store/grant-role.tssrc/document-store/index.tssrc/document-store/transferOwnership.tssrc/token-registry-functions/index.tssrc/token-registry-functions/ownerOf.tssrc/token-registry-functions/types.tssrc/token-registry-functions/utils.tssrc/token-registry-v5/index.tssrc/utils/ethers/index.ts
💤 Files with no reviewable changes (1)
- src/document-store/deploy.ts
|
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/deploy/token-registry.ts (2)
184-185:⚠️ Potential issue | 🔴 CriticalReturn a mined receipt in quick-start mode.
deployerContract.deploy(...)returns a transaction response (v5/v6), not a receipt, but the function promises aTransactionReceipt. Await.wait()before returning.✅ Proposed fix
- return await deployerContract.deploy(tokenRegistryImplAddress, initParam, txOptions); + const tx = await deployerContract.deploy(tokenRegistryImplAddress, initParam, txOptions); + return await tx.wait();In ethers v5/v6, what does a Contract method transaction return, and is `.wait()` required to obtain a TransactionReceipt?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/deploy/token-registry.ts` around lines 184 - 185, The current return uses deployerContract.deploy(...) which yields a TransactionResponse in ethers v5/v6, but the function signature promises a TransactionReceipt; update the code to await the transaction response's .wait() and return that receipt (i.e., call deployTx = await deployerContract.deploy(tokenRegistryImplAddress, initParam, txOptions); const receipt = await deployTx.wait(); return receipt) so deployerContract.deploy, tokenRegistryImplAddress, initParam and txOptions are used as before but the function returns the mined TransactionReceipt.
115-165:⚠️ Potential issue | 🟠 MajorGuard against missing
signer.providerbefore provider-based helpers.
Signer.providercan be undefined (especially in ethers v5), which will break provider-dependent helpers andisV6EthersProvider. Add an explicit guard near the top of the function.🔧 Proposed fix
export const deployTokenRegistry = async ( registryName: string, registrySymbol: string, signer: SignerV5 | SignerV6, options: DeployOptions = {}, ): Promise<TransactionReceipt> => { + if (!signer.provider) { + throw new Error('Signer has no provider; cannot deploy Token Registry'); + } // Extract gas options const { maxFeePerGas, maxPriorityFeePerGas } = options;In ethers v5 and v6, is `Signer.provider` optional/undefined for certain signer types, and is a guard recommended before accessing it?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/deploy/token-registry.ts` around lines 115 - 165, Add an explicit guard for a missing signer.provider at the start of deployTokenRegistry: check if signer.provider is defined before calling any provider-dependent helpers (e.g., getEthersContractFromProvider, isV6EthersProvider, getChainIdSafe) and either throw a clear error (e.g., "signer.provider is required for deployTokenRegistry in non-standalone mode") or handle the fallback path (force standalone mode) depending on desired behavior; update references in deployTokenRegistry to use this guard so provider-based calls only occur when signer.provider is present.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/deploy/token-registry.ts`:
- Around line 184-185: The current return uses deployerContract.deploy(...)
which yields a TransactionResponse in ethers v5/v6, but the function signature
promises a TransactionReceipt; update the code to await the transaction
response's .wait() and return that receipt (i.e., call deployTx = await
deployerContract.deploy(tokenRegistryImplAddress, initParam, txOptions); const
receipt = await deployTx.wait(); return receipt) so deployerContract.deploy,
tokenRegistryImplAddress, initParam and txOptions are used as before but the
function returns the mined TransactionReceipt.
- Around line 115-165: Add an explicit guard for a missing signer.provider at
the start of deployTokenRegistry: check if signer.provider is defined before
calling any provider-dependent helpers (e.g., getEthersContractFromProvider,
isV6EthersProvider, getChainIdSafe) and either throw a clear error (e.g.,
"signer.provider is required for deployTokenRegistry in non-standalone mode") or
handle the fallback path (force standalone mode) depending on desired behavior;
update references in deployTokenRegistry to use this guard so provider-based
calls only occur when signer.provider is present.
rongquan1
approved these changes
Feb 25, 2026
nghaninn
pushed a commit
that referenced
this pull request
Feb 25, 2026
## [2.9.0](v2.8.0...v2.9.0) (2026-02-25) ### Features * token registry deploy ([#130](#130)) ([9faf75f](9faf75f))
|
🎉 This PR is included in version 2.9.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Deployment Feature Implementation
🚀 New Features
1. Token Registry Deployment (
src/deploy/token-registry.ts)A complete deployment module for TradeTrust Token Registry contracts with two deployment modes:
Quick-start Mode (Default)
Standalone Mode
Key Features:
Function Signature:
2. Document Store Deployment (
src/deploy/document-store.ts)A deployment module for TrustVC Document Store contracts with two store types:
Standard Document Store (Default)
Transferable Document Store
Key Features:
🧪 Testing
Comprehensive Test Suite (
src/__tests__/token-registry-functions/deploy.test.ts)Token Registry Tests
Document Store Tests
🔧 Utility Enhancements
Enhanced
src/token-registry-functions/utils.tsAdded critical utility functions:
getChainIdSafe()- Safe chain ID extraction from signersisValidAddress()- Ethereum address validationisSupportedTitleEscrowFactory()- Factory interface verificationgetDefaultContractAddress()- Network-specific contract addressesEnhanced Type Definitions (
src/token-registry-functions/types.ts)Added comprehensive deployment-related types:
DeployOptionsinterfaceTransactionReceiptunion typeGasValue)Summary by CodeRabbit
New Features
Refactor
Tests