checkpoint-labs / checkpoint

Checkpoint is a library for indexing data of Starknet contracts.
https://checkpoint.box
MIT License
55 stars 22 forks source link

bug: duplicate results with same id in GraphQL API #306

Open bonustrack opened 4 days ago

bonustrack commented 4 days ago

I've noticed an issue when save multiple entities within a Promise.all, it happen sometime that an entity would appear twice in the GraphQL results while having the exact same id. I've noticed this issue after updating to Checkpoint v37 and doing this change:

https://github.com/snapshot-labs/delegates-api/pull/1/commits/39400456bd6e5df1e4d784a4cb4f1e899cda8323

image

When 2 results appear they are exactly the same, making it feel like it's not 2 different rows on the database with different block range otherwise I imagine the results would be different.

Sekhmet commented 3 hours ago

You mentioned that it seemed to get fixed by some change on writer side? Can you provide some details?

bonustrack commented 1 hour ago

After removing the 2 promise.all from https://github.com/snapshot-labs/delegates-api/commit/39400456bd6e5df1e4d784a4cb4f1e899cda8323 the duplicate issue did not appear anymore

Sekhmet commented 49 minutes ago

I see, getEntity creates entity if it doesn't exist already, if we execute multiple of those at the same time we might end up in situation where they are created multiple times (because both check if entity exists in parallel).

And before you basically called it twice at the same time so both lines ended up creating new entity:

getEntity(TokenHolder, from, governanceId),
getEntity(TokenHolder, to, governanceId),

and inside new Governance is created:

if (entity === TokenHolder && id != ZERO_ADDRESS && governanceId) {
  const governance = await getEntity(Governance, governanceId);
  governance.totalTokenHolders += 1;
  await governance.save();
}

I will add special index for entities so only single instance can exist that is valid in the same range, but this would still be considered bug in the writer itself, now it would only cause an error instead.

bonustrack commented 32 minutes ago

Make sense, I imagine this could be solved with #307