graph-network-subgraph icon indicating copy to clipboard operation
graph-network-subgraph copied to clipboard

Update Pools for delayed collections

Open Data-Nexus opened this issue 2 years ago • 1 comments

Very minor change to detect delayed collections (collect in an epoch grater than the closedAtEpoch).

Per the Staking.sol contract, query fees are added to the rebate pool in which the allocation was closed (not when the collect function is run).

Updated pool to attempt to use the closedAtEpoch if available, otherwise use the current epoch, but only update epoch.queryFeesCollected and pool.totalQueryFees when the allocation is closed so it can mirror what's seen in the rebate pool.

Example: Epoch 634 has a total fees of 765540551752580194585 per the Staking Contract yet the pool.totalQueryFees shows 918822211489531994645. query

Consideration: What happens if an allocation is collected on twice post allocation closure? This doesn't appear to be prevented in the contract and could result in double counting.

Data-Nexus avatar Oct 05 '22 01:10 Data-Nexus

Great catch @Data-Nexus!

Although after going through the code I found another potential bug that wouldn't allow this to work as expected:

Because of how createOrLoadEpoch works, if the epoch was already created at some point, we won't get the epoch we want, but actually the current one: https://github.com/Data-Nexus-Web3/graph-network-subgraph/blob/b7380721c35d0b9c9a2af3b86b481422335d58c5/src/mappings/helpers.ts#L379-L408

Because of this line: https://github.com/Data-Nexus-Web3/graph-network-subgraph/blob/b7380721c35d0b9c9a2af3b86b481422335d58c5/src/mappings/helpers.ts#L403-L406 I actually didn't code that logic, so I was unaware of that caveat there might be a reason, given that it might not be extremely trivial to calculate which epoch any block number would correspond to, but it should be possible to retroactively calculate the particular epoch number if we replicate the contracts' logic (which I think it's just a fixed amount of blocks per epoch, which would make this possible, but we need to be careful with the extra caveat of epoch length changes, which might require some sort of historical snapshotting to be able to proper account for those offsets).

juanmardefago avatar Oct 05 '22 15:10 juanmardefago