Add MinterSetPriceTieredOnChainAllowV0#1987
Conversation
lindsgil
changed the title
|
No dependency changes detected. Learn more about Socket for GitHub. 👍 No dependency changes detected in pull request |
|
It looks like yarn 4.15.0 was released a few days ago causing the ci pipelines to fail, will update this in a separate PR https://github.com/yarnpkg/berry/releases |
ryley-o
left a comment
ryley-o
left a comment
There was a problem hiding this comment.
everything I see is pretty minor nits - nothing critical, nice work here!
would like to talk through why we decided allow purchase multiple in this one. I think it might also might be worth calling out a bit more specifically in the contract natspec about how this allowlist is likely used with more of a privileged relay-type system to integrate off-chain payment options, and it is not fully featured to support many allowlist scenarios due to lack of per-allowlisted-wallet invocation limits.
| function getAllowlistPriceInfo( | ||
| uint256 projectId, | ||
| address coreContract | ||
| ) external view returns (uint256 allowlistPricePerTokenInWei) { |
There was a problem hiding this comment.
minor nit: might be also nice to mirror the return type of getPriceInfo here, including isConfigured and currency symbol, address - just thinking about how to reduce multiple calls later on.
| uint256 projectId, | ||
| address coreContract, | ||
| address wallet | ||
| ) external view returns (bool isConfigured, uint256 tokenPriceInWei) { |
There was a problem hiding this comment.
same minor nit about probably worth including currency symbol and address here to mirror getPriceInfo return types
| for (uint256 i; i < quantity; ) { | ||
| SplitFundsLib.splitRevenuesETHNoRefund({ | ||
| projectId: projectId, | ||
| valueInWei: pricePerTokenInWei, | ||
| coreContract: coreContract | ||
| }); | ||
| unchecked { | ||
| ++i; | ||
| } | ||
| } |
There was a problem hiding this comment.
gas efficiency nit - can we just do this in one single split for gas efficiency? might even be cleanest to use the battle tested splitFundsETHRefundSender() function with a price per token set to pricePerTokenInWei * quantity or something. then we could remove the one-off refund logic in this minter.
| // Diamond storage for allowlist price configuration (minter-specific) | ||
| bytes32 constant ALLOWLIST_PRICE_STORAGE_POSITION = | ||
| keccak256("mintersetpricetieredonchainallowv0.allowlistprice.storage"); | ||
|
|
||
| struct AllowlistPriceStorage { | ||
| mapping(address coreContract => mapping(uint256 projectId => uint256 allowlistPricePerToken)) prices; | ||
| } |
There was a problem hiding this comment.
Using addressed storage is fine here, but since the definition is in the concrete minter implementation instead of a library (where , I don't think there is a benefit vs. just using a typical contract mapping definition.
| event AllowlistPricePerTokenUpdated( | ||
| uint256 indexed projectId, | ||
| address indexed coreContract, | ||
| uint256 indexed allowlistPricePerToken |
There was a problem hiding this comment.
nit: typically the value here wouldn't be an indexed parameter
| IMinterFilterV1 private immutable _minterFilter; | ||
|
|
||
| /// minterType for this minter | ||
| string public constant minterType = "MinterSetPriceTieredOnChainAllowV0"; |
There was a problem hiding this comment.
I don't know if we ever cast these as bytes32 items, but we have kept these to <31 characters just in case in the past. I think the minterfilter might do something with that, so we might want to pick something shorter if we make this a globally approved minter...
3 participants
Description of the change
Add MinterSetPriceTieredOnChainAllowV0, fork of MinterSetPriceOnChainAllowV0 that adds separate allowlist and public pricing, as well as the ability to batch mint