Skip to content

Conversation

@sirpy
Copy link
Contributor

@sirpy sirpy commented Feb 2, 2026

Description

Copy link

@sourcery-ai sourcery-ai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey - I've found 2 issues, and left some high level feedback:

  • In IdentityV4.initialize you call _setupRole with avatar before DAO/nameService are initialized, so roles are being granted to the zero address; consider moving avatar role setup entirely into initDAO (or passing the DAO in initialize) to avoid granting admin/pauser/identity roles to an address that will never be used.
  • In _setDID you require isWhitelisted(account), which depends on the time‑based reverification logic; this prevents DID updates for users whose status is still 1 but whose authentication has expired—if you intend to allow updates for such accounts, consider checking identities[account].status == 1 instead of isWhitelisted.
Prompt for AI Agents
Please address the comments from this code review:

## Overall Comments
- In `IdentityV4.initialize` you call `_setupRole` with `avatar` before DAO/nameService are initialized, so roles are being granted to the zero address; consider moving `avatar` role setup entirely into `initDAO` (or passing the DAO in `initialize`) to avoid granting admin/pauser/identity roles to an address that will never be used.
- In `_setDID` you require `isWhitelisted(account)`, which depends on the time‑based reverification logic; this prevents DID updates for users whose status is still `1` but whose authentication has expired—if you intend to allow updates for such accounts, consider checking `identities[account].status == 1` instead of `isWhitelisted`.

## Individual Comments

### Comment 1
<location> `test/helpers.ts:125` </location>
<code_context>
+    kind: "uups"
+  })) as IdentityV4;
+  // put reverify far in the future to avoid interference with tests
+  if (identity === "v4") await Identity.setReverifyDaysOptions([180]);
   const daoCreator = await DAOCreatorFactory.deploy();
   const FeeFormula = await FeeFormulaFactory.deploy(0);
</code_context>

<issue_to_address>
**issue (testing):** Global override of `reverifyDaysOptions` for v4 identities conflicts with tests that expect the default schedule

In `createDAO`, `reverifyDaysOptions([180])` is always set for `identity === "v4"`, but `IdentityV4.test.ts` relies on the contract’s default schedule (e.g. `[1,7,180]`) to exercise multi-step reverify behavior. Any test using `createDAO()` with the default `identity` will now see `[180]`, making those tests fail or give misleading coverage. Please either move this override into a dedicated fixture used only where a `[180]` schedule is required, or update the relevant tests to explicitly expect the overridden value instead of the initializer defaults.
</issue_to_address>

### Comment 2
<location> `test/identity/IdentityV4.test.ts:391-400` </location>
<code_context>
+  it("should follow reverify schedule and cycle authCount", async () => {
</code_context>

<issue_to_address>
**issue (testing):** Reverify schedule test assumes a specific default schedule that may not hold given the shared DAO fixture

This test assumes `reverifyDaysOptions` is `[1,7,180]`, but the shared `createDAO` fixture overrides v4 identities to `[180]`. So the asserted schedule may not match the actual contract state. To make this test reliable, either (a) use a dedicated IdentityV4 fixture that doesn’t override the defaults, (b) explicitly call `setReverifyDaysOptions([1,7,180])` in this test and assert those values, or (c) update the expectations to match the intended `[180]` override if that’s the true requirement.
</issue_to_address>

Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@openzeppelin-code
Copy link

openzeppelin-code bot commented Feb 2, 2026

Feat/identity v4

Generated at commit: 0038d56eb5b62c138352bffd80dfaaca5bc866d6

🚨 Report Summary

Severity Level Results
Contracts Critical
High
Medium
Low
Note
Total
3
5
0
16
44
68
Dependencies Critical
High
Medium
Low
Note
Total
0
0
1
0
135
136

For more details view the full report in OpenZeppelin Code Inspector

@sirpy sirpy requested review from L03TJ3 and blueogin February 2, 2026 13:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants