Feat/update contract scripts #1212
Conversation
|
You have run out of free Bugbot PR reviews for this billing cycle. This will reset on October 17. To receive reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial. |
Tranquil-Flow
had a problem deploying
to
development
GitHub Actions
Failure
Tranquil-Flow
had a problem deploying
to
development
GitHub Actions
Failure
WalkthroughUpdates Aadhaar-related verification and registry semantics (configurable registration window, error signature change, pubkey commitments boolean model), adjusts circuits build param for Aadhaar, replaces test pubkeys, adds Ignition modules for deploying/updating verifiers/registries and Hub V2 upgrade, and adds per-chain deployment data and network config. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Client
participant Hub as IdentityVerificationHubImplV2
participant Registry as IdentityRegistryAadhaarImplV1
Note over Hub: Configurable AADHAAR_REGISTRATION_WINDOW
User->>Client: Submit Aadhaar proof
Client->>Hub: verifyAadhaar(proofSignals)
Hub->>Registry: isRegisteredUidaiPubkeyCommitment(commitment)
Registry-->>Hub: bool (registered)
alt registered
Hub->>Hub: validate proof.timestamp within window
alt within window
Hub-->>Client: success (output incl. forbiddenCountriesListPacked, pubKeyHash)
else outside window
Hub-->>Client: revert InvalidUidaiTimestamp(blockTs, ts)
end
else not registered
Hub-->>Client: revert (unregistered commitment)
end
sequenceDiagram
autonumber
participant Operator
participant Ignition
participant Deployed as deployed_addresses.json
participant HubProxy as IdentityVerificationHub (proxy)
Operator->>Ignition: run DeployNewHubAndUpgrade
Ignition->>Deployed: load addresses
Ignition->>Ignition: deploy Impl V2 (with CustomVerifier)
Ignition->>HubProxy: upgradeToAndCall(ImplV2, initialize())
Ignition->>HubProxy: setAadhaarRegistrationWindow(value)
Note over Ignition: then batch update verifiers & registries using deployed_addresses
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
circuits/tests/register/pubkeys.ts (1)
81-190: Address the prettier formatting issue.The pipeline reports a code style issue. Run
prettier --writeto fix formatting before merging.#!/bin/bash # Description: Fix prettier formatting issues cd circuits/tests/register prettier --write pubkeys.tscontracts/contracts/registry/IdentityRegistryAadhaarImplV1.sol (1)
165-169: Initializer bug: parameter shadows storage; hub never set.
function initialize(address _hub) { _hub = _hub; }assigns the parameter to itself; storage_hubremains zero. All onlyHub flows will revert.Apply:
- function initialize(address _hub) external initializer { + function initialize(address hubAddress) external initializer { __ImplRoot_init(); - _hub = _hub; - emit RegistryInitialized(_hub); + _hub = hubAddress; + emit RegistryInitialized(hubAddress); }
🧹 Nitpick comments (6)
contracts/ignition/modules/registry/deployAadhaarRegistry.ts (1)
41-43: Encode init data using the Aadhaar impl ABI to avoid drift.Safer to encode with IdentityRegistryAadhaarImplV1 ABI in case of overrides.
- const registryArtifact = artifacts.readArtifactSync("IdentityRegistryImplV1"); + const registryArtifact = artifacts.readArtifactSync("IdentityRegistryAadhaarImplV1"); const registryInterface = new ethers.Interface(registryArtifact.abi);contracts/ignition/modules/upgrade/deployNewHubAndUpgrade.ts (1)
39-39: Parameterize Aadhaar registration window instead of hard-coding 120.Prevents misconfiguration across networks/environments and improves reproducibility.
- m.call(hubProxy, "setAadhaarRegistrationWindow", [120], {after: [a]}); + const windowMins = m.getParameter("aadhaarRegistrationWindow", 20); + m.call(hubProxy, "setAadhaarRegistrationWindow", [windowMins], { after: [a] });contracts/contracts/IdentityVerificationHubImplV2.sol (2)
995-1001: Fix off-by-one/asymmetric date range checks (can accept unintended dates).The checks use
startOfDay - 1 days + 1andendOfDay + 1 days, which creates an uneven window and a 1‑second “hole.” Tighten to symmetric ±1 day (or exact same day) explicitly.Apply this safer, clear range check:
function _performCurrentDateCheck( @@ - uint256 endOfDay = startOfDay + 1 days - 1; - - if (currentTimestamp < startOfDay - 1 days + 1 || currentTimestamp > endOfDay + 1 days) { + uint256 endOfDay = startOfDay + 1 days - 1; + uint256 lowerBound = startOfDay > 1 days ? startOfDay - 1 days : 0; + uint256 upperBound = endOfDay + 1 days; + if (currentTimestamp < lowerBound || currentTimestamp > upperBound) { revert CurrentDateNotInValidRange(); }function _performNumericCurrentDateCheck( @@ - uint256 endOfDay = startOfDay + 1 days - 1; - - if (currentTimestamp < startOfDay - 1 days + 1 || currentTimestamp > endOfDay + 1 days) { + uint256 endOfDay = startOfDay + 1 days - 1; + uint256 lowerBound = startOfDay > 1 days ? startOfDay - 1 days : 0; + uint256 upperBound = endOfDay + 1 days; + if (currentTimestamp < lowerBound || currentTimestamp > upperBound) { revert CurrentDateNotInValidRange(); }If the intent is same‑day only, set bounds to [startOfDay, endOfDay]. As per coding guidelines
Also applies to: 1014-1021
328-335: Emit event and consider sane bounds for AADHAAR_REGISTRATION_WINDOW.Without an event, auditors and ops can’t track governance changes. Also consider upper bound to prevent disabling the check by setting an excessively large window.
function setAadhaarRegistrationWindow(uint256 window) external virtual onlyProxy onlyOwner { - AADHAAR_REGISTRATION_WINDOW = window; + // Optional cap; tune as appropriate (e.g., <= 120 minutes) + require(window <= 120, "window too large"); + uint256 old = AADHAAR_REGISTRATION_WINDOW; + AADHAAR_REGISTRATION_WINDOW = window; + emit AadhaarRegistrationWindowUpdated(old, window); }Add the event (outside this hunk):
event AadhaarRegistrationWindowUpdated(uint256 oldWindow, uint256 newWindow);I can wire tests/events across the deployment scripts if needed. As per coding guidelines
contracts/ignition/modules/registry/updateRegistries.ts (2)
67-90: Guard per‑registry method availability to avoid accidental reverts.This orchestrator can call
updateCscaRoot/updatePassportNoOfacRootfor any registry if config is populated. Aadhaar registry doesn’t expose these; a misconfigured entry will revert mid‑batch.
- Introduce a per‑registry allowlist of supported methods and gate calls accordingly, or split handlers per registry type (passport/id/aadhaar) to eliminate accidental API mismatches.
57-66: Avoid console.log in deployment graph if logs may leak into CI artifacts.Addresses aren’t sensitive, but consider using Ignition’s reporting or structured logs to keep CI noise minimal and reproducible.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (21)
circuits/scripts/build/build_disclose_circuits.sh(1 hunks)circuits/tests/register/pubkeys.ts(2 hunks)circuits/tests/register/register_aadhaar.test.ts(2 hunks)contracts/.gitignore(1 hunks)contracts/contracts/IdentityVerificationHubImplV2.sol(8 hunks)contracts/contracts/interfaces/IIdentityRegistryAadhaarV1.sol(1 hunks)contracts/contracts/registry/IdentityRegistryAadhaarImplV1.sol(4 hunks)contracts/contracts/tests/testUpgradedIdentityVerificationHubImplV2.sol(1 hunks)contracts/error-selectors.json(15 hunks)contracts/hardhat.config.ts(2 hunks)contracts/ignition/deployments/chain-11142220/deployed_addresses.json(1 hunks)contracts/ignition/deployments/chain-42220/deployed_addresses.json(1 hunks)contracts/ignition/modules/hub/updateRegistries.ts(1 hunks)contracts/ignition/modules/hub/updateVerifiers.ts(1 hunks)contracts/ignition/modules/registry/deployAadhaarRegistry.ts(1 hunks)contracts/ignition/modules/registry/updateRegistries.ts(1 hunks)contracts/ignition/modules/upgrade/deployNewHubAndUpgrade.ts(1 hunks)contracts/ignition/modules/verifiers/deployAllVerifiersNew.ts(1 hunks)contracts/package.json(1 hunks)contracts/test/utils/deploymentV2.ts(2 hunks)contracts/test/v2/registerAadhaar.test.ts(8 hunks)
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{js,ts,tsx,jsx,sol,nr}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,ts,tsx,jsx,sol,nr}: NEVER log sensitive data including PII (names, DOB, passport numbers, addresses), credentials, tokens, API keys, private keys, or session identifiers.
ALWAYS redact/mask sensitive fields in logs using consistent patterns (e.g.,***-***-1234for passport numbers,J*** D***for names).
Files:
circuits/tests/register/register_aadhaar.test.tscontracts/test/utils/deploymentV2.tscontracts/hardhat.config.tscontracts/contracts/interfaces/IIdentityRegistryAadhaarV1.solcontracts/ignition/modules/registry/deployAadhaarRegistry.tscontracts/ignition/modules/hub/updateVerifiers.tscircuits/tests/register/pubkeys.tscontracts/test/v2/registerAadhaar.test.tscontracts/ignition/modules/verifiers/deployAllVerifiersNew.tscontracts/contracts/tests/testUpgradedIdentityVerificationHubImplV2.solcontracts/ignition/modules/hub/updateRegistries.tscontracts/ignition/modules/upgrade/deployNewHubAndUpgrade.tscontracts/ignition/modules/registry/updateRegistries.tscontracts/contracts/registry/IdentityRegistryAadhaarImplV1.solcontracts/contracts/IdentityVerificationHubImplV2.sol
**/*.{test,spec}.{ts,js,tsx,jsx}
⚙️ CodeRabbit configuration file
**/*.{test,spec}.{ts,js,tsx,jsx}: Review test files for:
- Test coverage completeness
- Test case quality and edge cases
- Mock usage appropriateness
- Test readability and maintainability
Files:
circuits/tests/register/register_aadhaar.test.tscontracts/test/v2/registerAadhaar.test.ts
contracts/**
📄 CodeRabbit inference engine (AGENTS.md)
Compile Solidity contracts using
yarn workspace @selfxyz/contracts build
Files:
contracts/test/utils/deploymentV2.tscontracts/hardhat.config.tscontracts/contracts/interfaces/IIdentityRegistryAadhaarV1.solcontracts/ignition/deployments/chain-42220/deployed_addresses.jsoncontracts/error-selectors.jsoncontracts/ignition/deployments/chain-11142220/deployed_addresses.jsoncontracts/ignition/modules/registry/deployAadhaarRegistry.tscontracts/ignition/modules/hub/updateVerifiers.tscontracts/test/v2/registerAadhaar.test.tscontracts/ignition/modules/verifiers/deployAllVerifiersNew.tscontracts/package.jsoncontracts/contracts/tests/testUpgradedIdentityVerificationHubImplV2.solcontracts/ignition/modules/hub/updateRegistries.tscontracts/ignition/modules/upgrade/deployNewHubAndUpgrade.tscontracts/ignition/modules/registry/updateRegistries.tscontracts/contracts/registry/IdentityRegistryAadhaarImplV1.solcontracts/contracts/IdentityVerificationHubImplV2.sol
contracts/**/*.sol
⚙️ CodeRabbit configuration file
contracts/**/*.sol: Review Solidity smart contracts for:
- Security vulnerabilities (reentrancy, overflow, etc.)
- Gas optimization opportunities
- Access control patterns
- Event emission for important state changes
- Code documentation and NatSpec comments
Files:
contracts/contracts/interfaces/IIdentityRegistryAadhaarV1.solcontracts/contracts/tests/testUpgradedIdentityVerificationHubImplV2.solcontracts/contracts/registry/IdentityRegistryAadhaarImplV1.solcontracts/contracts/IdentityVerificationHubImplV2.sol
**/package.json
📄 CodeRabbit inference engine (AGENTS.md)
Use Yarn only for package management (yarn install/add/remove); do not use npm or pnpm in scripts
Files:
contracts/package.json
contracts/contracts/IdentityVerificationHubImplV2.sol
📄 CodeRabbit inference engine (.cursorrules)
Identity Verification Hub should manage multi-step verification for passports and EU ID cards, handle document attestation with zero-knowledge proofs, and implement verification paths: E-PASSPORT and EU_ID_CARD.
Files:
contracts/contracts/IdentityVerificationHubImplV2.sol
🧠 Learnings (1)
📚 Learning: 2025-09-22T11:10:22.019Z
Learnt from: CR
PR: selfxyz/self#0
File: .cursorrules:0-0
Timestamp: 2025-09-22T11:10:22.019Z
Learning: Applies to contracts/contracts/registry/IdentityRegistryImplV1.sol : Identity Registry Management maintains separate registries for passports and ID cards, handles DSC key commitment registration, and tracks nullifiers for duplicate prevention.
Applied to files:
contracts/error-selectors.json
🧬 Code graph analysis (2)
circuits/tests/register/register_aadhaar.test.ts (2)
circuits/tests/register/pubkeys.ts (1)
pubkeys(1-191)common/src/utils/bytes.ts (1)
splitToWords(140-153)
contracts/ignition/modules/hub/updateVerifiers.ts (1)
contracts/ignition/modules/verifiers/deployAllVerifiersNew.ts (2)
CircuitName(4-89)circuitIds(92-180)
🪛 GitHub Actions: Circuits CI
circuits/tests/register/register_aadhaar.test.ts
[warning] 1-1: Code style issues found in this file. Run 'prettier --write' to fix.
circuits/tests/register/pubkeys.ts
[warning] 1-1: Code style issues found in this file. Run 'prettier --write' to fix.
🔇 Additional comments (16)
contracts/hardhat.config.ts (1)
90-97: LGTM!The custom chain configuration for celo-sepolia looks correct. Using Blockscout for contract verification is appropriate for this network, and the chainId matches the network definition.
contracts/.gitignore (1)
18-24: LGTM!The per-chain ignore rules are well-structured and align with the new chain-specific deployment files added in this PR. The pattern consistently preserves only
deployed_addresses.jsonfor each chain while ignoring other artifacts.contracts/error-selectors.json (1)
254-260: Enhanced error reporting with contextual parameters.The
InvalidUidaiTimestamperror now includes twouint256parameters (likelyblockTimestampandtimestamp), providing better debugging information when UIDAI timestamp validation fails. This is a beneficial change that improves error diagnostics.circuits/tests/register/register_aadhaar.test.ts (1)
188-188: LGTM!Extending the test output to include
pubKeyHashenhances test coverage and aligns with the PR's goal of expanding verification workflows.contracts/ignition/deployments/chain-42220/deployed_addresses.json (1)
1-95: LGTM!This deployment data file for chain 42220 follows the expected structure and aligns with the per-chain deployment pattern established in the
.gitignorechanges.contracts/contracts/interfaces/IIdentityRegistryAadhaarV1.sol (1)
104-104: Simplified commitment registration interface.Removing the
expiryTimestampparameter simplifies the UIDAI pubkey commitment registration flow. This aligns with the PR's shift to a configurable registration window model inIdentityVerificationHubImplV2.Note: This is a breaking change for any external contracts calling
registerUidaiPubkeyCommitment. Ensure all consumers are updated accordingly.contracts/test/utils/deploymentV2.ts (1)
286-288: LGTM!The removal of the
expiryTimestampparameter correctly aligns with the updated interface signature forregisterUidaiPubkeyCommitment.circuits/scripts/build/build_disclose_circuits.sh (1)
20-20: Verify vc_and_disclose_aadhaar.r1cs constraint count is ≤2^17
Rebuild in a proper environment (with circom, yarn, wget) and runsnarkjs r1cs info build/disclose/vc_and_disclose_aadhaar/vc_and_disclose_aadhaar.r1csto confirm the constraint count ≤ 131072 and ensure all existing tests pass.
contracts/ignition/modules/registry/deployAadhaarRegistry.ts (1)
1-3: No action needed: ethers v6 and hardhat-ethers v3 confirmed
All package.json files reference ethers v6 and @nomicfoundation/hardhat-ethers is at v3.0.5 (which supports ethers v6).contracts/contracts/IdentityVerificationHubImplV2.sol (4)
765-771: Timestamp window logic: overall OK but consider monotonicity edge cases.The ±window check is correct. If you want strictly inclusive minutes, add comments clarifying inclusivity and consider capping
AADHAAR_REGISTRATION_WINDOW(see setter comment). No change required.
If desired, add a test asserting boundary inclusivity at exactly ±window minutes to prevent regressions.Also applies to: 768-770
1247-1252: Copying Aadhaar forbiddenCountriesListPacked looks good.Consistent with passport/EU patterns; no concerns.
11-18: No corrections needed for Aadhaar verifier imports.Both IAadhaarRegisterCircuitVerifier and IVcAndDiscloseAadhaarCircuitVerifier are defined in their respective interface files, so the imports are valid.
Likely an incorrect or invalid review comment.
231-236: Confirm and correct initializer version
NatSpec still mentionsreinitializer(2)while the code usesreinitializer(11); ensure this matches your upgrade sequence and update the comment accordingly.contracts/contracts/tests/testUpgradedIdentityVerificationHubImplV2.sol (1)
48-57: Test harness initializer looks sound.Reinitializer(3) with onlyProxy views matches upgrade test needs. No issues.
contracts/ignition/modules/hub/updateVerifiers.ts (2)
52-66: Remove unnecessary fallback forDeployAllVerifiersNewkeys All entries in deployed_addresses.json use theDeployAllVerifiers#Verifier_*prefix, so the suggested fallback is never used and can be removed.Likely an incorrect or invalid review comment.
128-137: Proxy address confirmed –deployed_addresses.jsoncontains onlyDeployHubV2#IdentityVerificationHub, sohubAddresscorrectly references the proxy.
| import { convertBigIntToByteArray, decompressByteArray, splitToWords } from '@anon-aadhaar/core'; | ||
| import assert from 'assert'; | ||
| import { customHasher } from '@selfxyz/common/utils/hash'; | ||
| import forge from 'node-forge'; |
There was a problem hiding this comment.
❓ Verification inconclusive
Verify node-forge dependency is declared.
The test now imports node-forge to parse certificates and compute pubkey commitments. Ensure that node-forge (version ^1.3.5 based on learnings) is properly declared in package.json.
Run the following script to verify the dependency:
🏁 Script executed:
#!/bin/bash
# Description: Check if node-forge is declared in package.json
rg -n "node-forge" package.jsonLength of output: 0
🏁 Script executed:
#!/bin/bash
# Verify if node-forge is declared in any package manifest
rg -nP '"node-forge"\s*:' package*.jsonLength of output: 0
Add node-forge to devDependencies
The test imports node-forge but it’s not declared in package.json, causing CI/test failures. Add "node-forge": "^1.3.5" (or current compatible version) under devDependencies.
🤖 Prompt for AI Agents
In circuits/tests/register/register_aadhaar.test.ts around line 9, the test
imports "node-forge" but the package is not listed in package.json; add
"node-forge": "^1.3.5" (or a current compatible version) under devDependencies
in package.json, run your package manager to install (npm/yarn/pnpm) so lockfile
updates, and commit the updated package.json and lockfile to resolve CI/test
failures.
| /// @notice Mapping from UIDAI pubkey to a boolean indicating registration. | ||
| mapping(uint256 => bool) internal _uidaiPubkeyCommitments; | ||
|
|
There was a problem hiding this comment.
Upgrade-safety breach: storage layout/type change in V1.
Replacing the old UIDAI pubkey mapping (expiry timestamps) with mapping(uint256 => bool) in the same V1 storage slot violates upgradeable storage rules. If this replaces a deployed contract, it can corrupt state and create undefined behavior.
- Do NOT change existing storage types in V1.
- Options:
- Keep the old
mapping(uint256 => uint256)and treat any non‑zero value as “registered” (store 1). Migrate reads/writes accordingly. - Or, create
IdentityRegistryAadhaarStorageV2appending new variables at the end, and implementIdentityRegistryAadhaarImplV2using reinitializer to migrate data or switch logic, leaving V1 storage untouched.
- Keep the old
I can draft the V2 storage/impl scaffold if you confirm the chosen path. As per coding guidelines
Also applies to: 195-201, 251-256, 309-311, 317-319, 321-327
contracts/hardhat.config.ts
Outdated
| "celo-sepolia": { | ||
| chainId: 11142220, | ||
| url: process.env.CELO_SEPOLIA_RPC_URL || "https://rpc.ankr.com/celo_sepolia", | ||
| accounts: [process.env.PRIVATE_KEY as string], | ||
| } |
There was a problem hiding this comment.
Validate PRIVATE_KEY before using it in network configurations.
The type assertion process.env.PRIVATE_KEY as string will cast undefined to the literal string "undefined" if the environment variable is not set, leading to cryptic deployment failures. Consider adding validation at the top of the config file or using a helper function that throws an error with a clear message if the key is missing.
Example validation you could add before the config object:
function requireEnv(key: string): string {
const value = process.env[key];
if (!value) {
throw new Error(`Missing required environment variable: ${key}`);
}
return value;
}Then use it in network configs:
- accounts: [process.env.PRIVATE_KEY as string],
+ accounts: [requireEnv('PRIVATE_KEY')],Note: This issue also affects other networks (mainnet, sepolia, celo, alfajores), so consider applying the fix consistently across all of them.
🤖 Prompt for AI Agents
In contracts/hardhat.config.ts around lines 62 to 66, the network config uses
process.env.PRIVATE_KEY as a blunt type assertion which can cast undefined into
the string "undefined"; add a small required-env helper at the top of the file
(e.g., requireEnv(key): string that throws a clear Error if missing) and replace
direct assertions with requireEnv("PRIVATE_KEY") for the celo-sepolia entry;
apply the same change to all other networks (mainnet, sepolia, celo, alfajores)
that currently use process.env.PRIVATE_KEY so deployments fail fast with a clear
message instead of using an invalid literal.
| @@ -0,0 +1,67 @@ | |||
| import { buildModule, IgnitionModuleBuilder } from "@nomicfoundation/ignition-core"; | |||
There was a problem hiding this comment.
Use Hardhat Ignition’s module import for consistency and plugin integration.
Mixing ignition-core and hardhat-ignition/modules can cause runtime/type mismatches during deploy.
-import { buildModule, IgnitionModuleBuilder } from "@nomicfoundation/ignition-core";
+import { buildModule, ModuleBuilder as IgnitionModuleBuilder } from "@nomicfoundation/hardhat-ignition/modules";📝 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.
| import { buildModule, IgnitionModuleBuilder } from "@nomicfoundation/ignition-core"; | |
| import { buildModule, ModuleBuilder as IgnitionModuleBuilder } from "@nomicfoundation/hardhat-ignition/modules"; |
🤖 Prompt for AI Agents
In contracts/ignition/modules/hub/updateRegistries.ts around line 1, the file
imports from "@nomicfoundation/ignition-core" which can cause runtime/type
mismatches with Hardhat Ignition; replace that import with the Hardhat Ignition
module import (e.g., import { buildModule, IgnitionModuleBuilder } from
"hardhat-ignition/modules") so the project uses the same plugin-provided types
and runtime; also ensure hardhat-ignition is installed and update any other
imports/aliases in this file to match the hardhat-ignition API if necessary.
| for (const circuit of Object.keys(circuitIds) as CircuitName[]) { | ||
| const [shouldDeploy] = circuitIds[circuit]; | ||
|
|
||
| if (!shouldDeploy) { | ||
| console.log(`Skipping Verifier_${circuit}`); | ||
| continue; | ||
| } | ||
|
|
||
| const name = `Verifier_${circuit}`; | ||
| console.log(`Deploying ${name}...`); | ||
|
|
||
| // Create dependency on the last deployed contract to ensure sequential deployment | ||
| const deployOptions = lastDeployedContract ? { after: [lastDeployedContract] } : {}; | ||
| deployments[name] = m.contract(name, [], deployOptions); | ||
| lastDeployedContract = deployments[name]; | ||
| } |
There was a problem hiding this comment.
Gate deployments to avoid artifact-not-found failures and reduce blast radius
Deploying every verifier unconditionally will fail if any artifact is missing (common across networks/CI) and wastes time/gas. Allowlist via env and iterate only selected circuits; keep sequential dependency to avoid nonce conflicts.
Apply this diff:
export default buildModule("DeployAllVerifiers", (m) => {
const deployments: Record<string, any> = {};
let lastDeployedContract: any = null;
- for (const circuit of Object.keys(circuitIds) as CircuitName[]) {
+ // Comma‑separated list, e.g., CIRCUITS="register_aadhaar,vc_and_disclose"
+ const only = (process.env.CIRCUITS ?? "")
+ .split(",")
+ .map((s) => s.trim())
+ .filter(Boolean) as CircuitName[];
+
+ for (const circuit of (Object.keys(circuitIds) as CircuitName[]).filter(
+ (c) => only.length === 0 || only.includes(c)
+ )) {
const [shouldDeploy] = circuitIds[circuit];
if (!shouldDeploy) {
console.log(`Skipping Verifier_${circuit}`);
continue;
}
const name = `Verifier_${circuit}`;
console.log(`Deploying ${name}...`);
// Create dependency on the last deployed contract to ensure sequential deployment
const deployOptions = lastDeployedContract ? { after: [lastDeployedContract] } : {};
deployments[name] = m.contract(name, [], deployOptions);
lastDeployedContract = deployments[name];
}
console.log(`Deployments will execute sequentially to prevent nonce conflicts`);
return deployments;
});Also applies to: 182-205
| "deploy:registry:idcard": "npx dotenv-cli -- bash -c 'yarn hardhat ignition deploy ignition/modules/registry/deployIdCardRegistry.ts --network ${NETWORK:-localhost} ${VERIFY:+--verify}'", | ||
| "deploy:test:selfverificationroot": "npx dotenv-cli -- bash -c 'yarn hardhat ignition deploy ignition/modules/deployTestSelfVerificationRoot.ts --network ${NETWORK:-localhost} ${VERIFY:+--verify}'", | ||
| "deploy:verifier:id": "npx dotenv-cli -- bash -c 'yarn hardhat ignition deploy ignition/modules/verifiers/deployAllVerifiers.ts --network ${NETWORK:-localhost} ${VERIFY:+--verify}'", | ||
| "deploy:verifier:id": "npx dotenv-cli -- bash -c 'yarn hardhat ignition deploy ignition/modules/verifiers/deployAllVerifiersNew.ts --network ${NETWORK:-localhost} ${VERIFY:+--verify}'", |
There was a problem hiding this comment.
🧩 Analysis chain
Script intent vs. target module mismatch
deploy:verifier:id points to deployAllVerifiersNew.ts (deploys ALL verifiers), which can unintentionally deploy far more than expected on prod.
Confirm intended behavior. If this should deploy only “id” verifiers, repoint to the id‑only module; otherwise, rename the script to reflect “all verifiers” to prevent misuse.
🏁 Script executed:
#!/bin/bash
echo "Deploy scripts in contracts/package.json:"
rg -n '"deploy:verifier' -n contracts/package.json
echo
echo "Searching for deployAllVerifiersNew.ts file path:"
fd -HI deployAllVerifiersNew.ts
echo
echo "Printing first 50 lines of deployAllVerifiersNew.ts:"
file=$(fd -HI deployAllVerifiersNew.ts | head -n 1)
sed -n '1,50p' "$file"Length of output: 3421
🏁 Script executed:
#!/bin/bash
sed -n '50,200p' contracts/ignition/modules/verifiers/deployAllVerifiersNew.tsLength of output: 7255
Rename or repoint deploy:verifier:id
This script invokes deployAllVerifiersNew.ts (which deploys every verifier). If it’s meant to target only “id” circuits, repoint it to an id-only deploy module (or add filtering); otherwise, rename the script to indicate it deploys all verifiers.
🤖 Prompt for AI Agents
In contracts/package.json around line 33, the npm script key
"deploy:verifier:id" currently runs deployAllVerifiersNew.ts (deploys every
verifier) but the name implies it should target only "id" circuits; either
repoint the script to the id-only deploy module (e.g., the deploy file that only
deploys id verifiers) or rename the script key to something like
"deploy:verifiers:all" to reflect it deploys all verifiers; make the change in
the package.json scripts block and preserve the existing env handling
(${NETWORK:-localhost} and ${VERIFY:+--verify}).
| "deploy:verifier:id": "npx dotenv-cli -- bash -c 'yarn hardhat ignition deploy ignition/modules/verifiers/deployAllVerifiersNew.ts --network ${NETWORK:-localhost} ${VERIFY:+--verify}'", | ||
| "deploy:verifiers": "npx dotenv-cli -- bash -c 'yarn hardhat ignition deploy ignition/modules/verifiers/deployVerifiers.ts --network ${NETWORK:-localhost} ${VERIFY:+--verify}'", | ||
| "deploy:verifiers:all": "npx dotenv-cli -- bash -c 'yarn hardhat ignition deploy ignition/modules/verifiers/deployAllVerifiers.ts --network ${NETWORK:-localhost} ${VERIFY:+--verify}'", | ||
| "deploy:verifiers:all": "npx dotenv-cli -- bash -c 'yarn hardhat ignition deploy ignition/modules/verifiers/deployAllVerifiersNew.ts --network ${NETWORK:-localhost}' --verify", |
There was a problem hiding this comment.
Bug: --verify isn’t passed to Hardhat
--verify sits outside the quoted bash command, so Hardhat never receives it (and dotenv-cli might choke on it). Move it inside using the existing ${VERIFY:+--verify} pattern.
Apply this diff:
- "deploy:verifiers:all": "npx dotenv-cli -- bash -c 'yarn hardhat ignition deploy ignition/modules/verifiers/deployAllVerifiersNew.ts --network ${NETWORK:-localhost}' --verify",
+ "deploy:verifiers:all": "npx dotenv-cli -- bash -c 'yarn hardhat ignition deploy ignition/modules/verifiers/deployAllVerifiersNew.ts --network ${NETWORK:-localhost} ${VERIFY:+--verify}'",🤖 Prompt for AI Agents
In contracts/package.json around line 35 the npm script places --verify outside
the single-quoted bash command so Hardhat never receives it; update the script
to move the verify flag inside the quoted command and use the existing
conditional expansion pattern (${VERIFY:+--verify}) so the final command
becomes: put ${VERIFY:+--verify} inside the single quotes right after the deploy
script call (i.e., before the closing quote) ensuring dotenv-cli invocation
remains the same.
|
|
||
| let vcAndDiscloseIdVerifierArtifact; | ||
| // Deploy VC and Disclose ID verifier | ||
| // // Deploy VC and Disclose ID verifier |
There was a problem hiding this comment.
Remove the extra comment slash.
Line 66 has an extra slash in the comment (// //). This appears to be a typo.
- // // Deploy VC and Disclose ID verifier
+ // Deploy VC and Disclose ID verifier📝 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.
| // // Deploy VC and Disclose ID verifier | |
| // Deploy VC and Disclose ID verifier |
🤖 Prompt for AI Agents
In contracts/test/utils/deploymentV2.ts around line 66, there's a typo in the
comment containing an extra slash ("// // Deploy VC and Disclose ID verifier");
remove the redundant slash so the comment is a single-line comment ("// Deploy
VC and Disclose ID verifier") to keep style consistent.
| // await registryContract.updateCscaRoot(csca_root, { from: owner }); | ||
| // await registryIdContract.updateCscaRoot(csca_root, { from: owner }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Commented-out code should be removed or explained.
The updateCscaRoot calls are commented out without explanation. If these are no longer needed, remove them entirely. If they're temporarily disabled for testing, add a TODO comment explaining why and when they should be re-enabled.
- // await registryContract.updateCscaRoot(csca_root, { from: owner });
- // await registryIdContract.updateCscaRoot(csca_root, { from: owner });Either remove these lines completely or add context:
+ // TODO: Re-enable once CSCA root initialization is needed for tests
+ // await registryContract.updateCscaRoot(csca_root, { from: owner });
+ // await registryIdContract.updateCscaRoot(csca_root, { from: owner });📝 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.
| // await registryContract.updateCscaRoot(csca_root, { from: owner }); | |
| // await registryIdContract.updateCscaRoot(csca_root, { from: owner }); | |
| // TODO: Re-enable once CSCA root initialization is needed for tests | |
| // await registryContract.updateCscaRoot(csca_root, { from: owner }); | |
| // await registryIdContract.updateCscaRoot(csca_root, { from: owner }); |
🤖 Prompt for AI Agents
In contracts/test/utils/deploymentV2.ts around lines 284-285, two updateCscaRoot
calls are commented out with no explanation; either delete these two commented
lines entirely if they're no longer needed, or replace them with a single clear
TODO comment that states why they are disabled, under what condition they should
be re-enabled (e.g., specific test or environment), and who is responsible; keep
the file tidy by not leaving unexplained commented-out code.
Tranquil-Flow
temporarily deployed
to
development
GitHub Actions
Inactive
Tranquil-Flow
temporarily deployed
to
development
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
contracts/hardhat.config.ts (1)
74-89: customChains endpoints for Celo/Alfajores look incorrect.Use the explorers’ native API endpoints; the current
api.etherscan.io/v2/api?chainid=...won’t work.{ network: "celo", chainId: 42220, urls: { - apiURL: "https://api.etherscan.io/v2/api?chainid=42220", - browserURL: "https://celoscan.io/", + apiURL: "https://api.celoscan.io/api", + browserURL: "https://celoscan.io", }, }, { network: "alfajores", chainId: 44787, urls: { - apiURL: "https://api.etherscan.io/v2/api?chainid=44787", - browserURL: "https://alfajores.celoscan.io", + apiURL: "https://api-alfajores.celoscan.io/api", + browserURL: "https://alfajores.celoscan.io", }, },contracts/test/v2/registerAadhaar.test.ts (1)
21-23: Avoid disabling Mocha timeouts globally (risk of hung CI).
this.timeout(0)can hide deadlocks and hang pipelines. Set a reasonable suite timeout (e.g., 120_000) or only disable for specific long-running tests.
♻️ Duplicate comments (3)
circuits/tests/register/pubkeys.ts (1)
162-191: Another expired certificate added to test data.This certificate expired on 2020-06-07, over 5 years ago. The same concerns apply as with the previous expired certificate.
contracts/hardhat.config.ts (1)
62-66: Fail fast on missing PRIVATE_KEY (avoid "undefined" deployments).
process.env.PRIVATE_KEY as stringwill pass"undefined"when unset. Validate and require it.Apply this diff locally here, and consider applying the same change to all networks using PRIVATE_KEY:
- accounts: [process.env.PRIVATE_KEY as string], + accounts: [requireEnv("PRIVATE_KEY")],Add this helper near the top of the file:
function requireEnv(key: string): string { const v = process.env[key]; if (!v) throw new Error(`Missing required env: ${key}`); return v; }contracts/ignition/modules/hub/updateVerifiers.ts (1)
1-1: Use the Hardhat Ignition plugin import, not ignition-core.Align with other modules to avoid compatibility issues.
-import { buildModule, IgnitionModuleBuilder } from "@nomicfoundation/ignition-core"; +import { buildModule, IgnitionModuleBuilder } from "@nomicfoundation/hardhat-ignition/modules";
🧹 Nitpick comments (4)
circuits/tests/register/pubkeys.ts (1)
1-191: Add documentation and verify certificate source appropriateness.Consider adding:
- Header comments explaining the purpose and source of these certificates
- Indication of whether these are production, staging, or synthetic test certificates
- Criteria for certificate selection (e.g., different issuers, validity periods)
For test data, best practices recommend using synthetic or dedicated test certificates rather than what appear to be real UIDAI production certificates, unless these are officially provided test certificates.
Add documentation like:
+/** + * Test public key certificates for Aadhaar verification circuit testing. + * + * Note: These certificates include both valid and expired certificates to test + * various validation scenarios. Sources: [specify source/authority] + * + * Certificate details: + * - Index 0: [description, validity period] + * - Index 1: [description, validity period] + * ... + */ export const pubkeys = [contracts/ignition/modules/upgrade/deployNewHubAndUpgrade.ts (2)
12-12: Typo in module name.The module name "DeployNewHubAndUpgradee" contains a typo. Consider renaming to "DeployNewHubAndUpgrade" for clarity.
Apply this diff:
-export default buildModule("DeployNewHubAndUpgradee", (m) => { +export default buildModule("DeployNewHubAndUpgrade", (m) => {
41-41: Hard-coded registration window should be configurable.The Aadhaar registration window is hard-coded to 120 seconds. For deployment flexibility across environments (testnet, mainnet, different chains), consider making this a module parameter.
Example refactor to add parameter support:
export default buildModule("DeployNewHubAndUpgrade", (m) => { const registrationWindow = m.getParameter("aadhaarRegistrationWindow", 120); // ... existing code ... m.call(hubProxy, "setAadhaarRegistrationWindow", [registrationWindow], { after: [a] });Then pass parameters during deployment:
// In deployment script or parameters file { "DeployNewHubAndUpgrade": { "aadhaarRegistrationWindow": 120 } }contracts/ignition/modules/registry/updateRegistries.ts (1)
1-1: Use the Hardhat Ignition plugin import, not ignition-core.Prevents version drift and plugin incompatibility.
-import { buildModule, IgnitionModuleBuilder } from "@nomicfoundation/ignition-core"; +import { buildModule, IgnitionModuleBuilder } from "@nomicfoundation/hardhat-ignition/modules";
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
circuits/tests/register/pubkeys.ts(2 hunks)circuits/tests/register/register_aadhaar.test.ts(2 hunks)contracts/contracts/tests/testUpgradedIdentityVerificationHubImplV2.sol(1 hunks)contracts/hardhat.config.ts(2 hunks)contracts/ignition/modules/hub/updateRegistries.ts(1 hunks)contracts/ignition/modules/hub/updateVerifiers.ts(1 hunks)contracts/ignition/modules/registry/updateRegistries.ts(1 hunks)contracts/ignition/modules/upgrade/deployNewHubAndUpgrade.ts(1 hunks)contracts/test/v2/registerAadhaar.test.ts(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- contracts/contracts/tests/testUpgradedIdentityVerificationHubImplV2.sol
- circuits/tests/register/register_aadhaar.test.ts
- contracts/ignition/modules/hub/updateRegistries.ts
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{js,ts,tsx,jsx,sol,nr}
📄 CodeRabbit inference engine (.cursorrules)
**/*.{js,ts,tsx,jsx,sol,nr}: NEVER log sensitive data including PII (names, DOB, passport numbers, addresses), credentials, tokens, API keys, private keys, or session identifiers.
ALWAYS redact/mask sensitive fields in logs using consistent patterns (e.g.,***-***-1234for passport numbers,J*** D***for names).
Files:
contracts/test/v2/registerAadhaar.test.tscontracts/ignition/modules/hub/updateVerifiers.tscontracts/ignition/modules/registry/updateRegistries.tscircuits/tests/register/pubkeys.tscontracts/hardhat.config.tscontracts/ignition/modules/upgrade/deployNewHubAndUpgrade.ts
contracts/**
📄 CodeRabbit inference engine (AGENTS.md)
Compile Solidity contracts using
yarn workspace @selfxyz/contracts build
Files:
contracts/test/v2/registerAadhaar.test.tscontracts/ignition/modules/hub/updateVerifiers.tscontracts/ignition/modules/registry/updateRegistries.tscontracts/hardhat.config.tscontracts/ignition/modules/upgrade/deployNewHubAndUpgrade.ts
**/*.{test,spec}.{ts,js,tsx,jsx}
⚙️ CodeRabbit configuration file
**/*.{test,spec}.{ts,js,tsx,jsx}: Review test files for:
- Test coverage completeness
- Test case quality and edge cases
- Mock usage appropriateness
- Test readability and maintainability
Files:
contracts/test/v2/registerAadhaar.test.ts
🧬 Code graph analysis (1)
contracts/ignition/modules/hub/updateVerifiers.ts (1)
contracts/ignition/modules/verifiers/deployAllVerifiersNew.ts (2)
CircuitName(4-89)circuitIds(92-180)
⏰ Context from checks skipped due to timeout of 300000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: run_circuit_tests
🔇 Additional comments (5)
circuits/tests/register/pubkeys.ts (1)
80-124: Handle expired certificate explicitly in tests.The new PEM cert expired on 2024-02-27, yet no expiry checks were found in circuits/tests/register—expired certs may be silently accepted or cause unexpected failures. Either:
- Document that this cert is intentionally expired
- Add test assertions to verify correct rejection or handling of expired certificates
contracts/ignition/modules/upgrade/deployNewHubAndUpgrade.ts (2)
18-21: No action needed: deployment key verified
KeyDeployHubV2#IdentityVerificationHubis present in deployed_addresses.json, so the lookup will succeed.
37-41: Ensure atomic registration-window configuration
AADHAAR_REGISTRATION_WINDOWis hard-coded to 20 ininitialize()and only updated to 120 via a separatesetAadhaarRegistrationWindowcall; confirm this two-step upgrade flow is intentional or consider extendinginitialize()to accept and set the window atomically.contracts/hardhat.config.ts (2)
69-73: Scope etherscan.apiKey per network: using a singleprocess.env.CELOSCAN_API_KEYprevents verification on mainnet/Sepolia—replace with an object mapping (mainnet/sepolia→ETHERSCAN_API_KEY,celo/alfajores→CELOSCAN_API_KEY). Confirm @nomicfoundation/hardhat-verify v2.x supports per-network keys.
90-97: Blockscout plugin compatibility: @nomicfoundation/hardhat-verify v^2.0.6 is in use; confirm it supports customChains with Blockscout endpoints.
| const verifierName = `Verifier_${circuitName}`; | ||
| const verifierAddress = deployedAddresses[`DeployAllVerifiers#${verifierName}`]; | ||
|
|
||
| if (verifierAddress) { | ||
| verifiers[circuitName] = verifierAddress; | ||
| } | ||
| } |
There was a problem hiding this comment.
Verifier addresses likely not found (module name mismatch).
Deployment script is deployAllVerifiersNew.ts; keys are probably prefixed with DeployAllVerifiersNew#..., not DeployAllVerifiers#.... Current lookup will silently skip verifiers.
- const verifierName = `Verifier_${circuitName}`;
- const verifierAddress = deployedAddresses[`DeployAllVerifiers#${verifierName}`];
+ const verifierName = `Verifier_${circuitName}`;
+ const verifierAddress =
+ deployedAddresses[`DeployAllVerifiersNew#${verifierName}`] ??
+ deployedAddresses[`DeployAllVerifiers#${verifierName}`];Optional hardening: scan by suffix to decouple from module name:
const verifierAddress =
Object.entries(deployedAddresses).find(([k]) => k.endsWith(`#${verifierName}`))?.[1];🤖 Prompt for AI Agents
In contracts/ignition/modules/hub/updateVerifiers.ts around lines 61-67, the
code looks up deployed verifier addresses using the key prefix
"DeployAllVerifiers#...", but the deployment script used is
deployAllVerifiersNew.ts so the actual keys are likely
"DeployAllVerifiersNew#..."; update the lookup to use the correct module prefix
or, better, make it robust by scanning deployedAddresses for keys that end with
`#${verifierName}` (use Object.entries(...).find(([k]) =>
k.endsWith(`#${verifierName}`))?.[1]) so verifiers are not silently skipped when
module name changes.
| if (registryData.hub) { | ||
| const callOptions = { after: [currentOperation], id: ids() }; | ||
| currentOperation = m.call(registryContract, "updateHub", [registryData.hub], callOptions); | ||
| } |
There was a problem hiding this comment.
Hard-coded hub address reduces portability; derive from deployments.
Using a literal hub in registries ties the module to one chain. Fallback to the hub address in deployed_addresses if not provided.
- if (registryData.hub) {
- const callOptions = { after: [currentOperation], id: ids() };
- currentOperation = m.call(registryContract, "updateHub", [registryData.hub], callOptions);
- }
+ {
+ const resolvedHub =
+ registryData.hub ?? deployedAddresses["DeployHubV2#IdentityVerificationHub"];
+ if (!resolvedHub) {
+ throw new Error("Hub address missing: provide in registries[] or deployments file");
+ }
+ const callOptions = { after: [currentOperation], id: ids() };
+ currentOperation = m.call(registryContract, "updateHub", [resolvedHub], callOptions);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (registryData.hub) { | |
| const callOptions = { after: [currentOperation], id: ids() }; | |
| currentOperation = m.call(registryContract, "updateHub", [registryData.hub], callOptions); | |
| } | |
| { | |
| const resolvedHub = | |
| registryData.hub ?? deployedAddresses["DeployHubV2#IdentityVerificationHub"]; | |
| if (!resolvedHub) { | |
| throw new Error("Hub address missing: provide in registries[] or deployments file"); | |
| } | |
| const callOptions = { after: [currentOperation], id: ids() }; | |
| currentOperation = m.call(registryContract, "updateHub", [resolvedHub], callOptions); | |
| } |
🤖 Prompt for AI Agents
In contracts/ignition/modules/registry/updateRegistries.ts around lines 75 to
78, the code currently uses registryData.hub directly which hard-codes the hub
address; change it to derive the hub from registryData.hub with a fallback to
the deployed_addresses hub entry (or a deployment lookup helper) for the current
chain. Concretely, resolve a local variable hub = registryData.hub ??
deployed_addresses.hub (or call the existing deployment resolver with the
chain/context) and pass [hub] into m.call(... "updateHub", [hub], callOptions);
keep callOptions and other logic unchanged.
Summary by CodeRabbit
New Features
Chores
Tests