From a42c1e810662774fbd255ef54a723474574fdfec Mon Sep 17 00:00:00 2001 From: Aditya Anand M C Date: Thu, 22 Jun 2023 21:09:47 +0530 Subject: [PATCH] chore: address feedback --- contracts/core/Registry.sol | 117 ++++++++++++++++++++++-------------- 1 file changed, 72 insertions(+), 45 deletions(-) diff --git a/contracts/core/Registry.sol b/contracts/core/Registry.sol index d650cdb17..5a14ac40d 100644 --- a/contracts/core/Registry.sol +++ b/contracts/core/Registry.sol @@ -1,20 +1,28 @@ // SPDX-License-Identifier: MIT pragma solidity ^0.8.0; -import { OwnableRoles } from "../../lib/solady/src/auth/OwnableRoles.sol"; +import { AccessControl } from "../../lib/openzeppelin-contracts/contracts/access/AccessControl.sol"; + import { Metadata } from "./libraries/Metadata.sol"; -// use Solady Roles for ownership of projects: https://github.com/Vectorized/solady/blob/main/src/auth/OwnableRoles.sol -contract Registry is OwnableRoles { +contract Registry is AccessControl { + + error NO_ACCESS_TO_ROLE(); - // ASK: should this be renamed to identityIndex ? + /// @notice Unique identifier for an identity uint private identityId; + /// @notice Types of roles assigned to an identity + enum RoleType { + OWNER, + MEMBER + } + + /// @notice Struct to hold details of an identity struct IdentityDetails { string name; Metadata metadata; address attestationAddr; - // address coreOwner; // ASK: Should this be added? } /// @notice identityId -> IdentityDetails @@ -25,27 +33,13 @@ contract Registry is OwnableRoles { event IdentityNameUpdated(uint indexed identityId, string name); event IdentityMetadataUpdated(uint indexed identityId, Metadata metadata); - // ASK: Is this needed if we have the mapping as public? - // // getter for projects mapping - // function identities(uint _identityId) external view returns (IdentityDetails memory) { - - // } - - // create a new project with metadata - // hash some unique value to create the identityId - is there enough immutable to do this or just use incrementer? - // @todo if owners were immutable, we could make it a merkle proof of addresses and prove ownership that way? probably overkill - // sets roles so that all owners are owners of the project - // attestationAddr = addr(hash(id, name, owner)) - // OR deploy right away with CREATE2 as proxy so they can do anything (or just forward ERC20s and ETH) - // id => IdentityDetails - - // ASK: changing the function signature as attestationAddr has to be generated from the name and caller /// @notice Creates a new identity /// @dev This will also set the attestation address generated from msg.sender and name function createIdentity( string memory name, Metadata memory metadata, - address[] memory _owners + address[] memory _owners, + address[] memory _members ) external returns (uint256) { IdentityDetails memory identityDetails = IdentityDetails( @@ -53,15 +47,23 @@ contract Registry is OwnableRoles { metadata, // ASK : should identityId be used as the hash for attestationAddr? // cause project can't have same attestAddr across chains - generateAttestationAddr(name, msg.sender) + _generateAttestationAddr(name, msg.sender) ); identities[identityId] = identityDetails; + // generate roles + bytes32 ownerRole = _generateRole(identityId, RoleType.OWNER); + bytes32 memberRole = _generateRole(identityId, RoleType.MEMBER); + + // assign roles for(uint i = 0; i < _owners.length; i++) { - _grantRoles(_owners[i], identityId); + _grantRole(ownerRole, _owners[i]); } - + for(uint i = 0; i < _members.length; i++) { + _grantRole(memberRole, _members[i]); + } + emit IdentityCreated(identityId, name, metadata, identityDetails.attestationAddr); identityId++; @@ -69,48 +71,73 @@ contract Registry is OwnableRoles { return identityId; } - /// @notice Updates the name of the identity - /// @dev This will also change the attestation address, since it's a hash that includes the name - // ASK: should only coreOwner be able to change name? - function updateIdentityName(uint _identityId, string memory _name) onlyRoles(identityId) external { + /// @notice Updates the name of the identity. + /// @param _identityId The identityId of the identity + /// @param _name The new name of the identity + /// @dev Only owner can update the name. Also updated the attestation address + function updateIdentityName(uint _identityId, string memory _name) external { + + bytes32 ownerRole = _generateRole(identityId, RoleType.OWNER); + + if (!hasRole(ownerRole, msg.sender)) { + revert NO_ACCESS_TO_ROLE(); + } + identities[_identityId].name = _name; - // ASK: this has to be core owner and not msg.sender - identities[_identityId].attestationAddr = generateAttestationAddr(_name, msg.sender); + identities[_identityId].attestationAddr = _generateAttestationAddr(_name, msg.sender); emit IdentityNameUpdated(_identityId, _name); } - function changeCoreOwner(uint _identityId, address _newCoreOwner) external { - // ASK -> the actual owner cannot be tracked by Solady ROLES. - // Would this be stored on IdentityDetails ? + // @todo override changing owners (core owner, not contributors) to make sure it updates attestation address - // @todo check if msg sender is owner - // @todo check if new owner is not already owner - // @todo regenrate attestation address - // @todo change owner - } + /// @notice update the metadata of the identity + /// @param _identityId The identityId of the identity + /// @param _metadata The new metadata of the identity + /// @dev Only owner or member can update metadata + function updateIdentityMetadata(uint _identityId, Metadata memory _metadata) external { - // @todo override changing owners (core owner, not contributors) to make sure it updates attestation address + bytes32 ownerRole = _generateRole(_identityId, RoleType.OWNER); + bytes32 memberRole = _generateRole(_identityId, RoleType.MEMBER); + + if ( + !hasRole(ownerRole, msg.sender) || + !hasRole(memberRole, msg.sender) + ) { + revert NO_ACCESS_TO_ROLE(); + } - // update the metadata of the identity - // checks ownership role first, probably separate roles for this vs using it (owner vs user?) - function updateIdentityMetadata(uint _identityId, Metadata memory _metadata) onlyRoles(identityId) external { identities[_identityId].metadata = _metadata; emit IdentityMetadataUpdated(_identityId, _metadata); } - /// @notice Returns if the given address is an owner of the project + /// @notice Returns if the given address is an owner of the identity function isOwnerOfIdentity(uint _identityId, address _owner) external view returns (bool) { - return hasAnyRole(_owner, _identityId); + bytes32 ownerRole = _generateRole(_identityId, RoleType.OWNER); + return hasRole(ownerRole, _owner); + } + + + /// @notice Returns if the given address is an member of the identity + function isMemberOfIdentity(uint _identityId, address _owner) external view returns (bool) { + bytes32 memberRole = _generateRole(_identityId, RoleType.MEMBER); + return hasRole(memberRole, _owner); } /// @notice Generates the attestation address for the given identity - function generateAttestationAddr(string memory _name, address _owner) internal pure returns (address) { + function _generateAttestationAddr(string memory _name, address _owner) internal pure returns (address) { bytes32 attestationHash = keccak256( abi.encodePacked(_name, _owner) ); return address(uint160(uint256(attestationHash))); } + + /// @notice Generates the OZ role for an given identity + function _generateRole(uint _identityId, RoleType _roleType) internal pure returns (bytes32 roleHash) { + roleHash = keccak256( + abi.encodePacked(_identityId, _roleType) + ); + } } \ No newline at end of file