add a similar batchRemoveExpired() for a single address to save gas

[sunsetlover]

Description

Every time .to(address) is called, it costs around 10,000 gas (see the ethereum yellow paper). If the expired offers that are being removed belonged to the same address, then you can just send the escrow funds just once at the end to save gas.

Scenario

In a case where a single address has many expired bids (which seems like a likely scenario), batchRemoveExpired() processes refunds one at a time when instead it could process all the refunds at once since it's going to the same address.

Impact

Can save 10,000 gas per cat per user significantly lowering the gas cost. (However, after roughly testing out a skeleton of my fix below, you net ~6,000 in gas savings at least in the cases I tested due to new overhead).

Fix

Create a nearly identical function as batchRemoveExpired() except at every iteration keep track of the previous offer's bidder.

Use require() to require that the previous offer's bidder is the same as the current bidder.

Also keep track of the cumulative refund, an array of all the cats in all the bids, and an array of each refund price. (One caveat is that the size of this array must be preallocated. Any array that is locally declared but dynamically sized is a treated as a storage variable. However, I don't think this is a huge issue since there are only so many cats that can be batched removed at one time anyway based on ethereum's block gas limit).

You will need to also add a new event like to call once at the end of this new function:

            emit ExpiredOfferRemovedSameAddress(
                tokenIdArray,
                bidder,
                toRefundArray,
                cfoEarningsCumulative
            );

(You could continue to emit this event at each iteration. However, I believe this can make event tracking off-chain more difficult. It seems likely due to human error there could be a scenario where the entire transaction fails due to the require() that checks if the previous offer's bidder is the same as the current offers bidder. In this case, it will have emitted events before failing and will require whoever is checking for events to check for whether or not the tx failed).

Because the arrays for keeping track of cats/bids were preallocated in size, of course you will likely have trailing 0's in the entries that correspond to the cats/bids in the events that get emitted. However, this is not confusing whatsoever since there is no cat with a token of 0 and likely no bid with an offer of 0.

You will also have to appropriately modify _tryPushFunds or create an analogous function.

If you want to modify, you should allow it to take an array of cats and array of respective refunds (and of course update anything that calls _tryPushFunds). Inside, _tryPushFunds currently, the following event is emitted: emit PushFundsFailed(_tokenId, _to, _amount);, so again you will either need to modify this event to take an array of cats and refunds and call this event once or create a new event that can handle arrays.

In fact, if saving this amount of gas is important, it would not be too difficult to create a more general function analogous to batchRemoveExpired() which again takes an array of cats such that the first k_1 cats have bids associated with the same bidder, the next k_2 cats have the same bidder, etc. and an array whose length is the number of unique bidders and whose i-th entry says how many cats will have an offer with that bidder. You can perform an analogous check as in the single address case above that the input is correct about the bid owners as you iterate through the refunds.

[ghost]

If all the cats that are being removed belong to the same person

But batchRemoveExpired() doesn't remove cats, it removes expired offers. So it wouldn't matter that all of the cats belong to the same person, what would matter is that all of the ETH for the offers came from the same person.

Use require() to require that the previous offer's bidder is the same as the current bidder.
Also keep track of the cumulative refund, an array of all the cats in all the bids, and an array of each refund price.
You will need to also add a new event like to call once at the end of this new function:
You will also have to appropriately modify _tryPushFunds or create an analogous function.

Might all of this additional overhead end up offsetting the potential savings from lumping all of the transfers into a single transfer?

[sunsetlover]

@TomLeeFounder Sorry, thanks. That first part was a bit of a typo and bad choice of words. I updated it.

Yeah, I believe that a significant amount of gas can be saved. I just tested out on remix a skeleton of this suggestion just to see how much gas it would add (i.e., simulating using require at each iteration, keeping track of a list of cats/offers, emitting events with arrays instead of numbers) and it seems to be ~4000 overhead. So you are still saving ~6000 per cat.

The only caveat is that for the local array that keeps track of cats and previous bids, you need to preallocate its size beforehand. Otherwise, dynamically sized arrays are treated as storage variables. However, I don't think this is a huge issue since obviously you can only call so many cats on batchRemoveExpired() without going over ethereum's block gas limit.

But this means that when the events get emitted, there will be trailing 0's in the arrays corresponding to cats and offers, which again I don't think is a huge deal. They already have an event that gets emitted when an offer is updated where one of the values only makes sense when it is non-zero (meaning that someone not only extended the time on their offer but actually increased their offer monetarily).

[hwrdtm]

Thanks @sunsetlover for your feedback! We will take it into consideration.

[arthcmr]

Thanks for your participation, @sunsetlover! Our team has reviewed your submission, and while we are grateful, we have decided it does not warrant a reward.