refactor: use custom errors in solidity (#127)

* refactor: change ap reverts to custom error

* test: fix tests for custom errors

* refactor: change setApprovalForAccessPoint string reverts to custom error

contracts/contracts/FleekAccessControl.sol

@@ -10,7 +10,6 @@ error MustHaveAtLeastOneOwner();
 error RoleAlreadySet();
 
 contract FleekAccessControl is Initializable {
-
     /**
      * @dev All available collection roles.
      */
@@ -127,8 +126,7 @@ contract FleekAccessControl is Initializable {
      */
     function _revokeCollectionRole(CollectionRoles role, address account) internal {
         if (!hasCollectionRole(role, account)) revert RoleAlreadySet();
-        if (role == CollectionRoles.Owner && _collectionRolesCounter[role] == 1)
+        if (role == CollectionRoles.Owner && _collectionRolesCounter[role] == 1) revert MustHaveAtLeastOneOwner();
-            revert MustHaveAtLeastOneOwner();
 
         _collectionRoles[role][account] = false;
         _collectionRolesCounter[role] -= 1;

contracts/contracts/FleekERC721.sol

@@ -9,8 +9,14 @@ import "./FleekAccessControl.sol";
 import "./util/FleekStrings.sol";
 import "./FleekPausable.sol";
 
+error AccessPointNotExistent();
+error AccessPointAlreadyExists();
+error AccessPointScoreCannotBeLower();
+error MustBeAccessPointOwner();
 error MustBeTokenOwner(uint256 tokenId);
 error ThereIsNoTokenMinted();
+error InvalidTokenIdForAccessPoint();
+error AccessPointCreationStatusAlreadySet();
 
 contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl, FleekPausable {
     using Strings for uint256;
@@ -40,11 +46,7 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl, Fl
     event NewAccessPoint(string apName, uint256 indexed tokenId, address indexed owner);
     event RemoveAccessPoint(string apName, uint256 indexed tokenId, address indexed owner);
 
-    event ChangeAccessPointAutoApproval(
+    event ChangeAccessPointAutoApproval(uint256 indexed token, bool indexed settings, address indexed triggeredBy);
-        uint256 indexed token,
-        bool indexed settings,
-        address indexed triggeredBy
-    );
 
     event ChangeAccessPointScore(string apName, uint256 indexed tokenId, uint256 score, address indexed triggeredBy);
 
@@ -132,7 +134,7 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl, Fl
      * @dev Checks if the AccessPoint exists.
      */
     modifier requireAP(string memory apName) {
-        require(_accessPoints[apName].owner != address(0), "FleekERC721: invalid AP");
+        if (_accessPoints[apName].owner == address(0)) revert AccessPointNotExistent();
         _;
     }
 
@@ -161,7 +163,7 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl, Fl
     ) public payable requireCollectionRole(CollectionRoles.Owner) returns (uint256) {
         uint256 tokenId = _appIds;
         _mint(to, tokenId);
-        
+
         _appIds += 1;
 
         App storage app = _apps[tokenId];
@@ -453,7 +455,7 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl, Fl
     function addAccessPoint(uint256 tokenId, string memory apName) public payable whenNotPaused {
         // require(msg.value == 0.1 ether, "You need to pay at least 0.1 ETH"); // TODO: define a minimum price
         _requireMinted(tokenId);
-        require(_accessPoints[apName].owner == address(0), "FleekERC721: AP already exists");
+        if (_accessPoints[apName].owner != address(0)) revert AccessPointAlreadyExists();
 
         emit NewAccessPoint(apName, tokenId, msg.sender);
 
@@ -494,14 +496,8 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl, Fl
         bool approved
     ) public requireTokenOwner(tokenId) {
         AccessPoint storage accessPoint = _accessPoints[apName];
-        require(
+        if (accessPoint.tokenId != tokenId) revert InvalidTokenIdForAccessPoint();
-            accessPoint.tokenId == tokenId,
+        if (accessPoint.status != AccessPointCreationStatus.DRAFT) revert AccessPointCreationStatusAlreadySet();
-            "FleekERC721: the passed tokenId is not the same as the access point's tokenId."
-        );
-        require(
-            accessPoint.status == AccessPointCreationStatus.DRAFT,
-            "FleekERC721: the access point creation status has been set before."
-        );
 
         if (approved) {
             // Approval
@@ -528,7 +524,7 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl, Fl
      *
      */
     function removeAccessPoint(string memory apName) public whenNotPaused requireAP(apName) {
-        require(msg.sender == _accessPoints[apName].owner, "FleekERC721: must be AP owner");
+        if (msg.sender != _accessPoints[apName].owner) revert MustBeAccessPointOwner();
         _accessPoints[apName].status = AccessPointCreationStatus.REMOVED;
         uint256 tokenId = _accessPoints[apName].tokenId;
         emit ChangeAccessPointStatus(apName, tokenId, AccessPointCreationStatus.REMOVED, msg.sender);
@@ -587,7 +583,7 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl, Fl
      *
      */
     function decreaseAccessPointScore(string memory apName) public requireAP(apName) {
-        require(_accessPoints[apName].score > 0, "FleekERC721: score cant be lower");
+        if (_accessPoints[apName].score == 0) revert AccessPointScoreCannotBeLower();
         _accessPoints[apName].score--;
         emit ChangeAccessPointScore(apName, _accessPoints[apName].tokenId, _accessPoints[apName].score, msg.sender);
     }

contracts/test/foundry/FleekERC721/AccessPoints/AccessPointsAutoApprovalOff.t.sol

@@ -6,7 +6,7 @@ import "../TestBase.sol";
 import {Strings} from "@openzeppelin/contracts/utils/Strings.sol";
 import {FleekAccessControl} from "contracts/FleekAccessControl.sol";
 import "../../../../contracts/FleekERC721.sol";
-import './ApBase.sol';
+import "./ApBase.sol";
 
 contract Test_FleekERC721_AccessPoint is Test_FleekERC721_Base, APConstants {
     using Strings for address;
@@ -37,7 +37,7 @@ contract Test_FleekERC721_AccessPoint is Test_FleekERC721_Base, APConstants {
         string memory accessPointName = "accesspoint.com";
         CuT.addAccessPoint(tokenId, accessPointName);
         CuT.removeAccessPoint(accessPointName);
- 
+
         APConstants.assertAccessPointJSON(
             accessPointName,
             "0",

contracts/test/foundry/FleekERC721/TestBase.sol

@@ -20,19 +20,19 @@ abstract contract Test_FleekERC721_Assertions is Test {
     }
 
     function expectRevertWithAPAlreadyExists() public {
-        vm.expectRevert("FleekERC721: AP already exists");
+        vm.expectRevert(AccessPointAlreadyExists.selector);
     }
 
     function expectRevertWithMustBeAPOwner() public {
-        vm.expectRevert("FleekERC721: must be AP owner");
+        vm.expectRevert(MustBeAccessPointOwner.selector);
     }
 
     function expectRevertWithInvalidAP() public {
-        vm.expectRevert("FleekERC721: invalid AP");
+        vm.expectRevert(AccessPointNotExistent.selector);
     }
 
     function expectRevertWithMinimalScore() public {
-        vm.expectRevert("FleekERC721: score cant be lower");
+        vm.expectRevert(AccessPointScoreCannotBeLower.selector);
     }
 
     function expectRevertWithInvalidTokenId() public {

contracts/test/hardhat/contracts/FleekERC721/access-point/access-points-autoapproval-off.t.ts

@@ -1,7 +1,6 @@
 import { loadFixture } from '@nomicfoundation/hardhat-network-helpers';
 import { expect } from 'chai';
-import { before } from 'mocha';
+import { TestConstants, Fixtures, Errors } from '../helpers';
-import { TestConstants, Fixtures } from '../helpers';
 const { AccessPointStatus } = TestConstants;
 
 describe('FleekERC721.AccessPoints.AutoApprovalOff', () => {
@@ -54,7 +53,7 @@ describe('FleekERC721.AccessPoints.AutoApprovalOff', () => {
 
     await expect(
       contract.getAccessPointJSON('accesspoint.com')
-    ).to.be.revertedWith('FleekERC721: invalid AP');
+    ).to.be.revertedWithCustomError(contract, Errors.AccessPointNotExistent);
   });
 
   it('should increase the AP score', async () => {
@@ -126,7 +125,7 @@ describe('FleekERC721.AccessPoints.AutoApprovalOff', () => {
 
     await expect(
       contract.connect(otherAccount).removeAccessPoint('accesspoint.com')
-    ).to.be.revertedWith('FleekERC721: must be AP owner');
+    ).to.be.revertedWithCustomError(contract, Errors.MustBeAccessPointOwner);
   });
 
   it('should not be allowed to add the same AP more than once', async () => {
@@ -136,7 +135,7 @@ describe('FleekERC721.AccessPoints.AutoApprovalOff', () => {
 
     await expect(
       contract.addAccessPoint(tokenId, 'accesspoint.com')
-    ).to.be.revertedWith('FleekERC721: AP already exists');
+    ).to.be.revertedWithCustomError(contract, Errors.AccessPointAlreadyExists);
   });
 
   it('should change "contentVerified" to true', async () => {
@@ -204,10 +203,7 @@ describe('FleekERC721.AccessPoints.AutoApprovalOff', () => {
   it('should token owner be able to change the auto approval settings to on', async () => {
     const { contract, tokenId, owner } = fixture;
 
-
+    await contract.connect(owner).setAccessPointAutoApproval(tokenId, true);
-    await contract
-      .connect(owner)
-      .setAccessPointAutoApproval(tokenId, true);
 
     await contract.addAccessPoint(tokenId, 'accesspoint.com');
 

contracts/test/hardhat/contracts/FleekERC721/access-point/access-points-autoapproval-on.t.ts

@@ -1,7 +1,6 @@
 import { loadFixture } from '@nomicfoundation/hardhat-network-helpers';
 import { expect } from 'chai';
-import { before } from 'mocha';
+import { TestConstants, Fixtures, Errors } from '../helpers';
-import { TestConstants, Fixtures } from '../helpers';
 const { AccessPointStatus } = TestConstants;
 
 describe('FleekERC721.AccessPoints.AutoApprovalOn', () => {
@@ -45,9 +44,9 @@ describe('FleekERC721.AccessPoints.AutoApprovalOn', () => {
   it('should revert if AP does not exist', async () => {
     const { contract, tokenId } = fixture;
 
-    await expect(contract.getAccessPointJSON('random.com')).to.be.revertedWith(
+    await expect(
-      'FleekERC721: invalid AP'
+      contract.getAccessPointJSON('random.com')
-    );
+    ).to.be.revertedWithCustomError(contract, Errors.AccessPointNotExistent);
   });
 
   it('should increase the AP score', async () => {
@@ -120,7 +119,7 @@ describe('FleekERC721.AccessPoints.AutoApprovalOn', () => {
 
     await expect(
       contract.connect(otherAccount).removeAccessPoint(DefaultAP)
-    ).to.be.revertedWith('FleekERC721: must be AP owner');
+    ).to.be.revertedWithCustomError(contract, Errors.MustBeAccessPointOwner);
   });
 
   it('should not be allowed to add the same AP more than once', async () => {
@@ -128,7 +127,7 @@ describe('FleekERC721.AccessPoints.AutoApprovalOn', () => {
 
     await expect(
       contract.addAccessPoint(tokenId, DefaultAP)
-    ).to.be.revertedWith('FleekERC721: AP already exists');
+    ).to.be.revertedWithCustomError(contract, Errors.AccessPointAlreadyExists);
   });
 
   it('should change "contentVerified" to true', async () => {

contracts/test/hardhat/contracts/FleekERC721/helpers/errors.ts

@@ -1,4 +1,8 @@
 export const Errors = Object.freeze({
+  AccessPointNotExistent: 'AccessPointNotExistent',
+  AccessPointAlreadyExists: 'AccessPointAlreadyExists',
+  AccessPointScoreCannotBeLower: 'AccessPointScoreCannotBeLower',
+  MustBeAccessPointOwner: 'MustBeAccessPointOwner',
   MustHaveCollectionRole: 'MustHaveCollectionRole',
   MustHaveTokenRole: 'MustHaveTokenRole',
   MustHaveAtLeastOneOwner: 'MustHaveAtLeastOneOwner',

subgraph/src/fleek-nfa.ts

@@ -159,8 +159,7 @@ export function handleNewMint(event: NewMintEvent): void {
   let commitHash = event.params.commitHash;
   let logo = event.params.logo;
   let color = event.params.color;
-  let accessPointAutoApproval =
+  let accessPointAutoApproval = event.params.accessPointAutoApproval;
-    event.params.accessPointAutoApproval;
   let tokenId = event.params.tokenId;
   let ownerAddress = event.params.owner;
 
@@ -173,8 +172,7 @@ export function handleNewMint(event: NewMintEvent): void {
   newMintEntity.gitRepository = gitRepository;
   newMintEntity.logo = logo;
   newMintEntity.color = color;
-  newMintEntity.accessPointAutoApproval =
+  newMintEntity.accessPointAutoApproval = accessPointAutoApproval;
-    accessPointAutoApproval;
   newMintEntity.triggeredBy = event.params.minter;
   newMintEntity.tokenOwner = ownerAddress;
   newMintEntity.blockNumber = event.block.number;
@@ -229,8 +227,10 @@ export function handleNewMint(event: NewMintEvent): void {
   token.save();
 }
 
-export function handleMetadataUpdateWithStringValue(event: MetadataUpdateEvent): void {
+export function handleMetadataUpdateWithStringValue(
-  /** 
+  event: MetadataUpdateEvent
+): void {
+  /**
    * Metadata handled here:
    * setTokenExternalURL
    * setTokenENS
@@ -252,7 +252,9 @@ export function handleMetadataUpdateWithStringValue(event: MetadataUpdateEvent):
   entity.save();
 
   // UPDATE TOKEN
-  let token = Token.load(Bytes.fromByteArray(Bytes.fromBigInt(event.params._tokenId)));
+  let token = Token.load(
+    Bytes.fromByteArray(Bytes.fromBigInt(event.params._tokenId))
+  );
 
   if (token) {
     if (event.params.key == 'externalURL') {
@@ -266,12 +268,14 @@ export function handleMetadataUpdateWithStringValue(event: MetadataUpdateEvent):
     } else {
       // logo
       token.logo = event.params.value;
-    }   
+    }
     token.save();
   }
 }
 
-export function handleMetadataUpdateWithDoubleStringValue(event: MetadataUpdateEvent2): void {
+export function handleMetadataUpdateWithDoubleStringValue(
+  event: MetadataUpdateEvent2
+): void {
   /**
    * setTokenBuild
    */
@@ -289,7 +293,9 @@ export function handleMetadataUpdateWithDoubleStringValue(event: MetadataUpdateE
   entity.save();
 
   // UPDATE TOKEN
-  let token = Token.load(Bytes.fromByteArray(Bytes.fromBigInt(event.params._tokenId)));
+  let token = Token.load(
+    Bytes.fromByteArray(Bytes.fromBigInt(event.params._tokenId))
+  );
 
   if (token) {
     if (event.params.key == 'build') {
@@ -302,11 +308,13 @@ export function handleMetadataUpdateWithDoubleStringValue(event: MetadataUpdateE
       token.gitRepository = event.params.value[1];
       token.save();
       gitRepositoryEntity.save();
-    }  
+    }
   }
 }
 
-export function handleMetadataUpdateWithIntValue(event: MetadataUpdateEvent1): void {
+export function handleMetadataUpdateWithIntValue(
+  event: MetadataUpdateEvent1
+): void {
   /**
    * setTokenColor
    */
@@ -323,7 +331,9 @@ export function handleMetadataUpdateWithIntValue(event: MetadataUpdateEvent1): v
 
   entity.save();
 
-  let token = Token.load(Bytes.fromByteArray(Bytes.fromBigInt(event.params._tokenId)));
+  let token = Token.load(
+    Bytes.fromByteArray(Bytes.fromBigInt(event.params._tokenId))
+  );
 
   if (token) {
     if (event.params.key == 'color') {