Skip to content

[Bug]: Fix: enforce required identity fields (name + email/phone) #63

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Nikuunj wants to merge 24 commits into
base: main
Choose a base branch
from
Open

[Bug]: Fix: enforce required identity fields (name + email/phone) #63

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
cf43f80
delete-attribute
aniket866 Mar 18, 2026
12a9ab7
Code rabbit follow up
aniket866 Mar 18, 2026
5926ba3
Code rabbit fix
aniket866 Mar 18, 2026
8f60e5d
Code rabbit fix
aniket866 Mar 18, 2026
6a0d7f8
fix: schema by update important field
Nikuunj Mar 20, 2026
0b6ee25
update: error for name and contact
Nikuunj Mar 20, 2026
340f4d9
fix: validate user need to pass phone / email , and name is mandatory
Nikuunj Mar 20, 2026
7c36ca1
update: call the fn
Nikuunj Mar 20, 2026
f229460
Merge branch 'main' into delete-attribute
aniket866 Mar 20, 2026
cb415de
formatting
aniket866 Mar 20, 2026
a926546
Query-functions
aniket866 Mar 20, 2026
6c30f6d
fix: all test and validatefn call fix
Nikuunj Mar 23, 2026
417a1be
fix: fmt
Nikuunj Mar 23, 2026
a192669
Merge pull request #64 from aniket866/Query-Functions
KanishkSogani Mar 23, 2026
98dc951
update: test case for phn and email hashcompatibility
Nikuunj Mar 23, 2026
4cfdb02
Revert "update: test case for phn and email hashcompatibility"
Nikuunj Mar 23, 2026
049f8fc
fix: test_schemaConstants
Nikuunj Mar 23, 2026
c656af9
Merge pull request #62 from Nikuunj/main
KanishkSogani Mar 23, 2026
10608be
Merge branch 'bug' of github.com:Nikuunj/IdentityTokens-EVM-Contracts
Nikuunj Mar 23, 2026
5ea5109
update: validate setattribute and set contract fn
Nikuunj Mar 23, 2026
42ade6b
fix: test case for missing name and contact
Nikuunj Mar 23, 2026
d8590fc
update: identitytoken validate remove if field name , email, ph no
Nikuunj Mar 24, 2026
aa8df60
update: test case for and use single batch test fix
Nikuunj Mar 24, 2026
1f50c7e
remove: validate condition from batch set
Nikuunj Mar 24, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 21 additions & 0 deletions src/IdentityToken.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,25 @@ contract IdentityToken is ERC721, IIdentityToken {
return tokenId;
}

/**
* @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();
}
}
Comment on lines +84 to +101
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Mar 23, 2026

Choose a reason for hiding this comment

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

🧹 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.

Comment on lines +89 to +101
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

_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.

Copilot uses AI. Check for mistakes.

/**
* @dev Sets a metadata attribute (e.g., name, social link) for an identity.
*/
Expand Down Expand Up @@ -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);
}
Comment on lines 89 to 149
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

_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).

Copilot uses AI. Check for mistakes.

/**
Expand Down
2 changes: 2 additions & 0 deletions src/libraries/Errors.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,4 +16,6 @@ library Errors {
error AlreadyHasIdentity();
error DuplicateEndorsement();
error ArrayLengthMismatch();
error MissingName();
error MissingContact();
}
52 changes: 43 additions & 9 deletions test/IdentityToken.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,11 @@ contract IdentityTokenTest is Test {
vm.prank(alice);
uint256 tokenId = identityToken.mint();

// 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"));
Comment on lines +43 to +45
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.

Comment thread
coderabbitai[bot] marked this conversation as resolved.
bytes32 keyHash = keccak256(abi.encodePacked("name"));
bytes memory retrievedValue = identityToken.attributes(tokenId, keyHash);
Expand All @@ -52,6 +55,8 @@ contract IdentityTokenTest is Test {

vm.prank(alice);
identityToken.setAttribute(tokenId, "name", bytes("Alice Nakamoto"));
vm.prank(alice);
identityToken.setAttribute(tokenId, "email", bytes("alice@example.com"));

assertEq(string(identityToken.getAttribute(tokenId, "name")), "Alice Nakamoto");
}
Expand All @@ -60,6 +65,12 @@ contract IdentityTokenTest is Test {
vm.prank(alice);
uint256 tokenId = identityToken.mint();

// Set required fields first
vm.prank(alice);
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("https://github.com/alice"));

Expand All @@ -73,6 +84,11 @@ contract IdentityTokenTest is Test {
vm.prank(alice);
uint256 tokenId = identityToken.mint();

vm.prank(alice);
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("https://github.com/alice"));
vm.prank(alice);
Expand All @@ -92,6 +108,8 @@ contract IdentityTokenTest is Test {
vm.prank(alice);
identityToken.setAttribute(tokenId, "name", bytes("Alice"));
vm.prank(alice);
identityToken.setAttribute(tokenId, "email", bytes("alice@example.com"));
vm.prank(alice);
identityToken.setAttribute(tokenId, "name", bytes("Alice Nakamoto"));

assertEq(string(identityToken.getAttribute(tokenId, "name")), "Alice Nakamoto");
Expand All @@ -102,15 +120,24 @@ contract IdentityTokenTest is Test {
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);
Comment on lines +109 to +116
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
}

function test_SetAttribute_LongURL() public {
vm.prank(alice);
uint256 tokenId = identityToken.mint();

// Required fields first
vm.prank(alice);
identityToken.setAttribute(tokenId, "name", bytes("Alice Nakamoto"));
vm.prank(alice);
identityToken.setAttribute(tokenId, "email", bytes("alice@example.com"));

string memory url = "https://www.linkedin.com/in/alice-nakamoto-very-long-profile-url-example-1234567890";
vm.prank(alice);
identityToken.setAttribute(tokenId, "linkedin", bytes(url));
Expand Down Expand Up @@ -150,17 +177,19 @@ contract IdentityTokenTest is Test {
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);
Expand All @@ -169,17 +198,22 @@ contract IdentityTokenTest is Test {
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");
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot Mar 23, 2026

Choose a reason for hiding this comment

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

🧹 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).


vm.prank(alice);
identityToken.setAttributesBatch(tokenId, keys, values);
Comment on lines 161 to 200
Copy link

Copilot AI Mar 23, 2026

Choose a reason for hiding this comment

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

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.

Copilot uses AI. Check for mistakes.
Expand Down
Loading