refactor: make a single event for token metadata changes (#111)

* refactor: add new unified events and remove old multiple ones

* fix: remove indexed from string params on events

* test: add hardhat tests for metadata update changes

* refactor: change event name

* test: add foundry tests for event emits in metadata changes

* refactor: undo changes on accespoint events

* chore: remove subgraph ci tests on prs for develop

* refactor: overload MetadataUpdate event to have multiple type of parameters

---------

Co-authored-by: Shredder <110225819+EmperorOrokuSaki@users.noreply.github.com>

.github/workflows/subgraph.yml

@@ -4,7 +4,6 @@ on:
   pull_request:
     branches:    
       - main
-      - develop
   push:
     paths:
       - 'subgraph/**'

contracts/contracts/FleekERC721.sol

@@ -15,18 +15,16 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl {
     using FleekStrings for FleekERC721.App;
     using FleekStrings for FleekERC721.AccessPoint;
     using FleekStrings for string;
+    using FleekStrings for uint24;
 
-    event NewBuild(uint256 indexed tokenId, string commitHash, address indexed triggeredBy);
-    event NewTokenName(uint256 indexed tokenId, string name, address indexed triggeredBy);
-    event NewTokenDescription(uint256 indexed tokenId, string description, address indexed triggeredBy);
-    event NewTokenLogo(uint256 indexed tokenId, string logo, address indexed triggeredBy);
-    event NewTokenExternalURL(uint256 indexed tokenId, string externalURL, address indexed triggeredBy);
-    event NewTokenENS(uint256 indexed tokenId, string ENS, address indexed triggeredBy);
-    event NewTokenColor(uint256 indexed tokenId, uint24 color, address indexed triggeredBy);
+    event MetadataUpdate(uint256 indexed _tokenId, string key, string value, address indexed triggeredBy);
+    event MetadataUpdate(uint256 indexed _tokenId, string key, uint24 value, address indexed triggeredBy);
+    event MetadataUpdate(uint256 indexed _tokenId, string key, string[2] value, address indexed triggeredBy);
 
     event NewAccessPoint(string apName, uint256 indexed tokenId, address indexed owner);
     event RemoveAccessPoint(string apName, uint256 indexed tokenId, address indexed owner);
     event ChangeAccessPointScore(string apName, uint256 indexed tokenId, uint256 score, address indexed triggeredBy);
+
     event ChangeAccessPointNameVerify(
         string apName,
         uint256 tokenId,
@@ -39,7 +37,6 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl {
         bool indexed verified,
         address indexed triggeredBy
     );
-
     /**
      * The properties are stored as string to keep consistency with
      * other token contracts, we might consider changing for bytes32
@@ -230,7 +227,7 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl {
     ) public virtual requireTokenRole(tokenId, Roles.Controller) {
         _requireMinted(tokenId);
         _apps[tokenId].externalURL = _tokenExternalURL;
-        emit NewTokenExternalURL(tokenId, _tokenExternalURL, msg.sender);
+        emit MetadataUpdate(tokenId, "externalURL", _tokenExternalURL, msg.sender);
     }
 
     /**
@@ -250,7 +247,7 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl {
     ) public virtual requireTokenRole(tokenId, Roles.Controller) {
         _requireMinted(tokenId);
         _apps[tokenId].ENS = _tokenENS;
-        emit NewTokenENS(tokenId, _tokenENS, msg.sender);
+        emit MetadataUpdate(tokenId, "ENS", _tokenENS, msg.sender);
     }
 
     /**
@@ -270,7 +267,7 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl {
     ) public virtual requireTokenRole(tokenId, Roles.Controller) {
         _requireMinted(tokenId);
         _apps[tokenId].name = _tokenName;
-        emit NewTokenName(tokenId, _tokenName, msg.sender);
+        emit MetadataUpdate(tokenId, "name", _tokenName, msg.sender);
     }
 
     /**
@@ -290,7 +287,7 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl {
     ) public virtual requireTokenRole(tokenId, Roles.Controller) {
         _requireMinted(tokenId);
         _apps[tokenId].description = _tokenDescription;
-        emit NewTokenDescription(tokenId, _tokenDescription, msg.sender);
+        emit MetadataUpdate(tokenId, "description", _tokenDescription, msg.sender);
     }
 
     /**
@@ -310,7 +307,7 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl {
     ) public virtual requireTokenRole(tokenId, Roles.Controller) {
         _requireMinted(tokenId);
         _apps[tokenId].logo = _tokenLogo;
-        emit NewTokenLogo(tokenId, _tokenLogo, msg.sender);
+        emit MetadataUpdate(tokenId, "logo", _tokenLogo, msg.sender);
     }
 
     /**
@@ -330,7 +327,7 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl {
     ) public virtual requireTokenRole(tokenId, Roles.Controller) {
         _requireMinted(tokenId);
         _apps[tokenId].color = _tokenColor;
-        emit NewTokenColor(tokenId, _tokenColor, msg.sender);
+        emit MetadataUpdate(tokenId, "color", _tokenColor, msg.sender);
     }
 
     /**
@@ -504,7 +501,7 @@ contract FleekERC721 is Initializable, ERC721Upgradeable, FleekAccessControl {
     ) public virtual requireTokenRole(tokenId, Roles.Controller) {
         _requireMinted(tokenId);
         _apps[tokenId].builds[++_apps[tokenId].currentBuild] = Build(_commitHash, _gitRepository);
-        emit NewBuild(tokenId, _commitHash, msg.sender);
+        emit MetadataUpdate(tokenId, "build", [_commitHash, _gitRepository], msg.sender);
     }
 
     /**

contracts/test/foundry/FleekERC721/TokenURI.t.sol

@@ -4,7 +4,38 @@ pragma solidity ^0.8.17;
 
 import "./TestBase.sol";
 
-contract Test_FleekERC721_TokenURI is Test_FleekERC721_Base {
+contract Test_FleekERC721_TokenURIAssertions is Test {
+    event MetadataUpdate(uint256 indexed _tokenId, string key, string value, address indexed triggeredBy);
+    event MetadataUpdate(uint256 indexed _tokenId, string key, string[2] value, address indexed triggeredBy);
+    event MetadataUpdate(uint256 indexed _tokenId, string key, uint24 value, address indexed triggeredBy);
+
+    function expectMetadataUpdate(
+        uint256 _tokenId,
+        string memory key,
+        string memory value,
+        address triggeredBy
+    ) public {
+        vm.expectEmit(true, true, true, true);
+        emit MetadataUpdate(_tokenId, key, value, triggeredBy);
+    }
+
+    function expectMetadataUpdate(
+        uint256 _tokenId,
+        string memory key,
+        string[2] memory value,
+        address triggeredBy
+    ) public {
+        vm.expectEmit(true, true, true, true);
+        emit MetadataUpdate(_tokenId, key, value, triggeredBy);
+    }
+
+    function expectMetadataUpdate(uint256 _tokenId, string memory key, uint24 value, address triggeredBy) public {
+        vm.expectEmit(true, true, true, true);
+        emit MetadataUpdate(_tokenId, key, value, triggeredBy);
+    }
+}
+
+contract Test_FleekERC721_TokenURI is Test_FleekERC721_Base, Test_FleekERC721_TokenURIAssertions {
     uint256 internal tokenId;
 
     function setUp() public {
@@ -47,4 +78,32 @@ contract Test_FleekERC721_TokenURI is Test_FleekERC721_Base {
     function testFail_tokenURIForInexistentId() public view {
         CuT.tokenURI(1);
     }
+
+    function test_shouldEmitEventForMetadataChanges() public {
+        expectMetadataUpdate(tokenId, "name", "New App Name", deployer);
+        CuT.setTokenName(tokenId, "New App Name");
+
+        expectMetadataUpdate(tokenId, "description", "New description for the app.", deployer);
+        CuT.setTokenDescription(tokenId, "New description for the app.");
+
+        expectMetadataUpdate(tokenId, "externalURL", "https://new-url.com", deployer);
+        CuT.setTokenExternalURL(tokenId, "https://new-url.com");
+
+        expectMetadataUpdate(tokenId, "ENS", "new-ens.eth", deployer);
+        CuT.setTokenENS(tokenId, "new-ens.eth");
+
+        expectMetadataUpdate(
+            tokenId,
+            "build",
+            ["ce1a3fc141e29f8e1d00a654e156c4982d7711bf", "https://github.com/other/repo"],
+            deployer
+        );
+        CuT.setTokenBuild(tokenId, "ce1a3fc141e29f8e1d00a654e156c4982d7711bf", "https://github.com/other/repo");
+
+        expectMetadataUpdate(tokenId, "logo", TestConstants.LOGO_1, deployer);
+        CuT.setTokenLogo(tokenId, TestConstants.LOGO_1);
+
+        expectMetadataUpdate(tokenId, "color", 0x654321, deployer);
+        CuT.setTokenColor(tokenId, 0x654321);
+    }
 }

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

@@ -0,0 +1,7 @@
+export const Events = Object.freeze({
+  MetadataUpdate: {
+    string: 'MetadataUpdate(uint256,string,string,address)',
+    uint24: 'MetadataUpdate(uint256,string,uint24,address)',
+    stringTuple: 'MetadataUpdate(uint256,string,string[2],address)',
+  },
+});

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

@@ -1,3 +1,4 @@
 export * from './constants';
 export * from './fixture';
 export * from './utils';
+export * from './events';

contracts/test/hardhat/contracts/FleekERC721/update-properties.t.ts

@@ -1,6 +1,6 @@
 import { loadFixture } from '@nomicfoundation/hardhat-network-helpers';
 import { expect } from 'chai';
-import { TestConstants, Fixtures } from './helpers';
+import { TestConstants, Fixtures, Events } from './helpers';
 
 const {
   Logos: { 1: Logo1 },
@@ -14,6 +14,51 @@ describe('FleekERC721.UpdateProperties', () => {
     fixture = await loadFixture(Fixtures.withMint);
   });
 
+  it('should emit event for external url change', async () => {
+    const { contract, tokenId, owner } = fixture;
+
+    await expect(contract.setTokenExternalURL(tokenId, 'https://app.com'))
+      .to.emit(contract, Events.MetadataUpdate.string)
+      .withArgs(tokenId, 'externalURL', 'https://app.com', owner.address);
+  });
+
+  it('should emit event for ens change', async () => {
+    const { contract, tokenId, owner } = fixture;
+
+    await expect(contract.setTokenENS(tokenId, 'app.eth'))
+      .to.emit(contract, Events.MetadataUpdate.string)
+      .withArgs(tokenId, 'ENS', 'app.eth', owner.address);
+  });
+
+  it('should emit event for name change', async () => {
+    const { contract, tokenId, owner } = fixture;
+
+    await expect(contract.setTokenName(tokenId, 'App'))
+      .to.emit(contract, Events.MetadataUpdate.string)
+      .withArgs(tokenId, 'name', 'App', owner.address);
+  });
+
+  it('should emit event for description change', async () => {
+    const { contract, tokenId, owner } = fixture;
+
+    await expect(contract.setTokenDescription(tokenId, 'App Description'))
+      .to.emit(contract, Events.MetadataUpdate.string)
+      .withArgs(tokenId, 'description', 'App Description', owner.address);
+  });
+
+  it('should emit event for build change', async () => {
+    const { contract, tokenId, owner } = fixture;
+
+    await expect(contract.setTokenBuild(tokenId, 'commitHash', 'gitRepository'))
+      .to.emit(contract, Events.MetadataUpdate.stringTuple)
+      .withArgs(
+        tokenId,
+        'build',
+        ['commitHash', 'gitRepository'],
+        owner.address
+      );
+  });
+
   it('should update token logo', async () => {
     const { contract, tokenId } = fixture;
     await contract.setTokenLogo(tokenId, Logo1);
@@ -32,6 +77,14 @@ describe('FleekERC721.UpdateProperties', () => {
     );
   });
 
+  it('should emit metadata updated event for logo change', async () => {
+    const { contract, tokenId, owner } = fixture;
+
+    await expect(contract.setTokenLogo(tokenId, Logo1))
+      .to.emit(contract, Events.MetadataUpdate.string)
+      .withArgs(tokenId, 'logo', Logo1, owner.address);
+  });
+
   it('should update token color', async () => {
     const { contract, tokenId } = fixture;
     await contract.setTokenColor(tokenId, Color1);
@@ -55,6 +108,14 @@ describe('FleekERC721.UpdateProperties', () => {
     );
   });
 
+  it('should emit metadata updated event for color change', async () => {
+    const { contract, tokenId, owner } = fixture;
+
+    await expect(contract.setTokenColor(tokenId, Color1))
+      .to.emit(contract, Events.MetadataUpdate.uint24)
+      .withArgs(tokenId, 'color', 0x123456, owner.address);
+  });
+
   it('should update the token logo and color', async () => {
     const { contract, tokenId } = fixture;
     await contract.setTokenLogoAndColor(tokenId, Logo1, Color1);
@@ -78,4 +139,14 @@ describe('FleekERC721.UpdateProperties', () => {
       TestConstants.ResultantImage['Logo1+Color1']
     );
   });
+
+  it('should emit metadata updated event for logo and color change', async () => {
+    const { contract, tokenId, owner } = fixture;
+
+    await expect(contract.setTokenLogoAndColor(tokenId, Logo1, Color1))
+      .to.emit(contract, Events.MetadataUpdate.string)
+      .withArgs(tokenId, 'logo', Logo1, owner.address)
+      .and.to.emit(contract, Events.MetadataUpdate.uint24)
+      .withArgs(tokenId, 'color', 0x123456, owner.address);
+  });
 });