Add ERC721AQueryable

[Vectorized]

This is made to continue from #114.

The main concern with adding a tokensOfOwner(owner) function is that if the collection grows too big, the method can fail for off-chain calls (at the moment of writing, Infura has a limit of 30 times of block gas limit).

This PR adds a tokensOfOwnerIn(owner, start, stop) function to allow querying token IDs in a certain range. This effectively allows all token IDs to be queried, regardless of how big the collection gets.

You can make a loop to call the function on your frontend / backend, as long as it stays below Infura's rate limits.

As such, the ERC721ALowCap extension will be generalizable to collections of all sizes, and should be renamed to ERC721AQueries.

Additional functions include rawOwnershipOf and rawOwnershipsOf, which are useful for various purposes
(imo, if the queries extension can only query tokensOfOwner, it feels kind of empty).

I've used different function names for these public/external functions instead of function overloading.
This makes it easier to use in web3 calls.

I highly recommend this, so that people won't need to copy and paste code (which can introduce errors).

[ahbanavi]

WDYT about adding a with no minted tokens context in tests?
To be sure that the for loops don't introduce bugs if so.

[Austinhs]

I think it's important to have

                // All tokens have been found, we don't need to keep searching
                if (tokenIdsIdx == holdingAmount) {
                    break;
                }

For whatever odd reason if someone we're to call this from another contract it could potentially save some gas for them.

[Vectorized]

@Austinhs It is in the boolean condition of the for loops.

[Austinhs]

@Vectorized 👍

Excuse me 🙇

[cygaar]

I think it'd be better to check if start >= stop at the top of the function and throw an error if it's true. These are invalid parameters so it's better to error out than return 0 semantically

[Vectorized]

I am leaning on the side of not reverting and just returning an empty array,
to make it easier for querying and avoid having an additional custom error.

Even if we early revert at the top, the rest of the function still needs to stay the same to pass the tests.

[Austinhs]

I agree with @Vectorized, Most of these features are for front-end queries anyways, it would be nice to be returned an empty array to handle instead of an error IMO

[cygaar]

It might be easier to handle on the frontend if you gracefully handle the error, but it doesn't mean it's the right way to do this. If you look at most of the functions in OZ's contracts, they all error out whenever you pass in invalid input parameters.

@Vectorized why do you need to keep the rest of the function the same? You will definitely have to change some tests if you make this change, I wouldn't expect the existing tests to pass.

[Vectorized]

If I remove the checks and set to zero after the clamping, the function can go into a near infinite loop.

[cygaar]

What's the exact scenario you're thinking of? I don't see why we still need the if-statement on line 88. Line 93 can never evaluate to true

[Vectorized]

Suppose a collection with _currentIndex = 8, and we pass in start = 10 and stop = 11.

After setting start to max(_startTokenId(), start), and
stop to min(_currentIndex, stop),
start will remain 10, and stop will be 8.

This will cause the search loop to be stuck, because we didn't early return.

Alternatives:

• Use (start < stop) instead of (start != stop) in the for loop. Each iteration costs more. Even though this is an external view function, a few gas multiplied by several thousand is quite a lot.
• Set start to clamp(start, _startTokenId(), _currentIndex) and stop to clamp(stop, _startTokenId(), _currentIndex). More opcode needed.

[cygaar]

What about doing if (start >= stop || start >= stopLimit || stop <= _startTokenId) revert InvalidQueryRange(); after line 80? Basically, do the input validation after you know the start index and stop limit.

[Vectorized]

That will cause it to revert for empty collections, which is not desired.

[cygaar]

Might be better named as "explicitOwnershipOf"

[Vectorized]

ok

[cygaar]

Let's name this ERC721AQueryable

[cygaar]

It might be easier to handle on the frontend if you gracefully handle the error, but it doesn't mean it's the right way to do this. If you look at most of the functions in OZ's contracts, they all error out whenever you pass in invalid input parameters.

@Vectorized why do you need to keep the rest of the function the same? You will definitely have to change some tests if you make this change, I wouldn't expect the existing tests to pass.