Suggested follow-up to #1203: extract helper + add IE090 tests#1205
Merged
tmigone merged 2 commits intotmigone/fix-overallocationfrom Apr 28, 2026
Merged
Conversation
Both reallocate paths (resolver and AllocationManager) duplicated the same 30-line check-and-throw block. Extract `assertNotOverAllocated` so future changes happen in one place. Parallelises the two independent reads (`allocationProvisionTracker` and `getDelegationRatio`) and clarifies the error message to point operators at `graph indexer allocations close` as the way to still collect rewards when over-allocated. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Adds unit coverage for assertNotOverAllocated: happy path skips the diagnostic reads, the over-allocated path throws IE090 with the GRT delta in the message, the verifier address is forwarded correctly to HorizonStaking.getTokensAvailable, and a negative delta from racing reads is clamped so the error message never prints a negative amount. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
While reviewing #1203, two things stood out: the new over-allocation guard is duplicated byte-for-byte across the resolver path (
reallocateHorizonAllocation) and the action-queue path (AllocationManager.populateReallocateTransaction), and the new IE090 throw has no test coverage. Without a shared helper, any future tweak to the guard — error message, additional diagnostic reads, metrics — has to happen twice and will eventually drift. Without tests, the guard could silently regress and we'd only find out when an over-allocated indexer hit the path in production.This branch is offered as a suggestion stacked on top of #1203, not a replacement. Take, modify, or discard.
Summary
Two commits, each independent:
refactor(allocations): extract over-allocation guard into shared helper— moves the duplicated 30-line block intoassertNotOverAllocatedin a newover-allocation.tsmodule. Both call sites in fix: gracefully handle overallocation when reallocating #1203 now invoke the helper. Side improvements while extracting:allocationProvisionTrackerandgetDelegationRatio) — saves one round-trip on the error path.0nto defend against a negative bigint formatting into the error message if the three reads race.graph indexer allocations close(which already handles over-allocation gracefully and still collects rewards) instead of the ambiguous "manually unallocate that amount."test(allocations): cover IE090 over-allocation guard for reallocate— four pure-unit tests againstassertNotOverAllocated:IndexerErrorwithcode === IE090and the GRT delta in the cause stringSubgraphService.target) and delegation ratio are forwarded correctly toHorizonStaking.getTokensAvailable0.0 GRT, no minus signVerified
yarn compileclean against locked@graphprotocol/toolshed@1.1.1yarn lintcleanyarn test src/indexer-management/__tests__/over-allocation.test.ts— 4/4 passingNotes
closeHorizonAllocationandpopulateUnallocateTransaction) which also callisOverAllocatedbut with different branching semantics. Worth a separate refactor PR but out of scope for fix: gracefully handle overallocation when reallocating #1203.