Issuance related changes and deployment (contract changes audited and merged)#1280
Open
RembrandtK wants to merge 60 commits intomainfrom
Open
Issuance related changes and deployment (contract changes audited and merged)#1280RembrandtK wants to merge 60 commits intomainfrom
RembrandtK wants to merge 60 commits intomainfrom
Conversation
The extraction script now produces consistent output by: - Sorting type exports alphabetically - Removing unused imports - Running prettier on extracted files This prevents spurious git diffs when regenerating the extracted GraphClient artifacts, as the types now appear in a stable order regardless of the source file's type ordering. Even with deterministic extraction, the extracted files were not stable. The query can return inconsistent results, although the differences do not matter for this package. Therefore, the build now only attempts to download if the extracted files are missing. This means a normal `pnpm clean` will not trigger downloading files again, but a `pnpm clean:all` will. Given the legacy nature of the package and that the normal thrash of downloaded inputs is not relevant, this seems pragmatic to avoid both git thrash and unnecessary package rebuilds.
Solidity upgrades: - Pragma 0.8.27 → 0.8.33 for issuance contracts - BaseUpgradeable: Use AccessControlEnumerable for role enumeration (enables getRoleMemberCount/getRoleMember for deployment verification) - Add local IGraphToken interface (resolves OZ v3/v5 dependency conflicts) - Add IAllocationManager interface for subgraph-service Forge integration: - Add foundry.toml and remappings.txt for issuance, horizon, subgraph-service - Configure forge-std in workspace catalog Hardhat 3 compatibility: - Update hardhat configs for HH3 + typechain v8 - Update package dependencies across affected packages - Add @nomicfoundation/hardhat-keystore to catalog Toolshed issuance support: - GraphIssuanceAddressBook for contract management - Extended address book types with deployment metadata - Updated exports for horizon and subgraph-service Verification infrastructure: - Remove legacy ArbiScan v1 customChains (v1 API deprecated) - Verification now requires v2 API (implemented in deployment package) Other: - Update .gitignore for typechain outputs and .nfs* stale handles - Update workspace dependencies (eslint, typescript-eslint, viem, etc.) Note: Tests for these changes exist but may not all run yet due to dependencies on changes in subsequent commits.
Moved to avoid recursive symlink Forge lint issues. Move packages/contracts/test to standalone packages/contracts-test: - New package with own hardhat.config.ts and package.json - Includes all unit tests for contracts package - Symlinks to contracts for compilation - Test scripts for unit, e2e, and coverage testing Reorganize packages/issuance tests: - Add testing/ directory with HH3 test files - Add testing-coverage/ for HH2 compatibility layer - Configure ethersHelper.ts for network.connect() pattern Update configurations: - Update contracts/hardhat.config.ts test paths - Update eslint.config.mjs for mocha globals in testing directories Capture Forge symlink issue in docs/ForgeLintSymlinkIssue.md
Add Forge linting step to CI workflow: - Run forge fmt --check on horizon and subgraph-service packages - Integrate with existing linting pipeline Update markdownlint configuration: - Add MD040 rule to allow code blocks without language specification Update linting documentation: - More succinct README.md linking to docs/Linting.md for detail Improve linting paths in package.json to lint correct files
Numerous fixes to satisfy (mainly) forge lint. No functional code changes. No production contract file changes.
New packages/deployment package: - Rocketh deployment scripts for issuance, rewards, and services - Hardhat v3 + Forge integration for deployment management - TX Builder for Safe governance transaction preparation - Address book sync and deployment metadata tracking - OZ proxy verification for Etherscan API V2 - Role management tasks: roles:list, roles:grant, roles:revoke - Deployment validation and contract verification utilities - Comprehensive documentation and testing infrastructure
Make this branch cleanly build/test/lint by temporarily excluding production contracts, associated packages, and failing tests. Full build/test/lint will be restored with audited contract code.
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub. |
Force-add gitignored placeholder types so toolshed can build when issuance package is excluded from workspace.
Baseline non-contract issuance related changes and deployment
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
Make this branch cleanly build/test/lint by temporarily excluding production contracts, associated packages, and failing tests. Full build/test/lint will be restored with audited contract code.
Tests: - Remove .skip from rewards test suites in contracts-test - Re-enable forge test and lint in subgraph-service package.json Workspace: - Remove issuance, deployment exclusions from pnpm-workspace.yaml - Restore toolshed issuance dependency to workspace:^ - Delete issuance types placeholders (no longer needed) - Update pnpm-lock.yaml Revert "fix(build): add placeholder issuance types for baseline build" This reverts commit 4288133.
Hardhat 3.x requires Node.js 22+ due to use of Iterator.prototype.flatMap
Move public members from toolshed into IIssuanceAllocator. Toolshed interfaces should only aggregate for type generation. See docs/InterfaceConsolidationPattern.md
Move public members from toolshed into IRewardsManager. Toolshed interfaces should only aggregate for type generation. See docs/InterfaceConsolidationPattern.md
Move public members from toolshed into IDisputeManager. Toolshed interfaces should only aggregate for type generation. See docs/InterfaceConsolidationPattern.md
Move public members from toolshed into IAllocationManager. Toolshed interfaces should only aggregate for type generation. See docs/InterfaceConsolidationPattern.md
Move public members from toolshed into IDataServicePausable. Toolshed interfaces should only aggregate for type generation. See docs/InterfaceConsolidationPattern.md
Move public storage getters from ISubgraphServiceToolshed into ISubgraphService. Toolshed interfaces should only aggregate for type generation, not define unique function signatures. See docs/InterfaceConsolidationPattern.md
Creates a local OZ5-compatible IGraphToken interface to resolve dependency conflicts between issuance package (OZ5) and interfaces package (OZ3). The minimal interface includes only IERC20 + mint(), which is all the issuance contracts require.
Correct acceptProxy and acceptProxyAndCall signatures. Interface mistakenly had single-param signatures from GraphUpgradeable instead of two-param signatures from GraphProxyAdmin.
Rename clarifies semantic meaning: these are conditions under which rewards behavior changes (staleness, denial, etc.), not reasons explaining why something happened. The term "condition" better reflects that these are states checked during reward collection that determine how rewards are handled.
Add NO_SIGNAL, BELOW_MINIMUM_SIGNAL, NO_ALLOCATION conditions. Enables granular reclaim routing for edge cases where rewards cannot be distributed to indexers.
Fallback address when reason-specific reclaim address not set. Adds setDefaultReclaimAddress (governor) and getDefaultReclaimAddress.
Consolidate reward claimability checks into single helper. Returns new rewards, signal amount, and denial condition (SUBGRAPH_DENIED, BELOW_MINIMUM_SIGNAL, or NONE).
Simplify denial logic with single reclaim attempt. Evaluates both denial conditions upfront, emits appropriate events, then uses first applicable reclaim address. Prefers SUBGRAPH_DENIED over INDEXER_INELIGIBLE when both apply.
Add fallback to defaultReclaimAddress. Priority: reason-specific address → default address → drop. Early return on zero rewards.
Reclaim rewards when no signal exists. Uses _getNewRewardsPerSignal to detect zero-signal periods and routes issuance to NO_SIGNAL reclaim address instead of dropping.
Extract helper to sum allocated tokens across all issuers. Enables consistent token counting for reward calculations when multiple reward issuers exist (Staking + SubgraphService).
- Adjust expected rewards to account for pre-allocation reclaim - Use evm_setAutomine to batch signal mints in same block - Move b1 snapshot after allocation creation - Check only subgraph1 rewards (subgraph2 has no allocation) - rewards-calculations: update expected subgraph rewards from 1400 to 1000 - rewards-distribution: batch signals in same block, check sg1 only - rewards-interface: update IRewardsManager interface ID
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1280 +/- ##
==========================================
+ Coverage 84.06% 86.95% +2.89%
==========================================
Files 42 46 +4
Lines 2071 2492 +421
Branches 615 746 +131
==========================================
+ Hits 1741 2167 +426
+ Misses 330 325 -5
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Add tests to verify timestamp is always updated when transitioning between zero and non-zero issuance rates, preventing over-issuance of rewards for periods when issuance was disabled.
Avoid redundant SLOADs by using local variables for accRewardsForSubgraph and the already-cached accRewardsPerAllocatedToken.
…gnalUpdate Avoid redundant SLOADs by using local variables instead of re-reading storage after writes.
Maikol
previously approved these changes
Feb 3, 2026
Member
Maikol
left a comment
Maikol
left a comment
There was a problem hiding this comment.
Looks good to me. I would want someone else to review since we're changing deployment scripts from using ignition which we just setup to a new custom service. I've seen it working and I agree it's much nicer but I think we should give others the opportunity to give their thoughts.
| @@ -156,16 +156,6 @@ const config = { | |||
| }, | |||
| etherscan: { | |||
| apiKey: process.env.ETHERSCAN_API_KEY, | |||
| customChains: [ | |||
Member
There was a problem hiding this comment.
Is this not needed any more for verification? If we deploy new vesting contracts will verification still work from this package? Or should we migrate to the new deployments package?
Contributor
Author
There was a problem hiding this comment.
These are V1 APIs URLs that no longer work, the V1 API has been turned off. This is already broken.
Putting in a V2 URL does not work, the way to use has been changed. I am not sure we will ever need to verify these, and did not put work into fixing it.
Having the V1 URLs leads to confusion, I have seen AI go through multiple cycles of thrashing trying to use them.
Maybe putting in a comment would be an alternative, but I figure if anyone ever tries to do verification for these contracts (I expect considerable other work would need to happen first anyway) if will be obvious it is missing and can be fixed. Likely this would be accompanied by upgraded tooling that uses V2 API.
Previously, rewards were silently dropped when signal/allocation hooks were called on non-claimable subgraphs, causing permanent loss. Both hooks now consistently reclaim rewards for SUBGRAPH_DENIED, BELOW_MINIMUM_SIGNAL, and NO_ALLOCATION conditions. Hook Changes (onSubgraphSignalUpdate & onSubgraphAllocationUpdate): - Add reward reclaim logic for all non-claimable conditions - When not claimable: reclaim immediately, accRewardsForSubgraph NOT updated - When claimable: add to accRewardsForSubgraph, increase accRewardsPerAllocatedToken - Centralize condition checking in _getSubgraphRewardsState helper - Implement fallback chain: when multiple conditions apply, prefer ones with configured reclaim addresses (DENIED → BELOW_MINIMUM → NO_ALLOCATION) - Use updateAccRewardsPerSignal for same-block caching and prompt NO_SIGNAL reclaim - Add early return when both snapshots already current (gas optimization) - Skip storage writes when accumulators unchanged (gas optimization) View Function Changes: - getAccRewardsForSubgraph: exclude new rewards for all reclaim conditions - getAccRewardsPerAllocatedToken: only include rewards when claimable - getRewards: reflect deterministic exclusions (denied, below minimum, no allocations) - Note: indexer eligibility is NOT checked by view functions - it can change independently and is only validated at claim time Implementation Details: - _getSubgraphRewardsState: centralized helper returns condition + allocated tokens - Preserves accRewardsPerAllocatedToken value when subgraphAllocatedTokens=0 (returns stored value instead of 0, maintains accumulator for when allocations return) - Track undistributed rewards (accRewardsForSubgraph - snapshot) for proper reclaim Testing: - Add rewards-signal-allocation-update.test.ts: comprehensive tests for same-block signal/allocation updates and edge cases where per-signal delta=0 - Update existing tests for new accumulator behavior and reclaim conditions Documentation: - Add RewardConditions.md: complete reference table of all conditions, handling layers, and reward outcomes - Remove DeniedSubgraphRewardsAnalysis.md (content consolidated) - Update RewardAccountingSafety.md accumulator behavior description
Use semantic constant consistent with rest of code.
Call onSubgraphAllocationUpdate when _presentPoi exits early for ALLOCATION_TOO_YOUNG or SUBGRAPH_DENIED conditions. Keeps reward tracking and reclaim accumulation current even when rewards aren't collected, ensuring accumulation or reclaims in predictable period. Also cache currentEpoch to avoid duplicate external call.
Add documentation for TRST-L-1 audit finding. When minimumSubgraphSignal changes, the new threshold applies to pending rewards retroactively when subgraphs are updated. Documents mitigation process requiring governance to call update hooks before changing the threshold. - Add NatSpec to setMinimumSubgraphSignal() with 5-step mitigation process - Add new section to RewardConditions.md explaining retroactive risk - Clarify governance vs participant responsibilities
- getRewards(): describe as gap between accumulator and allocation snapshot - Replace 'pre-denial rewards' with 'uncollected rewards' throughout docs - Enforce NONE cannot be used as reclaim reason in _reclaimRewards() - Update reclaimRewards() docs for NONE behavior
In _updateSubgraphRewards(), the check `subgraphAllocatedTokens != 0` is redundant: if undistributedRewards != 0 then condition == NONE, and _getSubgraphRewardsState() guarantees subgraphAllocatedTokens != 0 when condition is NONE (otherwise it sets NO_ALLOCATION which triggers reclaim above, zeroing undistributedRewards). Also fixes inaccurate NatSpec on _getSubgraphRewardsState return value.
The condition guards against zero tokens allocated to a subgraph, not the absence of allocation objects. A subgraph can have open allocations resized to zero tokens, making the old name misleading. Rename across contracts, tests, and docs for clarity.
_resizeAllocation() now checks staleness and reclaims accumulated pending rewards instead of allowing them to accrue for allocations that are not performing. Also fixes MockRewardsManager.calcRewards and reclaimRewards to return realistic values for meaningful test coverage.
Update rewards reclaiming, audited
RembrandtK
changed the title
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
2 participants
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.
Note the end state of this branch is designed to work in conjunction with contract changes in #1279.
It was going to be non-trivial to get an interim fully working state so it temporarily disables issuance and deployment packages, tests in subgraph-service, and some Rewards Manager tests. The initial commit in #1279 re-enables these.
UPDATE: #1279 has been merged, now ready to be merged to main.