Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Pools for delayed collections #197

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

Data-Nexus
Copy link

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.

@juanmardefago
Copy link
Collaborator

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).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants