Unnecessary fetches for pointers in three different functions when pointer already exists

[ghost]

Description

All of the following functions perform an unnecessary read from storage (which is costly in gas): .cancelOffer(), .fulfillOffer(), and .batchRemoveExpired().

Each of these functions calls tokenIdToOffer[tokenId] twice, rather than once.

Scenario

Each of these three functions perform an unnecessary read from storage to retrieve a pointer to the offer that they want to delete by calling tokenIdToOffer[tokenId], even though they already have a pointer to its location through Offer storage offer = tokenIdToOffer[_tokenId];

The keyword storage creates a pointer to the value of offer in storage, so any changes made to offer will persist. There is no need to find the pointer's location again by using tokenIdToOffer[tokenId].

Impact

Medium: when aggregating the number of times that these three functions will be called, the gas savings will end up being quite large.

Reproduction

All three of these functions use the following line of code to delete an offer:

delete tokenIdToOffer[_tokenId];

However, all three already have a pointer to that exact offer from this line of code:

Offer storage offer = tokenIdToOffer[_tokenId];

Since the keyword storage passes by reference rather than passing by value, any changes made to the offer variable will persist in storage.

Fix

In all three functions, change this:

delete tokenIdToOffer[_tokenId];

to this:

delete offer;

[hwrdtm]

Thanks @michaelKim4736 for your feedback! We will take this into consideration.

[arthcmr]

Thanks for your participation, @michaelKim4736! Our team has reviewed your submission and decided to reject the issue.