OpenZeppelin/openzeppelin-subgraphs

EIP721 Token URI Bug

twotimeseleven opened this issue · 3 comments

twotimeseleven commented

Hi all (or @Amxx),

I've found a bug in the EIP721 code, related to token URI's not being updated when they should be.

On each transfer() and approval() event, the subgraph calls the following function to retrieve the token from the graph-node's GraphQL store: here

However, this function only queries and saves the token URI in the event that the token entity is not already found. In many cases, however, a token's URI can change; as the code currently stands, only the originally set token URI will ever be returned.

This should be as simple as always bind()ing and checking the token URI when loading the token, so that the updated URI always gets saved back to the store.

I can create a PR to merge this change in and will attach it to this issue.

twotimeseleven commented

PR found here: #19

Amxx commented

Hello @Rburdett4 and thank you for the issue and the PR.

This has already been discussed for ERC1155, and I believe the answer should be the same.

In my opinion it is not reasonable to include a URI fetch for all approval/transfer functions. This would have a terrible impact on the indexing performance.

Ideally, any change to the tokenUri should emit an event. ERC1155 includes such an event, and the subgraph listens to it. If ERC721 was extended to include such an event, it would definitely be added to subgraph.

IMO, the tokenUri part of the subgraph is really a "best effort", and I would encourage consumers to fetch this value if they need it:

  • It can be queried at any point and doesn't require any "aggregation", so indexing it is not really essential
  • even with you PR, URIs might change and not be indexed, just because no transfer happened in the meantime.
schmidsi commented

The mutable nature of tokenUri is a complex topic. For some tokens it never changes and for some it changes often. As I understand this library, it provides a safe default for subgraphs for the standard contracts of OpenZeppelin. So, as a default I agree with @Amxx that it should not try to lookup the tokenUri for every transfer. In fact, every eth-call from inside a subgraph mapping is a huge drag on indexing performance. So the recommended approach is to avoid them as much as possible.

If your implementation has a different approach to the tokenUri, you can just take this boilerplate code and extend it to your specific needs. The same as you would extend the OpenZeppelin contracts.

  • 👍2