Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add NFT metadata #38

Open
wants to merge 4 commits into
base: develop
Choose a base branch
from
Open

Add NFT metadata #38

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a0a13b3
Add ERC721 metadata
xavikh Dec 10, 2024
ee6e24c
Add BaseURISet event
xavikh Dec 10, 2024
b03321b
Check baseTokenURI
xavikh Dec 10, 2024
e9a8d16
Test supportsInterface
xavikh Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
49 changes: 47 additions & 2 deletions src/escrow/increasing/Lock.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,23 @@
pragma solidity ^0.8.17;

import {ILock} from "@escrow-interfaces/ILock.sol";
import {ERC721Upgradeable as ERC721} from "@openzeppelin/contracts-upgradeable/token/ERC721/ERC721Upgradeable.sol";
import {ERC721EnumerableUpgradeable as ERC721Enumerable} from "@openzeppelin/contracts-upgradeable/token/ERC721/extensions/ERC721EnumerableUpgradeable.sol";
import {ERC721URIStorageUpgradeable} from "@openzeppelin/contracts-upgradeable/token/ERC721/extensions/ERC721URIStorageUpgradeable.sol";
import {ReentrancyGuardUpgradeable as ReentrancyGuard} from "@openzeppelin/contracts-upgradeable/security/ReentrancyGuardUpgradeable.sol";
import {UUPSUpgradeable} from "@openzeppelin/contracts-upgradeable/proxy/utils/UUPSUpgradeable.sol";
import {DaoAuthorizableUpgradeable as DaoAuthorizable} from "@aragon/osx/core/plugin/dao-authorizable/DaoAuthorizableUpgradeable.sol";
import {IDAO} from "@aragon/osx/core/dao/IDAO.sol";

/// @title NFT representation of an escrow locking mechanism
contract Lock is ILock, ERC721Enumerable, UUPSUpgradeable, DaoAuthorizable, ReentrancyGuard {
contract Lock is
ILock,
ERC721Enumerable,
ERC721URIStorageUpgradeable,
UUPSUpgradeable,
DaoAuthorizable,
ReentrancyGuard
{
/// @dev enables transfers without whitelisting
address public constant WHITELIST_ANY_ADDRESS =
address(uint160(uint256(keccak256("WHITELIST_ANY_ADDRESS"))));
Expand All @@ -23,6 +32,8 @@ contract Lock is ILock, ERC721Enumerable, UUPSUpgradeable, DaoAuthorizable, Reen
/// @notice Whitelisted contracts that are allowed to transfer
mapping(address => bool) public whitelisted;

string public baseTokenURI;

/*//////////////////////////////////////////////////////////////
Modifiers
//////////////////////////////////////////////////////////////*/
Expand All @@ -38,7 +49,7 @@ contract Lock is ILock, ERC721Enumerable, UUPSUpgradeable, DaoAuthorizable, Reen

function supportsInterface(
bytes4 _interfaceId
) public view override(ERC721Enumerable) returns (bool) {
) public view override(ERC721Enumerable, ERC721URIStorageUpgradeable) returns (bool) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Strictly speaking, we should test that the supports interface check is correctly returning as expected for Enumerable vs URI Storage. Of the 2 I think Enumerable is more important to be able to introspect so would want to make sure that is not lost through the override

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. I added 2 asserts on test/escrow/escrow/Lock.t.sol to verify both interfaces are supported. I also checked and all the parent functions use || super.supportsInterface(..) so they should propagate correctly through the inheritance chain.

return super.supportsInterface(_interfaceId) || _interfaceId == type(ILock).interfaceId;
}

Expand Down Expand Up @@ -83,6 +94,16 @@ contract Lock is ILock, ERC721Enumerable, UUPSUpgradeable, DaoAuthorizable, Reen
emit WhitelistSet(WHITELIST_ANY_ADDRESS, true);
}

/// @notice Override the beforeTokenTransfer as required by ERC721Enumerable
function _beforeTokenTransfer(
address _from,
address _to,
uint256 _tokenId,
uint256 batchSize
) internal override(ERC721, ERC721Enumerable) {
super._beforeTokenTransfer(_from, _to, _tokenId, batchSize);
}

/// @dev Override the transfer to check if the recipient is whitelisted
/// This avoids needing to check for mint/burn but is less idomatic than beforeTokenTransfer
function _transfer(address _from, address _to, uint256 _tokenId) internal override {
Expand Down Expand Up @@ -110,6 +131,30 @@ contract Lock is ILock, ERC721Enumerable, UUPSUpgradeable, DaoAuthorizable, Reen
_burn(_tokenId);
}

function _burn(uint256 _tokenId) internal override(ERC721, ERC721URIStorageUpgradeable) {
super._burn(_tokenId);
}

