[Bug]: Fix: enforce required identity fields (name + email/phone)#63
[Bug]: Fix: enforce required identity fields (name + email/phone)#63Nikuunj wants to merge 24 commits into
Conversation
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
|
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:
WalkthroughAdds required-field validation for identities (name plus email or phone), conditional validation in attribute setters, attribute deletion and contact setter APIs, identity view helpers, a Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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}
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 (2)
src/IdentityToken.sol (2)
79-108:⚠️ Potential issue | 🟠 MajorAdd regression coverage for the new revert paths and bootstrap flow.
This change introduces
MissingName/MissingContactand alters a critical write path, but the diff does not include corresponding tests. Please cover the negative cases (missing name, missing contact, clearing the last contact/name) and the valid initialization path that should remain possible after the bypass is fixed, especiallysetAttributesBatchsettingnameplus one contact in a single transaction.As per coding guidelines, "Verify that any modification to contract logic includes corresponding updates to automated tests." and "Ensure security-sensitive logic changes are not introduced without adequate test coverage."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/IdentityToken.sol` around lines 79 - 108, The PR introduced new revert paths Errors.MissingName and Errors.MissingContact via _validateRequiredFields called from setAttribute (and underlying _setAttribute), but no tests were added; add regression unit tests that assert: calling setAttribute to clear/omit "name" reverts with MissingName, clearing the last contact or setting both "email" and "phone" to empty reverts with MissingContact, and that a single-transaction initializer path using setAttributesBatch (or the batch setter that calls _setAttribute) can set "name" plus at least one contact successfully (happy path), plus tests covering the bootstrap/initialization sequence to ensure the bypassed flow still allows valid creation; use the function names _validateRequiredFields, setAttribute, _setAttribute, setAttributesBatch and the error symbols Errors.MissingName and Errors.MissingContact to locate assertions.
101-108:⚠️ Potential issue | 🔴 CriticalRequired-field validation is still bypassable.
Line 108 only wires the check into
setAttribute.setAttributesBatch,setName, andsetGithubstill write through_setAttributewithout any post-write validation, so callers can still persist incomplete identities through those public entry points. This also means first-time setup is pushed toward the unvalidated paths, becausesetAttributenow reverts until both required fields already exist.🔧 Possible fix
function setAttribute( uint256 tokenId, string calldata key, bytes calldata value ) external onlyTokenOwner(tokenId) notCompromised(tokenId) { _setAttribute(tokenId, key, value); _validateRequiredFields(tokenId); } @@ function setAttributesBatch( uint256 tokenId, string[] calldata keys, bytes[] calldata values ) external onlyTokenOwner(tokenId) notCompromised(tokenId) { if (keys.length != values.length) revert Errors.ArrayLengthMismatch(); for (uint256 i = 0; i < keys.length; i++) { _setAttribute(tokenId, keys[i], values[i]); } + _validateRequiredFields(tokenId); } @@ function setName(uint256 tokenId, string calldata name) external onlyTokenOwner(tokenId) notCompromised(tokenId) { _setAttribute(tokenId, "name", bytes(name)); + _validateRequiredFields(tokenId); } @@ function setGithub( uint256 tokenId, string calldata github ) external onlyTokenOwner(tokenId) notCompromised(tokenId) { _setAttribute(tokenId, "github", bytes(github)); + _validateRequiredFields(tokenId); }As per coding guidelines, "Review the Solidity contracts for security vulnerabilities and adherence to best practices." and "Ensure failure paths and revert scenarios are explicitly handled and validated."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/IdentityToken.sol` around lines 101 - 108, The required-field validation is only enforced in setAttribute, letting setAttributesBatch, setName, and setGithub bypass it; fix by ensuring validation runs after every attribute write — either call _validateRequiredFields(tokenId) at the end of setAttributesBatch, setName, and setGithub, or (preferably) centralize by invoking _validateRequiredFields(tokenId) at the end of _setAttribute after the state write so every path (setAttribute, setAttributesBatch, setName, setGithub) performs the same post-write validation.
🤖 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 `@src/IdentityToken.sol`:
- Around line 79-108: The PR introduced new revert paths Errors.MissingName and
Errors.MissingContact via _validateRequiredFields called from setAttribute (and
underlying _setAttribute), but no tests were added; add regression unit tests
that assert: calling setAttribute to clear/omit "name" reverts with MissingName,
clearing the last contact or setting both "email" and "phone" to empty reverts
with MissingContact, and that a single-transaction initializer path using
setAttributesBatch (or the batch setter that calls _setAttribute) can set "name"
plus at least one contact successfully (happy path), plus tests covering the
bootstrap/initialization sequence to ensure the bypassed flow still allows valid
creation; use the function names _validateRequiredFields, setAttribute,
_setAttribute, setAttributesBatch and the error symbols Errors.MissingName and
Errors.MissingContact to locate assertions.
- Around line 101-108: The required-field validation is only enforced in
setAttribute, letting setAttributesBatch, setName, and setGithub bypass it; fix
by ensuring validation runs after every attribute write — either call
_validateRequiredFields(tokenId) at the end of setAttributesBatch, setName, and
setGithub, or (preferably) centralize by invoking
_validateRequiredFields(tokenId) at the end of _setAttribute after the state
write so every path (setAttribute, setAttributesBatch, setName, setGithub)
performs the same post-write validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 76877050-0b35-4745-8936-c83fdabfc40f
📒 Files selected for processing (2)
src/IdentityToken.solsrc/libraries/Errors.sol
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/IdentityToken.sol (2)
140-152:⚠️ Potential issue | 🔴 CriticalCritical: Validation bypass via helper setters.
setNameandsetGithubalso bypass_validateRequiredFields. A user calling onlysetNamecan create an identity without any contact method, violating the invariant.🐛 Proposed fix: Add validation to helper setters
function setName(uint256 tokenId, string calldata name) external onlyTokenOwner(tokenId) notCompromised(tokenId) { _setAttribute(tokenId, "name", bytes(name)); + _validateRequiredFields(tokenId); } function setGithub( uint256 tokenId, string calldata github ) external onlyTokenOwner(tokenId) notCompromised(tokenId) { _setAttribute(tokenId, "github", bytes(github)); + _validateRequiredFields(tokenId); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/IdentityToken.sol` around lines 140 - 152, The helper setters setName and setGithub currently call _setAttribute directly and thereby bypass the invariant check in _validateRequiredFields; update both functions (setName and setGithub) to invoke _validateRequiredFields(tokenId) immediately after calling _setAttribute(tokenId, ...) so the token is validated (and reverted if required fields are missing) while preserving the existing onlyTokenOwner and notCompromised modifiers and use of _setAttribute.
101-107:⚠️ Potential issue | 🔴 CriticalCritical: Validation bypass via
setAttribute.The PR objective states validation should be integrated into
setAttribute, but this function does not call_validateRequiredFields. Users can bypass the mandatory name/contact requirement by callingsetAttributedirectly instead ofsetAttributesBatch, defeating the purpose of the fix for Issue#54.Either add validation here (with consideration for incremental attribute setting UX), or document and enforce that users must use
setAttributesBatchfor complete identity setup.🐛 Option 1: Add validation to setAttribute
function setAttribute( uint256 tokenId, string calldata key, bytes calldata value ) external onlyTokenOwner(tokenId) notCompromised(tokenId) { _setAttribute(tokenId, key, value); + _validateRequiredFields(tokenId); }Note: This approach may impact UX if users want to set attributes incrementally. Consider the trade-off or provide a separate initialization flow.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/IdentityToken.sol` around lines 101 - 107, The setAttribute function currently calls _setAttribute but omits required-field validation, allowing callers to bypass _validateRequiredFields; update setAttribute (or an initialization flow) to invoke _validateRequiredFields(tokenId) after _setAttribute so each single-attribute change cannot leave the token without required name/contact, or alternatively enforce/throw if callers attempt single updates and require use of setAttributesBatch (update setAttribute to revert with guidance to use setAttributesBatch); reference functions: setAttribute, _setAttribute, _validateRequiredFields, and setAttributesBatch when making the change.test/IdentityToken.t.sol (3)
176-202:⚠️ Potential issue | 🟠 MajorMissing test coverage for new validation errors.
No tests verify that
MissingNameandMissingContacterrors are correctly raised. Per coding guidelines, security-sensitive logic changes require adequate test coverage.💚 Proposed tests for validation errors
function test_RevertIf_BatchMissingName() public { vm.prank(alice); uint256 tokenId = identityToken.mint(); string[] memory keys = new string[](1); keys[0] = "email"; bytes[] memory values = new bytes[](1); values[0] = bytes("alice@example.com"); vm.prank(alice); vm.expectRevert(Errors.MissingName.selector); identityToken.setAttributesBatch(tokenId, keys, values); } function test_RevertIf_BatchMissingContact() public { vm.prank(alice); uint256 tokenId = identityToken.mint(); string[] memory keys = new string[](1); keys[0] = "name"; bytes[] memory values = new bytes[](1); values[0] = bytes("Alice Nakamoto"); vm.prank(alice); vm.expectRevert(Errors.MissingContact.selector); identityToken.setAttributesBatch(tokenId, keys, values); } function test_BatchWithPhoneAsContact() public { vm.prank(alice); uint256 tokenId = identityToken.mint(); string[] memory keys = new string[](2); keys[0] = "name"; keys[1] = "phone"; bytes[] memory values = new bytes[](2); values[0] = bytes("Alice Nakamoto"); values[1] = bytes("+1234567890"); vm.prank(alice); identityToken.setAttributesBatch(tokenId, keys, values); assertEq(string(identityToken.getAttribute(tokenId, "name")), "Alice Nakamoto"); assertEq(string(identityToken.getAttribute(tokenId, "phone")), "+1234567890"); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/IdentityToken.t.sol` around lines 176 - 202, The test suite lacks coverage asserting that setAttributesBatch enforces the new validation errors MissingName and MissingContact; add unit tests in IdentityToken.t.sol that mint a token as alice, call identityToken.setAttributesBatch with (a) only an "email" key and assert vm.expectRevert(Errors.MissingName.selector) and (b) only a "name" key and assert vm.expectRevert(Errors.MissingContact.selector), and also add a positive case that uses "name" + "phone" to verify phone is accepted as a valid contact; reference the identityToken.setAttributesBatch call, Errors.MissingName, Errors.MissingContact, and identityToken.getAttribute to locate where to add these tests.
1-1:⚠️ Potential issue | 🟡 MinorFix Prettier formatting issues.
The pipeline reports a Prettier check failure. Run
prettier --write "**/*.sol" --plugin=prettier-plugin-solidityto fix code style issues.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/IdentityToken.t.sol` at line 1, The test file IdentityToken.t.sol has Prettier formatting violations; run the project's Solidity Prettier formatter to fix it by executing the exact command suggested (prettier --write "**/*.sol" --plugin=prettier-plugin-solidity) and re-commit the reformatted IdentityToken.t.sol (and any other .sol files) so the Prettier pipeline check passes.
152-170:⚠️ Potential issue | 🟡 MinorTests for helper setters expose validation gap.
test_SetNameandtest_SetGithubpass without setting any contact method, confirming thatsetNameandsetGithubdo not enforce the required-fields validation. Once the validation bypass issue in the contract is fixed, these tests will need updates to also set an email or phone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/IdentityToken.t.sol` around lines 152 - 170, The tests test_SetName and test_SetGithub call identityToken.setName and setGithub without first setting a required contact method, which masks the missing validation in the contract; update both tests to, after minting (tokenId) and before calling setName/setGithub, set a contact method (e.g., call the token's setEmail or setPhone helper on identityToken for tokenId) so the required-fields validation is satisfied, then assert the attribute via identityToken.attributes(tokenId, Schema.NAME) and Schema.GITHUB as before; use Schema.EMAIL or Schema.PHONE in assertions where appropriate to mirror the contract's required-field check.
🤖 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/IdentityToken.sol`:
- Around line 79-96: Pre-compute and reuse the keccak256 hashes for the static
attribute keys instead of recomputing them in _validateRequiredFields: declare
bytes32 constants (e.g., KEY_NAME, KEY_EMAIL, KEY_PHONE) at the contract level
or import/reuse the equivalents from Schema.sol, then replace the inline
keccak256(abi.encodePacked("name"/"email"/"phone")) lookups in
_validateRequiredFields with attributes[tokenId][KEY_NAME],
attributes[tokenId][KEY_EMAIL], and attributes[tokenId][KEY_PHONE] to reduce
gas.
---
Outside diff comments:
In `@src/IdentityToken.sol`:
- Around line 140-152: The helper setters setName and setGithub currently call
_setAttribute directly and thereby bypass the invariant check in
_validateRequiredFields; update both functions (setName and setGithub) to invoke
_validateRequiredFields(tokenId) immediately after calling
_setAttribute(tokenId, ...) so the token is validated (and reverted if required
fields are missing) while preserving the existing onlyTokenOwner and
notCompromised modifiers and use of _setAttribute.
- Around line 101-107: The setAttribute function currently calls _setAttribute
but omits required-field validation, allowing callers to bypass
_validateRequiredFields; update setAttribute (or an initialization flow) to
invoke _validateRequiredFields(tokenId) after _setAttribute so each
single-attribute change cannot leave the token without required name/contact, or
alternatively enforce/throw if callers attempt single updates and require use of
setAttributesBatch (update setAttribute to revert with guidance to use
setAttributesBatch); reference functions: setAttribute, _setAttribute,
_validateRequiredFields, and setAttributesBatch when making the change.
In `@test/IdentityToken.t.sol`:
- Around line 176-202: The test suite lacks coverage asserting that
setAttributesBatch enforces the new validation errors MissingName and
MissingContact; add unit tests in IdentityToken.t.sol that mint a token as
alice, call identityToken.setAttributesBatch with (a) only an "email" key and
assert vm.expectRevert(Errors.MissingName.selector) and (b) only a "name" key
and assert vm.expectRevert(Errors.MissingContact.selector), and also add a
positive case that uses "name" + "phone" to verify phone is accepted as a valid
contact; reference the identityToken.setAttributesBatch call,
Errors.MissingName, Errors.MissingContact, and identityToken.getAttribute to
locate where to add these tests.
- Line 1: The test file IdentityToken.t.sol has Prettier formatting violations;
run the project's Solidity Prettier formatter to fix it by executing the exact
command suggested (prettier --write "**/*.sol"
--plugin=prettier-plugin-solidity) and re-commit the reformatted
IdentityToken.t.sol (and any other .sol files) so the Prettier pipeline check
passes.
- Around line 152-170: The tests test_SetName and test_SetGithub call
identityToken.setName and setGithub without first setting a required contact
method, which masks the missing validation in the contract; update both tests
to, after minting (tokenId) and before calling setName/setGithub, set a contact
method (e.g., call the token's setEmail or setPhone helper on identityToken for
tokenId) so the required-fields validation is satisfied, then assert the
attribute via identityToken.attributes(tokenId, Schema.NAME) and Schema.GITHUB
as before; use Schema.EMAIL or Schema.PHONE in assertions where appropriate to
mirror the contract's required-field check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: fac4d843-193a-43f3-aa71-beed0ad8a876
📒 Files selected for processing (2)
src/IdentityToken.soltest/IdentityToken.t.sol
| /** | ||
| * @dev Adds validation to ensure required identity fields are present. | ||
| * Name is mandatory and at least one contact method (email or phone) | ||
| * must be provided. | ||
| */ | ||
| function _validateRequiredFields(uint256 tokenId) internal view { | ||
| bytes memory name = attributes[tokenId][keccak256(abi.encodePacked("name"))]; | ||
| bytes memory email = attributes[tokenId][keccak256(abi.encodePacked("email"))]; | ||
| bytes memory phone = attributes[tokenId][keccak256(abi.encodePacked("phone"))]; | ||
|
|
||
| // Name is mandatory | ||
| if (name.length == 0) revert Errors.MissingName(); | ||
|
|
||
| // At least one contact method required | ||
| if (email.length == 0 && phone.length == 0) { | ||
| revert Errors.MissingContact(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Gas optimization: Pre-compute constant key hashes.
The function computes keccak256(abi.encodePacked("name")), keccak256(abi.encodePacked("email")), and keccak256(abi.encodePacked("phone")) on every call. Since these are static strings, declare them as constant bytes32 at the contract level (or reuse from Schema.sol if already defined) to save gas.
♻️ Proposed refactor
Add constants at contract level:
bytes32 private constant KEY_NAME = keccak256(abi.encodePacked("name"));
bytes32 private constant KEY_EMAIL = keccak256(abi.encodePacked("email"));
bytes32 private constant KEY_PHONE = keccak256(abi.encodePacked("phone"));Then refactor the function:
function _validateRequiredFields(uint256 tokenId) internal view {
- bytes memory name = attributes[tokenId][keccak256(abi.encodePacked("name"))];
- bytes memory email = attributes[tokenId][keccak256(abi.encodePacked("email"))];
- bytes memory phone = attributes[tokenId][keccak256(abi.encodePacked("phone"))];
+ bytes memory name = attributes[tokenId][KEY_NAME];
+ bytes memory email = attributes[tokenId][KEY_EMAIL];
+ bytes memory phone = attributes[tokenId][KEY_PHONE];
// Name is mandatory
if (name.length == 0) revert Errors.MissingName();
// At least one contact method required
if (email.length == 0 && phone.length == 0) {
revert Errors.MissingContact();
}
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/IdentityToken.sol` around lines 79 - 96, Pre-compute and reuse the
keccak256 hashes for the static attribute keys instead of recomputing them in
_validateRequiredFields: declare bytes32 constants (e.g., KEY_NAME, KEY_EMAIL,
KEY_PHONE) at the contract level or import/reuse the equivalents from
Schema.sol, then replace the inline
keccak256(abi.encodePacked("name"/"email"/"phone")) lookups in
_validateRequiredFields with attributes[tokenId][KEY_NAME],
attributes[tokenId][KEY_EMAIL], and attributes[tokenId][KEY_PHONE] to reduce
gas.
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 (1)
test/IdentityToken.t.sol (1)
176-202:⚠️ Potential issue | 🟠 MajorAdd negative-path tests for
MissingNameandMissingContactin batch validation.The updated batch tests cover only success flows. Please add explicit
vm.expectRevert(Errors.MissingName.selector)andvm.expectRevert(Errors.MissingContact.selector)cases to validate the new failure paths.🧪 Proposed test additions
+ function test_RevertIf_BatchMissingName() public { + vm.prank(alice); + uint256 tokenId = identityToken.mint(); + + string[] memory keys = new string[](1); + keys[0] = "email"; + bytes[] memory values = new bytes[](1); + values[0] = bytes("alice@example.com"); + + vm.prank(alice); + vm.expectRevert(Errors.MissingName.selector); + identityToken.setAttributesBatch(tokenId, keys, values); + } + + function test_RevertIf_BatchMissingContact() public { + vm.prank(alice); + uint256 tokenId = identityToken.mint(); + + string[] memory keys = new string[](1); + keys[0] = "name"; + bytes[] memory values = new bytes[](1); + values[0] = bytes("Alice Nakamoto"); + + vm.prank(alice); + vm.expectRevert(Errors.MissingContact.selector); + identityToken.setAttributesBatch(tokenId, keys, values); + }As per coding guidelines, "Ensure failure paths and revert scenarios are explicitly handled and validated."
Also applies to: 208-222
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/IdentityToken.t.sol` around lines 176 - 202, Add negative-path assertions to test_SetAttributesBatch by inserting vm.expectRevert calls for Errors.MissingName.selector and Errors.MissingContact.selector before calling identityToken.setAttributesBatch with batches that omit a non-empty "name" or both contact fields respectively; locate the test_SetAttributesBatch function and create two additional cases: one where keys/values omit or pass an empty "name" to trigger Errors.MissingName via identityToken.setAttributesBatch, and one where keys/values omit both contact entries (e.g., "email" and "github") or pass them empty to trigger Errors.MissingContact, using vm.prank as needed and wrapping each call with vm.expectRevert(Errors.MissingName.selector) or vm.expectRevert(Errors.MissingContact.selector).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@test/IdentityToken.t.sol`:
- Around line 204-216: The test function test_SetAttributesBatch_SingleEntry is
misnamed because it sets three attribute entries; rename the function to reflect
its behavior (e.g., test_SetAttributesBatch_MultipleEntries or
test_SetAttributesBatch_ThreeEntries) so its name matches the keys/values arrays
being used; update the function declaration identifier
test_SetAttributesBatch_SingleEntry to the new name throughout the file (keeping
the rest of the test body, including vm.prank, identityToken.mint(), keys array,
and values array, unchanged).
- Around line 40-45: Remove the pre-population of "name"/"email" in the
single-attribute tests and add explicit revert assertions that calling
identityToken.setAttribute(tokenId, "name", ... ) or
identityToken.setAttribute(tokenId, "email", ... ) (and other single-field
writes) without the complementary required field fails; use vm.prank to call
identityToken.setAttribute(...) and vm.expectRevert to assert the specific
revert message/selector emitted by setAttribute when validation is missing,
while keeping existing batch test coverage for setAttributes intact so you
verify both single-setter validation and batch validation paths.
---
Outside diff comments:
In `@test/IdentityToken.t.sol`:
- Around line 176-202: Add negative-path assertions to test_SetAttributesBatch
by inserting vm.expectRevert calls for Errors.MissingName.selector and
Errors.MissingContact.selector before calling identityToken.setAttributesBatch
with batches that omit a non-empty "name" or both contact fields respectively;
locate the test_SetAttributesBatch function and create two additional cases: one
where keys/values omit or pass an empty "name" to trigger Errors.MissingName via
identityToken.setAttributesBatch, and one where keys/values omit both contact
entries (e.g., "email" and "github") or pass them empty to trigger
Errors.MissingContact, using vm.prank as needed and wrapping each call with
vm.expectRevert(Errors.MissingName.selector) or
vm.expectRevert(Errors.MissingContact.selector).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: d22e7c21-9e09-4796-8a0f-15b6073fa99f
📒 Files selected for processing (1)
test/IdentityToken.t.sol
| function test_SetAttributesBatch_SingleEntry() public { | ||
| vm.prank(alice); | ||
| uint256 tokenId = identityToken.mint(); | ||
|
|
||
| string[] memory keys = new string[](1); | ||
| keys[0] = "age"; | ||
| string[] memory keys = new string[](3); | ||
| keys[0] = "name"; | ||
| keys[1] = "email"; | ||
| keys[2] = "age"; | ||
|
|
||
| bytes[] memory values = new bytes[](1); | ||
| values[0] = bytes("30"); | ||
| bytes[] memory values = new bytes[](3); | ||
| values[0] = bytes("Alice Nakamoto"); | ||
| values[1] = bytes("alice@example.com"); | ||
| values[2] = bytes("30"); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Rename test_SetAttributesBatch_SingleEntry to match its current behavior.
At Line 204, the test name says "SingleEntry" but the test now submits 3 entries. Rename to avoid confusion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/IdentityToken.t.sol` around lines 204 - 216, The test function
test_SetAttributesBatch_SingleEntry is misnamed because it sets three attribute
entries; rename the function to reflect its behavior (e.g.,
test_SetAttributesBatch_MultipleEntries or test_SetAttributesBatch_ThreeEntries)
so its name matches the keys/values arrays being used; update the function
declaration identifier test_SetAttributesBatch_SingleEntry to the new name
throughout the file (keeping the rest of the test body, including vm.prank,
identityToken.mint(), keys array, and values array, unchanged).
Implement various Identity Query Functions
There was a problem hiding this comment.
Pull request overview
This PR aims to enforce required on-chain identity metadata invariants for IdentityToken: a non-empty name plus at least one contact method (email or phone), addressing issue #54.
Changes:
- Added required-field validation logic (
_validateRequiredFields) and introduced new custom errors (MissingName,MissingContact). - Enforced required-field validation after
setAttributesBatch. - Updated existing tests to populate
name/emailin more scenarios and adjusted batch test inputs.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
src/IdentityToken.sol |
Adds required-field validation helper and invokes it after batch updates. |
src/libraries/Errors.sol |
Adds MissingName / MissingContact custom errors. |
test/IdentityToken.t.sol |
Updates tests to set name and email in more setups; expands batch test inputs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @@ -111,6 +130,8 @@ contract IdentityToken is ERC721, IIdentityToken { | |||
| for (uint256 i = 0; i < keys.length; i++) { | |||
| _setAttribute(tokenId, keys[i], values[i]); | |||
| } | |||
|
|
|||
| _validateRequiredFields(tokenId); | |||
| } | |||
There was a problem hiding this comment.
_validateRequiredFields is only invoked from setAttributesBatch, so identities can still end up missing name and/or contact by using setAttribute (or typed setters) to set other fields or to clear name/email/phone. If the intent is to enforce the invariant on-chain (per issue/PR description), ensure the validation is also applied on the single-attribute write paths (and consider allowing incremental setup by exempting calls that set name/email/phone themselves, while still preventing clearing the last required field).
| function _validateRequiredFields(uint256 tokenId) internal view { | ||
| bytes memory name = attributes[tokenId][keccak256(abi.encodePacked("name"))]; | ||
| bytes memory email = attributes[tokenId][keccak256(abi.encodePacked("email"))]; | ||
| bytes memory phone = attributes[tokenId][keccak256(abi.encodePacked("phone"))]; | ||
|
|
||
| // Name is mandatory | ||
| if (name.length == 0) revert Errors.MissingName(); | ||
|
|
||
| // At least one contact method required | ||
| if (email.length == 0 && phone.length == 0) { | ||
| revert Errors.MissingContact(); | ||
| } | ||
| } |
There was a problem hiding this comment.
_validateRequiredFields recomputes keccak256(abi.encodePacked(...)) on every call and copies the stored bytes values into memory. To reduce gas and avoid key-string drift/typos, consider using precomputed key hashes (e.g., extend/use the existing Schema library) and read lengths directly from storage (bytes storage) instead of copying to memory.
| // Set name first, then email to satisfy validation | ||
| vm.prank(alice); | ||
| identityToken.setAttribute(tokenId, "name", bytes("Alice Nakamoto")); | ||
| vm.prank(alice); | ||
| identityToken.setAttribute(tokenId, "email", bytes("alice@example.com")); |
There was a problem hiding this comment.
The added test comment says setting name/email is needed to “satisfy validation”, but the contract currently doesn’t validate on setAttribute (only after setAttributesBatch). Either add coverage that demonstrates the validation actually triggers for single-attribute updates, or adjust/remove this comment so the test reflects the real behavior being asserted.
| function test_SetAttributesBatch() public { | ||
| vm.prank(alice); | ||
| uint256 tokenId = identityToken.mint(); | ||
|
|
||
| string[] memory keys = new string[](4); | ||
| string[] memory keys = new string[](5); | ||
| keys[0] = "name"; | ||
| keys[1] = "github"; | ||
| keys[2] = "nationality"; | ||
| keys[3] = "residence"; | ||
| keys[4] = "email"; | ||
|
|
||
| bytes[] memory values = new bytes[](4); | ||
| bytes[] memory values = new bytes[](5); | ||
| values[0] = bytes("Alice Nakamoto"); | ||
| values[1] = bytes("https://github.com/alice"); | ||
| values[2] = bytes("Japanese"); | ||
| values[3] = bytes("Tokyo"); | ||
| values[4] = bytes("alice@example.com"); | ||
|
|
||
| vm.prank(alice); | ||
| identityToken.setAttributesBatch(tokenId, keys, values); | ||
|
|
||
| assertEq(string(identityToken.getAttribute(tokenId, "name")), "Alice Nakamoto"); | ||
| assertEq(string(identityToken.getAttribute(tokenId, "github")), "https://github.com/alice"); | ||
| assertEq(string(identityToken.getAttribute(tokenId, "nationality")), "Japanese"); | ||
| assertEq(string(identityToken.getAttribute(tokenId, "residence")), "Tokyo"); | ||
| assertEq(string(identityToken.getAttribute(tokenId, "email")), "alice@example.com"); | ||
| } | ||
|
|
||
| function test_SetAttributesBatch_SingleEntry() public { | ||
| vm.prank(alice); | ||
| uint256 tokenId = identityToken.mint(); | ||
|
|
||
| string[] memory keys = new string[](1); | ||
| keys[0] = "age"; | ||
| string[] memory keys = new string[](3); | ||
| keys[0] = "name"; | ||
| keys[1] = "email"; | ||
| keys[2] = "age"; | ||
|
|
||
| bytes[] memory values = new bytes[](1); | ||
| values[0] = bytes("30"); | ||
| bytes[] memory values = new bytes[](3); | ||
| values[0] = bytes("Alice Nakamoto"); | ||
| values[1] = bytes("alice@example.com"); | ||
| values[2] = bytes("30"); | ||
|
|
||
| vm.prank(alice); | ||
| identityToken.setAttributesBatch(tokenId, keys, values); |
There was a problem hiding this comment.
This PR introduces MissingName/MissingContact errors and required-field validation, but the tests shown only add name/email setup and don’t assert the new failure modes (e.g., batch update missing name, missing both email+phone, or attempts to clear the last contact). Add negative tests that expect reverts with Errors.MissingName.selector / Errors.MissingContact.selector to ensure the new invariant is actually enforced.
| function test_SetAttribute_EmptyValue() public { | ||
| vm.prank(alice); | ||
| uint256 tokenId = identityToken.mint(); | ||
|
|
||
| vm.prank(alice); | ||
| identityToken.setAttribute(tokenId, "name", bytes("")); | ||
|
|
||
| assertEq(identityToken.getAttribute(tokenId, "name").length, 0); | ||
| identityToken.setAttribute(tokenId, "name", bytes("Alice Nakamoto")); | ||
| vm.prank(alice); | ||
| identityToken.setAttribute(tokenId, "email", bytes("alice@example.com")); | ||
| vm.prank(alice); | ||
| identityToken.setAttribute(tokenId, "github", bytes("")); | ||
| assertEq(identityToken.getAttribute(tokenId, "github").length, 0); |
There was a problem hiding this comment.
The previous empty-value test case for the "name" attribute was replaced with an empty "github" value assertion. Given the new requirement that name is mandatory, it would be more valuable to keep/replace this with a test that demonstrates the expected revert when name is empty (and similarly for missing contact), rather than just asserting optional attributes can be empty.
This reverts commit 98dc951.
[Bug]: Fix missing contact fields in metadata schema
|
@Nikuunj please fix the issues pointed out by coderabbit and copilot above. |
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)
src/interfaces/IIdentityToken.sol (1)
18-23:⚠️ Potential issue | 🟠 MajorExpose
setContact(...)in the interface.
src/IdentityToken.solnow has a publicsetContact(uint256,string,string), butIIdentityTokenstill omits it. Any integration compiled against the interface gets an incomplete ABI and cannot call the new helper through the typed surface.♻️ Suggested fix
function setAttributesBatch(uint256 tokenId, string[] calldata keys, bytes[] calldata values) external; function setName(uint256 tokenId, string calldata name) external; + + function setContact(uint256 tokenId, string calldata email, string calldata phone) external; function setGithub(uint256 tokenId, string calldata github) external;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/interfaces/IIdentityToken.sol` around lines 18 - 23, The interface IIdentityToken is missing the setContact function declaration, causing an incomplete ABI; add a matching external declaration for setContact with the same parameter types as in IdentityToken (e.g., function setContact(uint256 tokenId, string calldata email, string calldata phone) external;) so consumers compiled against IIdentityToken can call IdentityToken.setContact via the interface.src/IdentityToken.sol (1)
161-205:⚠️ Potential issue | 🔴 CriticalThe helper and delete paths can still break the invariant.
setName(...)still accepts an empty name without checking contact,setContact(...)allows contact-only records because it never checksNAME_KEY, anddeleteAttribute(...)can removenameor the last remaining contact. If the contract invariant is “every stored identity has name + contact”, every external mutation path has to preserve it. If you still want ergonomic bootstrap calls, that needs to be a single atomic helper.♻️ Suggested fix
function setName(uint256 tokenId, string calldata name) external onlyTokenOwner(tokenId) notCompromised(tokenId) { _setAttribute(tokenId, "name", bytes(name)); + _validateRequiredFields(tokenId); } @@ function setContact( uint256 tokenId, string calldata email, string calldata phone ) external onlyTokenOwner(tokenId) notCompromised(tokenId) { @@ if (bytes(phone).length != 0) { _setAttribute(tokenId, "phone", bytes(phone)); } + + _validateRequiredFields(tokenId); } @@ function deleteAttribute( uint256 tokenId, string calldata key ) external onlyTokenOwner(tokenId) notCompromised(tokenId) { bytes32 keyHash = keccak256(abi.encodePacked(key)); delete attributes[tokenId][keyHash]; + + _validateRequiredFields(tokenId); emit Events.AttributeDeleted(tokenId, keyHash); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/IdentityToken.sol` around lines 161 - 205, setName, setContact, setGithub and deleteAttribute currently allow states that violate the invariant "every stored identity must have a name and at least one contact"; update these mutation paths so they always enforce that invariant: in setName and setContact validate the combined resulting attributes (e.g., call or reuse _validateRequiredFields or check NAME_KEY and contact keys) before writing so you cannot set an empty name or leave no contact, in setGithub keep the existing _validateRequiredFields call (or ensure it runs after any attribute change), and in deleteAttribute prevent deleting the name or the last contact (or require a combined atomic bootstrap helper that sets both name and contact in one call and only allow individual setters when _validateRequiredFields would still hold). Reference functions: setName, setContact, setGithub, deleteAttribute, _validateRequiredFields, and the NAME/contact key identifiers when adding the checks.
♻️ Duplicate comments (1)
src/IdentityToken.sol (1)
111-118:⚠️ Potential issue | 🔴 Critical
setAttribute(...)still permits invalid required-field transitions.Required-key writes skip
_validateRequiredFields, so a blank identity can store onlynameor onlyphone, and an existing identity can clearnameor its last contact by writingbytes(""). That keeps the original invariant unenforced on the most general write path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/IdentityToken.sol` around lines 111 - 118, The current setAttribute function bypasses _validateRequiredFields when the written key is one of NAME_KEY/EMAIL_KEY/PHONE_KEY, allowing required fields to be cleared or left missing; change the logic so that after calling _setAttribute(tokenId, key, value) you always run _validateRequiredFields(tokenId) (or, if you prefer, precompute the post-write state and validate that the resulting name and at least one contact exists) and revert the transaction if validation fails; update references in setAttribute, _setAttribute, and any callers to ensure writes of bytes("") are treated as removals but still subject to the same _validateRequiredFields check.
🤖 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/IdentityToken.sol`:
- Around line 309-312: The isExpired(uint256 tokenId) helper is unused because
identityStates[tokenId].validUntil is never written; either remove the API or
persist validUntil during the relevant state transition(s) — e.g., update
identityStates[tokenId].validUntil inside the functions that grant/renew/revoke
identities (look for functions that change identityStates or token lifecycle) so
on-chain writes set a non-zero validUntil, and keep isExpired as a view; if you
prefer removal, delete the isExpired method and any tests relying on it. Ensure
you reference identityStates and the validUntil field when adding the write and
update related unit tests to exercise the real state change rather than using
stdstore.
- Around line 142-155: In setAttributesBatch, the current loop sets
shouldValidate = false if any key is NAME_KEY, EMAIL_KEY, or PHONE_KEY which
skips post-write validation and allows incomplete identities; instead remove
that conditional and always call _validateRequiredFields(tokenId) after the loop
so the final state (after using _setAttribute for each key) is validated; modify
setAttributesBatch to stop gating _validateRequiredFields by
NAME_KEY/EMAIL_KEY/PHONE_KEY checks and ensure the call to
_validateRequiredFields(tokenId) runs unconditionally at the end.
---
Outside diff comments:
In `@src/IdentityToken.sol`:
- Around line 161-205: setName, setContact, setGithub and deleteAttribute
currently allow states that violate the invariant "every stored identity must
have a name and at least one contact"; update these mutation paths so they
always enforce that invariant: in setName and setContact validate the combined
resulting attributes (e.g., call or reuse _validateRequiredFields or check
NAME_KEY and contact keys) before writing so you cannot set an empty name or
leave no contact, in setGithub keep the existing _validateRequiredFields call
(or ensure it runs after any attribute change), and in deleteAttribute prevent
deleting the name or the last contact (or require a combined atomic bootstrap
helper that sets both name and contact in one call and only allow individual
setters when _validateRequiredFields would still hold). Reference functions:
setName, setContact, setGithub, deleteAttribute, _validateRequiredFields, and
the NAME/contact key identifiers when adding the checks.
In `@src/interfaces/IIdentityToken.sol`:
- Around line 18-23: The interface IIdentityToken is missing the setContact
function declaration, causing an incomplete ABI; add a matching external
declaration for setContact with the same parameter types as in IdentityToken
(e.g., function setContact(uint256 tokenId, string calldata email, string
calldata phone) external;) so consumers compiled against IIdentityToken can call
IdentityToken.setContact via the interface.
---
Duplicate comments:
In `@src/IdentityToken.sol`:
- Around line 111-118: The current setAttribute function bypasses
_validateRequiredFields when the written key is one of
NAME_KEY/EMAIL_KEY/PHONE_KEY, allowing required fields to be cleared or left
missing; change the logic so that after calling _setAttribute(tokenId, key,
value) you always run _validateRequiredFields(tokenId) (or, if you prefer,
precompute the post-write state and validate that the resulting name and at
least one contact exists) and revert the transaction if validation fails; update
references in setAttribute, _setAttribute, and any callers to ensure writes of
bytes("") are treated as removals but still subject to the same
_validateRequiredFields check.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1bd17faf-ddd9-4253-af72-47215d84abab
📒 Files selected for processing (6)
src/IdentityToken.solsrc/interfaces/IIdentityToken.solsrc/libraries/DataTypes.solsrc/libraries/Events.solsrc/libraries/Schema.soltest/IdentityToken.t.sol
| /// @notice Returns true if the token's validUntil is set and has passed. | ||
| function isExpired(uint256 tokenId) external view returns (bool) { | ||
| uint256 validUntil = identityStates[tokenId].validUntil; | ||
| return validUntil != 0 && block.timestamp > validUntil; |
There was a problem hiding this comment.
isExpired(...) is dead code in production right now.
This contract never writes identityStates[tokenId].validUntil, so the new field stays 0 and this helper always returns false outside tests. The current coverage only passes because the tests mutate storage directly with stdstore, which masks the missing on-chain update path. Either wire validUntil into a real state transition or drop this API from the PR.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/IdentityToken.sol` around lines 309 - 312, The isExpired(uint256 tokenId)
helper is unused because identityStates[tokenId].validUntil is never written;
either remove the API or persist validUntil during the relevant state
transition(s) — e.g., update identityStates[tokenId].validUntil inside the
functions that grant/renew/revoke identities (look for functions that change
identityStates or token lifecycle) so on-chain writes set a non-zero validUntil,
and keep isExpired as a view; if you prefer removal, delete the isExpired method
and any tests relying on it. Ensure you reference identityStates and the
validUntil field when adding the write and update related unit tests to exercise
the real state change rather than using stdstore.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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/IdentityToken.sol`:
- Around line 190-199: deleteAttribute currently removes an attribute without
re-checking required identity fields; update the function (deleteAttribute) to
call _validateRequiredFields(tokenId) immediately after the delete operation and
before emitting Events.AttributeDeleted so that the call will revert if the
deletion leaves the identity in an invalid state (e.g., removing "name" or both
"email" and "phone"), ensuring the state change cannot be committed without
passing validation.
- Line 142: The variable shouldValidate declared in IdentityToken.sol is unused
and should be removed; locate the declaration of uint8 shouldValidate (around
the function or constructor where it's defined) and delete the unused variable
so there are no dangling or dead local variables left in that scope, ensuring no
other code references it before removal.
- Around line 115-118: The current logic skips calling
_validateRequiredFields(tokenId) when keyHash equals NAME_KEY, EMAIL_KEY, or
PHONE_KEY, which allows incomplete identities to be persisted; change the flow
so validation always runs after the attribute is set (i.e., remove the
conditional that omits validation for NAME_KEY/EMAIL_KEY/PHONE_KEY and call
_validateRequiredFields(tokenId) regardless of which keyHash was updated) so the
final token state is checked; locate this in the attribute update routine that
references keyHash, NAME_KEY, EMAIL_KEY, PHONE_KEY and _validateRequiredFields
to implement the change.
In `@test/IdentityToken.t.sol`:
- Around line 304-316: The test suite is missing assertions that deleting
required fields reverts; add two tests that call identityToken.deleteAttribute
and expect reverts: one (e.g., test_RevertIf_DeleteAttribute_LeavesNoContact)
should vm.startPrank(alice), mint a token, setName(tokenId, "Alice") and
setContact(tokenId, "alice@example.com",""), then
vm.expectRevert(Errors.MissingContact.selector) before calling
deleteAttribute(tokenId, "email"); the other (e.g.,
test_RevertIf_DeleteAttribute_RemovesName) should similarly setName and
setContact then vm.expectRevert(Errors.MissingName.selector) before calling
deleteAttribute(tokenId, "name"); use identityToken.deleteAttribute,
identityToken.setContact, identityToken.setName and vm.expectRevert to enforce
the checks.
- Around line 331-337: The test_RevertIf_NotOwnerDeletesAttribute function
contains a redundant vm.startPrank(alice) call: remove the second
vm.startPrank(alice) before identityToken.setContact so the existing prank
started prior to identityToken.mint remains active; ensure the sequence is
vm.startPrank(alice), identityToken.mint(), identityToken.setContact(...),
vm.stopPrank() to keep the prank lifecycle correct and avoid duplicate starts.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: aac6dd96-7029-4ab8-aea1-da5847c437e6
📒 Files selected for processing (2)
src/IdentityToken.soltest/IdentityToken.t.sol
| // skip validation if the updated attribute is one of the required fields, since | ||
| if (keyHash != NAME_KEY && keyHash != EMAIL_KEY && keyHash != PHONE_KEY) { | ||
| _validateRequiredFields(tokenId); | ||
| } |
There was a problem hiding this comment.
Critical: Skipping validation on required fields allows incomplete identities.
The current logic skips _validateRequiredFields when the updated key is name, email, or phone. This defeats the purpose of the validation because:
- Setting only
name(without any contact) succeeds and persists an incomplete identity - Setting only
emailorphone(withoutname) succeeds and persists an incomplete identity
The validation should run after setting required fields to ensure the final state is valid.
🐛 Proposed fix
function setAttribute(
uint256 tokenId,
string calldata key,
bytes calldata value
) external onlyTokenOwner(tokenId) notCompromised(tokenId) {
- bytes32 keyHash = keccak256(abi.encodePacked(key));
-
_setAttribute(tokenId, key, value);
-
- // skip validation if the updated attribute is one of the required fields, since
- if (keyHash != NAME_KEY && keyHash != EMAIL_KEY && keyHash != PHONE_KEY) {
- _validateRequiredFields(tokenId);
- }
+ _validateRequiredFields(tokenId);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/IdentityToken.sol` around lines 115 - 118, The current logic skips
calling _validateRequiredFields(tokenId) when keyHash equals NAME_KEY,
EMAIL_KEY, or PHONE_KEY, which allows incomplete identities to be persisted;
change the flow so validation always runs after the attribute is set (i.e.,
remove the conditional that omits validation for NAME_KEY/EMAIL_KEY/PHONE_KEY
and call _validateRequiredFields(tokenId) regardless of which keyHash was
updated) so the final token state is checked; locate this in the attribute
update routine that references keyHash, NAME_KEY, EMAIL_KEY, PHONE_KEY and
_validateRequiredFields to implement the change.
| ) external onlyTokenOwner(tokenId) notCompromised(tokenId) { | ||
| if (keys.length != values.length) revert Errors.ArrayLengthMismatch(); | ||
|
|
||
| uint8 shouldValidate = 0; |
There was a problem hiding this comment.
Remove unused variable.
shouldValidate is declared but never used—leftover from a prior refactor.
🧹 Proposed fix
) external onlyTokenOwner(tokenId) notCompromised(tokenId) {
if (keys.length != values.length) revert Errors.ArrayLengthMismatch();
- uint8 shouldValidate = 0;
-
for (uint256 i = 0; i < keys.length; i++) {
_setAttribute(tokenId, keys[i], values[i]);
}
_validateRequiredFields(tokenId);
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/IdentityToken.sol` at line 142, The variable shouldValidate declared in
IdentityToken.sol is unused and should be removed; locate the declaration of
uint8 shouldValidate (around the function or constructor where it's defined) and
delete the unused variable so there are no dangling or dead local variables left
in that scope, ensuring no other code references it before removal.
| function deleteAttribute( | ||
| uint256 tokenId, | ||
| string calldata key | ||
| ) external onlyTokenOwner(tokenId) notCompromised(tokenId) { | ||
| bytes32 keyHash = keccak256(abi.encodePacked(key)); | ||
|
|
||
| delete attributes[tokenId][keyHash]; | ||
|
|
||
| emit Events.AttributeDeleted(tokenId, keyHash); | ||
| } |
There was a problem hiding this comment.
Critical: deleteAttribute bypasses required-field validation.
Deleting name, or deleting both email and phone, leaves the identity in an incomplete state. This directly contradicts the PR objective of enforcing required identity fields. The function must call _validateRequiredFields(tokenId) after the deletion.
🐛 Proposed fix
function deleteAttribute(
uint256 tokenId,
string calldata key
) external onlyTokenOwner(tokenId) notCompromised(tokenId) {
bytes32 keyHash = keccak256(abi.encodePacked(key));
delete attributes[tokenId][keyHash];
emit Events.AttributeDeleted(tokenId, keyHash);
+
+ _validateRequiredFields(tokenId);
}As per coding guidelines, "Ensure failure paths and revert scenarios are explicitly handled and validated."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/IdentityToken.sol` around lines 190 - 199, deleteAttribute currently
removes an attribute without re-checking required identity fields; update the
function (deleteAttribute) to call _validateRequiredFields(tokenId) immediately
after the delete operation and before emitting Events.AttributeDeleted so that
the call will revert if the deletion leaves the identity in an invalid state
(e.g., removing "name" or both "email" and "phone"), ensuring the state change
cannot be committed without passing validation.
| function test_DeleteAttribute() public { | ||
| vm.startPrank(alice); | ||
| uint256 tokenId = identityToken.mint(); | ||
|
|
||
| identityToken.setContact(tokenId, "alice@example.com", ""); | ||
|
|
||
| identityToken.deleteAttribute(tokenId, "email"); | ||
|
|
||
| bytes32 keyHash = keccak256(abi.encodePacked("email")); | ||
| bytes memory value = identityToken.attributes(tokenId, keyHash); | ||
|
|
||
| assertEq(value.length, 0); | ||
| } |
There was a problem hiding this comment.
Missing revert test: deleting required fields should fail.
Once the contract bug is fixed, deleting email when phone is empty (or deleting name) should revert with Errors.MissingContact or Errors.MissingName. Add explicit revert tests to cover this enforcement.
💚 Proposed additional tests
function test_RevertIf_DeleteAttribute_LeavesNoContact() public {
vm.startPrank(alice);
uint256 tokenId = identityToken.mint();
identityToken.setName(tokenId, "Alice");
identityToken.setContact(tokenId, "alice@example.com", "");
vm.expectRevert(Errors.MissingContact.selector);
identityToken.deleteAttribute(tokenId, "email");
}
function test_RevertIf_DeleteAttribute_RemovesName() public {
vm.startPrank(alice);
uint256 tokenId = identityToken.mint();
identityToken.setName(tokenId, "Alice");
identityToken.setContact(tokenId, "alice@example.com", "");
vm.expectRevert(Errors.MissingName.selector);
identityToken.deleteAttribute(tokenId, "name");
}As per coding guidelines, "Ensure security-sensitive logic changes are not introduced without adequate test coverage."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/IdentityToken.t.sol` around lines 304 - 316, The test suite is missing
assertions that deleting required fields reverts; add two tests that call
identityToken.deleteAttribute and expect reverts: one (e.g.,
test_RevertIf_DeleteAttribute_LeavesNoContact) should vm.startPrank(alice), mint
a token, setName(tokenId, "Alice") and setContact(tokenId,
"alice@example.com",""), then vm.expectRevert(Errors.MissingContact.selector)
before calling deleteAttribute(tokenId, "email"); the other (e.g.,
test_RevertIf_DeleteAttribute_RemovesName) should similarly setName and
setContact then vm.expectRevert(Errors.MissingName.selector) before calling
deleteAttribute(tokenId, "name"); use identityToken.deleteAttribute,
identityToken.setContact, identityToken.setName and vm.expectRevert to enforce
the checks.
| function test_RevertIf_NotOwnerDeletesAttribute() public { | ||
| vm.startPrank(alice); | ||
| uint256 tokenId = identityToken.mint(); | ||
|
|
||
| vm.startPrank(alice); | ||
| identityToken.setContact(tokenId, "alice@example.com", ""); | ||
| vm.stopPrank(); |
There was a problem hiding this comment.
Redundant vm.startPrank(alice) at line 335.
vm.startPrank(alice) was already called at line 332, so the second call at line 335 is unnecessary.
🧹 Proposed fix
function test_RevertIf_NotOwnerDeletesAttribute() public {
vm.startPrank(alice);
uint256 tokenId = identityToken.mint();
- vm.startPrank(alice);
identityToken.setContact(tokenId, "alice@example.com", "");
vm.stopPrank();
vm.startPrank(bob);📝 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.
| function test_RevertIf_NotOwnerDeletesAttribute() public { | |
| vm.startPrank(alice); | |
| uint256 tokenId = identityToken.mint(); | |
| vm.startPrank(alice); | |
| identityToken.setContact(tokenId, "alice@example.com", ""); | |
| vm.stopPrank(); | |
| function test_RevertIf_NotOwnerDeletesAttribute() public { | |
| vm.startPrank(alice); | |
| uint256 tokenId = identityToken.mint(); | |
| identityToken.setContact(tokenId, "alice@example.com", ""); | |
| vm.stopPrank(); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@test/IdentityToken.t.sol` around lines 331 - 337, The
test_RevertIf_NotOwnerDeletesAttribute function contains a redundant
vm.startPrank(alice) call: remove the second vm.startPrank(alice) before
identityToken.setContact so the existing prank started prior to
identityToken.mint remains active; ensure the sequence is vm.startPrank(alice),
identityToken.mint(), identityToken.setContact(...), vm.stopPrank() to keep the
prank lifecycle correct and avoid duplicate starts.
Addressed Issues:
Fixes #54
What’s Changed
Added
_validateRequiredFieldsfunction to enforce required identity fieldsEnsured name is mandatory
Ensured at least one contact method (email or phone) is provided
Integrated validation into
setAttributeto prevent incomplete identity dataAdded new error types:
MissingNameMissingContactCheck one of the checkboxes below:
I have used the following AI models and tools: TODO
Checklist
Summary by CodeRabbit