From a86c30a8da7045e0a4b120dc9b4aadddb82839a9 Mon Sep 17 00:00:00 2001 From: Felipe Mendes Date: Fri, 17 Feb 2023 09:59:55 -0300 Subject: [PATCH] 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 | 1 - contracts/contracts/FleekERC721.sol | 27 +++---- .../test/foundry/FleekERC721/TokenURI.t.sol | 61 +++++++++++++++- .../contracts/FleekERC721/helpers/events.ts | 7 ++ .../contracts/FleekERC721/helpers/index.ts | 1 + .../FleekERC721/update-properties.t.ts | 73 ++++++++++++++++++- 6 files changed, 152 insertions(+), 18 deletions(-) create mode 100644 contracts/test/hardhat/contracts/FleekERC721/helpers/events.ts diff --git a/.github/workflows/subgraph.yml b/.github/workflows/subgraph.yml index 4111d4e..0cd9f85 100644 --- a/.github/workflows/subgraph.yml +++ b/.github/workflows/subgraph.yml @@ -4,7 +4,6 @@ on: pull_request: branches: - main - - develop push: paths: - 'subgraph/**' diff --git a/contracts/contracts/FleekERC721.sol b/contracts/contracts/FleekERC721.sol index 660a5ed..d84e58a 100644 --- a/contracts/contracts/FleekERC721.sol +++ b/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); } /** diff --git a/contracts/test/foundry/FleekERC721/TokenURI.t.sol b/contracts/test/foundry/FleekERC721/TokenURI.t.sol index 0750a9c..c19c6ce 100644 --- a/contracts/test/foundry/FleekERC721/TokenURI.t.sol +++ b/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); + } } diff --git a/contracts/test/hardhat/contracts/FleekERC721/helpers/events.ts b/contracts/test/hardhat/contracts/FleekERC721/helpers/events.ts new file mode 100644 index 0000000..3c7cdd7 --- /dev/null +++ b/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)', + }, +}); diff --git a/contracts/test/hardhat/contracts/FleekERC721/helpers/index.ts b/contracts/test/hardhat/contracts/FleekERC721/helpers/index.ts index 8a99789..27cb470 100644 --- a/contracts/test/hardhat/contracts/FleekERC721/helpers/index.ts +++ b/contracts/test/hardhat/contracts/FleekERC721/helpers/index.ts @@ -1,3 +1,4 @@ export * from './constants'; export * from './fixture'; export * from './utils'; +export * from './events'; diff --git a/contracts/test/hardhat/contracts/FleekERC721/update-properties.t.ts b/contracts/test/hardhat/contracts/FleekERC721/update-properties.t.ts index 470f2e1..3e21895 100644 --- a/contracts/test/hardhat/contracts/FleekERC721/update-properties.t.ts +++ b/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); + }); });