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

refactor: simplify naming and logic in registries and registry coordinator #43

Merged
merged 9 commits into from
Nov 6, 2023

Conversation

wadealexc
Copy link
Collaborator

@wadealexc wadealexc commented Oct 31, 2023

Motivation:

  • fix word soup variable and function naming
  • take advantage of new quorum existence invariant due to RegistryCoordinator.createQuorum and its associated initializeQuorum methods in each registry
  • registries each keep "histories" of updates. The status quo is that every time anything changes, we push a new history update. this means that when updates happen multiple times in the same block, the history contains multiple updates from the same block number, making it harder to determine what the canonical state at a given block number is.

This PR:

  • Replaces logic that handles "0-length histories" with simple checks, since we now have an invariant that initialized quorums have nonzero histories (see any registry's initializeQuorum method)
  • changes registry histories to update their "most recent entry" rather than pushing a new entry, if the entry was made in the current block
  • Renames state variables in the registry coordinator and registry contracts to be less wordy

For reviewers, this order may work best:

  1. Review IndexRegistry changes in the first commit, which contains the functional changes mentioned above, as well as readability/logical simplification. This is the worst commit to review, sorry! The diff looks intimidating, so I'd recommend opening the new/old IndexRegistry files side by side and comparing like that. 🙏
  2. The rest of the commits are style/refactor commits. I'd recommend skimming the renaming changes below and only dipping into the commits themselves if you feel like it.

Renaming changes:

  • IndexRegistry state vars:
    • operatorIdToIndex -> currentOperatorIndex
    • _totalOperatorsHistory -> _operatorCountHistory
    • _indexToOperatorIdHistory -> _indexHistory
  • IndexRegistry functions:
    • _getTotalOperatorsForQuorumAtBlockNumber -> _operatorCountAtBlockNumber
    • _getOperatorIdAtIndexForQuorumAtBlockNumber -> _operatorIdForIndexAtBlockNumber
    • getOperatorIndexUpdateOfIndexForQuorumAtIndex -> getOperatorUpdateAtIndex
    • getTotalOperatorsForQuorumAtBlockNumberByIndex -> getTotalOperatorsForIndexAtBlockNumber
    • getOperatorListForQuorumAtBlockNumber -> getOperatorListAtBlockNumber
  • StakeRegistry state vars:
    • operatorIdToStakeHistory -> operatorStakeHistory
    • strategiesConsideredAndMultipliers -> strategyParams
  • StakeRegistry structs:
    • StrategyAndWeightingMultiplier -> StrategyParams
  • StakeRegistry functions:
    • strategiesConsideredAndMultipliersLength -> strategyParamsLength
    • strategyAndWeightingMultiplierForQuorumByIndex -> strategyParamsByIndex
    • getOperatorIdToStakeHistory -> getOperatorStakeHistory
    • getLengthOfTotalStakeHistoryForQuorum -> getTotalStakeHistoryLength
    • getTotalStakeUpdateForQuorumFromIndex -> getTotalStakeUpdateAtIndex
    • getStakeUpdateIndexForOperatorIdForQuorumAtBlockNumber -> getStakeUpdateIndexForOperatorAtBlockNumber
    • getTotalStakeIndicesByQuorumNumbersAtBlockNumber -> getTotalStakeIndicesAtBlockNumber
    • getStakeUpdateForQuorumFromOperatorIdAndIndex -> getStakeUpdateForOperatorAtIndex
    • getStakeForQuorumAtBlockNumberFromOperatorIdAndIndex -> getOperatorStakeAtBlockNumberAndIndex
    • getStakeForOperatorIdForQuorumAtBlockNumber -> getOperatorStakeAtBlockNumber
    • getLengthOfOperatorIdStakeHistoryForQuorum -> getOperatorStakeHistoryLength
  • BLSRegistryCoordinatorWithIndices state vars:
    • _quorumOperatorSetParams -> _quorumParams
    • _operatorIdToQuorumBitmapHistory -> _operatorBitmapHistory
    • _operators -> _operatorInfo
  • BLSRegistryCoordinatorWithIndices state modifying functions:
    • registerOperatorWithCoordinator -> registerOperator
    • registerOperatorWithCoordinator -> registerOperatorWithChurn
    • deregisterOperatorWithCoordinator -> deregisterOperator
    • ejectOperatorFromCoordinator -> ejectOperator
  • BLSRegistryCoordinatorWithIndices view functions:
    • getQuorumBitmapIndicesByOperatorIdsAtBlockNumber -> getQuorumBitmapIndicesAtBlockNumber
    • getQuorumBitmapByOperatorIdAtBlockNumberByIndex -> getQuorumBitmapAtBlockNumberByIndex
    • getQuorumBitmapUpdateByOperatorIdByIndex -> getQuorumBitmapUpdateByIndex
    • getCurrentQuorumBitmapByOperatorId -> getCurrentQuorumBitmap
    • getQuorumBitmapUpdateByOperatorIdLength -> getQuorumBitmapHistoryLength

IndexRegistry changes:

  • Refactored index registry history updates to have a more readable register/deregister flow:
    • registerOperator is now _increaseOperatorCount + _assignOperatorToIndex. Functional changes:
      • If the most recent QuorumUpdate/OperatorUpdate is from this block, we update the last entry rather than pushing a new one.
    • deregisterOperator is now _decreaseOperatorCount + _popLastOperator + _assignOperatorToIndex. Functional changes:
      • As above, histories are updated rather than pushed if the last entry was from the current block.
      • Previously, we assigned currentOperatorIndex[quorum][OPERATOR_DOES_NOT_EXIST_ID] to the removed index, and emitted an event for this update. This logic has been removed, since the currentOperatorIndex of the null id doesn't matter. See previous logic vs new logic

@wadealexc wadealexc requested a review from gpsanant October 31, 2023 21:00
@wadealexc wadealexc changed the title refactor: wip refactor for index and blspubkey registries to simplify… refactor: wip refactor for registries to simplify naming and logic Nov 1, 2023
@wadealexc wadealexc changed the title refactor: wip refactor for registries to simplify naming and logic refactor: simplify naming and logic in registries and registry coordinator Nov 1, 2023
@@ -386,35 +386,26 @@ contract StakeRegistry is StakeRegistryStorage {
return;
}

uint96 prevStake;
// Get our last-recorded stake update
uint256 historyLength = _totalStakeHistory[quorumNumber].length;
Copy link
Contributor

Choose a reason for hiding this comment

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

revert if 0

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I need to add an explicit comment about this, but this method assumes the quorum exists. all calling locations already perform this check, and will revert higher in the stack if it's caught - so we don't necessarily need an explicit check here.

updateStakes doesnt currently check this, but i'm moving that out of this contract anyway.


/// @notice Returns the most recent operator count update for a quorum
/// @dev Reverts if the quorum does not exist (history length == 0)
function _latestQuorumUpdate(uint8 quorumNumber) internal view returns (QuorumUpdate storage) {
Copy link
Contributor

Choose a reason for hiding this comment

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

_getLatestQuorumUpdate? same below?

@wadealexc wadealexc marked this pull request as ready for review November 2, 2023 19:33
fromBlockNumber: uint32(block.number)
}));
function _increaseOperatorCount(uint8 quorumNumber) internal returns (uint32) {
QuorumUpdate storage lastUpdate = _latestQuorumUpdate(quorumNumber);
Copy link
Contributor

Choose a reason for hiding this comment

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

seems unrelated to this PR(?) but I think calling this struct 'QuorumUpdate' is super vague. It should probably be an 'OperatorCountUpdate' or something similar

src/IndexRegistry.sol Outdated Show resolved Hide resolved
src/IndexRegistry.sol Outdated Show resolved Hide resolved
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.

IndexRegistry looking good, really liking the changes to it overall; I added some comments but I don't think anything is big, just nits or discussion points. Will be proceeding with remainder of review.

Comment on lines +198 to +201
_verifyChurnApproverSignature({
registeringOperatorId: operatorIdsToSwap[0],
operatorKickParams: operatorKickParams,
signatureWithSaltAndExpiry: signatureWithSaltAndExpiry
churnApproverSignature: churnApproverSignature
Copy link
Contributor

Choose a reason for hiding this comment

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

should we maybe do this earlier in this function?
seems like a fairly late revert condition, but I guess those are somewhat difficult to avoid in this design overall.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yes :)

i'm doing this in a future PR

Comment on lines +64 to +65
/// @notice maps operator address => operator id and status
mapping(address => Operator) internal _operatorInfo;
Copy link
Contributor

Choose a reason for hiding this comment

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

should we perhaps name the struct 'OperatorInfo' as well?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

+1. Will make this change in a future pr.

}

// set the operatorId to quorum bitmap history
_operatorIdToQuorumBitmapHistory[operatorId].push(QuorumBitmapUpdate({
_operatorBitmapHistory[operatorId].push(QuorumBitmapUpdate({
Copy link
Contributor

Choose a reason for hiding this comment

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

here we are still pushing an update, as opposed to modifying the previous update (if the block number is the same).
seems fine to implement later, just pointing this out

Copy link
Contributor

Choose a reason for hiding this comment

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

I see TODOs elsewhere so only called out this location

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.

Changes LGTM.
Feels fine to merge this as-is; I looked through everything and don't have any substantive issues, mostly just suggestions for further changes.

@wadealexc wadealexc merged commit c5d5dc8 into alex/refactor-quorums Nov 6, 2023
1 check passed
@wadealexc wadealexc deleted the alex/refactor-registries branch November 6, 2023 15:40
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.

3 participants