[feat] Reserve Auction v1.0 by kulkarohan · Pull Request #122 · ourzora/v3

kulkarohan · 2022-01-25T22:16:03Z

New Module Proposal

Description

This module outlines a way for any ERC-721 token holder to create a permissionless reserve auction.

Motivation and Context

ReserveAuctionV1 is an update to ZORA V2 Auction House. NFTs are taken into escrow after the reserve price is met, as well as the current highest bid. After an auction has ended, anyone can settle to payout respective parties and transfer the NFT to the winning bidder.

How has this been tested?

Relevant tests have been included via Foundry.

Checklist:

The module includes tests written for Foundry
The module is documented with NATSPEC
The documentation includes UML Diagrams for external and public functions
The module is a Hyperstructure

almndbtr

@ourzora - Thanks again for opening this to community review. ✌🏽

almndbtr · 2022-02-10T05:37:40Z

contracts/modules/ReserveAuction/V1/ReserveAuctionV1.sol

+            // Mark as first bid
+            firstBid = true;
+            // Transfer NFT into escrow
+            erc721TransferHelper.transferFrom(_tokenContract, auction.seller, address(this), _tokenId);


I propose that this is changed to safely transfer the ERC-721 into escrow:

Suggested change

erc721TransferHelper.transferFrom(_tokenContract, auction.seller, address(this), _tokenId);

erc721TransferHelper.safeTransferFrom(_tokenContract, auction.seller, address(this), _tokenId);

Context

Given the docs for transferFrom:

"Transfers the ownership of a given token ID to another address. Usage of this method is discouraged, use safeTransferFrom whenever possible. Requires the msg.sender to be the owner, approved, or operator."

Because the smart contract takes the ERC-721 in escrow, there might also need to be some additional work for implementing IERC721Receiver.onERC721Received, which is called upon a safe transfer as noted in these docs.

Stepping back, there could be a reason that it's designed this way, and I'm keeping an open mind. I'd love to get feedback from the team: is there a specific reason/rationale for taking this approach? If there is, noting that in the annotations would improve what's already here. 💭

good question - this is by design. safeTransferFrom is protection on the user side to prevent sending NFTs to contracts that may not be able to handle them. in our case, since we know we're the recipient and can handle them, we can use the simpler transferFrom and save gas

Ah, that context helps! Thank you @kulkarohan.

I was mulling over proposing a follow up change including an annotation there, but I think your input is enough for someone to look at this in the future and figure out why we use the simpler function. Considering this resolved 👍

Suggested change

erc721TransferHelper.transferFrom(_tokenContract, auction.seller, address(this), _tokenId);

// `transferFrom` is enough to move the NFT into escrow since the contract is the recipient

erc721TransferHelper.transferFrom(_tokenContract, auction.seller, address(this), _tokenId);

almndbtr · 2022-02-10T08:14:31Z

contracts/test/modules/ReserveAuction/V1/ReserveAuction.integration.t.sol

