Merged
Conversation
✅ Deploy Preview for reference-implementation ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for tradetrust-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
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:
📝 WalkthroughWalkthroughReplaces shell/CLI-based contract deployment and per-token minting with programmatic ethers.js flows; also bumps Changes
Sequence DiagramsequenceDiagram
participant Setup as TestSetup
participant Provider as JsonRpcProvider
participant Signer as Wallet/Signer
participant TEF as TitleEscrowFactory
participant TR as TokenRegistry
participant Proxy as ERC1967Proxy
participant Mint as mint()
Setup->>Provider: init provider
Setup->>Signer: create wallet/signer
Setup->>TEF: deploy TitleEscrowFactory (ContractFactory)
TEF-->>Setup: factory address
Setup->>Proxy: deploy ERC1967Proxy (point to impl)
Proxy-->>Setup: proxy address
Setup->>TR: deploy TokenRegistry (or attach to proxy)
TR-->>Setup: registry address & instance
loop per token
Setup->>Mint: call mint(tokenData, recipient) on TR
Mint->>TR: submit tx
TR-->>Setup: tx receipt or error
end
Setup->>Setup: log deployed addresses & results
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 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)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). 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: 2
🧹 Nitpick comments (1)
tests/e2e/setup-contracts.js (1)
7-14: Use root exports from@trustvc/trustvcinstead of deepnode_modules/distpaths.Lines 7–14 can import
v5Contractsandv5Utilsdirectly from the package root, consistent with the rest of the codebase (seesrc/common/hooks/useDeployTokenRegistry.tsx). Deep imports intonode_modules/dist/cjs/...are brittle and unnecessarily couple this script to package internals.♻️ Proposed refactor
-const path = require("path"); const { ethers, Wallet } = require("ethers"); -const { deployTokenRegistry, mint } = require("@trustvc/trustvc"); +const { deployTokenRegistry, mint, v5Contracts, v5Utils } = require("@trustvc/trustvc"); const ERC1967Proxy_artifact = require("../../src/test/fixture/artifacts/ERC1967Proxy.json"); -// Import `@trustvc/trustvc` modules for contract deployment -const v5ContractsPath = path.resolve( - __dirname, - "../../node_modules/@trustvc/trustvc/dist/cjs/token-registry-v5/contracts.js" -); -const v5Contracts = require(v5ContractsPath); - -const v5UtilsPath = path.resolve(__dirname, "../../node_modules/@trustvc/trustvc/dist/cjs/token-registry-v5/utils.js"); -const v5Utils = require(v5UtilsPath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/setup-contracts.js` around lines 7 - 14, Replace the brittle deep-node_modules requires for v5Contracts/v5Utils with the package-root exports from `@trustvc/trustvc`: remove v5ContractsPath and v5UtilsPath and instead require('@trustvc/trustvc') and pull the token-registry v5 contracts and utils the same way used in src/common/hooks/useDeployTokenRegistry.tsx, updating the variables v5Contracts and v5Utils to reference those root exports (so tests/e2e/setup-contracts.js no longer constructs paths into dist/cjs).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/e2e/setup-contracts.js`:
- Around line 154-171: The catch in the mint block currently swallows errors and
continues; modify it to fail fast by either rethrowing the caught error or
exiting with a non-zero status. Specifically, update the catch for the mint call
(the try surrounding mint(...) with signer, TOKEN_REGISTRY_ADDRESS and
CHAIN_ID.local / titleEscrowVersion "v5") to log the error with full details and
then call throw error (or process.exit(1)) so the test setup aborts on mint
failure rather than continuing silently.
- Around line 149-167: The mint loop is passing a hardcoded
TOKEN_REGISTRY_ADDRESS into mint (in the for...of over
tokensToMint.tokenRegistry) so tokens may be minted to the wrong contract;
change the first arg to the actual deployed registry runtime address produced
earlier (the variable created during deployment on lines ~48–53, e.g.,
tokenRegistry.address or the deployment result variable used there), ensure that
variable is in scope for the mint call, and remove/replace
TOKEN_REGISTRY_ADDRESS usage in the mint invocation inside the loop.
---
Nitpick comments:
In `@tests/e2e/setup-contracts.js`:
- Around line 7-14: Replace the brittle deep-node_modules requires for
v5Contracts/v5Utils with the package-root exports from `@trustvc/trustvc`: remove
v5ContractsPath and v5UtilsPath and instead require('@trustvc/trustvc') and pull
the token-registry v5 contracts and utils the same way used in
src/common/hooks/useDeployTokenRegistry.tsx, updating the variables v5Contracts
and v5Utils to reference those root exports (so tests/e2e/setup-contracts.js no
longer constructs paths into dist/cjs).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72797615-1319-4071-bb5f-5a3253bbadd0
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
package.jsontests/e2e/setup-contracts.js
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/setup-contracts.js (1)
96-112:⚠️ Potential issue | 🟠 MajorUse deployed contract address instead of hardcoded constant.
Line 108 uses
TOKEN_IMPLEMENTATION_ADDRESS(hardcoded at line 97) instead oftokenImplementationContract.address(deployed at line 94). If the chain state differs from the expected nonce, these addresses won't match andaddImplementationwill reference the wrong contract.🐛 Proposed fix
const addImplementationTx = await tDocDeployerThroughProxy.addImplementation( - TOKEN_IMPLEMENTATION_ADDRESS, + tokenImplementationContract.address, titleEscrowFactoryContract.address );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/setup-contracts.js` around lines 96 - 112, The test is using the hardcoded TOKEN_IMPLEMENTATION_ADDRESS constant instead of the actual deployed address; replace the constant with the deployed tokenImplementationContract.address when calling tDocDeployerThroughProxy.addImplementation so the call references the real deployed implementation. Update the call site that invokes tDocDeployerThroughProxy.addImplementation(TOKEN_IMPLEMENTATION_ADDRESS, titleEscrowFactoryContract.address) to use tokenImplementationContract.address as the first argument (keeping titleEscrowFactoryContract.address as-is) so the correct implementation is registered.
♻️ Duplicate comments (2)
tests/e2e/setup-contracts.js (2)
155-167:⚠️ Potential issue | 🔴 CriticalMint is targeting a hardcoded registry instead of the deployed registry.
Line 156 uses
TOKEN_REGISTRY_ADDRESS(hardcoded at line 25), while the actual deployed registry address istokenRegistryContract.address(from line 56-60). This will mint tokens to the wrong contract if the hardcoded address doesn't match the deployed one.🐛 Proposed fix
const receipt = await mint( - { tokenRegistryAddress: TOKEN_REGISTRY_ADDRESS }, + { tokenRegistryAddress: tokenRegistryContract.address }, signer, {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/setup-contracts.js` around lines 155 - 167, The mint call is using the hardcoded TOKEN_REGISTRY_ADDRESS constant instead of the deployed registry address; update the mint invocation to pass the deployed registry address (tokenRegistryContract.address) for the tokenRegistryAddress argument so minted tokens go to the actual deployed contract, and ensure tokenRegistryContract is in scope where mint(...) is called (replace TOKEN_REGISTRY_ADDRESS with tokenRegistryContract.address in the mint call).
169-171:⚠️ Potential issue | 🟠 MajorFail fast on mint errors in test setup.
The catch block swallows failures and continues the loop. For deterministic E2E test setup, mint failures should abort the script immediately to avoid running tests against incomplete state.
✅ Proposed fix
} catch (error) { console.error(`Failed to mint token ${element.tokenId}:`, error.message); + throw error; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/setup-contracts.js` around lines 169 - 171, The catch in the minting loop currently logs errors and continues, which hides failures; change the catch to fail fast by either rethrowing the caught error or exiting with non-zero status so the setup aborts on any mint failure—update the catch that logs `Failed to mint token ${element.tokenId}:` to throw the caught `error` (or call `process.exit(1)`), ensuring the test setup halts immediately when `mint` (the token minting call) fails.
🧹 Nitpick comments (3)
tests/e2e/setup-contracts.js (3)
6-14: Remove dead commented-out code.This commented block appears to be leftover from earlier debugging/alternative imports. Keeping dead code adds noise and maintenance burden.
🧹 Proposed cleanup
-// // Import `@trustvc/trustvc` modules for contract deployment -// const v5ContractsPath = path.resolve( -// __dirname, -// "../../node_modules/@trustvc/trustvc/dist/cjs/token-registry-v5/contracts.js" -// ); -// const v5Contracts = require(v5ContractsPath); - -// const v5UtilsPath = path.resolve(__dirname, "../../node_modules/@trustvc/trustvc/dist/cjs/token-registry-v5/utils.js"); -// const v5Utils = require(v5UtilsPath);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/setup-contracts.js` around lines 6 - 14, Remove the dead commented-out import block in tests/e2e/setup-contracts.js (the lines defining v5ContractsPath, v5Contracts, v5UtilsPath, v5Utils and the leading comment about `@trustvc/trustvc`) to eliminate noise; simply delete that entire commented block so only active import/require statements remain and run tests to verify nothing else depends on those commented references.
25-29: Clean up unused hardcoded address constants.
TITLE_ESCROW_FACTORY_ADDRESS(line 29) appears to be unused in the actual deployment flow. Similarly, lines 97-100 defineTITLE_ESCROW_FACTORY_ADDRESS2,TDOC_DEPLOYER_ADDRESS, andERC1967_PROXY_ADDRESS2which are also unused. Consider removing these to reduce confusion about which addresses are actually in use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/setup-contracts.js` around lines 25 - 29, Remove the unused hardcoded address constants to avoid confusion: delete the declarations for TITLE_ESCROW_FACTORY_ADDRESS, TITLE_ESCROW_FACTORY_ADDRESS2, TDOC_DEPLOYER_ADDRESS, and ERC1967_PROXY_ADDRESS2 from the file (and any adjacent unused ADDRESS_EXAMPLE_* if truly unused); ensure you only remove the const declarations and any exports or unused references, and if any function (e.g., setupContracts, getDeployedAddresses) actually needs those values, replace them with the real deployed value source or pass them in as parameters instead of leaving unused hardcoded constants.
174-180: Deployed addresses are logged but not exported for test consumption.The setup script logs deployed addresses to console but doesn't persist them anywhere. According to
tests/e2e/utils/index.js, only ACCOUNT constants are exported, not contract addresses. If downstream tests depend on these dynamically deployed addresses (rather than hardcoded ones), they have no way to access them.Consider either:
- Writing addresses to a JSON file that tests can import
- Exporting addresses from a shared module
- Ensuring all tests use the same hardcoded addresses that match expected deployment
💡 Example: Write to a shared config file
+const fs = require("fs"); + console.log("\n=== Contract Setup Complete ==="); console.log(`Title Escrow Factory: ${titleEscrowFactoryContractForStandalone.address}`); console.log(`Token Registry: ${tokenRegistryContract.address}`); console.log(`TDoc Deployer (Proxy): ${ERC1967ProxyFactoryContract.address}`); console.log(`Token Implementation: ${tokenImplementationContract.address}`); console.log(`Title Escrow Factory (V5): ${titleEscrowFactoryContract.address}`); + +// Export addresses for test consumption +const deployedAddresses = { + titleEscrowFactory: titleEscrowFactoryContractForStandalone.address, + tokenRegistry: tokenRegistryContract.address, + tDocDeployerProxy: ERC1967ProxyFactoryContract.address, + tokenImplementation: tokenImplementationContract.address, + titleEscrowFactoryV5: titleEscrowFactoryContract.address, +}; +fs.writeFileSync( + path.join(__dirname, "deployed-addresses.json"), + JSON.stringify(deployedAddresses, null, 2) +);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/setup-contracts.js` around lines 174 - 180, The deployed contract addresses (titleEscrowFactoryContractForStandalone.address, tokenRegistryContract.address, ERC1967ProxyFactoryContract.address, tokenImplementationContract.address, titleEscrowFactoryContract.address) are only logged and not exported, so downstream tests can't consume them; update the setup script to persist these addresses by writing them into a shared JSON or JS config that tests import (e.g., tests/e2e/config.json or a module that exports constants used by tests/e2e/utils/index.js), or alternatively export them from a shared module after deployment, and ensure tests import that file instead of relying on hardcoded values so the dynamic addresses are available to all E2E tests.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/e2e/setup-contracts.js`:
- Around line 96-112: The test is using the hardcoded
TOKEN_IMPLEMENTATION_ADDRESS constant instead of the actual deployed address;
replace the constant with the deployed tokenImplementationContract.address when
calling tDocDeployerThroughProxy.addImplementation so the call references the
real deployed implementation. Update the call site that invokes
tDocDeployerThroughProxy.addImplementation(TOKEN_IMPLEMENTATION_ADDRESS,
titleEscrowFactoryContract.address) to use tokenImplementationContract.address
as the first argument (keeping titleEscrowFactoryContract.address as-is) so the
correct implementation is registered.
---
Duplicate comments:
In `@tests/e2e/setup-contracts.js`:
- Around line 155-167: The mint call is using the hardcoded
TOKEN_REGISTRY_ADDRESS constant instead of the deployed registry address; update
the mint invocation to pass the deployed registry address
(tokenRegistryContract.address) for the tokenRegistryAddress argument so minted
tokens go to the actual deployed contract, and ensure tokenRegistryContract is
in scope where mint(...) is called (replace TOKEN_REGISTRY_ADDRESS with
tokenRegistryContract.address in the mint call).
- Around line 169-171: The catch in the minting loop currently logs errors and
continues, which hides failures; change the catch to fail fast by either
rethrowing the caught error or exiting with non-zero status so the setup aborts
on any mint failure—update the catch that logs `Failed to mint token
${element.tokenId}:` to throw the caught `error` (or call `process.exit(1)`),
ensuring the test setup halts immediately when `mint` (the token minting call)
fails.
---
Nitpick comments:
In `@tests/e2e/setup-contracts.js`:
- Around line 6-14: Remove the dead commented-out import block in
tests/e2e/setup-contracts.js (the lines defining v5ContractsPath, v5Contracts,
v5UtilsPath, v5Utils and the leading comment about `@trustvc/trustvc`) to
eliminate noise; simply delete that entire commented block so only active
import/require statements remain and run tests to verify nothing else depends on
those commented references.
- Around line 25-29: Remove the unused hardcoded address constants to avoid
confusion: delete the declarations for TITLE_ESCROW_FACTORY_ADDRESS,
TITLE_ESCROW_FACTORY_ADDRESS2, TDOC_DEPLOYER_ADDRESS, and ERC1967_PROXY_ADDRESS2
from the file (and any adjacent unused ADDRESS_EXAMPLE_* if truly unused);
ensure you only remove the const declarations and any exports or unused
references, and if any function (e.g., setupContracts, getDeployedAddresses)
actually needs those values, replace them with the real deployed value source or
pass them in as parameters instead of leaving unused hardcoded constants.
- Around line 174-180: The deployed contract addresses
(titleEscrowFactoryContractForStandalone.address, tokenRegistryContract.address,
ERC1967ProxyFactoryContract.address, tokenImplementationContract.address,
titleEscrowFactoryContract.address) are only logged and not exported, so
downstream tests can't consume them; update the setup script to persist these
addresses by writing them into a shared JSON or JS config that tests import
(e.g., tests/e2e/config.json or a module that exports constants used by
tests/e2e/utils/index.js), or alternatively export them from a shared module
after deployment, and ensure tests import that file instead of relying on
hardcoded values so the dynamic addresses are available to all E2E tests.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7bd36dc4-07c2-4db0-85d0-5d56cc04f667
📒 Files selected for processing (1)
tests/e2e/setup-contracts.js
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tests/e2e/setup-contracts.js (2)
160-179:⚠️ Potential issue | 🟠 MajorMake mint setup deterministic: wait for confirmation and fail fast on error.
Line 176 reports success before explicit confirmation, and Line 179 continues after failures. For E2E setup, this should block on confirmation and abort on failure.
✅ Proposed fix
for (const element of tokensToMint.tokenRegistry) { console.log(`Minting token ${element.tokenId}...`); try { - const receipt = await mint( + const tx = await mint( { tokenRegistryAddress: TOKEN_REGISTRY_ADDRESS }, signer, { beneficiaryAddress: element.owner, holderAddress: element.holder, tokenId: element.tokenId, }, { chainId: CHAIN_ID.local, titleEscrowVersion: "v5", } ); + await tx.wait(); console.log(`Token ${element.tokenId} minted successfully`); } catch (error) { - console.error(`Failed to mint token ${element.tokenId}:`, error.message); + console.error(`Failed to mint token ${element.tokenId}:`, error); + throw error; } }In `@trustvc/trustvc` v2.11.0, does `mint(...)` return an ethers TransactionResponse that requires `await tx.wait()` for confirmation?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/setup-contracts.js` around lines 160 - 179, The loop logs success before the mint transaction is confirmed and swallows errors; change the logic in the for-loop over tokensToMint.tokenRegistry so that you capture the TransactionResponse returned by mint (call it e.g. tx or transactionResponse), await its confirmation with await tx.wait() (or whatever promise the library provides) before logging success, and on any error rethrow or exit so the setup fails fast; update the try/catch to await confirmation inside the try and in the catch do not continue the loop—propagate the error (throw) or process.exit(1) to abort the E2E setup.
163-165:⚠️ Potential issue | 🔴 CriticalUse the runtime deployed registry for minting.
Line 164 still passes
TOKEN_REGISTRY_ADDRESSinstead of the deployed address, so minting can target the wrong contract in this run.🐛 Proposed fix
- const receipt = await mint( - { tokenRegistryAddress: TOKEN_REGISTRY_ADDRESS }, + const receipt = await mint( + { tokenRegistryAddress: tokenRegistryContract.address }, signer, { beneficiaryAddress: element.owner,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/setup-contracts.js` around lines 163 - 165, The mint call is still passing the static TOKEN_REGISTRY_ADDRESS instead of the runtime-deployed registry address; update the call to pass the deployed registry variable (e.g., replace TOKEN_REGISTRY_ADDRESS with the runtime variable such as tokenRegistry.address or deployedTokenRegistry.address that was created earlier in the test) so mint({ tokenRegistryAddress: ... }, signer) targets the actual deployed contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/e2e/setup-contracts.js`:
- Around line 160-179: The loop logs success before the mint transaction is
confirmed and swallows errors; change the logic in the for-loop over
tokensToMint.tokenRegistry so that you capture the TransactionResponse returned
by mint (call it e.g. tx or transactionResponse), await its confirmation with
await tx.wait() (or whatever promise the library provides) before logging
success, and on any error rethrow or exit so the setup fails fast; update the
try/catch to await confirmation inside the try and in the catch do not continue
the loop—propagate the error (throw) or process.exit(1) to abort the E2E setup.
- Around line 163-165: The mint call is still passing the static
TOKEN_REGISTRY_ADDRESS instead of the runtime-deployed registry address; update
the call to pass the deployed registry variable (e.g., replace
TOKEN_REGISTRY_ADDRESS with the runtime variable such as tokenRegistry.address
or deployedTokenRegistry.address that was created earlier in the test) so mint({
tokenRegistryAddress: ... }, signer) targets the actual deployed contract.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2172fb0a-9a7b-48ac-b420-c4b72d87f893
📒 Files selected for processing (1)
tests/e2e/setup-contracts.js
There was a problem hiding this comment.
♻️ Duplicate comments (2)
tests/e2e/setup-contracts.js (2)
156-160:⚠️ Potential issue | 🔴 CriticalMinting still targets a hardcoded registry address.
Line 157 uses
TOKEN_REGISTRY_ADDRESSeven though the script just deployedtokenRegistryContractat Line 59–Line 65. This can mint to the wrong contract.🐛 Proposed fix
- const tokenRegistryForMinting = new ethers.Contract( - TOKEN_REGISTRY_ADDRESS, + const tokenRegistryForMinting = new ethers.Contract( + tokenRegistryContract.address, TradeTrustTokenStandard__factory.abi, signer );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/setup-contracts.js` around lines 156 - 160, The minting code is instantiating tokenRegistryForMinting with the hardcoded TOKEN_REGISTRY_ADDRESS instead of using the address of the freshly deployed tokenRegistryContract; update the contract instantiation to use tokenRegistryContract.address (the deployed instance created earlier) when constructing tokenRegistryForMinting so minting targets the correct deployed registry (refer to symbols tokenRegistryForMinting, TOKEN_REGISTRY_ADDRESS, and tokenRegistryContract).
172-174:⚠️ Potential issue | 🟠 MajorMint errors are still swallowed instead of failing setup.
Line 172–Line 174 logs and continues, so setup can print completion even when minting failed.
✅ Proposed fix
} catch (error) { - console.error(`Failed to mint token ${element.tokenId}:`, error.message); + console.error(`Failed to mint token ${element.tokenId}:`, error); + throw error; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/setup-contracts.js` around lines 172 - 174, The catch block that currently logs mint failures (catch handling error and using console.error with element.tokenId) swallows errors and allows setup to continue; change the handler in the minting routine in tests/e2e/setup-contracts.js to fail fast by rethrowing the caught error (or call process.exit(1)) after logging so the setup fails on mint errors instead of printing completion—locate the catch that references error and element.tokenId and replace the silent-continue behavior with a rethrow or explicit failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@tests/e2e/setup-contracts.js`:
- Around line 156-160: The minting code is instantiating tokenRegistryForMinting
with the hardcoded TOKEN_REGISTRY_ADDRESS instead of using the address of the
freshly deployed tokenRegistryContract; update the contract instantiation to use
tokenRegistryContract.address (the deployed instance created earlier) when
constructing tokenRegistryForMinting so minting targets the correct deployed
registry (refer to symbols tokenRegistryForMinting, TOKEN_REGISTRY_ADDRESS, and
tokenRegistryContract).
- Around line 172-174: The catch block that currently logs mint failures (catch
handling error and using console.error with element.tokenId) swallows errors and
allows setup to continue; change the handler in the minting routine in
tests/e2e/setup-contracts.js to fail fast by rethrowing the caught error (or
call process.exit(1)) after logging so the setup fails on mint errors instead of
printing completion—locate the catch that references error and element.tokenId
and replace the silent-continue behavior with a rethrow or explicit failure.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 22432bac-c84e-4f94-8785-08a2c90dd009
📒 Files selected for processing (1)
tests/e2e/setup-contracts.js
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/e2e/setup-contracts.js (1)
98-110:⚠️ Potential issue | 🟠 MajorValidate the other nonce-dependent local addresses before continuing.
The constants on Lines 107-110 are not just documentation: local consumers still read those exact addresses from
src/test/setup-contracts-testcafe.js:72-75andsrc/common/hooks/useDeployTokenRegistry.tsx:40-54. Right now a rerun against a dirty node can complete successfully while the app points at different contracts.Proposed fix
const tDocDeployerFactoryContract = await tDocDeployerFactory.deploy(); const ERC1967ProxyFactoryContract = await ERC1967ProxyFactory.deploy( tDocDeployerFactoryContract.address, "0x8129fc1c" ); const titleEscrowFactoryContract = await titleEscrowFactory.deploy(); const tokenImplementationContract = await tokenImplementation.deploy(); + await Promise.all([ + tDocDeployerFactoryContract.deployed(), + ERC1967ProxyFactoryContract.deployed(), + titleEscrowFactoryContract.deployed(), + tokenImplementationContract.deployed(), + ]); @@ const TOKEN_IMPLEMENTATION_ADDRESS = "0x0952a6817E00fc2455418a5303395760A9c4EE71"; //tokenImplementationContract.address const TITLE_ESCROW_FACTORY_ADDRESS2 = "0x547Ca63C8fB3Ccb856DEb7040D327dBfe4e7d20F"; //titleEscrowFactoryContract.address; const TDOC_DEPLOYER_ADDRESS = "0xfE442b75786c67E1e7a7146DAeD8943F0f2c23d2"; //tDocDeployerFactoryContract.address const ERC1967_PROXY_ADDRESS2 = "0x3488EAA1bF4f606f758b24F5ef6eb2a1E32335be"; //ERC1967ProxyFactoryContract.address + + const expectedLocalAddresses = { + tokenImplementation: TOKEN_IMPLEMENTATION_ADDRESS, + titleEscrowFactory: TITLE_ESCROW_FACTORY_ADDRESS2, + tDocDeployer: TDOC_DEPLOYER_ADDRESS, + proxy: ERC1967_PROXY_ADDRESS2, + }; + const actualLocalAddresses = { + tokenImplementation: tokenImplementationContract.address, + titleEscrowFactory: titleEscrowFactoryContract.address, + tDocDeployer: tDocDeployerFactoryContract.address, + proxy: ERC1967ProxyFactoryContract.address, + }; + + for (const [name, expected] of Object.entries(expectedLocalAddresses)) { + const actual = actualLocalAddresses[name]; + if (actual.toLowerCase() !== expected.toLowerCase()) { + throw new Error( + `${name} address drifted: expected ${expected}, got ${actual}. Reset the local chain before running E2E setup.` + ); + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/setup-contracts.js` around lines 98 - 110, The hardcoded nonce-dependent addresses (TOKEN_IMPLEMENTATION_ADDRESS, TITLE_ESCROW_FACTORY_ADDRESS2, TDOC_DEPLOYER_ADDRESS, ERC1967_PROXY_ADDRESS2) can diverge from the actual deployed contracts; replace those magic constants with the runtime addresses from the deployed contract objects (tokenImplementationContract.address, titleEscrowFactoryContract.address, tDocDeployerFactoryContract.address, ERC1967ProxyFactoryContract.address) or assert equality and throw/fail if they differ so tests/app don't point at stale contracts; also update consumers (src/test/setup-contracts-testcafe.js and src/common/hooks/useDeployTokenRegistry.tsx) to read the exported/runtime addresses instead of the hardcoded values so reruns against a dirty node remain consistent.
♻️ Duplicate comments (2)
tests/e2e/setup-contracts.js (2)
64-70:⚠️ Potential issue | 🔴 CriticalUse the deployed registry instance for minting, and assert the fixed local address.
Lines 64-69 deploy
tokenRegistryContract, but Lines 161-165 reconnect to the hardcodedTOKEN_REGISTRY_ADDRESS. That only works if this run still lands on the same nonce-derived address; otherwise setup seeds one contract while the rest of the local stack still reads another.Proposed fix
const tokenRegistryContract = await tokenRegistryFactory.deploy( "DEMO TOKEN REGISTRY", "DTR", titleEscrowFactoryContractForStandalone.address ); await tokenRegistryContract.deployed(); + if (tokenRegistryContract.address.toLowerCase() !== TOKEN_REGISTRY_ADDRESS.toLowerCase()) { + throw new Error( + `Token registry address drifted: expected ${TOKEN_REGISTRY_ADDRESS}, got ${tokenRegistryContract.address}. Reset the local chain before running E2E setup.` + ); + } console.log(`Token Registry deployed at: ${tokenRegistryContract.address}`); @@ - const tokenRegistryForMinting = new ethers.Contract( - TOKEN_REGISTRY_ADDRESS, - TradeTrustTokenStandard__factory.abi, - signer - ); + const tokenRegistryForMinting = tokenRegistryContract.connect(signer);Also applies to: 161-165
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/setup-contracts.js` around lines 64 - 70, The test deploys tokenRegistryContract but later reconnects to a hardcoded TOKEN_REGISTRY_ADDRESS, which can diverge; update the setup so all subsequent interactions use the deployed tokenRegistryContract instance (or set TOKEN_REGISTRY_ADDRESS = tokenRegistryContract.address immediately after deploy) and replace the reconnection logic that re-instantiates from TOKEN_REGISTRY_ADDRESS with the already-deployed tokenRegistryContract; additionally add an assertion comparing tokenRegistryContract.address to the expected local constant (if any) to fail fast when the nonce-derived address differs.
167-175:⚠️ Potential issue | 🟠 MajorAbort setup on the first mint error.
Continuing after a failed mint leaves partially seeded state but still reaches the final “Contract Setup Complete” log. For E2E setup, this should stop immediately.
Proposed fix
} catch (error) { - console.error(`Failed to mint token ${element.tokenId}:`, error.message); + console.error(`Failed to mint token ${element.tokenId}:`, error); + throw error; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/e2e/setup-contracts.js` around lines 167 - 175, The mint loop currently swallows errors and continues after a failed call to tokenRegistryForMinting.mint, leaving partial state; update the catch in the loop that handles tokensToMint.tokenRegistry so that after logging the error (including error.message) it aborts the setup by rethrowing the error (or calling process.exit(1)) instead of continuing — this ensures the first failure in tokenRegistryForMinting.mint stops execution and the test setup fails fast.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/e2e/setup-contracts.js`:
- Around line 98-110: The hardcoded nonce-dependent addresses
(TOKEN_IMPLEMENTATION_ADDRESS, TITLE_ESCROW_FACTORY_ADDRESS2,
TDOC_DEPLOYER_ADDRESS, ERC1967_PROXY_ADDRESS2) can diverge from the actual
deployed contracts; replace those magic constants with the runtime addresses
from the deployed contract objects (tokenImplementationContract.address,
titleEscrowFactoryContract.address, tDocDeployerFactoryContract.address,
ERC1967ProxyFactoryContract.address) or assert equality and throw/fail if they
differ so tests/app don't point at stale contracts; also update consumers
(src/test/setup-contracts-testcafe.js and
src/common/hooks/useDeployTokenRegistry.tsx) to read the exported/runtime
addresses instead of the hardcoded values so reruns against a dirty node remain
consistent.
---
Duplicate comments:
In `@tests/e2e/setup-contracts.js`:
- Around line 64-70: The test deploys tokenRegistryContract but later reconnects
to a hardcoded TOKEN_REGISTRY_ADDRESS, which can diverge; update the setup so
all subsequent interactions use the deployed tokenRegistryContract instance (or
set TOKEN_REGISTRY_ADDRESS = tokenRegistryContract.address immediately after
deploy) and replace the reconnection logic that re-instantiates from
TOKEN_REGISTRY_ADDRESS with the already-deployed tokenRegistryContract;
additionally add an assertion comparing tokenRegistryContract.address to the
expected local constant (if any) to fail fast when the nonce-derived address
differs.
- Around line 167-175: The mint loop currently swallows errors and continues
after a failed call to tokenRegistryForMinting.mint, leaving partial state;
update the catch in the loop that handles tokensToMint.tokenRegistry so that
after logging the error (including error.message) it aborts the setup by
rethrowing the error (or calling process.exit(1)) instead of continuing — this
ensures the first failure in tokenRegistryForMinting.mint stops execution and
the test setup fails fast.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c9cd2950-ae4f-4fb2-a874-8a23cf6c8d3c
📒 Files selected for processing (1)
tests/e2e/setup-contracts.js
rongquan1
approved these changes
Mar 10, 2026
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Removing the tradetrust cli commands and update the test functions with trustvc sdk commads
Summary by CodeRabbit
Chores
Tests