From 70ee9f94e436eb0ab2e0daf60e8fe32093e76d90 Mon Sep 17 00:00:00 2001 From: Alain Nicolas Date: Fri, 22 Sep 2023 12:05:00 +0200 Subject: [PATCH] chore: Cleanup and improvements --- src/AttestationRegistry.sol | 1 + src/ModuleRegistry.sol | 10 +++++--- src/PortalRegistry.sol | 1 + src/Router.sol | 22 ++++++++++++++++- src/SchemaRegistry.sol | 7 ++++-- src/example/CorrectModule.sol | 9 ++++++- src/example/EASPortal.sol | 30 ++++++++++++++++++++--- src/example/IncorrectModule.sol | 9 ++++++- src/example/MsgSenderModule.sol | 3 ++- src/example/PayableModule.sol | 3 ++- src/interface/AbstractModule.sol | 16 +++++++++++- src/interface/AbstractPortal.sol | 12 ++++----- src/interface/IRouter.sol | 21 ++++++++++++++++ src/portal/DefaultPortal.sol | 7 ++++++ test/ModuleRegistry.t.sol | 32 ++++++++++++------------ test/PortalRegistry.t.sol | 6 ++--- test/Router.t.sol | 24 ++++++++---------- test/SchemaRegistry.t.sol | 34 +++++++++++++------------- test/example/EASPortal.t.sol | 4 +-- test/example/MsgSenderModule.t.sol | 6 ++--- test/mocks/AttestationRegistryMock.sol | 24 +++++++----------- test/mocks/ModuleRegistryMock.sol | 17 ++++++------- test/mocks/PortalRegistryMock.sol | 4 --- test/portal/DefaultPortal.t.sol | 6 ++--- 24 files changed, 201 insertions(+), 107 deletions(-) diff --git a/src/AttestationRegistry.sol b/src/AttestationRegistry.sol index 085dae48..f0a922f2 100644 --- a/src/AttestationRegistry.sol +++ b/src/AttestationRegistry.sol @@ -74,6 +74,7 @@ contract AttestationRegistry is OwnableUpgradeable { /** * @notice Changes the address for the Router + * @dev Only the registry owner can call this method */ function updateRouter(address _router) public onlyOwner { if (_router == address(0)) revert RouterInvalid(); diff --git a/src/ModuleRegistry.sol b/src/ModuleRegistry.sol index 8c2c3af9..11f1e2e3 100644 --- a/src/ModuleRegistry.sol +++ b/src/ModuleRegistry.sol @@ -67,6 +67,7 @@ contract ModuleRegistry is OwnableUpgradeable { /** * @notice Changes the address for the Router + * @dev Only the registry owner can call this method */ function updateRouter(address _router) public onlyOwner { if (_router == address(0)) revert RouterInvalid(); @@ -82,7 +83,8 @@ contract ModuleRegistry is OwnableUpgradeable { return contractAddress.code.length > 0; } - /** Register a Module, with its metadata and run some checks: + /** + * @notice Registers a Module, with its metadata and run some checks: * - mandatory name * - mandatory module's deployed smart contract address * - the module must be unique @@ -110,7 +112,8 @@ contract ModuleRegistry is OwnableUpgradeable { emit ModuleRegistered(name, description, moduleAddress); } - /** Execute the run method for all given Modules that are registered + /** + * @notice Executes the run method for all given Modules that are registered * @param modulesAddresses the addresses of the registered modules * @param attestationPayload the payload to attest * @param validationPayloads the payloads to check for each module (one payload per module) @@ -134,7 +137,8 @@ contract ModuleRegistry is OwnableUpgradeable { } } - /** Execute the modules validation for all attestations payloads for all given Modules that are registered + /** + * @notice Executes the modules validation for all attestations payloads for all given Modules that are registered * @param modulesAddresses the addresses of the registered modules * @param attestationsPayloads the payloads to attest * @param validationPayloads the payloads to check for each module diff --git a/src/PortalRegistry.sol b/src/PortalRegistry.sol index e77172b3..47cfa53c 100644 --- a/src/PortalRegistry.sol +++ b/src/PortalRegistry.sol @@ -59,6 +59,7 @@ contract PortalRegistry is OwnableUpgradeable { /** * @notice Changes the address for the Router + * @dev Only the registry owner can call this method */ function updateRouter(address _router) public onlyOwner { if (_router == address(0)) revert RouterInvalid(); diff --git a/src/Router.sol b/src/Router.sol index 31d2ecd4..bb15b9bd 100644 --- a/src/Router.sol +++ b/src/Router.sol @@ -7,7 +7,7 @@ import { IRouter } from "./interface/IRouter.sol"; /** * @title Router * @author Consensys - * @notice This contract aims to provides a single entrypoint for the Verax registries + * @notice This contract aims to register the addresses of the Verax registries */ contract Router is IRouter, OwnableUpgradeable { address private ATTESTATION_REGISTRY; @@ -24,37 +24,57 @@ contract Router is IRouter, OwnableUpgradeable { __Ownable_init(); } + /// @inheritdoc IRouter function getAttestationRegistry() external view override returns (address) { return ATTESTATION_REGISTRY; } + /** + * @notice Changes the address for the AttestationRegistry contract + * @param _attestationRegistry The new address of the AttestationRegistry contract + */ function updateAttestationRegistry(address _attestationRegistry) public onlyOwner { ATTESTATION_REGISTRY = _attestationRegistry; emit AttestationRegistryUpdated(_attestationRegistry); } + /// @inheritdoc IRouter function getModuleRegistry() external view override returns (address) { return MODULE_REGISTRY; } + /** + * @notice Changes the address for the ModuleRegistry contract + * @param _moduleRegistry The new address of the ModuleRegistry contract + */ function updateModuleRegistry(address _moduleRegistry) public onlyOwner { MODULE_REGISTRY = _moduleRegistry; emit ModuleRegistryUpdated(_moduleRegistry); } + /// @inheritdoc IRouter function getPortalRegistry() external view override returns (address) { return PORTAL_REGISTRY; } + /** + * @notice Changes the address for the PortalRegistry contract + * @param _portalRegistry The new address of the PortalRegistry contract + */ function updatePortalRegistry(address _portalRegistry) public onlyOwner { PORTAL_REGISTRY = _portalRegistry; emit PortalRegistryUpdated(_portalRegistry); } + /// @inheritdoc IRouter function getSchemaRegistry() external view override returns (address) { return SCHEMA_REGISTRY; } + /** + * @notice Changes the address for the SchemaRegistry contract + * @param _schemaRegistry The new address of the SchemaRegistry contract + */ function updateSchemaRegistry(address _schemaRegistry) public onlyOwner { SCHEMA_REGISTRY = _schemaRegistry; emit SchemaRegistryUpdated(_schemaRegistry); diff --git a/src/SchemaRegistry.sol b/src/SchemaRegistry.sol index 1ad8c5c0..22a502c2 100644 --- a/src/SchemaRegistry.sol +++ b/src/SchemaRegistry.sol @@ -58,6 +58,7 @@ contract SchemaRegistry is OwnableUpgradeable { /** * @notice Changes the address for the Router + * @dev Only the registry owner can call this method */ function updateRouter(address _router) public onlyOwner { if (_router == address(0)) revert RouterInvalid(); @@ -74,7 +75,8 @@ contract SchemaRegistry is OwnableUpgradeable { return keccak256(abi.encodePacked(schema)); } - /** Create a Schema, with its metadata and run some checks: + /** + * @notice Creates a Schema, with its metadata and runs some checks: * - mandatory name * - mandatory string defining the schema * - the Schema must be unique @@ -104,7 +106,8 @@ contract SchemaRegistry is OwnableUpgradeable { emit SchemaCreated(schemaId, name, description, context, schemaString); } - /** Update a context field of schema : + /** + * @notice Updates the context of a given schema * @param schemaId the schema ID * @param context the Schema context * @dev Retrieve the Schema with given ID and update its context with new value diff --git a/src/example/CorrectModule.sol b/src/example/CorrectModule.sol index f963c3cb..3ccb9cbc 100644 --- a/src/example/CorrectModule.sol +++ b/src/example/CorrectModule.sol @@ -4,12 +4,19 @@ pragma solidity 0.8.21; import { AbstractModule } from "../interface/AbstractModule.sol"; import { AttestationPayload } from "../types/Structs.sol"; +/** + * @title Correct Module + * @author Consensys + * @notice This contract illustrates a valid Module that follows the AbstractModule interface + */ contract CorrectModule is AbstractModule { + /// @dev This empty method prevents Foundry from counting this contract in code coverage function test() public {} + /// @inheritdoc AbstractModule function run( AttestationPayload memory /*attestationPayload*/, - bytes memory validationPayload, + bytes memory /*validationPayload*/, address /*txSender*/, uint256 /*value*/ ) public pure override {} diff --git a/src/example/EASPortal.sol b/src/example/EASPortal.sol index 6687b0ce..b589b298 100644 --- a/src/example/EASPortal.sol +++ b/src/example/EASPortal.sol @@ -6,6 +6,7 @@ import { AttestationPayload } from "../types/Structs.sol"; /** * @title EAS Portal + * @author Consensys * @notice This is an example of how to maintain interoperability with EAS - https://attest.sh */ contract EASPortal is AbstractPortal { @@ -29,7 +30,7 @@ contract EASPortal is AbstractPortal { AttestationRequestData data; } - bytes32 private relationshipSchemaId = 0x89bd76e17fd84df8e1e448fa1b46dd8d97f7e8e806552b003f8386a5aebcb9f0; + bytes32 private _relationshipSchemaId = 0x89bd76e17fd84df8e1e448fa1b46dd8d97f7e8e806552b003f8386a5aebcb9f0; /// @notice Error thrown when reference attestation with refUID is not registered error ReferenceAttestationNotRegistered(); @@ -40,8 +41,14 @@ contract EASPortal is AbstractPortal { constructor(address[] memory modules, address router) AbstractPortal(modules, router) {} + /// @inheritdoc AbstractPortal function withdraw(address payable to, uint256 amount) external override {} + /** + * @notice Issues a Verax attestation based on an EAS attestation + * @param attestationRequest the EAS payload to attest + * @dev If a related EAS attestation exists, it will also be attested on Verax and linked via the dedicated Schema + */ function attest(AttestationRequest memory attestationRequest) public payable { bytes[] memory validationPayload = new bytes[](0); @@ -60,7 +67,7 @@ contract EASPortal is AbstractPortal { bytes32 attestationId = bytes32(abi.encode(attestationIdCounter)); AttestationPayload memory relationshipAttestationPayload = AttestationPayload( - relationshipSchemaId, + _relationshipSchemaId, attestationRequest.data.expirationTime, abi.encodePacked(attestationRequest.data.refUID), abi.encode(attestationId, "EASrefUID", attestationRequest.data.refUID) @@ -70,21 +77,38 @@ contract EASPortal is AbstractPortal { } } + /** + * @notice Issues Verax attestations in bulk, based on a list of EAS attestations + * @param attestationsRequests the EAS payloads to attest + */ function bulkAttest(AttestationRequest[] memory attestationsRequests) external payable { for (uint256 i = 0; i < attestationsRequests.length; i++) { attest(attestationsRequests[i]); } } + /** + * @inheritdoc AbstractPortal + * @notice This portal doesn't allow for an attestation to be revoked + */ function _onRevoke(bytes32 /*attestationId*/) internal pure override { revert NoRevocation(); } + /** + * @inheritdoc AbstractPortal + * @notice This portal doesn't allow for attestations to be revoked + */ function _onBulkRevoke(bytes32[] memory /*attestationIds*/) internal pure override { revert NoBulkRevocation(); } - function _getAttester() public view override returns (address) { + /** + * @inheritdoc AbstractPortal + * @notice This portal considers the caller is the attester. + * In other words, an end-user can claim an attestation through this portal. + */ + function getAttester() public view override returns (address) { return msg.sender; } } diff --git a/src/example/IncorrectModule.sol b/src/example/IncorrectModule.sol index 73211a1e..fc849a18 100644 --- a/src/example/IncorrectModule.sol +++ b/src/example/IncorrectModule.sol @@ -1,4 +1,11 @@ // SPDX-License-Identifier: MIT pragma solidity 0.8.21; -contract IncorrectModule {} +/** + * @title Incorrect Module + * @author Consensys + * @notice This contract illustrates an invalid Module that doesn't follow the AbstractModule interface + */ +contract IncorrectModule { + +} diff --git a/src/example/MsgSenderModule.sol b/src/example/MsgSenderModule.sol index d7f05d08..5cdc3a95 100644 --- a/src/example/MsgSenderModule.sol +++ b/src/example/MsgSenderModule.sol @@ -25,7 +25,8 @@ contract MsgSenderModule is AbstractModule { } /** - * @notice The main method for the module, running the check + * @inheritdoc AbstractModule + * @notice If the transaction sender is not the expected address, an error is thrown */ function run( AttestationPayload memory /*_attestationPayload*/, diff --git a/src/example/PayableModule.sol b/src/example/PayableModule.sol index 14a5c82f..ddf1804c 100644 --- a/src/example/PayableModule.sol +++ b/src/example/PayableModule.sol @@ -32,7 +32,8 @@ contract PayableModule is Ownable, AbstractModule { } /** - * @notice The main method for the module, running the check + * @inheritdoc AbstractModule + * @notice If the paid value is below the expected amount, an error is thrown * @param _value The value sent for the attestation */ function run( diff --git a/src/interface/AbstractModule.sol b/src/interface/AbstractModule.sol index 8c7fda6d..4c333daf 100644 --- a/src/interface/AbstractModule.sol +++ b/src/interface/AbstractModule.sol @@ -4,7 +4,19 @@ pragma solidity 0.8.21; import { AttestationPayload } from "../types/Structs.sol"; import { IERC165 } from "openzeppelin-contracts/contracts/utils/introspection/IERC165.sol"; +/** + * @title Abstract Module + * @author Consensys + * @notice Defines the minimal Module interface + */ abstract contract AbstractModule is IERC165 { + /** + * @notice Executes the module's custom logic. + * @param attestationPayload The incoming attestation data. + * @param validationPayload Additional data required for verification. + * @param txSender The transaction sender's address. + * @param value The transaction value. + */ function run( AttestationPayload memory attestationPayload, bytes memory validationPayload, @@ -13,7 +25,9 @@ abstract contract AbstractModule is IERC165 { ) public virtual; /** - * @notice To check this contract implements the Module interface + * @notice Checks if the contract implements the Module interface. + * @param interfaceID The ID of the interface to check. + * @return A boolean indicating interface support. */ function supportsInterface(bytes4 interfaceID) public pure virtual override returns (bool) { return interfaceID == type(AbstractModule).interfaceId || interfaceID == type(IERC165).interfaceId; diff --git a/src/interface/AbstractPortal.sol b/src/interface/AbstractPortal.sol index aea0fa18..718c6d7e 100644 --- a/src/interface/AbstractPortal.sol +++ b/src/interface/AbstractPortal.sol @@ -40,7 +40,7 @@ abstract contract AbstractPortal is IERC165 { function withdraw(address payable to, uint256 amount) external virtual; /** - * @notice attest the schema with given attestationPayload and validationPayload + * @notice Attest the schema with given attestationPayload and validationPayload * @param attestationPayload the payload to attest * @param validationPayloads the payloads to validate via the modules to issue the attestations * @dev Runs all modules for the portal and registers the attestation using AttestationRegistry @@ -50,7 +50,7 @@ abstract contract AbstractPortal is IERC165 { _onAttest(attestationPayload); - attestationRegistry.attest(attestationPayload, _getAttester()); + attestationRegistry.attest(attestationPayload, getAttester()); } /** @@ -66,7 +66,7 @@ abstract contract AbstractPortal is IERC165 { _onBulkAttest(attestationsPayloads, validationPayloads); - attestationRegistry.bulkAttest(attestationsPayloads, _getAttester()); + attestationRegistry.bulkAttest(attestationsPayloads, getAttester()); } /** @@ -85,7 +85,7 @@ abstract contract AbstractPortal is IERC165 { _onReplace(attestationId, attestationPayload); - attestationRegistry.replace(attestationId, attestationPayload, _getAttester()); + attestationRegistry.replace(attestationId, attestationPayload, getAttester()); } /** @@ -103,7 +103,7 @@ abstract contract AbstractPortal is IERC165 { _onBulkReplace(attestationIds, attestationsPayloads, validationPayloads); - attestationRegistry.bulkReplace(attestationIds, attestationsPayloads, _getAttester()); + attestationRegistry.bulkReplace(attestationIds, attestationsPayloads, getAttester()); } /** @@ -149,7 +149,7 @@ abstract contract AbstractPortal is IERC165 { * @notice Defines the address of the entity issuing attestations to the subject * @dev We strongly encourage a reflection when overriding this rule: who should be set as the attester? */ - function _getAttester() public view virtual returns (address) { + function getAttester() public view virtual returns (address) { return msg.sender; } diff --git a/src/interface/IRouter.sol b/src/interface/IRouter.sol index ef6b4789..72faa5fa 100644 --- a/src/interface/IRouter.sol +++ b/src/interface/IRouter.sol @@ -1,12 +1,33 @@ // SPDX-License-Identifier: MIT pragma solidity 0.8.21; +/** + * @title Router + * @author Consensys + * @notice This contract aims to provides a single entrypoint for the Verax registries + */ interface IRouter { + /** + * @notice Gives the address for the AttestationRegistry contract + * @return The current address of the AttestationRegistry contract + */ function getAttestationRegistry() external view returns (address); + /** + * @notice Gives the address for the ModuleRegistry contract + * @return The current address of the ModuleRegistry contract + */ function getModuleRegistry() external view returns (address); + /** + * @notice Gives the address for the PortalRegistry contract + * @return The current address of the PortalRegistry contract + */ function getPortalRegistry() external view returns (address); + /** + * @notice Gives the address for the SchemaRegistry contract + * @return The current address of the SchemaRegistry contract + */ function getSchemaRegistry() external view returns (address); } diff --git a/src/portal/DefaultPortal.sol b/src/portal/DefaultPortal.sol index 26674153..0e28af63 100644 --- a/src/portal/DefaultPortal.sol +++ b/src/portal/DefaultPortal.sol @@ -10,7 +10,14 @@ import { AbstractPortal } from "../interface/AbstractPortal.sol"; * @dev This Portal does not add any logic to the AbstractPortal */ contract DefaultPortal is AbstractPortal { + /** + * @notice Contract constructor + * @param modules list of modules to use for the portal (can be empty) + * @param router the Router's address + * @dev This sets the addresses for the AttestationRegistry, ModuleRegistry and PortalRegistry + */ constructor(address[] memory modules, address router) AbstractPortal(modules, router) {} + /// @inheritdoc AbstractPortal function withdraw(address payable to, uint256 amount) external override {} } diff --git a/test/ModuleRegistry.t.sol b/test/ModuleRegistry.t.sol index 99e01b9a..5719a97e 100644 --- a/test/ModuleRegistry.t.sol +++ b/test/ModuleRegistry.t.sol @@ -42,7 +42,7 @@ contract ModuleRegistryTest is Test { ); } - function testAlreadyInitialized() public { + function test_initialize_ContractAlreadyInitialized() public { vm.expectRevert("Initializable: contract is already initialized"); moduleRegistry.initialize(); } @@ -64,7 +64,7 @@ contract ModuleRegistryTest is Test { testModuleRegistry.updateRouter(address(0)); } - function testIsContractAddress() public { + function test_isContractAddress() public { // isContractAddress should return false for EOA address address eoaAddress = vm.addr(1); bool eoaAddressResult = moduleRegistry.isContractAddress(eoaAddress); @@ -76,7 +76,7 @@ contract ModuleRegistryTest is Test { assertEq(contractAddressResult, true); } - function testRegisterModule() public { + function test_register() public { vm.expectEmit(); emit ModuleRegistered(expectedName, expectedDescription, expectedAddress); vm.prank(user); @@ -88,33 +88,33 @@ contract ModuleRegistryTest is Test { assertEq(description, expectedDescription); } - function testCannotRegisterModuleWithInvalidIssuer() public { + function test_register_OnlyIssuer() public { vm.expectRevert(ModuleRegistry.OnlyIssuer.selector); vm.startPrank(makeAddr("InvalidIssuer")); moduleRegistry.register(expectedName, expectedDescription, expectedAddress); vm.stopPrank(); } - function testCannotRegisterModuleWithoutName() public { + function test_register_ModuleNameMissing() public { vm.expectRevert(ModuleRegistry.ModuleNameMissing.selector); vm.prank(user); moduleRegistry.register("", expectedDescription, expectedAddress); } - function testCannotRegisterModuleWithInvalidModuleAddress() public { + function test_register_ModuleAddressInvalid() public { vm.expectRevert(ModuleRegistry.ModuleAddressInvalid.selector); vm.prank(user); moduleRegistry.register(expectedName, expectedDescription, vm.addr(1)); //vm.addr(1) gives EOA address } - function testCannotRegisterModuleWhichHasNotImplementedAbstractModule() public { + function test_register_ModuleInvalid() public { IncorrectModule incorrectModule = new IncorrectModule(); vm.expectRevert(ModuleRegistry.ModuleInvalid.selector); vm.prank(user); moduleRegistry.register(expectedName, expectedDescription, address(incorrectModule)); } - function testCannotRegisterModuleTwice() public { + function test_register_ModuleAlreadyExists() public { vm.prank(user); moduleRegistry.register(expectedName, expectedDescription, expectedAddress); vm.expectRevert(ModuleRegistry.ModuleAlreadyExists.selector); @@ -122,7 +122,7 @@ contract ModuleRegistryTest is Test { moduleRegistry.register(expectedName, expectedDescription, expectedAddress); } - function testStoreModuleAddress() public { + function test_getModulesNumber() public { uint256 modulesNumber = moduleRegistry.getModulesNumber(); assertEq(modulesNumber, 0); vm.prank(user); @@ -132,7 +132,7 @@ contract ModuleRegistryTest is Test { assertEq(modulesNumber, 1); } - function testRunModules() public { + function test_runModules() public { // Register 2 modules address[] memory moduleAddresses = new address[](2); moduleAddresses[0] = address(new CorrectModule()); @@ -147,7 +147,7 @@ contract ModuleRegistryTest is Test { moduleRegistry.runModules(moduleAddresses, attestationPayload, validationPayload, 0); } - function testRunModulesWithIncorrectNumberOfValidationPayload() public { + function test_runModules_ModuleValidationPayloadMismatch() public { // Register 2 modules address[] memory moduleAddresses = new address[](2); moduleAddresses[0] = address(new CorrectModule()); @@ -164,7 +164,7 @@ contract ModuleRegistryTest is Test { moduleRegistry.runModules(moduleAddresses, attestationPayload, validationPayload, 0); } - function testRunModulesWithoutSendingModuleAddresses() public { + function test_runModules_withoutModule() public { // Register a module address[] memory moduleAddresses = new address[](0); @@ -174,7 +174,7 @@ contract ModuleRegistryTest is Test { moduleRegistry.runModules(moduleAddresses, attestationPayload, validationPayload, 0); } - function testRunModulesForUnregisteredModules() public { + function test_runModules_ModuleNotRegistered() public { // Create 2 modules without registration address[] memory moduleAddresses = new address[](2); moduleAddresses[0] = address(new CorrectModule()); @@ -188,7 +188,7 @@ contract ModuleRegistryTest is Test { moduleRegistry.runModules(moduleAddresses, attestationPayload, validationPayload, 0); } - function testBulkRunModules() public { + function test_bulkRunModules() public { // Register 2 modules address[] memory moduleAddresses = new address[](2); moduleAddresses[0] = address(new CorrectModule()); @@ -213,7 +213,7 @@ contract ModuleRegistryTest is Test { vm.stopPrank(); } - function testGetModuleAddress() public { + function test_getModuleAddress() public { vm.prank(user); moduleRegistry.register(expectedName, expectedDescription, expectedAddress); @@ -221,7 +221,7 @@ contract ModuleRegistryTest is Test { assertEq(moduleAddress, expectedAddress); } - function testIsModuleRegistered() public { + function test_isRegistered() public { bool isRegistered = moduleRegistry.isRegistered(expectedAddress); assertFalse(isRegistered); vm.prank(user); diff --git a/test/PortalRegistry.t.sol b/test/PortalRegistry.t.sol index 6fc583ba..5465994f 100644 --- a/test/PortalRegistry.t.sol +++ b/test/PortalRegistry.t.sol @@ -46,7 +46,7 @@ contract PortalRegistryTest is Test { validPortalMock = new ValidPortalMock(new address[](0), address(router)); } - function test_alreadyInitialized() public { + function test_initialize_ContractAlreadyInitialized() public { vm.expectRevert("Initializable: contract is already initialized"); portalRegistry.initialize(); } @@ -60,7 +60,7 @@ contract PortalRegistryTest is Test { assertEq(routerAddress, address(1)); } - function test_updateRouter_InvalidParameter() public { + function test_updateRouter_RouterInvalid() public { PortalRegistry testPortalRegistry = new PortalRegistry(); vm.expectRevert(PortalRegistry.RouterInvalid.selector); @@ -68,7 +68,7 @@ contract PortalRegistryTest is Test { testPortalRegistry.updateRouter(address(0)); } - function test_setIssuer_isIssuer_removeIssuer() public { + function test_removeIssuer() public { vm.startPrank(address(0)); address issuerAddress = makeAddr("Issuer"); portalRegistry.setIssuer(issuerAddress); diff --git a/test/Router.t.sol b/test/Router.t.sol index 10cc6b44..24a14c85 100644 --- a/test/Router.t.sol +++ b/test/Router.t.sol @@ -23,62 +23,58 @@ contract RouterTest is Test { router.initialize(); } - function testInitialize() public { - vm.expectEmit(); - emit Initialized(1); - Router testRouter = new Router(); - testRouter.initialize(); + function test_initialize_ContractAlreadyInitialized() public { vm.expectRevert("Initializable: contract is already initialized"); - testRouter.initialize(); + router.initialize(); } - function testUpdateAttestationRegistry() public { + function test_updateAttestationRegistry() public { vm.expectEmit(); emit AttestationRegistryUpdated(attestationRegistry); router.updateAttestationRegistry(attestationRegistry); assertEq(router.getAttestationRegistry(), attestationRegistry); } - function testUpdateAttestationRegistryNotOwner() public { + function test_updateAttestationRegistry_NotOwner() public { vm.prank(user); vm.expectRevert("Ownable: caller is not the owner"); router.updateAttestationRegistry(attestationRegistry); } - function testUpdateModuleRegistry() public { + function test_updateModuleRegistry() public { vm.expectEmit(); emit ModuleRegistryUpdated(moduleRegistry); router.updateModuleRegistry(moduleRegistry); assertEq(router.getModuleRegistry(), moduleRegistry); } - function testUpdateModuleRegistryNotOwner() public { + function test_updateModuleRegistry_NotOwner() public { vm.prank(user); vm.expectRevert("Ownable: caller is not the owner"); router.updateModuleRegistry(moduleRegistry); } - function testUpdatePortalRegistry() public { + function test_updatePortalRegistry() public { vm.expectEmit(); emit PortalRegistryUpdated(portalRegistry); router.updatePortalRegistry(portalRegistry); assertEq(router.getPortalRegistry(), portalRegistry); } - function testUpdatePortalRegistryNotOwner() public { + function test_updatePortalRegistry_NotOwner() public { vm.prank(user); vm.expectRevert("Ownable: caller is not the owner"); router.updatePortalRegistry(portalRegistry); } - function testUpdateSchemaRegistry() public { + function test_updateSchemaRegistry() public { vm.expectEmit(); emit SchemaRegistryUpdated(schemaRegistry); router.updateSchemaRegistry(schemaRegistry); assertEq(router.getSchemaRegistry(), schemaRegistry); } - function testUpdateSchemaRegistryNotOwner() public { + function test_updateSchemaRegistry_NotOwner() public { vm.prank(user); vm.expectRevert("Ownable: caller is not the owner"); router.updateSchemaRegistry(schemaRegistry); diff --git a/test/SchemaRegistry.t.sol b/test/SchemaRegistry.t.sol index d85294e1..7e645357 100644 --- a/test/SchemaRegistry.t.sol +++ b/test/SchemaRegistry.t.sol @@ -34,7 +34,7 @@ contract SchemaRegistryTest is Test { portalRegistryMock.setIssuer(user); } - function testAlreadyInitialized() public { + function test_initialize_ContractAlreadyInitialized() public { vm.expectRevert("Initializable: contract is already initialized"); schemaRegistry.initialize(); } @@ -48,7 +48,7 @@ contract SchemaRegistryTest is Test { assertEq(routerAddress, address(1)); } - function test_updateRouter_InvalidParameter() public { + function test_updateRouter_RouterInvalid() public { SchemaRegistry testSchemaRegistry = new SchemaRegistry(); vm.expectRevert(SchemaRegistry.RouterInvalid.selector); @@ -56,12 +56,12 @@ contract SchemaRegistryTest is Test { testSchemaRegistry.updateRouter(address(0)); } - function testGetIdFromSchemaString() public { + function test_getIdFromSchemaString() public { bytes32 id = schemaRegistry.getIdFromSchemaString(expectedString); assertEq(id, expectedId); } - function testCreateSchema() public { + function test_createSchema() public { vm.expectEmit(); emit SchemaCreated(expectedId, expectedName, expectedDescription, expectedContext, expectedString); vm.startPrank(user); @@ -74,26 +74,26 @@ contract SchemaRegistryTest is Test { vm.stopPrank(); } - function testCannotCreateSchemaWithInvalidIssuer() public { + function test_createSchema_OnlyIssuer() public { vm.expectRevert(SchemaRegistry.OnlyIssuer.selector); vm.startPrank(makeAddr("InvalidIssuer")); schemaRegistry.createSchema(expectedName, expectedDescription, expectedContext, expectedString); vm.stopPrank(); } - function testCannotCreateSchemaWithoutName() public { + function test_createSchema_SchemaNameMissing() public { vm.expectRevert(SchemaRegistry.SchemaNameMissing.selector); vm.prank(user); schemaRegistry.createSchema("", expectedDescription, expectedContext, expectedString); } - function testCannotCreateSchemaWithoutString() public { + function test_createSchema_SchemaStringMissing() public { vm.expectRevert(SchemaRegistry.SchemaStringMissing.selector); vm.prank(user); schemaRegistry.createSchema(expectedName, expectedDescription, expectedContext, ""); } - function testCannotCreateSchemaTwice() public { + function test_createSchema_SchemaAlreadyExists() public { vm.startPrank(user); schemaRegistry.createSchema(expectedName, expectedDescription, expectedContext, expectedString); vm.expectRevert(SchemaRegistry.SchemaAlreadyExists.selector); @@ -101,7 +101,7 @@ contract SchemaRegistryTest is Test { vm.stopPrank(); } - function testUpdateContext() public { + function test_updateContext() public { vm.expectEmit(); emit SchemaCreated(expectedId, expectedName, expectedDescription, expectedContext, expectedString); vm.startPrank(user); @@ -118,21 +118,21 @@ contract SchemaRegistryTest is Test { vm.stopPrank(); } - function testCannotUpdateContextWithInvalidIssuer() public { + function test_updateContext_OnlyIssuer() public { vm.expectRevert(SchemaRegistry.OnlyIssuer.selector); vm.startPrank(makeAddr("InvalidIssuer")); schemaRegistry.updateContext(expectedId, "New context"); vm.stopPrank(); } - function testCannotUpdateContextWithSchemaNotRegistered() public { + function test_updateContext_SchemaNotRegistered() public { vm.startPrank(user); vm.expectRevert(SchemaRegistry.SchemaNotRegistered.selector); schemaRegistry.updateContext("Invalid ID", "New context"); vm.stopPrank(); } - function testCanUpdateContextWithEmptySchemaContext() public { + function test_updateContext_withEmptyContext() public { vm.startPrank(user); schemaRegistry.createSchema(expectedName, expectedDescription, expectedContext, expectedString); schemaRegistry.updateContext(expectedId, ""); @@ -140,7 +140,7 @@ contract SchemaRegistryTest is Test { vm.stopPrank(); } - function testGetSchema() public { + function test_getSchema() public { vm.startPrank(user); schemaRegistry.createSchema(expectedName, expectedDescription, expectedContext, expectedString); @@ -151,12 +151,12 @@ contract SchemaRegistryTest is Test { vm.stopPrank(); } - function testGetSchemaNotRegistered() public { + function test_getSchema_SchemaNotRegistered() public { vm.expectRevert(SchemaRegistry.SchemaNotRegistered.selector); schemaRegistry.getSchema(bytes32("not registered")); } - function testStoreSchemaId() public { + function test_getSchemasNumber() public { uint256 schemasNumber = schemaRegistry.getSchemasNumber(); assertEq(schemasNumber, 0); vm.startPrank(user); @@ -167,7 +167,7 @@ contract SchemaRegistryTest is Test { vm.stopPrank(); } - function testGetSchemaId() public { + function test_getSchemaIds() public { vm.startPrank(user); schemaRegistry.createSchema(expectedName, expectedDescription, expectedContext, expectedString); @@ -176,7 +176,7 @@ contract SchemaRegistryTest is Test { vm.stopPrank(); } - function testIsSchemaRegistered() public { + function test_isRegistered() public { bool isRegistered = schemaRegistry.isRegistered(expectedId); assertFalse(isRegistered); vm.startPrank(user); diff --git a/test/example/EASPortal.t.sol b/test/example/EASPortal.t.sol index 061d1938..404bfc89 100644 --- a/test/example/EASPortal.t.sol +++ b/test/example/EASPortal.t.sol @@ -150,7 +150,7 @@ contract EASPortalTest is Test { easPortal.bulkRevoke(attestationsToRevoke); } - function testSupportsInterface() public { + function test_supportsInterface() public { bool isIERC165Supported = easPortal.supportsInterface(type(ERC165Upgradeable).interfaceId); assertEq(isIERC165Supported, true); bool isEASAbstractPortalSupported = easPortal.supportsInterface(type(AbstractPortal).interfaceId); @@ -159,7 +159,7 @@ contract EASPortalTest is Test { function test_getAttester() public { vm.prank(attester); - address registeredAttester = easPortal._getAttester(); + address registeredAttester = easPortal.getAttester(); assertEq(registeredAttester, attester); } } diff --git a/test/example/MsgSenderModule.t.sol b/test/example/MsgSenderModule.t.sol index ce0be45b..9459f94f 100644 --- a/test/example/MsgSenderModule.t.sol +++ b/test/example/MsgSenderModule.t.sol @@ -24,7 +24,7 @@ contract MsgSenderModuleTest is Test { ); } - function testCorrectMsgSenderAddress() public { + function test_MsgSenderModule_correctAddress() public { assertEq(msgSenderModule.expectedMsgSender(), expectedMsgSender); bytes memory validationPayload = new bytes(0); address msgSender = expectedMsgSender; @@ -32,7 +32,7 @@ contract MsgSenderModuleTest is Test { msgSenderModule.run(attestationPayload, validationPayload, msgSender, 0); } - function testIncorrectMsgSenderAddress() public { + function test_MsgSenderModule_incorrectAddress() public { assertEq(msgSenderModule.expectedMsgSender(), expectedMsgSender); bytes memory validationPayload = new bytes(0); address incorrectMsgSender = address(1); @@ -41,7 +41,7 @@ contract MsgSenderModuleTest is Test { msgSenderModule.run(attestationPayload, validationPayload, incorrectMsgSender, 0); } - function testSupportsInterface() public { + function test_MsgSenderModule_supportsInterface() public { bool isAbstractModuleSupported = msgSenderModule.supportsInterface(type(AbstractModule).interfaceId); assertEq(isAbstractModuleSupported, true); } diff --git a/test/mocks/AttestationRegistryMock.sol b/test/mocks/AttestationRegistryMock.sol index c7d09434..61180e8f 100644 --- a/test/mocks/AttestationRegistryMock.sol +++ b/test/mocks/AttestationRegistryMock.sol @@ -17,9 +17,6 @@ contract AttestationRegistryMock { function test() public {} function attest(AttestationPayload calldata attestationPayload, address attester) public { - require(bytes32(attestationPayload.schemaId) != 0, "Invalid attestationPayload"); - require(attester != address(0), "Invalid attester"); - attestationIdCounter++; // Create attestation Attestation memory attestation = Attestation( @@ -41,33 +38,30 @@ contract AttestationRegistryMock { emit AttestationRegistered(); } - function bulkAttest(AttestationPayload[] calldata attestationsPayloads, address attester) public { - require(attestationsPayloads.length > 0, "Invalid attestationsPayloads"); - require(attester != address(0), "Invalid attester"); + function bulkAttest(AttestationPayload[] calldata /*attestationsPayloads*/, address /*attester*/) public { emit BulkAttestationsRegistered(); } - function replace(bytes32 /*attestationId*/, AttestationPayload calldata attestationPayload, address attester) public { - require(bytes32(attestationPayload.schemaId) != 0, "Invalid attestationPayload"); - require(attester != address(0), "Invalid attester"); - + function replace( + bytes32 /*attestationId*/, + AttestationPayload calldata /*attestationPayload*/, + address /*attester*/ + ) public { emit AttestationRegistered(); emit AttestationReplaced(); } function bulkReplace( - bytes32[] calldata attestationId, - AttestationPayload[] calldata attestationPayload, - address attester + bytes32[] calldata /*attestationId*/, + AttestationPayload[] calldata /*attestationPayload*/, + address /*attester*/ ) public {} function revoke(bytes32 attestationId) public { - require(bytes32(attestationId) != 0, "Invalid attestation"); emit AttestationRevoked(attestationId); } function bulkRevoke(bytes32[] memory attestationIds) public { - require(attestationIds.length > 0, "Invalid attestation"); emit BulkAttestationsRevoked(attestationIds); } diff --git a/test/mocks/ModuleRegistryMock.sol b/test/mocks/ModuleRegistryMock.sol index 33f2c86d..24a2a74d 100644 --- a/test/mocks/ModuleRegistryMock.sol +++ b/test/mocks/ModuleRegistryMock.sol @@ -10,20 +10,17 @@ contract ModuleRegistryMock { function test() public {} function runModules( - address[] memory modulesAddresses, - AttestationPayload memory attestationPayload, - bytes[] memory validationPayload, - uint256 value + address[] memory /*modulesAddresses*/, + AttestationPayload memory /*attestationPayload*/, + bytes[] memory /*validationPayload*/, + uint256 /*value*/ ) public {} function bulkRunModules( - address[] memory modulesAddresses, - AttestationPayload[] memory attestationsPayloads, - bytes[][] memory validationPayloads + address[] memory /*modulesAddresses*/, + AttestationPayload[] memory /*attestationsPayloads*/, + bytes[][] memory /*validationPayloads*/ ) public { - require(modulesAddresses.length >= 0, "Invalid modulesAddresses"); - require(attestationsPayloads.length >= 0, "Invalid attestationsPayloads"); - require(validationPayloads.length >= 0, "Invalid validationPayloads"); emit ModulesBulkRunForAttestation(); } } diff --git a/test/mocks/PortalRegistryMock.sol b/test/mocks/PortalRegistryMock.sol index f342cd38..beac6263 100644 --- a/test/mocks/PortalRegistryMock.sol +++ b/test/mocks/PortalRegistryMock.sol @@ -36,10 +36,6 @@ contract PortalRegistryMock { issuers[issuer] = true; } - /** - * @notice Checks if a given address is an issuer - * @return A flag indicating whether the given address is an issuer - */ function isIssuer(address issuer) public view returns (bool) { return issuers[issuer]; } diff --git a/test/portal/DefaultPortal.t.sol b/test/portal/DefaultPortal.t.sol index 8bc83448..4812ba18 100644 --- a/test/portal/DefaultPortal.t.sol +++ b/test/portal/DefaultPortal.t.sol @@ -44,7 +44,7 @@ contract DefaultPortalTest is Test { portalRegistryMock.register(address(defaultPortal), "Name", "Description", true, "Owner name"); } - function test_setup() public { + function test_setUp() public { assertEq(address(defaultPortal.modules(0)), address(modules[0])); assertEq(address(defaultPortal.moduleRegistry()), address(moduleRegistryMock)); assertEq(address(defaultPortal.attestationRegistry()), address(attestationRegistryMock)); @@ -195,7 +195,7 @@ contract DefaultPortalTest is Test { defaultPortal.bulkRevoke(attestationsToRevoke); } - function testSupportsInterface() public { + function test_supportsInterface() public { bool isIERC165Supported = defaultPortal.supportsInterface(type(ERC165Upgradeable).interfaceId); assertEq(isIERC165Supported, true); bool isAbstractPortalSupported = defaultPortal.supportsInterface(type(AbstractPortal).interfaceId); @@ -205,7 +205,7 @@ contract DefaultPortalTest is Test { function test_getAttester() public { address attester = makeAddr("attester"); vm.prank(attester); - address registeredAttester = defaultPortal._getAttester(); + address registeredAttester = defaultPortal.getAttester(); assertEq(registeredAttester, attester); } }