Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Operator<>AVS Mapping Support #73

Closed
wants to merge 5 commits into from

Conversation

ypatil12
Copy link
Contributor

@ypatil12 ypatil12 commented Nov 27, 2023

This draft PR adds functionality to support off-chain indexing for avs and operator info. Specifically, the funcationality enables support for the 5 use cases defined here.

Summary of Changes:

  • Functionality in RegistryCoordinator to call DelegationManager when an operator registers or is deregistered
  • UpdateAVSMetadataURI function in RegistryCoordinator
  • To support indexing of strategies supported by AVSs and restated strategies of operators, we define two functions: getOperatorRestakedStrategies and getRestakeableStrategies. These functions utilize quorum bitmaps of the AVS (new functionality) and operator respectively.

@ypatil12 ypatil12 force-pushed the feat/avs-operator-mapping-m2Mainnet branch from ff851d1 to 066c679 Compare November 27, 2023 15:58
@wadealexc
Copy link
Collaborator

Can you retarget the PR against m2-mainnet?

@@ -39,6 +39,9 @@ abstract contract VoteWeigherBaseStorage is Initializable, IVoteWeigher {
/// @notice The number of quorums that the VoteWeigher is considering.
uint16 public quorumCount;

/// @notice Bitmap of quorums that are being used by the middleware.
uint192 public quorumBitmap;
Copy link
Contributor

Choose a reason for hiding this comment

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

would prefer to just use quorum count here?

bytes memory quorumNumbers = BitmapUtils.bitmapToBytesArray(quorumBitmap);
IStrategy[] memory strategies = new IStrategy[](_getNumStrategiesInBitmap(quorumNumbers));
uint256 index = 0;
for(uint256 i = 0; i < quorumNumbers.length; i++){
Copy link
Contributor

Choose a reason for hiding this comment

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

use quourmCount across the board

for(uint256 i = 0; i < quorumNumbers.length; i++){
StrategyAndWeightingMultiplier[] memory strategiesAndMultipliers = strategiesConsideredAndMultipliers[uint8(quorumNumbers[i])];
for(uint256 j = 0; j < strategiesAndMultipliers.length; j++){
strategies[index] = strategiesAndMultipliers[j].strategy;
Copy link
Contributor

Choose a reason for hiding this comment

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

this might end up having a lot of duplicates. lets try to avoid that


/**
* @notice Return a list of all strategies and corresponding share amounts for which the operator has restaked
* @dev There may strategies for which the operator has restaked on the quorum but has no shares. In this case,
Copy link
Contributor

Choose a reason for hiding this comment

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

}
}

return (strategies, shares);
Copy link
Contributor

Choose a reason for hiding this comment

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

i don't think it makes sense to return shares? let's just assume all stake of the strategy is staked until slashing is live

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, discussed with Bowen and we have a pipeline in the backend for getting operator shares so will remove

@gpsanant gpsanant self-requested a review November 27, 2023 17:53
@ypatil12 ypatil12 changed the base branch from master to m2-mainnet November 27, 2023 18:05
@ypatil12 ypatil12 force-pushed the feat/avs-operator-mapping-m2Mainnet branch 2 times, most recently from 1fd2a54 to 3e28473 Compare November 27, 2023 18:12
@ypatil12 ypatil12 force-pushed the feat/avs-operator-mapping-m2Mainnet branch from 3e28473 to b70b2be Compare November 27, 2023 18:27
Comment on lines +205 to +206

quorumBitmap = uint192(quorumBitmap.plus(uint192(1) << quorumNumber));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would prefer we add a helper for this, because it's easy to fuck up. I'm using this in another branch:

/**
* @notice Returns a copy of `bitmap` with `bit` set.
* @dev IMPORTANT: we're dealing with stack values here, so this doesn't modify
* the original bitmap. Using this correctly requires an assignment statement:
* `bitmap = bitmap.setBit(bit);`
*/
function setBit(uint256 bitmap, uint8 bit) internal pure returns (uint256) {
return bitmap | (1 << bit);
}


// If no strategies exist in quorum, turn bit off
if (_strategyParams.length == 0) {
quorumBitmap = uint192(quorumBitmap.minus(uint192(1) << quorumNumber));
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would prefer a helper for this. Could use something similar to the setBit method I linked, but with a bool on/off switch?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or just unsetBit 🤷

Copy link
Collaborator

@wadealexc wadealexc left a comment

Choose a reason for hiding this comment

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

Can we remove the StakeRegistry changes in favor of exposing similar methods in the registry coordinator?

Specifically, I like how currently only one contract keeps track of the current set of initialized quorums, and I want to keep that logic there.

I think if you expose the for loops you're doing in both view StakeRegistry methods, the registry coordinator is a better place to keep this logic.

Copy link
Contributor

@ChaoticWalrus ChaoticWalrus left a comment

Choose a reason for hiding this comment

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

PR seems very reasonable, but would like to get the other PR this relies on just a bit more finalized first.
The new 'view' functions being added to the StakeRegistry seem ~inefficient, but AFAICT these are designed to only ever be called off-chain, and thus seem OK as-is.

@bowenli86
Copy link
Contributor

IIUC, we don't need this PR anymore in middle repo right? @ypatil12

@ChaoticWalrus
Copy link
Contributor

IIUC, we don't need this PR anymore in middle repo right? @ypatil12

We'd still need the parts that integrate with the new changes to the 'core' contracts, but we can delete the added functions in the StakeRegistry + associated interface from this PR.

@bowenli86
Copy link
Contributor

IIUC, we don't need this PR anymore in middle repo right? @ypatil12

We'd still need the parts that integrate with the new changes to the 'core' contracts, but we can delete the added functions in the StakeRegistry + associated interface from this PR.

ok, will work on a new pr for it

@@ -19,7 +19,8 @@ import "src/StakeRegistryStorage.sol";
* @author Layr Labs, Inc.
*/
contract StakeRegistry is StakeRegistryStorage {

using BitmapUtils for *;
Copy link
Contributor

Choose a reason for hiding this comment

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

hm. why not just uint256

Copy link
Collaborator

Choose a reason for hiding this comment

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

doesn't make a difference to the bytecode 🤷

@ypatil12
Copy link
Contributor Author

ypatil12 commented Dec 8, 2023

Closing this PR as functionality is out of scope.

@ypatil12 ypatil12 closed this Dec 8, 2023
Comment on lines +90 to +102
* @notice Registers msg.sender as an operator with the middleware
* @param quorumNumbers are the bytes representing the quorum numbers that the operator is registering for
* @param registrationData is the data that is decoded to get the operator's registration information
* @param signatureWithSaltAndExpiry is the signature of the operator used by the avs to register the operator in the delegation manager
*/
function registerOperatorWithCoordinator(bytes memory quorumNumbers, bytes calldata registrationData, SignatureWithSaltAndExpiry memory signatureWithSaltAndExpiry) external;

/**
* @notice Deregisters the msg.sender as an operator from the middleware
* @param quorumNumbers are the bytes representing the quorum numbers that the operator is registered for
* @param deregistrationData is the the data that is decoded to get the operator's deregistration information
*/
function deregisterOperatorWithCoordinator(bytes calldata quorumNumbers, bytes calldata deregistrationData) external;
Copy link
Contributor

@bowenli86 bowenli86 Dec 8, 2023

Choose a reason for hiding this comment

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

QQ: why these 2 api are not implemented? @ypatil12

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Old methods that were not removed correctly on rebase

Copy link
Collaborator

Choose a reason for hiding this comment

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

they're implemented, just renamed registerOperator and deregisterOperator

@bowenli86
Copy link
Contributor

see https://eigenlabs.slack.com/archives/C05PX94L3H9/p1702240195992199?thread_ts=1702146213.952949&cid=C05PX94L3H9


i really wanna do the middleware pr, but looking at my calendar - i'll be OOO on 14th, and hv a pile of backend work need to finish before then, realistically i won't be able to finish middleware pr then

will be great if @ypatil12 can continue the middleware pr

@ypatil12 ypatil12 deleted the feat/avs-operator-mapping-m2Mainnet branch December 11, 2023 19:50
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.

5 participants