Issuance Allocator (IA) (rebased) #1257
Draft
Conversation
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
Cherry-picked from ec0c984 (issuance-baseline-2/3) Rebased onto main with regenerated lockfile
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
Closed
Issuance Allocator (IA) (rebased)
🚨 Report Summary
For more details view the full report in OpenZeppelin Code Inspector |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1257 +/- ##
==========================================
+ Coverage 84.05% 86.14% +2.08%
==========================================
Files 42 45 +3
Lines 2070 2345 +275
Branches 615 698 +83
==========================================
+ Hits 1740 2020 +280
+ 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:
|
The default allocation receives issuance that is not explicitly allocated to another target. - Added setDefaultAllocationAddress() to set. - getTotalAllocation() excludes default when it is the zero address. - Tests and documentation updated.
Added two configurable reclaim addresses: - indexerEligibilityReclaimAddress - subgraphDeniedReclaimAddress When rewards are denied and the reclaim address is set (non-zero), tokens are minted to that address instead of not being minted at all. Defaults to address(0) (original behavior). Also updated associated documentation and tests.
- Add clarity to availablePPM calculation explaining it comprises default allocation's allocator-minting PPM, target's allocator-minting PPM, and target's self-minting PPM to maintain 100% allocation invariant - Refine reentrancy comment to explicitly reference that calculations occur after notifications to prevent reentrancy issues Addresses PR feedback from code review
…wing (#1268) Replace vm.assume with bounded inputs to fix "vm.assume rejected too many inputs" error. The previous implementation used overly restrictive constraints that caused the fuzzer to reject most random inputs. Now limits amountThawing and amountCollected to half of MAX_STAKING_TOKENS, guaranteeing valid deposit ranges while maintaining test coverage.
…address Change setDefaultAllocationAddress to distribute any pending issuance to the old default address before changing to the new address. Add evenIfDistributionPending parameter to handle the case when distribution cannot proceed (e.g., when paused). This allows callers to either: - Return false when distribution is pending (evenIfDistributionPending=false) - Force the change anyway (evenIfDistributionPending=true) Implementation: - Calls _handleDistributionBeforeAllocation before changing default address - Distributes pending issuance to old default address up to current block - Then changes allocation to new default address - When paused and evenIfDistributionPending=false, returns false instead of changing - Original setDefaultAllocationAddress(address) signature wraps new implementation with evenIfDistributionPending=false
Refactor _removeTargetFromList to _removeTarget to logically group target removal operations (array removal and mapping deletion) in a single function. Loop starts at index 1 to protect default allocation at index 0. Also fixes MILLION/PPM terminology, adds missing natspec, and adds test for address(0) default allocation.
When changing default allocation address, allocation data is copied from oldAddress to newAddress, which was overwriting newAddress's lastChangeNotifiedBlock. - If newAddress was just notified, the notification record was lost - If newAddress had a future block set (via forceTargetNoChangeNotificationBlock), that restriction was lost - When changing from address(0) (which is never notified), newAddress's lastChangeNotifiedBlock was incorrectly set to 0 Fix: preserve newAddress's lastChangeNotifiedBlock when copying allocation data. Also clarifies that notification blocks are target-specific, not about the default in general.
IA: Audit fix/response for Issuance Allocator Incorporated into audit report (Graph_IssuanceAllocator_v01.pdf) and now part of baseline for subsequent.
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.
Rebase of: #1245
Issuance Allocator contracts, see: packages/issuance/contracts/allocate/IssuanceAllocator.md