chore: move all interfaces to dedicated package #1187

tmigone · 2025-06-11T19:24:06Z

List of changes:

Fixed some issues due to previous sync merge with main. These only surfaced with integration tests.
Rename common to interfaces. I feel this represents more accurately what this package should be, but I'm happy to further discuss the name and/or use of it.
Updated horizon, subgraph-service, toolshed and hardhat-graph-protocol to use interfaces. Most notably this eliminates the circular dependency allowing for arbitrary building of the packages and static import of hardhat-graph-protocol.
Fix linting issues in contracts package.
Move sdk package to it's own repository at https://github.com/graphprotocol/sdk. contracts package (actually child packages) remains the only place where it's used, it's now pinned to latest published version.
hardhat-graph-protocol now accepts a flag to specify if address book should be created or not if it doesn't exist.
Moved test fixtures to toolshed
Refactored how toolshed manages types and interface imports, its now much simpler.

Pending:

While bytecode shouldn't have changed for horizon and subgraph-service contracts it's worth checking with audit team wether or not they want to take a look at the changes.
contracts package still uses it's own interfaces (though they are duplicated in interfaces). This seemed like a big lift atm.
IGraphToken interface is widely used and for the most part being imported from contracts and not interfaces. This is due to the usage of TokenUtils.sol which also imports the file. Importing it from two sources creates problems. Perhaps this warrants a common shared solidity package, not interfaces but implementations/libs?

Signed-off-by: Tomás Migone <[email protected]>

socket-security · 2025-06-11T19:27:43Z

Review the following changes in direct dependencies. Learn more about Socket for GitHub.

Diff	Package	Supply Chain Security	Vulnerability	Quality	Maintenance	License
	@openzeppelin/contracts@3.4.1
	@graphprotocol/sdk@0.6.0
	hardhat@2.24.2

View full report

openzeppelin-code · 2025-06-11T19:40:05Z

chore: move all interfaces to dedicated package

Generated at commit: 8b7f14cc7270aee30a37f98f15222cfd1930b8b0

🚨 Report Summary

	Severity Level	Results
Contracts	Critical High Medium Low Note Total	2 4 0 15 39 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

Signed-off-by: Tomás Migone <[email protected]>

codecov · 2025-06-11T20:48:14Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 82.84%. Comparing base (82e68ab) to head (63b8c27).
Report is 75 commits behind head on horizon.

Additional details and impacted files

@@             Coverage Diff             @@
##           horizon    #1187      +/-   ##
===========================================
- Coverage    92.56%   82.84%   -9.72%     
===========================================
  Files           47       47              
  Lines         2435     2093     -342     
  Branches       440      620     +180     
===========================================
- Hits          2254     1734     -520     
- Misses         181      359     +178

Flag	Coverage Δ
unittests	`82.84% <ø> (-9.72%)`	⬇️

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.

Signed-off-by: Tomás Migone <[email protected]>

socket-security · 2025-06-12T14:10:17Z

Warning

Review the following alerts detected in dependencies.

According to your organization's Security Policy, it is recommended to resolve "Warn" alerts. Learn more about Socket for GitHub.

Action	Severity	Alert (click for details)
Warn		`@openzeppelin/[email protected]` has a Critical CVE. CVE: GHSA-fg47-3c2x-m2wr TimelockController vulnerability in OpenZeppelin Contracts (CRITICAL) Affected versions: >= 4.0.0 < 4.3.1; >= 3.3.0 < 3.4.2 Patched version: 3.4.2 From: packages/interfaces/package.json → `npm/@openzeppelin/[email protected]` ℹ Read more on: This package \| This alert \| What is a critical CVE? Next steps: Take a moment to review the security alert above. Review the linked package source code to understand the potential risk. Ensure the package is not malicious before proceeding. If you're unsure how to proceed, reach out to your security team or ask the Socket team for help at `[email protected]`. Suggestion: Remove or replace dependencies that include known critical CVEs. Consumers can use dependency overrides or npm audit fix --force to remove vulnerable dependencies. Mark the package as acceptable risk. To ignore this alert only in this pull request, reply with the comment `@SocketSecurity ignore npm/@openzeppelin/[email protected]`. You can also ignore all packages with `@SocketSecurity ignore-all`. To ignore an alert for all future pull requests, use Socket's Dashboard to change the triage state of this alert.

View full report

Signed-off-by: Tomás Migone <[email protected]>

packages/horizon/package.json

.gitignore

RembrandtK · 2025-06-19T10:37:46Z

package.json

@@ -10,7 +10,7 @@
    "postinstall": "husky",
    "clean": "pnpm -r run clean",
    "clean:all": "pnpm clean && rm -rf node_modules packages/*/node_modules",
-    "build": "chmod +x ./scripts/build && ./scripts/build",
+    "build": "pnpm -r run build",


Good for this PR, a discussion point though:

Locally I am trying splitting build to build:dep (build dependencies) and build:self (only build package currently in). Then:

In non-root packages build -> pnpm build:dep && pnpm build:self

In root package build -> pnpm -r run build:self, because this one should not be recursive, just do each in individually once in dependency order.

In other targets like test in (non-root) package at pnpm build && to existing. Bascially I don't want to have to worry about rebuilding, although might be worth making original target test:no-build or something so can do that.

Worth noting might be some tuning in places to make it more efficient where no rebuild in required.

Do you think we should keep child packages long term? I don't exactly remember at this point why we added those, I think to break the cyclic dependency? If so, perhaps with interfaces package we can simplify it?

I think we need child packages, and/or siblings that combine dependencies across contract packages.

They are to break cyclic dependencies, I think interfaces does not by itself fully solve that. It might mean that contract packages no longer rely on toolshed? Or is that still via dynamic loading? However, deployment has cross package needs for compiled contracts. (I think there is an argument for having a single deploy sibling, that spans contract packages.)

The same scenario for tests, similar nuances to deploy, but I think not as strong a case for (just) a single test package spanning contract packages.

If we do not separate them, I think we can appear to get away with it by recompiling dependencies, which the hardhat-dependency-compiler can do, but then you end up with other issues due to that duplication and inconsistency.

I suspect contract compilation should be separate reusable step from what we do with the compilied artifacts (deployment, testing, etc). But I might be missing something; I am new to this package management system.

Got it. I'm not too sold on any alternative to be honest, there are things i dislike in both :p. But happy to keep iterating.

RembrandtK · 2025-06-19T10:43:52Z

packages/horizon/contracts/libraries/LinkedList.sol

I have not looked where used, however we would normally avoid creating something like this? Would an OpenZeppelin EnumerableSet or similar be an alternative?

Sorry not sure I follow here. Avoid creating what?

A linked list implementation.

Ah got it. So back then we looked at the available options and there was nothing that was exactly what we needed. I don't remember top of my head what exactly was missing from OZ's EnumerableSet implementation but it didn't quite fit the bill.

RembrandtK · 2025-06-19T10:48:03Z

packages/horizon/hardhat.config.ts


 import { hardhatBaseConfig, isProjectBuilt, loadTasks } from '@graphprotocol/toolshed/hardhat'
 import type { HardhatUserConfig } from 'hardhat/types'

-// Skip importing hardhat-graph-protocol when building the project, it has circular dependency
+// Some tasks need compiled artifacts to run so we avoid loading them when building the project


I would try to change package structure to remove the dependency. I suspect runtime loading can be eliminated, and it is better to eliminate.

Do you mean with child packages?

RembrandtK · 2025-06-19T10:51:35Z

packages/horizon/remappings.txt

@@ -2,4 +2,5 @@
 @openzeppelin/contracts-upgradeable/=node_modules/@openzeppelin/contracts-upgradeable/
 @openzeppelin/foundry-upgrades/=node_modules/@openzeppelin/foundry-upgrades/src/
 @graphprotocol/contracts/=node_modules/@graphprotocol/contracts/


FYI locally I have an updated natspect config that eliminates need for this file.
If you have it, remapping @graphprotocol/=node_modules/@graphprotocol/ also works and covers them all, unless you have references to the package it is in that form, which I think is generally not the case for self references. Likewise a shorter pattern for others.

Does it also work for forge commands? Even if the natspec config doesnt need it forge probably does no?

Maybe, I will need to check. I think foundry.toml handles that better?

Both foundry.toml and remappings.txt should be the same, but we need to have the remappings somewhere because they are installed via node modules.
Let's revisit this when we merge with your natspec config, we can probably move any remapping definitions to foundry.toml and delete the remappings.txt file

RembrandtK · 2025-06-19T10:57:00Z

packages/interfaces/README.md

+Note that contracts in the `toolshed/` directory are not meant to be imported by Solidity code, they only exist to
+generate complete TypeScript types.


I noticed them. Not looked at why, I presume to resolve something it was hard to otherwise do?

The reason is interfaces are not 1:1 with contract implementations. Storage variables, dependencies without interface files are some examples of things that add external functions to an implementation but are not usually in the interface files. The toolshed stuff (very bad name btw) is supposed to bridge that gap by adding back anything that's missing.
You should be able to include everything into the actual contract interface that gets imported and usually extended by the implementation, but I didn't want to mess with audited code at this point.

RembrandtK · 2025-06-19T11:04:27Z

packages/interfaces/contracts/toolshed/IRewardsManagerToolshed.sol

General observation, sorry if painful to implement. :) Would a suffix like Type be better than Toolshed? (Make it about the purpose rather than where introduced?)

yes to all. It would be better and slightly painful, but worth it I believe. I'll add it to my to-do list, but need to knock off some higher prio items first.

RembrandtK · 2025-06-19T11:08:25Z

packages/interfaces/hardhat.config.ts

This might be a problem you inherited from the way I set things up initially, if you adopted some of my config: locally my common does not, although it did at one stage, compile locally. The local compilation causes knock-on issues. Does it also contribute to needing for Toolshed interfaces?

Not following 😄

I was at one point doing Hardhat compilation of common (equivalent to interfaces). Not 100% why anymore, many moving parts I was trying to get working, but I came full cycle and concluded it was a mistake. When elminating compilation here it appeared to help resolve subtle downstream issues and fragility. I was able to undo pushing dependencies into root package, remove remappings.txt, and get natspect working nicely. These were layering hacks on top of each other in a fragile way to work around inconsistencies caused by doing compilation in the package. I think, anyway. There were complex interdepencies I might still not have fully understood.

A key reason why it created problems is the conversation we had about the combination of Open Zeppelin contract version and compiler version. The interfaces need to be compiled with the compiler version of the consumer, at interface level there is unresolved (but necessary) ambiguity about which compiler version and which Open Zeppelin contract version is being used. Consuming packages need to resolve that ambiguity through their own settings, and compile correctly according to their own context.

I will add I do not understand the implication of now creating types in interfaces, which I think you are? That might create a dependency on compilation?

If it does, I would check if actually this is the right level to create types at. If not, that in turn adds to the need to have separate child projects from the contracts. The types also might need to be specific to the context compiled in.

I think we can consider types to be compiler agnostic. They only use the ABIs from the compiled artifacts, which shouldn't change much between different compiler versions. Even if a new compiler version added a new feature that would change the ABI significantly, the previous ABI would still be compatible and useful for typings. I think it's unlikely we hit any problems related to this so I wouldn't add extra complexity for now. Also I suspect always compiling with the latest possible compiler would produce ABIs that are universal.

Sounds reasonable. We could generate ABIs without compilation?

Signed-off-by: Tomás Migone <[email protected]>

tmigone added 2 commits June 11, 2025 16:23

chore: move all interfaces to dedicated package

be8d5a1

Signed-off-by: Tomás Migone <[email protected]>

chore: lint

942c1aa

Signed-off-by: Tomás Migone <[email protected]>

tmigone added 2 commits June 11, 2025 17:01

chore: build interface types

f86ffbb

Signed-off-by: Tomás Migone <[email protected]>

fix: import in contracts package

63b8c27

Signed-off-by: Tomás Migone <[email protected]>

tmigone added 2 commits June 12, 2025 10:15

chore: move sdk to own repo, update versions

32ef00c

Signed-off-by: Tomás Migone <[email protected]>

chore: update toolshed to use interfaces package

f8bb3a5

Signed-off-by: Tomás Migone <[email protected]>

tmigone added 13 commits June 12, 2025 14:30

chore: break circular dep in toolshed, fix contracts ethers versions

7f21b0b

Signed-off-by: Tomás Migone <[email protected]>

chore: lint

be1acc5

Signed-off-by: Tomás Migone <[email protected]>

chore: fully implement interfaces for toolshed

8aef58d

Signed-off-by: Tomás Migone <[email protected]>

chore: import hre statically in horizon and subgraph service

0112d1b

Signed-off-by: Tomás Migone <[email protected]>

chore: remove unused files

9c67f29

Signed-off-by: Tomás Migone <[email protected]>

chore: lint packages

495e576

Signed-off-by: Tomás Migone <[email protected]>

fix: remove dead import

71b90c4

Signed-off-by: Tomás Migone <[email protected]>

feat: create address book if doesnt exist when running deploy task

0f7080d

Signed-off-by: Tomás Migone <[email protected]>

fix: add some missing interface details

2bfd7e0

Signed-off-by: Tomás Migone <[email protected]>

fix: couple fixes to make integration tests pass

62eb5aa

Signed-off-by: Tomás Migone <[email protected]>

chore: update subgraph service to work wiht latest interfaces

5f707a8

Signed-off-by: Tomás Migone <[email protected]>

chore: final changes to use interfaces package

2c94da4

Signed-off-by: Tomás Migone <[email protected]>

chore: lint and replace some imports

5ef5fe9

Signed-off-by: Tomás Migone <[email protected]>

tmigone requested review from Maikol and RembrandtK June 17, 2025 15:25

tmigone added 5 commits June 18, 2025 09:53

chore: bump package versions and publish

e75ae57

Signed-off-by: Tomás Migone <[email protected]>

chore: make interfaces package public

039a32e

Signed-off-by: Tomás Migone <[email protected]>

chore: bump package versions

f6c2e1c

Signed-off-by: Tomás Migone <[email protected]>

fix: ensure dist is published for interfaces package

31edeed

Signed-off-by: Tomás Migone <[email protected]>

chore: add missing interfaces and fix npm publishing issues

4f23eea

Signed-off-by: Tomás Migone <[email protected]>

tmigone added 2 commits June 18, 2025 11:23

fix: add ICuration to L2Curation interface

fbe38f9

Signed-off-by: Tomás Migone <[email protected]>

fix: add more stuff missing from interfaces

ed51926

Signed-off-by: Tomás Migone <[email protected]>

RembrandtK reviewed Jun 19, 2025

View reviewed changes

packages/horizon/package.json Outdated Show resolved Hide resolved

RembrandtK reviewed Jun 19, 2025

View reviewed changes

.gitignore Outdated Show resolved Hide resolved

RembrandtK reviewed Jun 19, 2025

View reviewed changes

tmigone added 3 commits June 19, 2025 10:36

chore: ensure workspace dependencies are not pinned to specific versions

b4fe809

Signed-off-by: Tomás Migone <[email protected]>

chore: better gitignore

a8469af

Signed-off-by: Tomás Migone <[email protected]>

chore: fix pnpm lock file

8b7f14c

Signed-off-by: Tomás Migone <[email protected]>

		Note that contracts in the `toolshed/` directory are not meant to be imported by Solidity code, they only exist to
		generate complete TypeScript types.

chore: move all interfaces to dedicated package #1187

Are you sure you want to change the base?

chore: move all interfaces to dedicated package #1187

Conversation

tmigone commented Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

socket-security bot commented Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openzeppelin-code bot commented Jun 11, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

chore: move all interfaces to dedicated package

🚨 Report Summary

Uh oh!

codecov bot commented Jun 11, 2025

Codecov Report

Uh oh!

socket-security bot commented Jun 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

RembrandtK Jun 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

tmigone commented Jun 11, 2025 •

edited

Loading

socket-security bot commented Jun 11, 2025 •

edited

Loading

openzeppelin-code bot commented Jun 11, 2025 •

edited

Loading

socket-security bot commented Jun 12, 2025 •

edited

Loading

RembrandtK Jun 19, 2025 •

edited

Loading