function tokenURI(
uint256 _tokenId
) public view override(ERC721, ERC721URIStorageUpgradeable) returns (string memory) {
return super.tokenURI(_tokenId);
}

function _baseURI() internal view override returns (string memory) {
return baseTokenURI;
}

function setBaseURI(string memory _baseTokenURI) external auth(LOCK_ADMIN_ROLE) {
baseTokenURI = _baseTokenURI;

emit BaseURISet(baseTokenURI);
}

function setTokenURI(uint256 _tokenId, string memory _tokenURI) external auth(LOCK_ADMIN_ROLE) {
_setTokenURI(_tokenId, _tokenURI);
}

/*//////////////////////////////////////////////////////////////
UUPS Upgrade
//////////////////////////////////////////////////////////////*/
Expand Down
14 changes: 13 additions & 1 deletion src/escrow/increasing/interfaces/ILock.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,16 @@
// SPDX-License-Identifier: MIT
pragma solidity ^0.8.0;

/*///////////////////////////////////////////////////////////////
METADATA
//////////////////////////////////////////////////////////////*/

interface IMetadataEvents {
/// @notice Event emmited when the base metadata URI is updated
/// @param uri New base URI
event BaseURISet(string uri);
}

/*///////////////////////////////////////////////////////////////
WHITELIST
//////////////////////////////////////////////////////////////*/
Expand All @@ -21,7 +31,9 @@ interface IWhitelist is IWhitelistEvents, IWhitelistErrors {
function whitelisted(address addr) external view returns (bool);
}

interface ILock is IWhitelist {
interface IMetadata is IMetadataEvents {}

interface ILock is IWhitelist, IMetadata {
error OnlyEscrow();

/// @notice Address of the escrow contract that holds underyling assets
Expand Down
50 changes: 50 additions & 0 deletions test/escrow/escrow/Lock.t.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {EscrowBase} from "./EscrowBase.sol";
import {console2 as console} from "forge-std/console2.sol";
import {IDAO} from "@aragon/osx/core/dao/IDAO.sol";
import {DAO} from "@aragon/osx/core/dao/DAO.sol";
import {DaoUnauthorized} from "@aragon/osx/core/utils/auth.sol";
import {Multisig, MultisigSetup} from "@aragon/multisig/MultisigSetup.sol";

import {ProxyLib} from "@libs/ProxyLib.sol";
Expand All @@ -31,6 +32,7 @@ contract TestLockMintBurn is EscrowBase, IEscrowCurveTokenStorage, IGaugeVote {
assertEq(_nftLock.name(), _name);
assertEq(_nftLock.symbol(), _symbol);
assertEq(_nftLock.escrow(), _escrow);
assertEq(_nftLock.baseTokenURI(), "");
assertEq(address(_nftLock.dao()), _dao);
}

Expand Down Expand Up @@ -95,6 +97,54 @@ contract TestLockMintBurn is EscrowBase, IEscrowCurveTokenStorage, IGaugeVote {
vm.expectRevert("revert");
newLock.mint(address(reentrant), 1);
}

function testSetNFTMetadata() public {
vm.prank(address(escrow));
nftLock.mint(address(123), 1);

assertEq(nftLock.tokenURI(1), "");

vm.prank(address(this));
nftLock.setBaseURI("https://example.com/");
assertEq(nftLock.baseTokenURI(), "https://example.com/");
assertEq(nftLock.tokenURI(1), "https://example.com/1");

vm.prank(address(this));
nftLock.setTokenURI(1, "?tokenId=1");
assertEq(nftLock.tokenURI(1), "https://example.com/?tokenId=1");

vm.prank(address(escrow));
nftLock.mint(address(123), 2);

assertEq(nftLock.tokenURI(2), "https://example.com/2");
}

function testOnlyOwnerCanSetNFTMetadata(address _notEscrow) public {
vm.assume(_notEscrow != address(this));

bytes memory data = abi.encodeWithSelector(
DaoUnauthorized.selector,
address(dao),
address(nftLock),
address(_notEscrow),
nftLock.LOCK_ADMIN_ROLE()
);

vm.prank(address(escrow));
nftLock.mint(address(123), 1);

vm.prank(_notEscrow);
vm.expectRevert(data);
nftLock.setBaseURI("https://example.com/");

assertEq(nftLock.baseTokenURI(), "");
assertEq(nftLock.tokenURI(1), "");

vm.prank(_notEscrow);
vm.expectRevert(data);
nftLock.setTokenURI(1, "?tokenId=1");
assertEq(nftLock.tokenURI(1), "");
}
}

contract NFTReentrant {
Expand Down
Loading