Feat: adjust compendium for per-avs deployment #97
Conversation
requires changing a bunch of files to make things work. I had to comment out a few calls in tests (now marked with a `TODO`) to get this past the compiler. Will have to do additional cleanup to get this ready for review.
These changes appear to fix all existing tests. I've done what I can to at least comment specifically where harnessed functions are used, as this pattern could give some false assurance and I'd like to avoid that.
I created this mock while trying to sort out compilation issues and test failures This mock is no longer being used anywhere, so I am deleting it for cleanup
this commit fixes a ton of compiler warnings on build, in particular.
|
OK, tests are now passing once again. |
src/BLSApkRegistry.sol
Outdated
| operator, | ||
| address(this), | ||
| block.chainid, | ||
| "EigenLayer_BN254_Pubkey_Registration" |
There was a problem hiding this comment.
Should probably change the message hash "EigenLayer_BN254_Pubkey_Registration" to something that's more AVS specific, or just remove entirely since there won't be a singleton compendium contract anymore
There was a problem hiding this comment.
hmm, perhaps we can just delete the EigenLayer_ part?
it's overkill to have this at all, but it's another input to help protect from any kind of replay issues, so...I feel like it's not a bad idea to have some string here at least.
There was a problem hiding this comment.
realized I should probably just convert this to use EIP-712 instead; will add that to my TODOs
There was a problem hiding this comment.
addressed as part of #102 -- I think these changes are quite appropriate
…stryCoordinator also make a version of `RegistryCoordinator. registerOperator` that accepts these inputs new function passes these inputs on appropriately TODO: remove old `registerOperator` function and fix associated tests
…ubkey registration inputs also fix tests to use the new method
note: code is currently not compiling due to a stack-too-deep error
error here:
src/RegistryCoordinator.sol:254:69:
|
254 | _deregisterOperator(operatorKickParams[i].operator, quorumNumbers[i:i+1]);
There was a problem hiding this comment.
This looks great -- pulling out from Slack:
My suggestion for adding pubkey registration to the workflow is to:
- add a new method to the reg coord:
registerOperatorAndPubkey - change
ApkRegistry.registerBLSPublicKeyto be called by reg coord only - change
ApkRegistry.registerBLSPublicKeyto accept a param for the operator address rather than using msg.sender.
registerOperatorAndPubkey works just like registerOperator, except it includes the pubkey params needed for the ApkRegistry. It fails if the operator already has a registered pubkey.
see reasoning here #100 (comment) this allows undoing a previous fix for a stack-too-deep error, which... helps make the logic a little clearer / easier to parse here.
NOTE: this changes the IBLSApkRegistry interface to make `registerBLSPublicKey` return a bytes32 object
`pubkeyRegistrationParams` => `params` see discussion here: #100 (comment)
…ation-flow Feat: combined operator registration flow
this commit also moves the definition to the RegistryCoordinator contract and updates the function naming to be clearer
There was a problem hiding this comment.
Since I've approved your other chore/cleanup PR, i'm going to go ahead and approve this one as well - so you can merge em all at once (assuming you're happy)
I reviewed the additional changes to operatorId fetching from the last PR, and as mentioned the cleanup PR looks great!
Thanks for putting these together 😄
…hanges Chore: cleanup for compendium changes
…middleware into feat-adjust-compendium-for-per-avs-deployment
these weren't caught in merging but were causing compiler errors
I previously implemented this fix, then reverted it as not needed. Now upon merging with m2-mainnet, `registerOperatorWithChurn` has an additional input. This is reintroducing the same stack-too-deep error so I am reintroducing the same fix (one less local var)
see suggestion here: #102 (comment) saves ~0.25kb in code size
also reduce memory copying operations (I think, at least) in the `registerOperator` function specifically, only copy the `numOperatorsPerQuorum` returned by the internal `_registerOperator` function
The `operatorId` in this check is already fetched from the `blsApkRegistry` With the other changes, this return data is no longer necessary at all
I believe this change cuts down on the memory copying being done here
Likely not the ideal fix, but it gets the job done for the moment, at least.
`RegistryCoordinatorHarness.sol` -> `RegistryCoordinatorHarness.t.sol`
…-issues fix: address contract code size issues
ChaoticWalrus
deleted the
feat-adjust-compendium-for-per-avs-deployment
branch
|
Summary of key changes ultimately contained in this PR:
|
Using a shared BLSPubkeyCompendium impacts separation of concerns, so we would like to transition to operators registering a public key with each AVS independently.
This means we can absorb the BLSPubkeyCompendium into the BLSApkRegistry (possibly re-re-named to BLSPubkeyRegistry?), which this PR looks to accomplish.
PR initially marked as draft; will request review when I think it's ready.See later updates / I edited this message and crossed out some items.Open items + open questions:
- fix tests (marked a few things with TODOs)- decide on combined 'register operator + register pubkey' flow / function(completed / merged into this PR in #100)- renaming functions / cleaning up interfaces(completed in #102)- settle the message hash for operators -- see discussion here #97 (comment) and here #100 (comment)(completed in #102)- fix contract code size limits issue arising after resolving merge conflicts(completed in #105)