feat: improved attestation registry which allows attester to revoke h…

…is own attestation

contracts/src/AttestationRegistry.sol

@@ -29,8 +29,10 @@ contract AttestationRegistry is OwnableUpgradeable {
   error RouterInvalid();
   /// @notice Error thrown when an attestation is not registered in the AttestationRegistry
   error AttestationNotAttested();
-  /// @notice Error thrown when an attempt is made to revoke an attestation by an entity other than the attesting portal
+  /// @notice Error thrown when attestation is revoked by an entity other than the attesting portal
   error OnlyAttestingPortal();
+  /// @notice Error thrown when attestation is revoked by an attestor not associated or not the portal owner
+  error OnlyAttestorOrPortalOwner();
   /// @notice Error thrown when a schema id is not registered
   error SchemaNotRegistered();
   /// @notice Error thrown when an attestation subject is empty
@@ -167,11 +169,11 @@ contract AttestationRegistry is OwnableUpgradeable {
    * @notice Replaces an attestation for the given identifier and replaces it with a new attestation
    * @param attestationId the ID of the attestation to replace
    * @param attestationPayload the attestation payload to create the new attestation and register it
-   * @param attester the account address issuing the attestation
+   * @param replacer the account address replacing the attestation
    */
-  function replace(bytes32 attestationId, AttestationPayload calldata attestationPayload, address attester) public {
-    attest(attestationPayload, attester);
-    revoke(attestationId);
+  function replace(bytes32 attestationId, AttestationPayload calldata attestationPayload, address replacer) public {
+    attest(attestationPayload, replacer);
+    revoke(attestationId, replacer);
     bytes32 replacedBy = generateAttestationId(attestationIdCounter);
     attestations[attestationId].replacedBy = replacedBy;
 
@@ -182,29 +184,34 @@ contract AttestationRegistry is OwnableUpgradeable {
    * @notice Replaces attestations for given identifiers and replaces them with new attestations
    * @param attestationIds the list of IDs of the attestations to replace
    * @param attestationPayloads the list of attestation payloads to create the new attestations and register them
-   * @param attester the account address issuing the attestation
+   * @param replacer the account address replacing the attestation
    */
   function bulkReplace(
     bytes32[] calldata attestationIds,
     AttestationPayload[] calldata attestationPayloads,
-    address attester
+    address replacer
   ) public {
     if (attestationIds.length != attestationPayloads.length) revert ArrayLengthMismatch();
     for (uint256 i = 0; i < attestationIds.length; i = uncheckedInc256(i)) {
-      replace(attestationIds[i], attestationPayloads[i], attester);
+      replace(attestationIds[i], attestationPayloads[i], replacer);
     }
   }
 
   /**
    * @notice Revokes an attestation for a given identifier
    * @param attestationId the ID of the attestation to revoke
+   * @param revoker the account address revoking the attestation
    */
-  function revoke(bytes32 attestationId) public {
+  function revoke(bytes32 attestationId, address revoker) public {
     if (!isRegistered(attestationId)) revert AttestationNotAttested();
     if (attestations[attestationId].revoked) revert AlreadyRevoked();
     if (msg.sender != attestations[attestationId].portal) revert OnlyAttestingPortal();
     if (!isRevocable(attestations[attestationId].portal)) revert AttestationNotRevocable();
 
+    PortalRegistry portalRegistry = PortalRegistry(router.getPortalRegistry());
+    address portalOwner = portalRegistry.getPortalOwnerAddress(attestations[attestationId].portal);
+    if (revoker != attestations[attestationId].attester && revoker != portalOwner) revert OnlyAttestorOrPortalOwner();
+
     attestations[attestationId].revoked = true;
     attestations[attestationId].revocationDate = uint64(block.timestamp);
 
@@ -214,10 +221,11 @@ contract AttestationRegistry is OwnableUpgradeable {
   /**
    * @notice Bulk revokes a list of attestations for the given identifiers
    * @param attestationIds the IDs of the attestations to revoke
+   * @param revoker the account address revoking the attestation
    */
-  function bulkRevoke(bytes32[] memory attestationIds) external {
+  function bulkRevoke(bytes32[] memory attestationIds, address revoker) external {
     for (uint256 i = 0; i < attestationIds.length; i = uncheckedInc256(i)) {
-      revoke(attestationIds[i]);
+      revoke(attestationIds[i], revoker);
     }
   }
 

contracts/src/PortalRegistry.sol

@@ -213,6 +213,16 @@ contract PortalRegistry is OwnableUpgradeable {
     return portals[id];
   }
 
+  /**
+   * @notice Get a Portal owner address by its address
+   * @param id The address of the Portal
+   * @return The Portal owner address
+   */
+  function getPortalOwnerAddress(address id) public view returns (address) {
+    if (!isRegistered(id)) revert PortalNotRegistered();
+    return portals[id].ownerAddress;
+  }
+
   /**
    * @notice Check if a Portal is registered
    * @param id The address of the Portal

contracts/src/abstracts/AbstractPortal.sol

@@ -23,9 +23,6 @@ abstract contract AbstractPortal is IPortal {
   AttestationRegistry public attestationRegistry;
   PortalRegistry public portalRegistry;
 
-  /// @notice Error thrown when someone else than the portal's owner is trying to revoke
-  error OnlyPortalOwner();
-
   /**
    * @notice Contract constructor
    * @param _modules list of modules to use for the portal (can be empty)
@@ -120,7 +117,7 @@ abstract contract AbstractPortal is IPortal {
   function revoke(bytes32 attestationId) public {
     _onRevoke(attestationId);
 
-    attestationRegistry.revoke(attestationId);
+    attestationRegistry.revoke(attestationId, getAttester());
   }
 
   /**
@@ -130,7 +127,7 @@ abstract contract AbstractPortal is IPortal {
   function bulkRevoke(bytes32[] memory attestationIds) public {
     _onBulkRevoke(attestationIds);
 
-    attestationRegistry.bulkRevoke(attestationIds);
+    attestationRegistry.bulkRevoke(attestationIds, getAttester());
   }
 
   /**
@@ -201,17 +198,11 @@ abstract contract AbstractPortal is IPortal {
 
   /**
    * @notice Optional method run when an attestation is revoked or replaced
-   * @dev    IMPORTANT NOTE: By default, revocation is only possible by the portal owner
    */
-  function _onRevoke(bytes32 /*attestationId*/) internal virtual {
-    if (msg.sender != portalRegistry.getPortalByAddress(address(this)).ownerAddress) revert OnlyPortalOwner();
-  }
+  function _onRevoke(bytes32 /*attestationId*/) internal virtual {}
 
   /**
    * @notice Optional method run when a batch of attestations are revoked or replaced
-   * @dev    IMPORTANT NOTE: By default, revocation is only possible by the portal owner
    */
-  function _onBulkRevoke(bytes32[] memory /*attestationIds*/) internal virtual {
-    if (msg.sender != portalRegistry.getPortalByAddress(address(this)).ownerAddress) revert OnlyPortalOwner();
-  }
+  function _onBulkRevoke(bytes32[] memory /*attestationIds*/) internal virtual {}
 }

contracts/test/AttestationRegistry.t.sol

@@ -14,6 +14,7 @@ contract AttestationRegistryTest is Test {
   address public portal = makeAddr("portal");
   address public user = makeAddr("user");
   address public attester = makeAddr("attester");
+  address public portalOwner = makeAddr("portalOwner");
   Router public router;
   AttestationRegistry public attestationRegistry;
   AttestationRegistryHarness public attestationRegistryHarness;
@@ -49,6 +50,7 @@ contract AttestationRegistryTest is Test {
     router.updatePortalRegistry(portalRegistryAddress);
     router.updateSchemaRegistry(schemaRegistryAddress);
 
+    vm.prank(portalOwner);
     PortalRegistry(portalRegistryAddress).register(portal, "Name", "Description", true, "Owner name");
   }
 
@@ -305,7 +307,7 @@ contract AttestationRegistryTest is Test {
     attestationRegistry.bulkReplace(attestationIds, payloadsToAttest, attester);
   }
 
-  function test_revoke(AttestationPayload memory attestationPayload) public {
+  function test_revoke_ByPortalOwner(AttestationPayload memory attestationPayload) public {
     vm.assume(attestationPayload.subject.length != 0);
     vm.assume(attestationPayload.attestationData.length != 0);
     SchemaRegistryMock schemaRegistryMock = SchemaRegistryMock(router.getSchemaRegistry());
@@ -325,7 +327,38 @@ contract AttestationRegistryTest is Test {
 
     vm.expectEmit(true, true, true, true);
     emit AttestationRevoked(attestationId);
-    attestationRegistry.revoke(attestationId);
+    attestationRegistry.revoke(attestationId, portalOwner);
+    vm.stopPrank();
+
+    // Assert final state is a revoked attestation
+    Attestation memory revokedAttestation = attestationRegistry.getAttestation(
+      attestationRegistryHarness.exposed_generateAttestationId(1)
+    );
+    assertTrue(revokedAttestation.revoked);
+    assertEq(revokedAttestation.revocationDate, block.timestamp);
+  }
+
+  function test_revoke_ByValidAttestor(AttestationPayload memory attestationPayload) public {
+    vm.assume(attestationPayload.subject.length != 0);
+    vm.assume(attestationPayload.attestationData.length != 0);
+    SchemaRegistryMock schemaRegistryMock = SchemaRegistryMock(router.getSchemaRegistry());
+    attestationPayload.schemaId = schemaRegistryMock.getIdFromSchemaString("schemaString");
+    schemaRegistryMock.createSchema("name", "description", "context", "schemaString");
+    vm.startPrank(portal, attester);
+    attestationRegistry.attest(attestationPayload, attester);
+    // Assert initial state is a valid attestation
+    bytes32 attestationId = attestationRegistryHarness.exposed_generateAttestationId((1));
+    Attestation memory registeredAttestation = attestationRegistry.getAttestation(attestationId);
+
+    assertEq(registeredAttestation.attestationId, attestationRegistryHarness.exposed_generateAttestationId(1));
+    assertFalse(registeredAttestation.revoked);
+    assertEq(registeredAttestation.revocationDate, 0);
+    assertEq(registeredAttestation.portal, portal);
+    assertEq(registeredAttestation.attester, attester);
+
+    vm.expectEmit(true, true, true, true);
+    emit AttestationRevoked(attestationId);
+    attestationRegistry.revoke(attestationId, attester);
     vm.stopPrank();
 
     // Assert final state is a revoked attestation
@@ -349,18 +382,18 @@ contract AttestationRegistryTest is Test {
 
     vm.expectEmit();
     emit AttestationRevoked(attestationIdToRevoke);
-    attestationRegistry.revoke(attestationIdToRevoke);
+    attestationRegistry.revoke(attestationIdToRevoke, attester);
 
     vm.expectRevert(AttestationRegistry.AlreadyRevoked.selector);
-    attestationRegistry.revoke(attestationIdToRevoke);
+    attestationRegistry.revoke(attestationIdToRevoke, attester);
 
     vm.stopPrank();
   }
 
   function test_revoke_AttestationNotAttested() public {
     bytes32 attestationIdToRevoke = attestationRegistryHarness.exposed_generateAttestationId(1);
     vm.expectRevert(AttestationRegistry.AttestationNotAttested.selector);
-    attestationRegistry.revoke(attestationIdToRevoke);
+    attestationRegistry.revoke(attestationIdToRevoke, attester);
   }
 
   function test_revoke_OnlyAttestingPortal(AttestationPayload memory attestationPayload) public {
@@ -376,7 +409,7 @@ contract AttestationRegistryTest is Test {
     bytes32 attestationIdToRevoke = attestationRegistryHarness.exposed_generateAttestationId(1);
     vm.expectRevert(AttestationRegistry.OnlyAttestingPortal.selector);
     vm.prank(user);
-    attestationRegistry.revoke(attestationIdToRevoke);
+    attestationRegistry.revoke(attestationIdToRevoke, attester);
   }
 
   function test_revoke_AttestationNotRevocable(AttestationPayload memory attestationPayload) public {
@@ -402,7 +435,24 @@ contract AttestationRegistryTest is Test {
     bytes32 attestationIdToRevoke = attestationRegistryHarness.exposed_generateAttestationId(1);
 
     vm.expectRevert(AttestationRegistry.AttestationNotRevocable.selector);
-    attestationRegistry.revoke(attestationIdToRevoke);
+    attestationRegistry.revoke(attestationIdToRevoke, attester);
+
+    vm.stopPrank();
+  }
+
+  function test_revoke_OnlyAttestorOrPortalOwner(AttestationPayload memory attestationPayload) public {
+    vm.assume(attestationPayload.subject.length != 0);
+    vm.assume(attestationPayload.attestationData.length != 0);
+    SchemaRegistryMock schemaRegistryMock = SchemaRegistryMock(router.getSchemaRegistry());
+    attestationPayload.schemaId = schemaRegistryMock.getIdFromSchemaString("schemaString");
+    schemaRegistryMock.createSchema("name", "description", "context", "schemaString");
+    vm.startPrank(portal, attester);
+    attestationRegistry.attest(attestationPayload, attester);
+
+    bytes32 attestationIdToRevoke = attestationRegistryHarness.exposed_generateAttestationId(1);
+
+    vm.expectRevert(AttestationRegistry.OnlyAttestorOrPortalOwner.selector);
+    attestationRegistry.revoke(attestationIdToRevoke, makeAddr("InvalidAttester"));
 
     vm.stopPrank();
   }
@@ -461,7 +511,7 @@ contract AttestationRegistryTest is Test {
     vm.expectEmit(true, true, true, true);
     emit AttestationRevoked(attestationsToRevoke[1]);
 
-    attestationRegistry.bulkRevoke(attestationsToRevoke);
+    attestationRegistry.bulkRevoke(attestationsToRevoke, attester);
     vm.stopPrank();
 
     // Assert final state is a revoked attestation and a replaced attestation

contracts/test/DefaultPortal.t.sol

@@ -135,7 +135,7 @@ contract DefaultPortalTest is Test {
     defaultPortal.bulkReplace(attestationIds, payloadsToAttest, validationPayloads);
   }
 
-  function test_revoke_byPortalOwner() public {
+  function test_revoke() public {
     // Create attestation payload
     AttestationPayload memory attestationPayload = AttestationPayload(
       bytes32(uint256(1)),
@@ -154,36 +154,11 @@ contract DefaultPortalTest is Test {
     defaultPortal.attest(attestationPayload, validationPayload);
 
     // Revoke the attestation as portal owner
-    vm.prank(portalOwner);
     vm.expectEmit(true, true, true, true);
     emit AttestationRevoked(bytes32(abi.encode(1)));
     defaultPortal.revoke(bytes32(abi.encode(1)));
   }
 
-  function test_revokeFail_OnlyOwner() public {
-    // Create attestation payload
-    AttestationPayload memory attestationPayload = AttestationPayload(
-      bytes32(uint256(1)),
-      uint64(block.timestamp + 1 days),
-      bytes("subject"),
-      new bytes(1)
-    );
-
-    // Create validation payload
-    bytes[] memory validationPayload = new bytes[](2);
-
-    // Do register the attestation
-    vm.prank(makeAddr("attester"));
-    vm.expectEmit(true, true, true, true);
-    emit AttestationRegistered();
-    defaultPortal.attest(attestationPayload, validationPayload);
-
-    // Revoke the attestation as a random user
-    vm.prank(makeAddr("random"));
-    vm.expectRevert(AbstractPortal.OnlyPortalOwner.selector);
-    defaultPortal.revoke(bytes32(abi.encode(1)));
-  }
-
   function test_bulkRevoke() public {
     bytes32[] memory attestationsToRevoke = new bytes32[](2);
     attestationsToRevoke[0] = bytes32("1");

contracts/test/PortalRegistry.t.sol

@@ -265,6 +265,17 @@ contract PortalRegistryTest is Test {
     portalRegistry.getPortalByAddress(address(validPortalMock));
   }
 
+  function test_getPortalOwnerAddress() public {
+    vm.prank(user);
+    portalRegistry.register(address(validPortalMock), expectedName, expectedDescription, true, expectedOwnerName);
+    assertEq(portalRegistry.getPortalOwnerAddress(address(validPortalMock)), user);
+  }
+
+  function test_getPortalOwnerAddress_PortalNotRegistered() public {
+    vm.expectRevert(PortalRegistry.PortalNotRegistered.selector);
+    portalRegistry.getPortalOwnerAddress(address(validPortalMock));
+  }
+
   function test_isRegistered() public {
     assertEq(portalRegistry.isRegistered(address(validPortalMock)), false);
     vm.prank(user);

contracts/test/mocks/AttestationRegistryMock.sol

@@ -45,7 +45,7 @@ contract AttestationRegistryMock {
   function replace(
     bytes32 /*attestationId*/,
     AttestationPayload calldata /*attestationPayload*/,
-    address /*attester*/
+    address /*replacer*/
   ) public {
     emit AttestationRegistered();
     emit AttestationReplaced();
@@ -54,14 +54,14 @@ contract AttestationRegistryMock {
   function bulkReplace(
     bytes32[] calldata /*attestationId*/,
     AttestationPayload[] calldata /*attestationPayload*/,
-    address /*attester*/
+    address /*replacer*/
   ) public {}
 
-  function revoke(bytes32 attestationId) public {
+  function revoke(bytes32 attestationId, address /*revoker*/) public {
     emit AttestationRevoked(attestationId);
   }
 
-  function bulkRevoke(bytes32[] memory attestationIds) public {
+  function bulkRevoke(bytes32[] memory attestationIds, address /*revoker*/) public {
     emit BulkAttestationsRevoked(attestationIds);
   }
 

contracts/test/mocks/PortalRegistryMock.sol

@@ -32,6 +32,10 @@ contract PortalRegistryMock {
     return portals[id];
   }
 
+  function getPortalOwnerAddress(address id) public view returns (address) {
+    return portals[id].ownerAddress;
+  }
+
   function setIssuer(address issuer) public {
     issuers[issuer] = true;
   }

sdk/doc/cli-examples.md

@@ -41,6 +41,8 @@
 
     pnpm portal getPortalByAddress '{\"portalAddress\":\"0x8b833796869b5debb9b06370d6d47016f0d7973b\"}'
 
+    pnpm portal getPortalOwnerAddress '{\"portalAddress\":\"0x8b833796869b5debb9b06370d6d47016f0d7973b\"}'
+
     pnpm portal isPortalRegistered '{\"portalAddress\":\"0x8b833796869b5debb9b06370d6d47016f0d7973b\"}'
 
     pnpm portal getPortalsCount