-
Notifications
You must be signed in to change notification settings - Fork 39
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
Random Games #143
base: main
Are you sure you want to change the base?
Random Games #143
Conversation
uint64 timestamp, | ||
bytes calldata signature | ||
) external override payable virtual nonReentrant { | ||
_mintInternal(qty, msg.sender, 0, proof, timestamp, signature); |
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.
require(qty > 0, "Quantity cannot be zero")
I think it handles qty 0 gracefully and the results are benign depending on the proxy contract behavior. But the require could save some gas.
args: IDeployRandomGameParams, | ||
hre: HardhatRuntimeEnvironment, | ||
) => { | ||
console.log("123"); |
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 do this too. You're going to catch me console.log("roflcopter")
a bunch of times.
uint64 timestamp, | ||
bytes calldata signature | ||
) external override payable virtual nonReentrant { | ||
_mintInternal(qty, msg.sender, limit, proof, timestamp, signature); |
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 the same qty check here?
} | ||
|
||
function setProxyContract(address newProxyContract) external onlyOwner { | ||
_proxyContract = newProxyContract; |
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.
require(newProxyContract != address(0), "Proxy contract cannot be zero address");
If you want to be able to disable the proxy by setting it to the zero address then I would do the recommendation of adding the check in the mint.
fundReceiver | ||
) | ||
{ | ||
_proxyContract = proxyContract; |
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.
require(newProxyContract != address(0), "Proxy contract cannot be zero address");
You may want to deploy without setting an address, I see your setter function. Then I would add a similar check to the mint
and mintWithLimit
functions to ensure the _proxyContract
address has been set at the beginning of the funciton.
_mintInternal(qty, msg.sender, 0, proof, timestamp, signature); | ||
|
||
address[] memory minters = new address[](qty); | ||
for (uint256 i = 0; i < qty; i++) { |
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.
It is 20,000 gas to write to an array. So there is a theoretical limit here to how large qty could be before the loop will run out of gas. It will probably in the range of 1,000 - 1,500 on the high end.
Overall structure
Customize 721CM by adding a
call
to Random Games proxy contract at the end of minting