+        auctions.settleAuction(address(token), 0);
+    }
+
+    function test_ETHIntegration() public {


The assertions in this function take into account the royalties paid out to the creator and the finder respectively. However, there doesn’t seem to be an assertion that checks the balances if a protocol fee is set at the time the auction is settled:

v3/contracts/modules/ReserveAuction/V1/ReserveAuctionV1.sol

Line 494 in a5b3c77

remainingProfit = _handleProtocolFeePayout(remainingProfit, auction.currency);

Even if no protocol fee were actually set in these test cases (zero), would the team be open to adding one more require to show that is truly the case, or include a separate set of tests that illustrate what happens when an auction is settled with a non-zero protocol fee? 💭

I see your 👍 @kulkarohan -- I'll take some time this week and cut a PR that introduces a test for the protocol fee. 🔪

(If there's a known, hard deadline, do let me know and I can prioritize accordingly.)

@kulkarohan - Hi again! 👋

I opened #140 introducing tests for when the protocol fee is set to 1 basis point (1 bps or 0.01%). I'd love your evaluative review on it so we have complete test coverage for this module. Thanks in advance!

This reverts commit 24d8d9c.

almndbtr · 2022-02-11T02:41:08Z

👋 @kulkarohan -- I see some updates being pushed 👀

Is it best to hold off from further review until those settle? 💭

kulkarohan · 2022-02-11T04:33:59Z

hey @almndbtr sorry about that haha we found a few small tweaks earlier that we wanted to make. feel free to further review!

tbtstl · 2022-02-11T16:14:40Z

contracts/modules/ReserveAuction/V1/ReserveAuctionV1.sol

+/// @notice This contract allows users to list and bid on ERC-721 tokens with timed reserve auctions
+contract ReserveAuctionV1 is ReentrancyGuard, UniversalExchangeEventV1, IncomingTransferSupportV1, FeePayoutSupportV1, ModuleNamingSupportV1 {
+    /// @dev The indicator to pass all remaining gas when paying out royalties
+    uint256 private constant USE_ALL_GAS_FLAG = 0;


unused var here

I cut a PR to replace this with a new constant for the outgoing transfer gas limit: #137

Reserve Auction v1.0: add tests when the protocol fee is non-zero

kulkarohan · 2022-03-09T22:05:27Z

hey @reefloretto ! we're just restructuring a couple things that we'll push today. planning to deploy Friday!

almndbtr · 2022-03-11T19:23:28Z

planning to deploy Friday!

🙏 Hoping the deploy goes well! Godspeed, Zora team 🚀

iainnash · 2022-03-11T20:57:15Z

contracts/modules/ReserveAuction/V1/ReserveAuctionV1.sol

+        address sellerFundsRecipient;
+        address bidder;
+        address finder;
+        uint16 findersFeeBps;


One way to save 2 slots would be to limit the size of a few fields to a more reasonable number.

Duration can easily be uint32 (probably not a great idea to have an auction beyond 100 years etc).

uint32+uint16+address is still less than uint256 (one storage slot)

firstBidTime and startTime can both be uint126 to save one slot as well

address uint16 finderFeeBps uint32 duration uint128 firstBidTime uint128 startTime

saves two slots over the current configuration.

@iainnash hahaha i have these changes in the update. pushing soon!

…reserve-auction

kulkarohan · 2022-03-13T03:00:50Z

@everyone

I've restructured Reserve Auction into four gas-optimized variants:

Core ETH
Core ERC-20
Finders ETH
Finders ERC-20

'Core' are minimal reserve auctions and 'Finders' add support for finders fee.

An important note: while ETH variants are ETH-only, the ERC-20 variants are still compatible with ETH via setting address(0) as the currency.

So if you'd like the simplicity of interacting with a sole contract, the Finders ERC-20 variant will support all four use-cases (ETH, ERC20s, no finders fee, finders fee) as initially designed. But if you'd like to optimize per use-case, you'll now be able to do so as well.

Lemme know if there are any Qs -- thanks for your patience.

[feat] Reserve Auction v1.0

027e449

kulkarohan added Draft / RFC Community Audit Awaiting Community Audits and removed Draft / RFC Community Audit Awaiting Community Audits labels Jan 25, 2022

tbtstl force-pushed the main branch from 23f1426 to ef8807f Compare January 28, 2022 16:08

Rohan Kulkarni added 4 commits February 6, 2022 02:34

[feat] update for community review

b086795

[feat] golf

cd0600b

[feat] uml

d8c3680

Merge branch 'main' into reserve-auction

a5b3c77

tbtstl marked this pull request as ready for review February 9, 2022 21:00

tbtstl added Community Audit Awaiting Community Audits ZORA Bug Bounty Bug Bounty Rewards Available and removed Draft / RFC labels Feb 9, 2022

almndbtr reviewed Feb 10, 2022

View reviewed changes

jgeary and others added 4 commits February 10, 2022 13:43

fix outgoing transfer gas issue

24d8d9c

Revert "fix outgoing transfer gas issue"

f615519

This reverts commit 24d8d9c.

[fix] temp gas limit for payouts

5511daf

[feat] merge events auction bid & auction extended

e7f9fdc

tbtstl reviewed Feb 11, 2022

View reviewed changes

almndbtr mentioned this pull request Feb 12, 2022

Fix: replace USE_ALL_GAS_FLAG constant with OUTGOING_TRANSFER_GAS_LIMIT #137

Closed

4 tasks

almndbtr added 2 commits February 18, 2022 12:49

test: add assertions when protocol fee is non-zero

04e9c66

refactor: set test protocol fee to 1bps

dfba6b3

almndbtr mentioned this pull request Feb 18, 2022

Reserve Auction v1.0: add tests when the protocol fee is non-zero #140

Merged

4 tasks

Rohan Kulkarni and others added 2 commits February 21, 2022 20:40

Merge branch 'main' into reserve-auction

a73de98

Merge pull request #140 from almndbtr/test-protocol-fee

933af7d

Reserve Auction v1.0: add tests when the protocol fee is non-zero

iainnash reviewed Mar 11, 2022

View reviewed changes

iainnash mentioned this pull request Mar 11, 2022

Auction V3 Storage Slot Optimization #152

Closed

Rohan Kulkarni added 6 commits March 11, 2022 19:50

Merge branch 'main' into reserve-auction

db3d1c3

Merge branch 'reserve-auction' of https://github.com/ourzora/v3 into …

3aa35c3

…reserve-auction

feat: Reserve Auction Core ETH

d1593ea

feat: Reserve Auction Core ERC-20

3b05915

feat: Reserve Auction Finders ETH

e37a11d

feat: Reserve Auction Finders ERC-20

9da0f7c

Rohan Kulkarni added 17 commits March 12, 2022 23:00

feat: accept any start time -- via @iainnash

e191c70

feat: extra safety

df9ba23

Merge branch 'main' into reserve-auction

9c65036

chore: add revert scenarios upon bid

d548eee

chore: param naming in constructor

e6c0028

deploy: kovan

eee43c3

deploy: ropsten

82cc339

chore: clean

4798f1c

fix: remove old v1 tests

945008d

feat: uml

5d422b5

chore: update natspec

582b9a8

feat: better erc20 downcasting, errors, and optimizations

5183997

Merge branch 'main' into reserve-auction

d9b73ea

feat: interface support

b5f4ae0

chore: clean

8ed29b9

feat: ropsten deploy

1480b1d

feat: mainnet deploy

efa481f

kulkarohan merged commit d231be2 into main Mar 22, 2022

kulkarohan deleted the reserve-auction branch March 22, 2022 18:19

	erc721TransferHelper.transferFrom(_tokenContract, auction.seller, address(this), _tokenId);
	erc721TransferHelper.safeTransferFrom(_tokenContract, auction.seller, address(this), _tokenId);

	erc721TransferHelper.transferFrom(_tokenContract, auction.seller, address(this), _tokenId);
	// `transferFrom` is enough to move the NFT into escrow since the contract is the recipient
	erc721TransferHelper.transferFrom(_tokenContract, auction.seller, address(this), _tokenId);

Conversation

kulkarohan commented Jan 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

New Module Proposal

Description

Motivation and Context

How has this been tested?

Checklist:

Uh oh!

almndbtr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Context

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

almndbtr commented Feb 11, 2022

Uh oh!

kulkarohan commented Feb 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kulkarohan commented Mar 9, 2022

Uh oh!

almndbtr commented Mar 11, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kulkarohan commented Mar 13, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

kulkarohan commented Jan 25, 2022 •

edited

Loading