-
Notifications
You must be signed in to change notification settings - Fork 0
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.
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
just for comments #1
base: main
Are you sure you want to change the base?
Conversation
struct Contribution { | ||
string contributionURI; | ||
uint256 createdAt; | ||
address payable creator; | ||
uint256 price; | ||
} |
There was a problem hiding this comment.
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;
}
There was a problem hiding this comment.
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.
|
||
using Counters for Counters.Counter; | ||
Counters.Counter private _tokenIds; | ||
mapping (uint256 => uint256) public endsAt; // endsAt[tokenId] |
There was a problem hiding this comment.
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;
There was a problem hiding this comment.
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 (address => uint256[]) public contributedTokens; // contributedTokens[address] | ||
mapping (address => bool) public allowlist; // allowlist[address] | ||
|
||
/// ============ Immutable storage ============ |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 _mintFee, | ||
address _feeAddress | ||
) | ||
ReentrancyGuard() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
) { | ||
require(_memberLimit >= 2, "_memberLimit cannot be smaller than 2"); | ||
require(_totalSupply > 0, "_totalSupply cannot be 0"); | ||
require(_baseMintFee > 0, "_mintFee cannot be 0"); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏽
uint256 _sessionLimitPerAccount, | ||
uint256 _baseMintFee, | ||
uint256 _mintFee, | ||
address _feeAddress |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏽
_; | ||
} | ||
modifier isEnded(uint256 _tokenId) { | ||
require(endsAt[_tokenId] <= block.timestamp, |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Fixed these!
{ | ||
require(_tokenIds.current() < totalSupply, "reached token supply limit"); | ||
require(_members.length <= memberLimit, "reached member limit"); | ||
|
There was a problem hiding this comment.
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");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍🏽
isNotMinted(_tokenId) | ||
{ | ||
Contribution storage _contribution = contributed[_tokenId][msg.sender]; | ||
_contribution.price = _price; |
There was a problem hiding this comment.
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;
There was a problem hiding this comment.
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!
totalPrice += contributions[i].price; | ||
} | ||
if (totalPrice > 0) { | ||
finalMintFee = totalPrice * mintFee / 100; |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
if (totalPrice > 0) { | ||
finalMintFee = totalPrice * mintFee / 100; | ||
} | ||
require(msg.value == totalPrice + finalMintFee, "Payment must be equal to listing price + mint fee"); |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
feeAddress.transfer(finalMintFee); | ||
|
||
_safeMint(msg.sender, _tokenId); | ||
_setTokenURI(_tokenId, _tokenURI); |
There was a problem hiding this comment.
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...
/// @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; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
close this PR after useful. Just so that i can leave comments on per line.