[feat] Collection Offers v1.0 #125
Conversation
kulkarohan
added
Draft / RFC
Community Audit
tbtstl
added
Community Audit
|
from my understanding. changed a few and got better gas results: compare from solidity docs: |
|
@joshieDo lfg, ty ser |
|
store changes: |
|
should have their gas limited on all instances. |
|
nice catches. we were just about to add the hard gas limit across all our modules lol |
| if (ceilingOfferAmount[_collection] >= _minAmount) { | ||
| return ceilingOfferId[_collection]; | ||
| // Else return no offer found | ||
| } else { |
There was a problem hiding this comment.
Inspired by ESLint's no-else-return rule and Palantir's TSLint unnecessary-else rule -- since the if block contains a return statement, the else block becomes unnecessary.
Suggested change:
function _getMatchingOffer(address _collection, uint256 _minAmount) internal view returns (uint32) {
// If current ceiling offer is greater than or equal to maker's minimum, return its id to fill
if (ceilingOfferAmount[_collection] >= _minAmount) {
return ceilingOfferId[_collection];
}
// Else return no offer found
return 0;
}Related: #141 (comment)
There was a problem hiding this comment.
@kulkarohan - Thank you for opening this for Community Audit!
I left one requested change (annotation) and one suggested change (logic). 🙇♂️
| require((beforeSellerBalance - afterSellerBalance) == 1 ether); | ||
| // 0.05 ETH creator royalty | ||
| require((afterRoyaltyRecipientBalance - beforeRoyaltyRecipientBalance) == 0.05 ether); | ||
| // 100 bps finders fee (Remaining 0.95 ETH * 10% finders fee = 0.0095 ETH) |
There was a problem hiding this comment.
A default finders fee is configured in CollectionOffersV1's constructor:
Since the maximum finders fee is 10000, a 100 bps finders fee is 1%, not 10%.
It follows that 0.95 ETH * 1% is 0.0095 ETH as it's expressed in the assertion.
Given this context, the following annotation should be updated accordingly for correctness:
| // 100 bps finders fee (Remaining 0.95 ETH * 10% finders fee = 0.0095 ETH) | |
| // 100 bps finders fee (Remaining 0.95 ETH * 1% finders fee = 0.0095 ETH) |
|
@kulkarohan - Hello! I noticed the integration test file doesn't test for the case when the protocol fee is non-zero. I'm wondering if you'd be open to me cutting a PR for introducing that test case, similar to the work in #140? 💭 |
|
@almndbtr please! |
|
@tbtstl - Thank you! I'll get to work on this today & tomorrow. 🙇♂️ |
[feat] golf from @joshieDo
…o collection-offers
…col-fee Collection Offers v1.0: add tests when the protocol fee is non-zero
| uint256 prevOffer = offers[_collection][_offerId].prevId; | ||
| return | ||
| ((_increase == true) && (_newAmount <= offers[_collection][nextOffer].amount)) || | ||
| ((_increase == false) && (_newAmount > offers[_collection][prevOffer].amount)); |
There was a problem hiding this comment.
return
((_increase == true) && (_newAmount <= offers[_collection][offers[_collection][_offerId].nextId].amount)) ||
((_increase == false) && (_newAmount > offers[_collection][offers[_collection][_offerId].prevId].amount));I ran the tests with this code above instead and there should be some gas savings if _increase is true, since we don't have to load prevOffer.
Before:
[PASS] test_DecreaseCeilingInPlace() (gas: 381622)
[PASS] test_DecreaseCeilingToFloor() (gas: 386137)
[PASS] test_DecreaseCeilingToMiddle() (gas: 389288)
[PASS] test_DecreaseFloor() (gas: 384806)
[PASS] test_DecreaseMiddleInPlace() (gas: 381311)
[PASS] test_DecreaseMiddleToFloor() (gas: 386035)
[PASS] test_DecreaseMiddleToMiddle() (gas: 386269)
[PASS] test_IncreaseCeiling() (gas: 384767)
[PASS] test_IncreaseFloorInPlace() (gas: 380793)
[PASS] test_IncreaseFloorToMiddle() (gas: 387346)
[PASS] test_IncreaseFloorToNewCeiling() (gas: 383857)
[PASS] test_IncreaseMiddleInPlace() (gas: 379259)
[PASS] test_IncreaseMiddleToCeiling() (gas: 382625)
[PASS] test_IncreaseMiddleToMiddle() (gas: 385422)
[PASS] test_UpdateFindersFee() (gas: 22932)
--------------------------------------------------------------------------
After:
[PASS] test_DecreaseCeilingInPlace() (gas: 381495)
[PASS] test_DecreaseCeilingToFloor() (gas: 386010)
[PASS] test_DecreaseCeilingToMiddle() (gas: 389161)
[PASS] test_DecreaseFloor() (gas: 384679)
[PASS] test_DecreaseMiddleInPlace() (gas: 381184)
[PASS] test_DecreaseMiddleToFloor() (gas: 385908)
[PASS] test_DecreaseMiddleToMiddle() (gas: 386142)
[PASS] test_IncreaseCeiling() (gas: 384650)
[PASS] test_IncreaseFloorInPlace() (gas: 380676)
[PASS] test_IncreaseFloorToMiddle() (gas: 387229)
[PASS] test_IncreaseFloorToNewCeiling() (gas: 383740)
[PASS] test_IncreaseMiddleInPlace() (gas: 379142)
[PASS] test_IncreaseMiddleToCeiling() (gas: 382508)
[PASS] test_IncreaseMiddleToMiddle() (gas: 385305)
[PASS] test_UpdateFindersFee() (gas: 22932)
There was a problem hiding this comment.
@0xkowloon good looks!! iirc at the time i went with nextOffer prevOffer for readability, but im down for the optimized version and providing an extra comment or two for clarification if needed.
thanks!
|
saw the notification from the previous comment and decided to take another quick look lol. Noticed that we're not making use of storage pointers where they should be used. I changed just in one function (below), but there are more places which could use the same logic: mapping(uint256 => Offer) storage _collectionOffers = offers[_collection];diff --git a/.gas-snapshot b/.gas-snapshot
index 73601f3..dea4159 100644
--- a/.gas-snapshot
+++ b/.gas-snapshot
@@ -46,31 +46,31 @@ CollectionOffersV1IntegrationTest:test_WithdrawOfferFromSeller() (gas: 184776)
CollectionOffersV1IntegrationTest:test_WithdrawOfferIncreaseFromSeller() (gas: 197261)
CollectionOffersV1Test:testGas_CreateFirstCollectionOffer() (gas: 174747)
CollectionOffersV1Test:testRevert_CancelOfferMustBeMaker() (gas: 187016)
-CollectionOffersV1Test:testRevert_FillMinimumTooHigh() (gas: 379478)
+CollectionOffersV1Test:testRevert_FillMinimumTooHigh() (gas: 378990)
CollectionOffersV1Test:testRevert_FindersFeeCannotExceed10000() (gas: 18357)
-CollectionOffersV1Test:testRevert_MustOwnCollectionToken() (gas: 376643)
+CollectionOffersV1Test:testRevert_MustOwnCollectionToken() (gas: 376155)
CollectionOffersV1Test:testRevert_UpdateFindersFeeMustBeRegistrar() (gas: 15703)
-CollectionOffersV1Test:testRevert_UpdateOfferMustBeMaker() (gas: 369824)
+CollectionOffersV1Test:testRevert_UpdateOfferMustBeMaker() (gas: 369336)
CollectionOffersV1Test:test_CancelCollectionOffer() (gas: 158330)
-CollectionOffersV1Test:test_CreateCeilingOffer() (gas: 251040)
+CollectionOffersV1Test:test_CreateCeilingOffer() (gas: 250963)
CollectionOffersV1Test:test_CreateCollectionOffer() (gas: 188619)
-CollectionOffersV1Test:test_CreateFloorOffer() (gas: 313780)
-CollectionOffersV1Test:test_CreateMiddleOffer() (gas: 368766)
-CollectionOffersV1Test:test_DecreaseCeilingInPlace() (gas: 381622)
-CollectionOffersV1Test:test_DecreaseCeilingToFloor() (gas: 386137)
-CollectionOffersV1Test:test_DecreaseCeilingToMiddle() (gas: 389288)
-CollectionOffersV1Test:test_DecreaseFloor() (gas: 384806)
-CollectionOffersV1Test:test_DecreaseMiddleInPlace() (gas: 381311)
-CollectionOffersV1Test:test_DecreaseMiddleToFloor() (gas: 386035)
-CollectionOffersV1Test:test_DecreaseMiddleToMiddle() (gas: 386269)
-CollectionOffersV1Test:test_FillCollectionOffer() (gas: 448540)
-CollectionOffersV1Test:test_IncreaseCeiling() (gas: 384767)
-CollectionOffersV1Test:test_IncreaseFloorInPlace() (gas: 380793)
-CollectionOffersV1Test:test_IncreaseFloorToMiddle() (gas: 387346)
-CollectionOffersV1Test:test_IncreaseFloorToNewCeiling() (gas: 383857)
-CollectionOffersV1Test:test_IncreaseMiddleInPlace() (gas: 379259)
-CollectionOffersV1Test:test_IncreaseMiddleToCeiling() (gas: 382625)
-CollectionOffersV1Test:test_IncreaseMiddleToMiddle() (gas: 385422)
+CollectionOffersV1Test:test_CreateFloorOffer() (gas: 313626)
+CollectionOffersV1Test:test_CreateMiddleOffer() (gas: 368278)
+CollectionOffersV1Test:test_DecreaseCeilingInPlace() (gas: 381134)
+CollectionOffersV1Test:test_DecreaseCeilingToFloor() (gas: 385649)
+CollectionOffersV1Test:test_DecreaseCeilingToMiddle() (gas: 388800)
+CollectionOffersV1Test:test_DecreaseFloor() (gas: 384318)
+CollectionOffersV1Test:test_DecreaseMiddleInPlace() (gas: 380823)
+CollectionOffersV1Test:test_DecreaseMiddleToFloor() (gas: 385547)
+CollectionOffersV1Test:test_DecreaseMiddleToMiddle() (gas: 385781)
+CollectionOffersV1Test:test_FillCollectionOffer() (gas: 448052)
+CollectionOffersV1Test:test_IncreaseCeiling() (gas: 384279)
+CollectionOffersV1Test:test_IncreaseFloorInPlace() (gas: 380305)
+CollectionOffersV1Test:test_IncreaseFloorToMiddle() (gas: 386858)
+CollectionOffersV1Test:test_IncreaseFloorToNewCeiling() (gas: 383369)
+CollectionOffersV1Test:test_IncreaseMiddleInPlace() (gas: 378771)
+CollectionOffersV1Test:test_IncreaseMiddleToCeiling() (gas: 382137)
+CollectionOffersV1Test:test_IncreaseMiddleToMiddle() (gas: 384934)
CollectionOffersV1Test:test_UpdateFindersFee() (gas: 22932)
OffersV1IntegrationTest:test_ERC20Integration() (gas: 313243)
OffersV1IntegrationTest:test_ETHIntegration() (gas: 237182)
diff --git a/contracts/modules/CollectionOffers/V1/CollectionOfferBookV1.sol b/contracts/modules/CollectionOffers/V1/CollectionOfferBookV1.sol
index 7285436..eb023ed 100644
--- a/contracts/modules/CollectionOffers/V1/CollectionOfferBookV1.sol
+++ b/contracts/modules/CollectionOffers/V1/CollectionOfferBookV1.sol
@@ -75,9 +75,10 @@ contract CollectionOfferBookV1 {
// Else if offer is greater than current ceiling, mark as new ceiling
} else if (_isNewCeiling(_collection, _amount)) {
uint256 prevCeilingId = ceilingOfferId[_collection];
+ mapping(uint256 => Offer) storage _collectionOffers = offers[_collection];
- offers[_collection][prevCeilingId].nextId = uint32(_offerCount);
- offers[_collection][_offerCount] = Offer({
+ _collectionOffers[prevCeilingId].nextId = uint32(_offerCount);
+ _collectionOffers[_offerCount] = Offer({
maker: _maker,
amount: _amount,
id: uint32(_offerCount),
@@ -91,9 +92,10 @@ contract CollectionOfferBookV1 {
// Else if offer is less than or equal to current floor, mark as new floor
} else if (_isNewFloor(_collection, _amount)) {
uint256 prevFloorId = floorOfferId[_collection];
+ mapping(uint256 => Offer) storage _collectionOffers = offers[_collection];
- offers[_collection][prevFloorId].prevId = uint32(_offerCount);
- offers[_collection][_offerCount] = Offer({
+ _collectionOffers[prevFloorId].prevId = uint32(_offerCount);
+ _collectionOffers[_offerCount] = Offer({
maker: _maker,
amount: _amount,
id: uint32(_offerCount),
@@ -106,16 +108,18 @@ contract CollectionOfferBookV1 {
// Else offer is between floor and ceiling --
} else {
+ mapping(uint256 => Offer) storage _collectionOffers = offers[_collection];
+
// Start at floor
- Offer memory offer = offers[_collection][floorOfferId[_collection]];
+ Offer memory offer = _collectionOffers[floorOfferId[_collection]];
// Traverse towards ceiling; stop when an offer greater than or equal to new offer is reached
while ((offer.amount < _amount) && (offer.nextId != 0)) {
- offer = offers[_collection][offer.nextId];
+ offer = _collectionOffers[offer.nextId];
}
// Insert new offer before (time priority)
- offers[_collection][_offerCount] = Offer({
+ _collectionOffers[_offerCount] = Offer({
maker: _maker,
amount: _amount,
id: uint32(_offerCount),
@@ -124,8 +128,8 @@ contract CollectionOfferBookV1 {
});
// Update neighboring pointers
- offers[_collection][offer.id].prevId = uint32(_offerCount);
- offers[_collection][offer.prevId].nextId = uint32(_offerCount);
+ _collectionOffers[offer.id].prevId = uint32(_offerCount);
+ _collectionOffers[offer.prevId].nextId = uint32(_offerCount);
}
return _offerCount; |
|
Hi, I guess I'm a little late to the party. But, I was reading through the CollectionOfferBookV1 contract and had a question: Why are you going through the hassle of storing a sorted offer book on-chain? Isn't that a waste of gas? It seems like the only thing you need to fulfill the CollectionOffer module's needs is a list of all the existing offers and a pointer to the current ceiling. All the other data is not really relevant to the contract. The floor and an ordered list of all offers for a collection could be computed off-chain and displayed there. For That way the CollectionOfferBookV1 contract should be way simpler and should also cost less gas (intuition, didn't test that) |
New Module Proposal
Description
This module outlines a way for a potential buyer to create offers for any valid ERC-721 token from an NFT collection.
Motivation and Context
Following up on OffersV1, CollectionOffersV1 is meant to serve as the buy side of NFT collections. Offers are taken into escrow, added to the associated collection's list, and allow a potential seller to exercise the highest one above a specified limit. Floor and Ceiling data for each collection is stored and updated onchain.
How has this been tested?
Relevant tests have been included via Foundry.
Checklist: