Consider adding an upper limit to number of active auction sets.

[brianmcmichael]

The OpenZeppelin EnumerableSet is used here for maintaining the list of auctionsets and includes a warning against using enumerate() on large data sets.

I haven't benchmarked the gas usage of this contract yet but enumerate() is called in getActiveAuctions() but critically in _getActiveAuctionVatDaiTotal() which is relied upon in many locations, including defect(), getDefectAmount(), getDaiBalance(), and getDaiBalanceForAuctions()

The _getActiveAuctionVatDaiTotal() function calls enumerate() on the set at line 350 and then makes another complete loop across the set at 354. This function will end up being quite expensive due to the consecutive loops, mloads, and safemath operations.

Starting 3/19 there will be approximately 50 auctions starting in quick succession, followed by another 50 or so in the following days. I'm concerned that adding too many auctions to this set will cause the _getActiveAuctionVatDaiTotal() function to begin failing due to gas limitations and start preventing other functionality.

At this point the safest solution might be to add a check in the EnumerableSet's add() function that sets a maximum number of auctions that can safely be maintained at one time. I have not benchmarked the gas usage of this function yet, however, and so I do not yet know what the safe number of auctions might be.

[PhABC]

Quick back of the napkin math:

• At 5,500,000 DAI in surplus, we will have around 110 auctions in total.
• For each active auction, we do flopper.bids(auctionID)
• Each bids() calls queries 5 variables from storage, so 700 gas * 5 and cross-contract call cost of say 2000 gas (not sure of that latter, but feels conservative).
• For the enumerate, we would do at most 110 SLOAD calls, so 700 gas * 110

So the "maximum" cost for that loop with the assumptions would be 110 * 700 + 5 * 110 * 700 + 110 * 2000 = ~700k gas

assuming we participate in all 110 auctions. This is high, but this number goes does as auctions get finalized and eventually that array will be of length 0.

[brianmcmichael]

Thanks for doing the math on this!

I know this is already deployed and accepting funds, just wanted to raise the flag on this for anyone planning to borrow this contract as a pattern for flip auctions.