feat: fetch addresses from registry #12000
Conversation
This stack of pull requests is managed by Graphite. Learn more about stacking. |
just-mitch
force-pushed
the
02-13-feat_fetch_addresses_from_registry
branch
from
50eaaf6 to
3dbe043
Compare
just-mitch
force-pushed
the
02-13-feat_fetch_addresses_from_registry
branch
8 times, most recently
from
0093784 to
c64b04d
Compare
|
You may want to update |
|
+1. Will stack! |
just-mitch
force-pushed
the
02-13-feat_fetch_addresses_from_registry
branch
2 times, most recently
from
c9994ec to
ab4338d
Compare
There was a problem hiding this comment.
I would prefer not having the slash-factory as part of the rollup, as it:
- is essentially decided off-chain by sequencers
- is not fixed, and putting it in there give it the feel that it don't change
Really the rollup don't need to know about its existence, so I would prefer it not knowing. It is one of those things that make more sense to me that we either provide, or have provided as part of the slasher_client being used.
just-mitch
force-pushed
the
02-13-feat_fetch_addresses_from_registry
branch
6 times, most recently
from
379759b to
35ab869
Compare
just-mitch
force-pushed
the
02-13-feat_fetch_addresses_from_registry
branch
2 times, most recently
from
8bddbcb to
c1a2bd1
Compare
| const governanceAddresses = await registry.getGovernanceAddresses(); | ||
| const rollupAddress = await registry.getRollupAddress(rollupVersion); | ||
|
|
||
| if (rollupAddress === undefined) { |
| }; | ||
| } | ||
|
|
||
| public static async collectAddressesSafe( |
There was a problem hiding this comment.
The difference between these two is that one does not throw? im not sure why we need to duplicate the whole method for this
There was a problem hiding this comment.
The safe name as written feels like a footgun that the return type will have undefined items in it on top of not throwing
There was a problem hiding this comment.
PTSD thinking of "safeTransfer" in ERC721
There was a problem hiding this comment.
I was going for https://zod.dev/?id=safeparse
But we only use this in the test, so I will remove.
| ); | ||
| const newVersionSalt = originalVersionSalt + 1; | ||
|
|
||
| const { rollup: newRollup, payloadAddress } = await deployRollupAndPeriphery( |
There was a problem hiding this comment.
periphery is vague,
its not clear to me that its deploying a governance payload and slash factory
There was a problem hiding this comment.
What is the reasong that it is not just doing the "usual" deployment, and then deploying the extra components separate? Is it also to be used in the cli. Otherwise it seems like a bit of an overkill to ingrain it all the way in there.
Beyond that, package wise, should we be pulling in all the deployment stuff for using contracts as well? I mean things like deploying the contracts etc is not needed for the sequencer or any of the others nodes but won't they end up loading some of it anyway.
There was a problem hiding this comment.
I'm a little confused on the first point- we do a usual deployment in the beforeAll? Or are you saying "usual" here just for deploying the rollup contract and then have separate functions for deploying the rollup's slash factory and upgrade proposal? If so, then yes this function is a convenience wrapper over some utilities that do just that.
For the second one, I agree, but since the deploy stuff is already in @aztec/ethereum alongside these contract utility classes, the sequencer is already pulling all of it in.
There was a problem hiding this comment.
But also, yes, this is used in the CLI as part of #12127
There was a problem hiding this comment.
I like the changes more this way, but not sure about how deep we should have the deployments that are mainly testing inside the ethereum package - but this is hypocritical as we already throw it all the way in.
Added few comments, some not related to changes here but just observations.
| @@ -247,6 +255,144 @@ export function createL1Clients( | |||
| return { walletClient, publicClient } as L1Clients; | |||
| } | |||
|
|
|||
| export const deployRollupAndPeriphery = async ( | |||
There was a problem hiding this comment.
As @Maddiaa0 mentioned earlier this one can be a bit confusing to follow, as it is not clear what periphery is here. For example, the registry could be seen as periphery to the contracts as well. Also it seems like primarily something you would be using for a test here, so less clear to me why it would be lal the way in here? (Guess this is true all the deployments though 🤷🤔)
There was a problem hiding this comment.
Yeah agreed, it's not a great name. Will stack to update, since it does more stuff in one of the higher PRs.
| return payloadAddress; | ||
| }; | ||
|
|
||
| export const deployRollup = async ( |
There was a problem hiding this comment.
More of a comment related to actual production usage of some of this, but right now the owner would be the deployer, and not end up being the governance.
|
|
||
| const txHashes: Hex[] = []; | ||
|
|
||
| if (args.initialValidators && args.initialValidators.length > 0) { |
There was a problem hiding this comment.
Think I would prefer if we could pull this into its own function and make it very clear that it is a big cheat. If already moving it around might as well make the cheating clear.
|
|
||
| const registryAddress = await govDeployer.deploy(l1Artifacts.registry, [account.address.toString()]); | ||
| const registryAddress = await deployer.deploy(l1Artifacts.registry, [account.address.toString()]); |
There was a problem hiding this comment.
All of the things down here seems like stuff that would be simple to batch together into a bigger contract deploying it such that there is not all the fiddling in here. Not needed in this flow, just a comment as related really.
| // Transaction hashes to await | ||
| const txHashes: Hex[] = []; | ||
|
|
||
| { | ||
| if (!(await feeAsset.read.freeForAll())) { |
There was a problem hiding this comment.
For idempotency. If we re-run, and try to transfer again, it reverts because we are not the owner.
| }; | ||
| } | ||
|
|
||
| public static async collectAddressesSafe( |
There was a problem hiding this comment.
PTSD thinking of "safeTransfer" in ERC721
|
|
||
| const snapshot = await this.registry.read.getSnapshot([version]); | ||
| const address = EthAddress.fromString(snapshot.rollup); | ||
| return address.equals(EthAddress.ZERO) ? undefined : address; |
There was a problem hiding this comment.
Personally I would prefer either returning what the function actually returned, or throwing.
| public async getCanonicalAddress(): Promise<EthAddress | undefined> { | ||
| const snapshot = await this.registry.read.getCurrentSnapshot(); | ||
| const address = EthAddress.fromString(snapshot.rollup); | ||
| return address.equals(EthAddress.ZERO) ? undefined : address; |
| ); | ||
| const newVersionSalt = originalVersionSalt + 1; | ||
|
|
||
| const { rollup: newRollup, payloadAddress } = await deployRollupAndPeriphery( |
There was a problem hiding this comment.
What is the reasong that it is not just doing the "usual" deployment, and then deploying the extra components separate? Is it also to be used in the cli. Otherwise it seems like a bit of an overkill to ingrain it all the way in there.
Beyond that, package wise, should we be pulling in all the deployment stuff for using contracts as well? I mean things like deploying the contracts etc is not needed for the sequencer or any of the others nodes but won't they end up loading some of it anyway.
| client: publicClient, | ||
| }); | ||
|
|
||
| const lockAmount = 10000n * 10n ** 18n; |
There was a problem hiding this comment.
If configs are changed this would likely be breaking. Also, if they are the same value, we could reduce the number of magic numbers and not have them both be defined with a constant like this 🫡
just-mitch
force-pushed
the
02-13-feat_fetch_addresses_from_registry
branch
from
c1a2bd1 to
cd46471
Compare
* master: (31 commits) feat: Slack message to ci channel tagging owners on flakes. (#12284) fix: slack notify was broken by quoted commit titles revert: "chore: Fix and reenable fees-settings test (#12302)" fix: run arm64 on master (#12307) yolo fix chore: Fix and reenable fees-settings test (#12302) feat!: rename compute_nullifier_without_context (#12308) chore: Lazy loading artifacts everywhere (#12285) chore: Reenable dapp subscription test (#12304) chore: Run prover test with fake proofs when requested (#12305) chore: Do not set CI_FULL outside CI (#12300) chore: new mnemonic deployments on sepolia (#12076) chore!: enable multiple L1 nodes to be used (#11945) chore: remove no longer supported extension from vscode/extension.json (#12303) fix(e2e): p2p_reqresp (#12297) feat: Sync from noir (#12298) chore: enabling `e2e_contract_updates` in CI + nuking irrelevant test (#12293) feat: prepend based merge (#12093) feat: fetch addresses from registry (#12000) feat: live logs (#12271) ...
Key changes: