[feat] Offers v1.0 #126
[feat] Offers v1.0 #126
Conversation
kulkarohan
added
Community Audit
tbtstl
added
Community Audit
There was a problem hiding this comment.
@kulkarohan - Thank you for inviting the community to a code review!
I read through contracts/test/modules/Offers/V1/Offers.t.sol and see a handful of tests that check the require() statements in the external functions.
However, I noticed that it doesn't currently test the required finders fee bps (basis points) in createOffer()` nor does it test the required update is valid, where the new amount is not equivalent to the previous.
If it's welcome by you and the team, I cut #132 to introduce these tests on this PR for "fuller" test coverage.
| emit ExchangeExecuted(offer.maker, msg.sender, userAExchangeDetails, userBExchangeDetails); | ||
| emit OfferFilled(_tokenContract, _tokenId, _offerId, msg.sender, _finder, offer); | ||
|
|
||
| delete offers[_tokenContract][_tokenId][_offerId]; |
There was a problem hiding this comment.
Asking to learn: from what I understand, this deletes the metadata for a given offer. However, there is no explicit call to interact with the offersForNft mapping afterwards. Is this design intended to maintain the history of offers (bids) for the NFT, even for those that have already been fulfilled? 💭
There was a problem hiding this comment.
haha originally we designed to delete offer IDs in addition to the metadata after fill/cancel, but ironically this ends up raising gas costs as we'd need to keep track of each offer ID's index in its associated offersForNFT array. so the tradeoff we settled for was keeping the history of every offer, as you mentioned, but having costs be relatively lower
There was a problem hiding this comment.
@kulkarohan - Thank you for that context around tradeoffs; I learned something new. 👍
I'll go ahead and resolve this comment from the PR conversation. ✌️
| // Validate offer and take custody | ||
| _handleIncomingTransfer(_amount, _currency); | ||
|
|
||
| offerCount++; |
There was a problem hiding this comment.
can make this unchecked, the sun will devour the earth before it could ever overflow
| // Else offer decrease -- | ||
| } else { | ||
| // Get delta | ||
| uint256 decreaseAmount = prevAmount - _amount; |
There was a problem hiding this comment.
can uncheck this, you ensure no underflow via the if statement
| // If offer increase -- | ||
| if (_amount > prevAmount) { | ||
| // Get delta | ||
| uint256 increaseAmount = _amount - prevAmount; |
There was a problem hiding this comment.
can uncheck this, you ensure no underflow via the if statement
| uint256 _tokenId, | ||
| uint256 _offerId | ||
| ) external nonReentrant { | ||
| Offer storage offer = offers[_tokenContract][_tokenId][_offerId]; |
There was a problem hiding this comment.
changing to read from memory, we're not overriding
| uint256 _amount, | ||
| address _finder | ||
| ) external nonReentrant { | ||
| Offer storage offer = offers[_tokenContract][_tokenId][_offerId]; |
There was a problem hiding this comment.
changing to read from memory, we're not overriding
test: add tests for required finders fee bps and ensuring a valid update on newly set offers
| _handleIncomingTransfer(_amount, _currency); | ||
|
|
||
| // "the sun will devour the earth before it could ever overflow" - @transmissions11 | ||
| unchecked { |
There was a problem hiding this comment.
I've run some snapshots with and without unchecked, and ... without actually gives out better gas-savings.
Although the increment part is cheaper with unchecked, the opcodes after become more expensive for some reason.
Can someone confirm? Tried with both forge and dapp snapshot.
There was a problem hiding this comment.
test_ERC20Integration() (gas: -698 (-0.003%))
test_DecreaseETHOfferWithERC20() (gas: -698 (-0.004%))
test_IncreaseETHOfferWithERC20() (gas: -698 (-0.004%))
test_DecreaseETHOffer() (gas: -698 (-0.004%))
test_IncreaseETHOffer() (gas: -698 (-0.004%))
testRevert_CannotUpdateOfferWithPreviousAmount() (gas: -698 (-0.004%))
test_ETHIntegration() (gas: -698 (-0.004%))
testRevert_CannotFillInactiveOffer() (gas: -698 (-0.004%))
testRevert_CannotCancelInactiveOffer() (gas: -698 (-0.004%))
testRevert_CannotUpdateInactiveOffer() (gas: -698 (-0.004%))
test_FillNFTOffer() (gas: -698 (-0.005%))
testRevert_AcceptCurrencyMustMatchOffer() (gas: -698 (-0.005%))
testRevert_AcceptAmountMustMatchOffer() (gas: -698 (-0.005%))
testRevert_OnlyTokenHolderCanFillOffer() (gas: -698 (-0.005%))
testRevert_CannotIncreaseOfferWithoutAttachingFunds() (gas: -698 (-0.005%))
testRevert_OnlySellerCanCancelOffer() (gas: -698 (-0.005%))
testRevert_OnlySellerCanUpdateOffer() (gas: -698 (-0.005%))
test_CreateOffer() (gas: -698 (-0.005%))
testGas_CreateOffer() (gas: -698 (-0.005%))
test_CancelNFTOffer() (gas: -698 (-0.007%))
testRevert_UserMustApproveModule() (gas: -2462 (-0.071%))
testFail_UserMustApproveTransferHelper() (gas: -3851 (-0.099%))
test_ERC20Integration() (gas: -59673 (-0.268%))
test_ETHIntegration() (gas: -59813 (-0.381%))
diff --git a/contracts/modules/Offers/V1/OffersV1.sol b/contracts/modules/Offers/V1/OffersV1.sol
index c3323dd..c3c36d9 100644
--- a/contracts/modules/Offers/V1/OffersV1.sol
+++ b/contracts/modules/Offers/V1/OffersV1.sol
@@ -162,9 +162,9 @@ contract OffersV1 is ReentrancyGuard, UniversalExchangeEventV1, IncomingTransfer
_handleIncomingTransfer(_amount, _currency);
// "the sun will devour the earth before it could ever overflow" - @transmissions11
- unchecked {
+ // unchecked {
offerCount++;
- }
+ // }
offers[_tokenContract][_tokenId][offerCount] = Offer({
maker: msg.sender,
There was a problem hiding this comment.
yeah this is super weird, do u have an asm diff we can look at?
There was a problem hiding this comment.
diff is too big because of labels so here are the files generated with foundry:
asm and iroptimized
checked.asm.txt
checked.iropt.txt
unchecked.asm.txt
unchecked.iropt.txt
There was a problem hiding this comment.
the createOffer avg gas rankings
unchecked offerCount++ — 127937
unchecked offerCount += 1 — 127933
unchecked ++offerCount — 127932
unchecked offerCount = offerCount + 1 — 127922
offerCount += 1 — 127308
offerCount++ — 127303
++offerCount — 127298
how sway
New Module Proposal
Description
This module outlines a way for any potential buyer to create offers for a valid ERC-721.
Motivation and Context
Following up on AsksV1_1, OffersV1 is meant to serve as the buy side of a market. Offers are taken into escrow and allow a potential seller to activate them.
How has this been tested?
Relevant tests have been included via Foundry.
Checklist: