feat(horizon): deploy horizon with Hardhat Ignition #1025

tmigone · 2024-09-10T19:56:23Z

No description provided.

Signed-off-by: Tomás Migone <tomas@edgeandnode.com>

openzeppelin-code · 2024-09-12T20:58:46Z

feat(horizon): deploy horizon with Hardhat Ignition

Generated at commit: b4a5fe46abf6db9b7d4fb3f9773b24bd53f9a1ee

🚨 Report Summary

	Severity Level	Results
Contracts	Critical High Medium Low Note Total	2 4 0 15 41 62
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 <tomas@edgeandnode.com>

alcuadrado

This is looking really good, @tmigone! While I didn't spend enough time to understand the architecture of Horizon, I looked for unsage patterns and the structure of the modules, and it does look like what we expect to see in a project this size.

If left a few comments, including a few questions about things we'd love to know to see if/how we can make Ignition better.

alcuadrado · 2024-09-16T20:46:18Z

packages/horizon/ignition/configs/graph.hardhat.json

@@ -0,0 +1,45 @@
+{
+  "GraphProxyAdmin": {
+    "governor": "0xFFcf8FDEE72ac11b5c542428B35EEF5769C409f0"


This is an example of a repeated value that could have been global, right?

I think we could add something like:

"$global": { "governor": "...." }

to this same json file, and then during execution we first do a lookup by module, and if not present in $global.

Yes exactly!

alcuadrado · 2024-09-16T20:48:42Z

packages/horizon/ignition/modules/core/GraphPayments.ts

+  const GraphPaymentsImplementation = m.contract('GraphPayments',
+    [Controller, protocolPaymentCut],
+    {
+      after: [PeripheryRegistered, HorizonRegistered],


I think this could be (after a small change in Ignition)

Suggested change

after: [PeripheryRegistered, HorizonRegistered],

after: [GraphPeripheryModule, HorizonProxiesModule],

That would be sweet 🙏

alcuadrado · 2024-09-16T20:49:36Z

packages/horizon/scripts/deploy.ts

@@ -0,0 +1,16 @@
+import { ignition } from 'hardhat'


I'm curious, what do you use this script for? How is it different from directly calling the ignition task?

I think I initially added this because one of the contract calls during setup was reverting and I wanted to log some stuff, so I should probably remove it.

But now that I think of it, there is one potential use case where it might be handy. For our products and internal tooling we use a contract address book with a specific format which is different from the one ignition outputs. Changing everything to support ignition's format seems more complicated than just adding a post-deployment processing step to convert between formats. Though ... perhaps it's best to keep that post processing on an isolated hardhat task and run ignition through CLI. What do you think?

I think that's a valid use case, especially for existing systems like yours that are adopting Ignition.

We could have a subtask that runs after a successful Ignition deployment, where you can run your data transformation. WDYT?

Something like:

subtask(ON_IGNITION_DEPLOYMENT, ({rusults}) => { // save them in your format });

The default implementation would be just a no-op.

/cc @zoeyTM

yeah that sounds great

alcuadrado · 2024-09-16T20:52:40Z

packages/horizon/ignition/modules/proxy/TransparentUpgradeableProxy.ts

+// Note that this module uses a dummy contract as the implementation as the proxy requires a valid contract
+// address to be deployed
+export function deployWithOZProxy(m: IgnitionModuleBuilder, contractName: string) {
+  const deployer = m.getAccount(0)


Nice! We were expecting this pattern: you use functions to create parts of the module and use explicit IDs to disambiguate between calls.

Was this easy to come up with? I guess we can document it better.

I think it was fairly straightforward to realize. Definitely nitpicking but maybe you could mention this pattern in this section of the docs: https://hardhat.org/ignition/docs/guides/creating-modules#future--ids

well now that i re-read it, the existing docs already sort of cover it, so not sure if it's worth further clarifying. Perhaps would be better as an example in the examples repo?

I created an issue, as I think we could have a short guide about this: NomicFoundation/hardhat-ignition#813

alcuadrado · 2024-09-16T20:54:28Z

packages/horizon/ignition/modules/proxy/TransparentUpgradeableProxy.ts

+export function deployWithOZProxy(m: IgnitionModuleBuilder, contractName: string) {
+  const deployer = m.getAccount(0)
+
+  const dummy = m.contract('Dummy', [], { id: `OZProxyDummy_${contractName}` })


Is Dummy in this case more of an EmptyImplementation? is there anything Ignition could do to improve on this? Or do you think it's an OZ issue?

What you could also do is pass the implementation future to this function.

I don't think there is anything Ignition could do better here, it's really a limitation of the OZ proxy not accepting 0x00 as a valid implementation address (or an address without code deployed to it).

Agreed that using the implementation future is cleaner, and in some other instances that's what we are doing (for example for GraphProxy), but in this specific case there is sort of a circular dependency here where the implementation itself needs to know the proxy address before it's deployed, but at the same time the proxy needs the implementation address... Again this is a combination of OZ proxies and some Horizon weirdness (which I hope we can fix in the future).

I see. Happy to help in any way if you want to explore an alternative in the future.

alcuadrado · 2024-09-16T20:56:42Z

packages/horizon/ignition/modules/proxy/GraphProxy.ts

+import GraphProxyAdminModule from '../periphery/GraphProxyAdmin'
+import GraphProxyArtifact from '@graphprotocol/contracts/build/contracts/contracts/upgrades/GraphProxy.sol/GraphProxy.json'
+
+export function deployWithGraphProxy(m: IgnitionModuleBuilder, contract: { name: string, artifact?: Artifact, args?: ArgumentType[] }, options?: ContractOptions) {


Note: I reviewed the OZ version of this helper in more detail as I'm more familiar with it.

alcuadrado · 2024-09-16T21:00:01Z

packages/horizon/ignition/modules/core/HorizonProxies.ts

+  const { Proxy: PaymentsEscrowProxy, ProxyAdmin: PaymentsEscrowProxyAdmin } = deployWithOZProxy(m, 'PaymentsEscrow')
+
+  // Register the proxies in the controller
+  const setProxyHorizonStaking = m.call(Controller, 'setContractProxy', [ethers.keccak256(ethers.toUtf8Bytes('Staking')), HorizonStakingProxy], { id: 'setContractProxy_HorizonStaking' })


I believe this hashing is a custom pattern, is it?

alcuadrado · 2024-09-16T21:02:22Z

packages/horizon/ignition/modules/core/HorizonStaking.ts

+  const acceptCall = m.call(GraphProxyAdmin, 'acceptProxy', [HorizonStakingImplementation, HorizonStakingProxy], { after: [upgradeCall] })
+
+  // Load contract with implementation ABI
+  const HorizonStaking = m.contractAt('HorizonStaking', HorizonStakingProxy, { after: [acceptCall], id: 'HorizonStaking_Instance' })


Is contractAt forcing you to use explicit ids too often? Or are you setting it here for a stylistic preference?

Oh, I guess this happens when you do "deploy implementation", "contractAt implementation". I'll think about how to improve this.

Maybe with something like what you did here: #1025 (comment)

yeah had to set it because of that!

Perhaps Ignition could detect this pattern:

const proxy = m.contract('Proxy') const implementation = m.contract('ContractName') const proxyWithImplementationABI = m.contractAt('ContractName', proxy)

And automatically add something to avoid id collision on the contractAt. But it's also not a big deal I think.

Thanks for confirming, I created an issue to keep track of this: NomicFoundation/hardhat-ignition#814

alcuadrado · 2024-09-16T21:08:43Z

packages/horizon/ignition/modules/periphery/EpochManager.ts

+import { deployWithGraphProxy } from '../proxy/GraphProxy'
+
+import ControllerModule from './Controller'
+import EpochManagerArtifact from '@graphprotocol/contracts/build/contracts/contracts/epochs/EpochManager.sol/EpochManager.json'


I noticed these imports in a few files. Are these npm packages that you had already published? Are these contracts verified already? I'm mostly asking to understand:

Are you loading from node_module or from this monorepo? If the latter, why?

Should we do anything with its build info/sources so that then it can be verified easily?

Good question. So, Horizon project (packages/horizon) relies on some contracts from the current version of the protocol (packages/contracts) which remain mostly unchanged. In order to be able to use modern tooling and make it easier for auditors we decided to spin up a new package (horizon) where we put all the new stuff then eventually the old stuff that's required will be moved to horizon and contracts will be deprecated.

So to answer your questions, anything we are pulling from @graphprotocol/contracts is being pulled from the monorepo package contracts and those are already deployed to production and verified. I don't know what would happen if I tried to verify something from the contracts package from within horizon 🤔 but in our specific case that will never happen as anything we pull from there should already be verified.

Ok, I think we are good for now. But it's an interesting use case. I think we may want to let you provide their build infos. Do you keep track of them?

Not sure I follow! Keep track of what?

alcuadrado · 2024-09-16T21:09:59Z

packages/horizon/ignition/modules/periphery/GraphToken.ts

+    args: [deployer],
+  })
+
+  // TODO: move this mint to a testnet only module


How have you been managing testnet/mainnet deployment differences so far?

I was actually wondering that. So far I have not been managing it at all. Everything so far assumes it's a testnet deployment, I'm curious what patterns you've seen for this?

I can think of two approaches:

Add testnet/mainnet modules with specific config/steps for each environment. Those modules would import the main Horizon module, deploy everything in there then add on top of that.

Deploy with a script instead of CLI and have any logic in there to deal with differences.

Another idea that we had explored was allowing the users to create "chain specific" modules, that would behave different depending on the chainId. Imagine something like this:

const mod = chainSpecificModule("Mod", { 1: (m) => { // mainnet logic }, 31337: (m) => { // hardhat network logic } })

(ignore the horrible syntax)

This could work, especially if all you do are operations inside of those modules. If you need to export contracts, maybe we'd have to force you to export the same contract futures or something like that, so that it plays nicely with typescript.

Do you normally do ops there?

Another pattern would be something like having your main module, and then for testnets you have testnet modules, which useModule(main), and run those ops. This feels closer to extending a class for testing purposes.

The disadvantage there is that is may force you to structure main in a certain way so that you can do those ops.

That last pattern you described is what I had in mind. Chain specific modules would be really helpful though, there are certain operations for which I think it makes a lot of sense (for example, minting initial supply of tokens on a testnet)

Signed-off-by: Tomás Migone <tomas@edgeandnode.com>

socket-security · 2024-09-24T17:46:54Z

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/dotenv@16.4.5	environment, filesystem	`0`	79.1 kB	motdotla

🚮 Removed packages: npm/typescript@4.9.5

View full report↗︎

Signed-off-by: Tomás Migone <tomas@edgeandnode.com>

tmigone added 2 commits September 10, 2024 16:55

feat(horizon): wip ignition deployment

030106e

feat: add ignition modules for horizon contracts

fa65cc4

Signed-off-by: Tomás Migone <tomas@edgeandnode.com>

tmigone marked this pull request as draft September 11, 2024 20:53

feat: add deploy scripts for horizon staking contracts

eb5bfa3

Signed-off-by: Tomás Migone <tomas@edgeandnode.com>

feat: first version of horizon contracts deployed with ignition

bf02979

Signed-off-by: Tomás Migone <tomas@edgeandnode.com>

alcuadrado reviewed Sep 16, 2024

View reviewed changes

tmigone added 3 commits September 17, 2024 17:00

feat: deploy subgraph service using ignition

e2e3bda

Signed-off-by: Tomás Migone <tomas@edgeandnode.com>

fix: remove deployment files for localhost

84c3eab

Signed-off-by: Tomás Migone <tomas@edgeandnode.com>

chore: gitignore localhost deployment files for ignition

4a8cc59

Signed-off-by: Tomás Migone <tomas@edgeandnode.com>

tmigone marked this pull request as ready for review September 17, 2024 20:09

tmigone requested review from Maikol and pcarranzav September 17, 2024 20:09

chore: add deployment files for arbitrum sepolia and sepolia

5b7c80a

Signed-off-by: Tomás Migone <tomas@edgeandnode.com>

This was referenced Sep 23, 2024

Allow creating global parameters NomicFoundation/hardhat-ignition#803

Open

Future after IgnitionModule NomicFoundation/hardhat-ignition#812

Open

tmigone mentioned this pull request Sep 24, 2024

Deployment verification looking for dbg.json files NomicFoundation/hardhat-ignition#817

Open

fix: deployment tweaks, add deployment files to github

3f13548

Signed-off-by: Tomás Migone <tomas@edgeandnode.com>

tmigone added 2 commits September 24, 2024 15:19

fix: use correct network name for arbitrum sepolia

98f2fd4

Signed-off-by: Tomás Migone <tomas@edgeandnode.com>

fix: hardhat build

b4a5fe4

Signed-off-by: Tomás Migone <tomas@edgeandnode.com>

	after: [PeripheryRegistered, HorizonRegistered],
	after: [GraphPeripheryModule, HorizonProxiesModule],

feat(horizon): deploy horizon with Hardhat Ignition #1025

Are you sure you want to change the base?

feat(horizon): deploy horizon with Hardhat Ignition #1025

Conversation

tmigone commented Sep 10, 2024

openzeppelin-code bot commented Sep 12, 2024 • edited Loading

feat(horizon): deploy horizon with Hardhat Ignition

🚨 Report Summary

alcuadrado left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tmigone Sep 17, 2024 • edited Loading

Choose a reason for hiding this comment

alcuadrado Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

socket-security bot commented Sep 24, 2024 • edited Loading

openzeppelin-code bot commented Sep 12, 2024 •

edited

Loading

tmigone Sep 17, 2024 •

edited

Loading

alcuadrado Sep 23, 2024 •

edited

Loading

socket-security bot commented Sep 24, 2024 •

edited

Loading