[Feature] Batch Offers #4
Conversation
ensures currency is approved and that offer is not expired
naming, tools, starts test
successfully adds an offer, but reverts
much easier to prank than create/receive as separate tests
make sure memory is cleaned up before transfers + uses currency from memory in offers rather than an input variable
creates a new storage mapping of creator -> root hash -> offer to prevent DoS style attacks from using the same merkle root to block offers
There was a problem hiding this comment.
Overall looks solid. There's also this old PR which has some extra functionality https://github.com/superrare/private-core/pull/1
| address _networkBeneficiary, | ||
| address _marketplaceSettings, | ||
| address _spaceOperatorRegistry, | ||
| address _royaltyEngine, | ||
| address _payments, | ||
| address _approvedTokenRegistry, | ||
| address _stakingSettings, | ||
| address _stakingRegistry |
There was a problem hiding this comment.
might want to check these for empty addresses
There was a problem hiding this comment.
some of the requires need to be cleaned up (extra quotations/messages/etc)
There was a problem hiding this comment.
I noticed too, and fixed. The linter went haywire or copilot added syntax when I didn't ask it to....
| marketConfig.checkIfCurrencyIsApproved(_currency); | ||
| require(_expiry > block.timestamp, "createBatchOffer::expiry must be in the future"); | ||
| require(_amount > 0, "offer::Amount cannot be 0"); | ||
|
|
| ) external payable nonReentrant { | ||
| require(_creatorToRootToOffer[msg.sender][_rootHash].creator == address(0), "createBatchOffer::offer exists"); | ||
| marketConfig.checkIfCurrencyIsApproved(_currency); | ||
| require(_expiry > block.timestamp, "createBatchOffer::expiry must be in the future"); |
There was a problem hiding this comment.
i would convert requires to custom errors to save gas
There was a problem hiding this comment.
we talked internally and won't do this conversion at this time
| require(_creatorToRootToOffer[msg.sender][_rootHash].creator == msg.sender, "createBatchOffer::must be owner"); | ||
|
|
||
| // Load Offer | ||
| BatchOffer memory offer = _creatorToRootToOffer[msg.sender][_rootHash]; |
There was a problem hiding this comment.
you can put this above the require to avoid the extra MLOAD
There was a problem hiding this comment.
good call: 11485ec0ce19be8a3031173648bd2a22a25e56f1
src/batchoffer/BatchOffer.sol
Outdated
| marketConfig.checkIfCurrencyIsApproved(currency); | ||
| require(offer.creator != address(0), "acceptBatchOffer::offer does not exist"); | ||
| require(offer.expiry > block.timestamp, "acceptBatchOffer::offer expired"); | ||
| require(offer.rootHash == _rootHash, "acceptBatchOffer::root mismatch"); |
There was a problem hiding this comment.
seems like this is redundant since you have it as the mapping
|
|
||
| // Transfer ERC721 token from the seller to the buyer | ||
| IERC721 erc721Token = IERC721(_contractAddress); | ||
| erc721Token.safeTransferFrom(msg.sender, offer.creator, _tokenId); |
There was a problem hiding this comment.
might want to check if token is now owned by sender
included in mapping and will not find offer if 0
|
@koloz193 super nice to see you still active here, miss ya |
Adds the first iteration for batch offer via Merkle