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

Jump to bottom

just for comments #1

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

just for comments #1

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
361 changes: 0 additions & 361 deletions contracts/Prompt.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,362 +1 @@
//SPDX-License-Identifier: MIT
pragma solidity 0.8.12;

import "@openzeppelin/contracts/utils/Counters.sol";
import "@openzeppelin/contracts/token/ERC721/ERC721.sol";
import "@openzeppelin/contracts/token/ERC721/extensions/ERC721URIStorage.sol";
import "@openzeppelin/contracts/security/ReentrancyGuard.sol";


/// @title Prompt
/// @author Burak Arıkan & Sam Hart
/// @notice Extends the ERC721 non-fungible token standard to enable time-bound verifiable collaborative authorship

contract Prompt is ERC721URIStorage, ReentrancyGuard {

/// ============ Events ============

event SessionCreated(uint256 tokenId, uint256 end, address[] members, string contributionURI, uint256 contributionPrice, address contributor, address reservedAddress);
event MemberAdded(uint256 tokenId, address account);
event Contributed(uint256 tokenId, string contributionURI, address creator, uint256 price);
event PriceSet(uint256 tokenId, address contributor, uint256 price);
event Minted(uint256 tokenId, string tokenURI, address creator);

/// ============ Structs ============

struct Contribution {
string contributionURI;
uint256 createdAt;
address payable creator;
uint256 price;
}
Comment on lines -26 to -31
Copy link
Collaborator Author

@okwme okwme May 22, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I heard once that declaring simplest variables first helps with gas costs. so this would be:

    struct Contribution {
        uint256 createdAt;
        uint256 price;
        address payable creator;
        string contributionURI;
    }

Copy link
Member

Choose a reason for hiding this comment

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

Thank you Billy! Yeah, I heard that too. I did the change, let's see how much it changes the costs.


/// ============ Mutable storage ============

using Counters for Counters.Counter;
Counters.Counter private _tokenIds;
mapping (uint256 => uint256) public endsAt; // endsAt[tokenId]
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

why not do these as mappings of structs?
you have a bunch of redundant mapping keys which is extra storage costs for each one.
maybe not worth it if there are a lot of addresses that won't have most of the info or a lot of tokenIds that won't have most of the info?

struct TokenInfo {
    uint256 endsAt;
    address reservedFor;
    address[] members;
    uint256 memberCount;
    uint256 contributionCount;
    mapping (address => Contribution) contributed;
}

mapping (uint256 => TokenInfo) public tokenInfo;

struct MemberInfo {
    uint256[] createdSessions;
    uint256[] contributedTokens;
    bool allowlist
}

mapping (address => MemberInfo) public memberInfo;

Copy link
Member

Choose a reason for hiding this comment

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

I used redundant mappings over structs to save from look up time / cost in specific checks we need to do.

We can do more with structs in future iterations I think, it would be costly to rewrite in structs now (has effects from tests to the client app).

mapping (uint256 => address) public reservedFor; // reservedFor[tokenId]
mapping (uint256 => bool) public minted; // minted[tokenId]
mapping (uint256 => address[]) public members; // members[tokenId]
mapping (address => uint256[]) public createdSessions; // createdSessions[address]
mapping (uint256 => mapping (address => bool)) public membership; // membership[tokenId][address]
mapping (uint256 => uint256) public memberCount; // memberCount[tokenId]
mapping (uint256 => uint256) public contributionCount; // contributionCount[tokenId]
mapping (uint256 => mapping (address => Contribution)) public contributed; // contributed[tokenId][address]
mapping (address => uint256[]) public contributedTokens; // contributedTokens[address]
mapping (address => bool) public allowlist; // allowlist[address]

/// ============ Immutable storage ============
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

immutable storage can be declared as constants to save gas

Copy link
Member

Choose a reason for hiding this comment

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

Thank you again! I made them immutable to be read-only, but assignable in the constructor. It saves a bit.


uint256 public memberLimit;
uint256 public totalSupply;
uint256 public sessionLimitPerAccount;
uint256 public baseMintFee;
uint256 public mintFee;
address payable feeAddress;

/// ============ Constructor ============

/// @notice Creates a new Prompt NFT contract
/// @param tokenName name of NFT
/// @param tokenSymbol symbol of NFT
/// @param _memberLimit member limit of each NFT
/// @param _totalSupply total NFTs to mint
/// @param _sessionLimitPerAccount max number of NFTs a member can create
/// @param _baseMintFee in wei per NFT
/// @param _mintFee in percentage per NFT
/// @param _feeAddress where mint fees are paid
constructor(
string memory tokenName,
string memory tokenSymbol,
uint256 _memberLimit,
uint256 _totalSupply,
uint256 _sessionLimitPerAccount,
uint256 _baseMintFee,
uint256 _mintFee,
address _feeAddress
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

why not just make address payable here?

address payable _feeAddress

Copy link
Member

Choose a reason for hiding this comment

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

👍🏽

)
ReentrancyGuard()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

idk if constructor functions can have reentrancy. probably decrease deploy costs if you remove this.

Copy link
Member

Choose a reason for hiding this comment

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

This is an openzeppelin lib initiated in the constructor, but I checked again and removed it, because we don't really have an issue in the mint function as it follows the checks, effects, interactions pattern.

ERC721(
tokenName,
tokenSymbol
) {
require(_memberLimit >= 2, "_memberLimit cannot be smaller than 2");
require(_totalSupply > 0, "_totalSupply cannot be 0");
require(_baseMintFee > 0, "_mintFee cannot be 0");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

error message should be _baseMintFee cannot be 0

Copy link
Member

Choose a reason for hiding this comment

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

👍🏽

require(_mintFee > 0, "_mintFee cannot be 0");
require(_feeAddress != address(0), "feeAddress cannot be null address");

memberLimit = _memberLimit;
totalSupply = _totalSupply;
sessionLimitPerAccount = _sessionLimitPerAccount;
baseMintFee = _baseMintFee;
mintFee = _mintFee;
feeAddress = payable(_feeAddress);
allowlist[msg.sender] = true;
}

/// ============ Modifiers ============

modifier isAllowed() {
require (allowlist[msg.sender],
'account is not in allowlist');
_;
}
modifier onlyMemberOf(uint256 _tokenId) {
if (!membership[_tokenId][msg.sender]) {
revert('not a session member');
}
_;
}
modifier canCreateSession() {
require (createdSessions[msg.sender].length < sessionLimitPerAccount,
'account reached session limit');
_;
}
modifier isNotEnded(uint256 _tokenId) {
require(endsAt[_tokenId] >= block.timestamp,
'session has ended');
_;
}
modifier isEnded(uint256 _tokenId) {
require(endsAt[_tokenId] <= block.timestamp,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

might not want it to be possible for a token to be ended and not ended at the same time.
i'd suggest just removing the = from both of them

Copy link
Member

Choose a reason for hiding this comment

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

Good catch! Fixed these!

'session has not ended yet');
_;
}
modifier isNotEmpty(string memory _content) {
require(bytes(_content).length != 0,
'URI cannot be empty');
_;
}
modifier memberNotContributed(uint256 _tokenId) {
require (contributed[_tokenId][msg.sender].creator == address(0),
'address already contributed');
_;
}
modifier memberContributed(uint256 _tokenId) {
require (contributed[_tokenId][msg.sender].creator == msg.sender,
'address is not the creator of this contribution');
_;
}
modifier isLastContribution(uint _tokenId) {
require(contributionCount[_tokenId] == memberLimit - 1,
'is not the last contribution');
_;
}
modifier isFinalized(uint _tokenId) {
require(contributionCount[_tokenId] == memberLimit || (endsAt[_tokenId] != 0 && endsAt[_tokenId] <= block.timestamp),
'not all members contributed or session has not ended yet');
_;
}
modifier isNotMinted(uint _tokenId) {
require(!minted[_tokenId],
'session already minted');
_;
}

/// ============ Functions ============

/// @notice Create a session with tokenID. A session becomes mintable when it is finalized (all members contributed or endsAt is exceeded)
/// @param _reservedAddress If set (optional), only this address can mint. Can be used for commissioned work.
/// @param _endsAt All contributions must be submited before this time
/// @param _members List of addresses who can contribute
/// @param _contributionURI The first contribution to the session
/// @param _contributionPrice The first contribution price
function createSession(
address _reservedAddress,
uint256 _endsAt,
address[] memory _members,
string memory _contributionURI,
uint256 _contributionPrice
)
external
isNotEmpty(_contributionURI)
isAllowed()
canCreateSession()
{
require(_tokenIds.current() < totalSupply, "reached token supply limit");
require(_members.length <= memberLimit, "reached member limit");

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

maybe make sure _endsAt isn't already in the past.

require(_endsAt > block.timestamp, "quit living in the past");

Copy link
Member

Choose a reason for hiding this comment

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

👍🏽

uint256 newTokenId = _tokenIds.current();

for (uint256 i=0; i < _members.length; i++) {
require(_members[i] != address(0), 'address cannot be null address');
require(!membership[newTokenId][_members[i]], 'address is already a member of session');
membership[newTokenId][_members[i]] = true;
memberCount[newTokenId]++;
members[newTokenId].push(_members[i]);
allowlist[_members[i]] = true;
}

endsAt[newTokenId] = _endsAt;

if (_reservedAddress != address(0)) {
reservedFor[newTokenId] = _reservedAddress;
}

createdSessions[msg.sender].push(newTokenId);

contributed[newTokenId][msg.sender] = Contribution(_contributionURI, block.timestamp, payable(msg.sender), _contributionPrice);
contributedTokens[msg.sender].push(newTokenId);
contributionCount[newTokenId]++;

_tokenIds.increment();

emit SessionCreated(newTokenId, _endsAt, _members, _contributionURI, _contributionPrice, msg.sender, _reservedAddress);
}

/// @notice msg.sender contributes to a session with tokenId, contribution URI and price
/// @param _tokenId The session to contribute
/// @param _contributionURI Contribution content
/// @param _contributionPrice Contribution price
function contribute(
uint256 _tokenId,
string memory _contributionURI,
uint256 _contributionPrice
)
external
isNotEnded(_tokenId)
onlyMemberOf(_tokenId)
memberNotContributed(_tokenId)
{
contributed[_tokenId][msg.sender] = Contribution(_contributionURI, block.timestamp, payable(msg.sender), _contributionPrice);
contributedTokens[msg.sender].push(_tokenId);
contributionCount[_tokenId]++;

emit Contributed(_tokenId, _contributionURI, msg.sender, _contributionPrice);
}

/// @notice Set price of the msg.sender's contribution to a session, if not yet minted
/// @param _tokenId The session of contribution
/// @param _price New contribution price
function setPrice(uint256 _tokenId, uint256 _price)
external
memberContributed(_tokenId)
isNotMinted(_tokenId)
{
Contribution storage _contribution = contributed[_tokenId][msg.sender];
_contribution.price = _price;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

did you confirm that declaring it as storage means that updating it actually stores the new price?
i always second guess this and would usually overwrite like contribute[_tokenId][msg.sender] = _contribution;

Copy link
Member

Choose a reason for hiding this comment

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

Yes it actually stores the new price, so this is ok!


emit PriceSet(_tokenId, msg.sender, _contribution.price);
}

/// @notice Anyone can mint paying the total
/// @param _tokenId The session to mint
/// @param _tokenURI Content of the finalized session
function mint(uint256 _tokenId, string memory _tokenURI)
external
payable
nonReentrant()
isFinalized(_tokenId)
isNotEmpty(_tokenURI)
{
if (reservedFor[_tokenId] != address(0)) {
require(reservedFor[_tokenId] == msg.sender, "Mint is reserved for another address");
}

uint256 finalMintFee = baseMintFee;
uint256 totalPrice = 0;

Contribution[] memory contributions = getContributions(_tokenId);

for (uint256 i=0; i < contributions.length; i++) {
totalPrice += contributions[i].price;
}
if (totalPrice > 0) {
finalMintFee = totalPrice * mintFee / 100;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

you should be clear about declaring mintFee as a having two decimals (if that's the purpose of the / 100).
Otherwise the mintfee seems a little misleading as a term

Copy link
Member

Choose a reason for hiding this comment

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

Right, that's a percentage value, I changed the variable name to mintFeeRate to be more clear. We use it as e.g., 5 instead of 0.05.

}
require(msg.value == totalPrice + finalMintFee, "Payment must be equal to listing price + mint fee");
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

why are you adding the totalPrice back to the finalMintFee, when the finalMintfee is already a product of the totalPrice?
Seems like a really confusing pricing mechanism

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ok i see so the contributors each get their listed price, that total is multiplied by some mintfee and after dividing by 100 that is what's left over for the feeAddress. why divide by 100? so it's 1% of the combination of the artist's fees?

Copy link
Member

Choose a reason for hiding this comment

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

Right, that's the dividend value of percentage value.


for (uint256 i=0; i < contributions.length; i++) {
if (contributions[i].price > 0) {
contributions[i].creator.transfer(contributions[i].price);
}
}

minted[_tokenId] = true;

feeAddress.transfer(finalMintFee);

_safeMint(msg.sender, _tokenId);
_setTokenURI(_tokenId, _tokenURI);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

seems weird that the buyer can set the tokenURI...


emit Minted(_tokenId, _tokenURI, msg.sender);
}

/// ============ Read-only functions ============

/// @notice Get current count of minted tokens
/// @return Returns number
function tokenCount() external view virtual returns (uint256) {
return _tokenIds.current();
}

/// @notice Check if an address is member of a session
/// @return Returns true or false
function isMember(uint256 _tokenId, address _account) external view virtual returns (bool) {
return membership[_tokenId][_account];
}

/// @notice Check if all session members contributed
/// @return Returns true or false
function isCompleted(uint256 _tokenId) external view virtual returns (bool) {
return contributionCount[_tokenId] == memberLimit;
}

/// @notice Check if account can create a new session
/// @return Returns true or false
function accountCanCreateSession(address _account) external view virtual returns (bool) {
return createdSessions[_account].length < sessionLimitPerAccount;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

why limit the total number of sessions an account can ever create?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

are these supposed to be garbage collected?

Copy link
Member

Choose a reason for hiding this comment

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

That's by (economic) design, each creator can create only one session, and can contribute to any number of sessions as long as someone invites them.

}

/// @notice Get sessions initiated by an account
/// @return Returns tokenIds
function sessionCountByAccount(address _account) external view virtual returns (uint256[] memory) {
return createdSessions[_account];
}

/// @notice Get tokens contributed by an account
/// @return Returns tokenIds
function getContributedTokens(address _account) external view virtual returns (uint256[] memory) {
return contributedTokens[_account];
}

/// @notice Get contributions of a token
/// @return Returns contributions
function getContributions(uint256 _tokenId) internal view returns (Contribution[] memory) {
Contribution[] memory contributions_arr = new Contribution[](members[_tokenId].length);
for (uint256 i=0; i < members[_tokenId].length; i++) {
contributions_arr[i] = (contributed[_tokenId][members[_tokenId][i]]);
}
return contributions_arr;
}

/// @notice Get session data
/// @return Returns (owner: address, endsAt: blocktime, tokenURI: string, members: address[], contributions: Contribution[], reserved: address)
function getSession(uint256 _tokenId) external view virtual
returns (
address,
uint256,
string memory,
address[] memory,
Contribution[] memory,
address
)
{
string memory tokenuri = "";
address sessionOwner = address(0);
if (minted[_tokenId]) {
tokenuri = tokenURI(_tokenId);
sessionOwner = ownerOf(_tokenId);
}
return(
sessionOwner,
endsAt[_tokenId],
tokenuri,
members[_tokenId],
getContributions(_tokenId),
reservedFor[_tokenId]
);
}
}