Skip to content

Conversation

RembrandtK
Copy link
Contributor

@RembrandtK RembrandtK commented Oct 8, 2025

Cleaning up in progress, and integration not complete yet. Likely to be rebased against main with a new PR later.

- Fix root test:coverage to use build:self:coverage for proper coverage artifacts
- Add build:coverage chain in issuance package for coverage-specific compilation
- Inline coverage script execution in test:coverage:self
- Remove scripts/coverage file (logic moved inline)

The original issue was that pnpm test:coverage reported incomplete coverage
after contract changes, requiring manual pnpm clean. This was caused by
coverage tests using regular build artifacts instead of coverage-specific
artifacts. The fix ensures coverage builds use the correct configuration
and generate proper artifacts automatically.
@RembrandtK RembrandtK self-assigned this Oct 8, 2025
@RembrandtK RembrandtK requested a review from Copilot October 8, 2025 21:31
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This pull request introduces an Issuance Allocator system for The Graph protocol, providing a centralized mechanism for distributing token issuance to various protocol components. The PR includes comprehensive testing infrastructure, performance optimizations, and support for both allocator-minting and self-minting allocation models.

  • Implements the core IssuanceAllocator contract with pause/resume functionality and proportional token distribution
  • Adds supporting contracts including DirectAllocation targets and comprehensive test infrastructure
  • Introduces optimized test utilities and fixtures to reduce test execution time and code duplication

Reviewed Changes

Copilot reviewed 35 out of 36 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
packages/issuance/contracts/allocate/IssuanceAllocator.sol Core contract implementing proportional token issuance allocation with pause/resume functionality
packages/issuance/contracts/allocate/DirectAllocation.sol Simple target contract that receives and redistributes allocated tokens
packages/issuance/test/utils/testPatterns.ts Comprehensive test utilities and patterns to reduce duplication across test files
packages/issuance/test/utils/optimizedFixtures.ts Performance-optimized test fixtures for faster test execution
packages/issuance/test/tests/helpers/fixtures.ts Updated test fixtures with support for new issuance system contracts
packages/interfaces/contracts/issuance/allocate/ Interface definitions for issuance allocation system

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Copy link

openzeppelin-code bot commented Oct 8, 2025

[Preview] Issuance Allocator

Generated at commit: e17797a4a3efae411c86f826a95b1bfca7b1b365

🚨 Report Summary

Severity Level Results
Contracts Critical
High
Medium
Low
Note
Total
3
4
0
15
38
60
Dependencies Critical
High
Medium
Low
Note
Total
0
0
0
0
0
0

For more details view the full report in OpenZeppelin Code Inspector

Copy link

codecov bot commented Oct 8, 2025

Codecov Report

❌ Patch coverage is 76.78571% with 65 lines in your changes missing coverage. Please review.
✅ Project coverage is 82.89%. Comparing base (f2dca10) to head (e17797a).

Files with missing lines Patch % Lines
...ontracts/contracts/tests/MockIssuanceAllocator.sol 27.27% 64 Missing ⚠️
...ges/contracts/contracts/rewards/RewardsManager.sol 95.45% 1 Missing ⚠️
Additional details and impacted files
@@                       Coverage Diff                        @@
##           rewards-eligibility-oracle-2    #1236      +/-   ##
================================================================
- Coverage                         83.69%   82.89%   -0.80%     
================================================================
  Files                                51       54       +3     
  Lines                              2183     2461     +278     
  Branches                            643      720      +77     
================================================================
+ Hits                               1827     2040     +213     
- Misses                              356      421      +65     
Flag Coverage Δ
unittests 82.89% <76.78%> (-0.80%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

- Fix syntax errors in RewardsManager.sol (missing closing braces)
- Add missing IRewardsIssuer import
- Complete MockIssuanceAllocator implementation with all required methods
- Update interface declarations and parameter names for consistency
- Fix interface support tests for IRewardsManager

All IssuanceAllocator integration tests now passing (9/9)
Contract compilation successful with proper ERC165 interface support
- Add InterfaceIdExtractor contract for extracting ERC165 interface IDs
- Add Python script to generate TypeScript interface ID constants
- Update tests to use auto-generated interface IDs instead of manual calculation
- Add interface ID consistency verification test
- Integrate interface ID generation into build process

This follows the same pattern as the issuance package and ensures
interface IDs are always consistent with Solidity's calculations.
- Move InterfaceIdExtractor contract to interfaces package
- Move interface ID generation script to interfaces package
- Integrate interface ID generation into interfaces package build process
- Update contracts package to use interface IDs from interfaces package
- Remove local interface ID generation from contracts package
- Place generated interface IDs in src/types/interfaceIds.ts following package conventions

This centralizes interface ID generation in the interfaces package where it belongs,
making it available for all packages to use and eliminating duplication.
Lint Fixes:
- Restore missing NatSpec documentation for IRewardsIssuer interface
- Remove TODO comment from MockIssuanceAllocator
- Add solhint-disable for named-parameters-mapping rule (not supported in Solidity 0.7.6)

Interface ID Centralization:
- Add IIssuanceAllocator and IRewardsEligibilityOracle to interfaces package InterfaceIdExtractor
- Update interfaces package generation script to include all interface IDs
- Remove duplicate interface ID generation from issuance package:
  * Delete InterfaceIdExtractor.sol
  * Delete generateInterfaceIds.py script
  * Delete local interfaceIds.ts helper
  * Remove generate:interfaces script from package.json
- Update issuance package to import interface IDs from @graphprotocol/interfaces
- Simplify interface compliance tests (remove redundant consistency checks)

All lint checks now pass (0 warnings) and all tests pass (161/161).
Interface IDs are now centrally managed in the interfaces package for consistency.
- Convert require() statements to ES6 imports where possible
- Improve type safety in test utility functions
- Export previously unused functions in issuanceCalculations.ts
- Standardize import organization across test files
- Fix mixed import styles in test files
- Organize imports properly in commonTestUtils.ts

All tests pass (161/161) and linting is clean.
- Add README.md with package documentation including setup, build,
  test, and lint commands
- Remove LICENSE from package.json files array (consistent with
  majority of packages; root LICENSE covers monorepo)
- Add Contracts section with links to detailed documentation
- Reference IssuanceAllocator.md and RewardsEligibilityOracle.md
- Include brief descriptions of each contract's purpose
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.

1 participant