feat: registry coordinator unit test improvements #121

ChaoticWalrus · 2023-12-21T23:49:39Z

Initially marking as a draft since this work is definitely not complete.

note that the `test_createQuorum` test is currently failing because initialization already sets up the max number of quorums something will need to be adjusted here

#116) * docs: update reg coord to include pubkey registration and service manager usage * docs: add service manager to tech docs intro * fix: update table of contents * docs: add docs for BLSSignatureChecker and OperatorStateRetriever * docs: address feedback

* docs: standardize capitalization of Operator since thats what we do everywhere else * docs: add BLSApkRegistry docs * chore: remove old files - i've incorporated all the info from these files into the current docs, so i'm removing them * docs: add wip for IndexRegistry and StakeRegistry, and fix spacing in StakeRegistry * docs: add IndexRegistry docs * docs: Add StakeRegistry * docs: clarify wording

also fix a couple test names

set up one less than the max number of quorums in a single test's setup, rather than the full max number, so that the test will properly allow the creation of a new quorum also fix a typo in a test name

note that the `test_createQuorum` test is currently failing because initialization already sets up the max number of quorums something will need to be adjusted here

also fix a couple test names

set up one less than the max number of quorums in a single test's setup, rather than the full max number, so that the test will properly allow the creation of a new quorum also fix a typo in a test name

…/github.com/Layr-Labs/eigenlayer-middleware into feat-RegistryManager-Unit-Test-Improvements

also improve some formatting + fix some typos, and add a touch more documentation

ChaoticWalrus · 2024-01-03T19:12:26Z

This is looking better now.
TODOs (editing as I add coverage):
testing for ~~updateOperators, updateOperatorsForQuorum, getQuorumBitmapIndicesAtBlockNumber, getQuorumBitmapAtBlockNumberByIndex,_registerOperator, _deregisterOperator,~~ _updateOperator, ~~and _updateOperatorBitmap~~

also clarify somewhat ambiguous wording in the tree file

…p` fnc

This was causing a *lot* of CI failures, including for merge commits With this commit, CI will still check the _latest_ commit for meeting conventions, it just won't run over all commits in a PR This may lead to a few more "unconventional" commits making it through, but the CI should still flag when someone is just not using conventional commits at all, which I think was the original goal.

ChaoticWalrus · 2024-01-04T23:48:02Z

I'm unsure if adding coverage for _updateOperator to these unit tests makes sense, as it is mostly a call to the StakeRegistry / feels a bit more apt as an "integration test".
Going to give a little more thought to the testing of the view functions.
Otherwise this PR is now looking good, I think.

note: this commit also adds a TODO around a currently-failing test. I plan to discuss the correct path forwards here and then push another commit.

…s registered the logic is now more in-line with the logic in the StakeRegistry -- for reference, see: https://github.com/layr-labs/eigenlayer-middleware/blob/ 98f8844/src/StakeRegistry.sol#L297-L299

src/RegistryCoordinator.sol

wadealexc · 2024-01-09T21:40:27Z

test/unit/RegistryCoordinatorUnit.t.sol

+        // TODO: any additional checks here? mostly this just calls the StakeRegistry, so more appropriate for an integration test
+        registryCoordinator.updateOperators(operatorsToUpdate);


Probably worth having a test that checks that if the StakeRegistry returns quorums to remove from the operator, that they're removed?

hmm yeah, I wasn't sure if this made more sense in "integration" tests. right now these tests use a ~~mock~~ harnessed StakeRegistry; I'll see if I can cook a good test up here.

It's definitely getting hit in integration tests, so up to you if you want to leave it out!

test added in 1f1a840
I think this test worked as a minimal version, but I added a more thorough fuzzed one.

wadealexc

Looks clean - a few small requests, but I'm gonna go ahead and approve. Just make sure the latest m2-mainnet is merged in, since it looks like this is a merge rather than a rebase :)

…h other internal fncs

also improve some formatting + fix some typos, and add a touch more documentation

also clarify somewhat ambiguous wording in the tree file

…p` fnc

This was causing a *lot* of CI failures, including for merge commits With this commit, CI will still check the _latest_ commit for meeting conventions, it just won't run over all commits in a PR This may lead to a few more "unconventional" commits making it through, but the CI should still flag when someone is just not using conventional commits at all, which I think was the original goal.

note: this commit also adds a TODO around a currently-failing test. I plan to discuss the correct path forwards here and then push another commit.

…s registered the logic is now more in-line with the logic in the StakeRegistry -- for reference, see: https://github.com/layr-labs/eigenlayer-middleware/blob/ 98f8844/src/StakeRegistry.sol#L297-L299

also improve wording in the 'tree' file

…h other internal fncs

…/github.com/Layr-Labs/eigenlayer-middleware into feat-RegistryManager-Unit-Test-Improvements

* test: refactor and add tests to bitmap unit * test: added tests and using asserts * chore: remove single quote * feat: addNumberToBitmap function * test: tree file and Alexs bitmap fix * test: add back updated stakeRegistry tests * test: config tests setMinimumStake tests addStrategies tests initializeQuorum tests * refactor: using MockAVSDeployer for test file * test: more config tests * refactor: stake registry harness weighting * fix: stake weighting refactor removed overriding weightOfOperator functions in StakeRegistry and fixed broken tests using the helper _setOperatorWeight() * test: refactor and add tests to bitmap unit * test: added tests and using asserts * chore: remove single quote * feat: addNumberToBitmap function * test: tree file and Alexs bitmap fix * test: add back updated stakeRegistry tests * test: config tests setMinimumStake tests addStrategies tests initializeQuorum tests * refactor: using MockAVSDeployer for test file * test: more config tests * refactor: stake registry harness weighting * fix: stake weighting refactor removed overriding weightOfOperator functions in StakeRegistry and fixed broken tests using the helper _setOperatorWeight() * fix: undo change * chore: rebase fixes * test: voteweighing tests * chore: updated eigenlayer-contract ref to m2-mainnet head (#119) * updated eigenlayer-contract ref to m2-mainnet head * fixed test - interface mismatch from previous update * docs: update reg coord to include pubkey registration and service man… (#116) * docs: update reg coord to include pubkey registration and service manager usage * docs: add service manager to tech docs intro * fix: update table of contents * docs: add docs for BLSSignatureChecker and OperatorStateRetriever * docs: address feedback * docs: add documentation for each registry (#125) * docs: standardize capitalization of Operator since thats what we do everywhere else * docs: add BLSApkRegistry docs * chore: remove old files - i've incorporated all the info from these files into the current docs, so i'm removing them * docs: add wip for IndexRegistry and StakeRegistry, and fix spacing in StakeRegistry * docs: add IndexRegistry docs * docs: Add StakeRegistry * docs: clarify wording * test: fix unit tests * feat: registry coordinator unit test improvements (#121) * feat: add tree diagram for RegistryManager * chore: reorder and rename tests * feat: add a couple simple tests note that the `test_createQuorum` test is currently failing because initialization already sets up the max number of quorums something will need to be adjusted here * docs: update reg coord to include pubkey registration and service man… (#116) * docs: update reg coord to include pubkey registration and service manager usage * docs: add service manager to tech docs intro * fix: update table of contents * docs: add docs for BLSSignatureChecker and OperatorStateRetriever * docs: address feedback * docs: add documentation for each registry (#125) * docs: standardize capitalization of Operator since thats what we do everywhere else * docs: add BLSApkRegistry docs * chore: remove old files - i've incorporated all the info from these files into the current docs, so i'm removing them * docs: add wip for IndexRegistry and StakeRegistry, and fix spacing in StakeRegistry * docs: add IndexRegistry docs * docs: Add StakeRegistry * docs: clarify wording * chore: fix tree file name * chore: add a couple post-checks on state also fix a couple test names * chore: fix breaking test set up one less than the max number of quorums in a single test's setup, rather than the full max number, so that the test will properly allow the creation of a new quorum also fix a typo in a test name * feat: add tree diagram for RegistryManager * chore: reorder and rename tests * feat: add a couple simple tests note that the `test_createQuorum` test is currently failing because initialization already sets up the max number of quorums something will need to be adjusted here * chore: fix tree file name * chore: add a couple post-checks on state also fix a couple test names * chore: fix breaking test set up one less than the max number of quorums in a single test's setup, rather than the full max number, so that the test will properly allow the creation of a new quorum also fix a typo in a test name * feat: add a test for partial deregistration also improve some formatting + fix some typos, and add a touch more documentation * feat: expose more internal functions in harnessed contract * feat: add some simple coverage for the internal `_registerOperator` fnc * feat: add test coverage for the internal `deregisterOperator` fnc also clarify somewhat ambiguous wording in the tree file * feat: add simple test coverage for the internal `_updateOperatorBitmap` fnc * chore: remove commitlint job that reviews all commits in PR from CI This was causing a *lot* of CI failures, including for merge commits With this commit, CI will still check the _latest_ commit for meeting conventions, it just won't run over all commits in a PR This may lead to a few more "unconventional" commits making it through, but the CI should still flag when someone is just not using conventional commits at all, which I think was the original goal. * feat: add some coverage for `updateOperators(ForQuorums)` fncs * feat: add testing for `updateOperatorsForQuorum` function * feat: add some coverage for complex view functions note: this commit also adds a TODO around a currently-failing test. I plan to discuss the correct path forwards here and then push another commit. * chore: clarify NatSpec comments * fix: make `getQuorumBitmapIndicesAtBlockNumber` revert if operator was registered the logic is now more in-line with the logic in the StakeRegistry -- for reference, see: https://github.com/layr-labs/eigenlayer-middleware/blob/ 98f8844/src/StakeRegistry.sol#L297-L299 * feat: add simple unit test for `getQuorumBitmapIndicesAtBlockNumber` also improve wording in the 'tree' file * chore: move `_getQuorumBitmapIndexAtBlockNumber` into the section with other internal fncs * chore: remove unnecessary require statement + improve code clarity * fix: correct a compiler error for implicit type conversion * feat: address TODOs in tests * feat: add tree diagram for RegistryManager * chore: fix tree file name * feat: add a test for partial deregistration also improve some formatting + fix some typos, and add a touch more documentation * feat: expose more internal functions in harnessed contract * feat: add some simple coverage for the internal `_registerOperator` fnc * feat: add test coverage for the internal `deregisterOperator` fnc also clarify somewhat ambiguous wording in the tree file * feat: add simple test coverage for the internal `_updateOperatorBitmap` fnc * chore: remove commitlint job that reviews all commits in PR from CI This was causing a *lot* of CI failures, including for merge commits With this commit, CI will still check the _latest_ commit for meeting conventions, it just won't run over all commits in a PR This may lead to a few more "unconventional" commits making it through, but the CI should still flag when someone is just not using conventional commits at all, which I think was the original goal. * feat: add some coverage for `updateOperators(ForQuorums)` fncs * feat: add testing for `updateOperatorsForQuorum` function * feat: add some coverage for complex view functions note: this commit also adds a TODO around a currently-failing test. I plan to discuss the correct path forwards here and then push another commit. * chore: clarify NatSpec comments * fix: make `getQuorumBitmapIndicesAtBlockNumber` revert if operator was registered the logic is now more in-line with the logic in the StakeRegistry -- for reference, see: https://github.com/layr-labs/eigenlayer-middleware/blob/ 98f8844/src/StakeRegistry.sol#L297-L299 * feat: add simple unit test for `getQuorumBitmapIndicesAtBlockNumber` also improve wording in the 'tree' file * chore: move `_getQuorumBitmapIndexAtBlockNumber` into the section with other internal fncs * chore: remove unnecessary require statement + improve code clarity * fix: correct a compiler error for implicit type conversion * feat: address TODOs in tests --------- Co-authored-by: Alex <[email protected]> * Test: bitmap utils unit tests (#101) * test: refactor and add tests to bitmap unit * test: added tests and using asserts * chore: remove single quote * feat: addNumberToBitmap function * chore: remove unused bitmap functions * feat: addNumberToBitmap function * test: tree file and Alexs bitmap fix * test: add back updated stakeRegistry tests * test: config tests setMinimumStake tests addStrategies tests initializeQuorum tests * refactor: using MockAVSDeployer for test file * test: more config tests * refactor: stake registry harness weighting * fix: stake weighting refactor removed overriding weightOfOperator functions in StakeRegistry and fixed broken tests using the helper _setOperatorWeight() * feat: addNumberToBitmap function * test: add back updated stakeRegistry tests * test: config tests setMinimumStake tests addStrategies tests initializeQuorum tests * refactor: using MockAVSDeployer for test file * test: more config tests * refactor: stake registry harness weighting * fix: stake weighting refactor removed overriding weightOfOperator functions in StakeRegistry and fixed broken tests using the helper _setOperatorWeight() * chore: rebase fixes * fix: undo change * test: voteweighing tests * test: fix unit tests * fix: rebase errors * fix: broken tests from rebase --------- Co-authored-by: Samuel Laferriere <[email protected]> Co-authored-by: Alex <[email protected]> Co-authored-by: ChaoticWalrus <[email protected]>

ChaoticWalrus and others added 5 commits December 20, 2023 13:46

feat: add tree diagram for RegistryManager

2eb591d

chore: reorder and rename tests

0a6d3cd

feat: add a couple simple tests

f9448fa

note that the `test_createQuorum` test is currently failing because initialization already sets up the max number of quorums something will need to be adjusted here

ChaoticWalrus changed the title ~~feat: registry manager unit test improvements~~ feat: registry coordinator unit test improvements Jan 2, 2024

ChaoticWalrus mentioned this pull request Jan 2, 2024

Test: RegistryCoordinator unit tests #91

Closed

4 tasks

ChaoticWalrus added 11 commits January 2, 2024 15:54

chore: fix tree file name

d2f6b63

chore: add a couple post-checks on state

ba24f6e

also fix a couple test names

chore: fix breaking test

0bbd089

set up one less than the max number of quorums in a single test's setup, rather than the full max number, so that the test will properly allow the creation of a new quorum also fix a typo in a test name

feat: add tree diagram for RegistryManager

fb36bff

chore: reorder and rename tests

007cc92

feat: add a couple simple tests

891f76d

note that the `test_createQuorum` test is currently failing because initialization already sets up the max number of quorums something will need to be adjusted here

chore: fix tree file name

bf3bf38

chore: add a couple post-checks on state

eb9a8da

also fix a couple test names

chore: fix breaking test

f4f9cf1

set up one less than the max number of quorums in a single test's setup, rather than the full max number, so that the test will properly allow the creation of a new quorum also fix a typo in a test name

Merge branch 'feat-RegistryManager-Unit-Test-Improvements' of https:/…

ffefd02

…/github.com/Layr-Labs/eigenlayer-middleware into feat-RegistryManager-Unit-Test-Improvements

feat: add a test for partial deregistration

798c461

also improve some formatting + fix some typos, and add a touch more documentation

ChaoticWalrus added 7 commits January 3, 2024 15:13

feat: expose more internal functions in harnessed contract

42e6838

feat: add some simple coverage for the internal _registerOperator fnc

3e95552

feat: add test coverage for the internal deregisterOperator fnc

83c5aa6

also clarify somewhat ambiguous wording in the tree file

feat: add simple test coverage for the internal `_updateOperatorBitma…

e43114d

…p` fnc

feat: add some coverage for updateOperators(ForQuorums) fncs

500d7ba

feat: add testing for updateOperatorsForQuorum function

5e5115f

ChaoticWalrus added 3 commits January 8, 2024 13:11

feat: add some coverage for complex view functions

814855c

note: this commit also adds a TODO around a currently-failing test. I plan to discuss the correct path forwards here and then push another commit.

chore: clarify NatSpec comments

8882f7c

fix: make getQuorumBitmapIndicesAtBlockNumber revert if operator wa…

26565d6

…s registered the logic is now more in-line with the logic in the StakeRegistry -- for reference, see: https://github.com/layr-labs/eigenlayer-middleware/blob/ 98f8844/src/StakeRegistry.sol#L297-L299

wadealexc reviewed Jan 9, 2024

View reviewed changes

src/RegistryCoordinator.sol Outdated Show resolved Hide resolved

wadealexc reviewed Jan 9, 2024

View reviewed changes

src/RegistryCoordinator.sol Outdated Show resolved Hide resolved

wadealexc reviewed Jan 9, 2024

View reviewed changes

wadealexc approved these changes Jan 9, 2024

View reviewed changes

ChaoticWalrus added 23 commits January 9, 2024 13:47

chore: move _getQuorumBitmapIndexAtBlockNumber into the section wit…

ab44a82

…h other internal fncs

chore: remove unnecessary require statement + improve code clarity

8cd21f4

fix: correct a compiler error for implicit type conversion

278b660

feat: address TODOs in tests

1f1a840

feat: add tree diagram for RegistryManager

22fb866

chore: fix tree file name

fd56b76

feat: add a test for partial deregistration

6b30068

also improve some formatting + fix some typos, and add a touch more documentation

feat: expose more internal functions in harnessed contract

21a4ad2

feat: add some simple coverage for the internal _registerOperator fnc

2fd5089

feat: add test coverage for the internal deregisterOperator fnc

9b1eabf

also clarify somewhat ambiguous wording in the tree file

feat: add simple test coverage for the internal `_updateOperatorBitma…

98dd6a7

…p` fnc

feat: add some coverage for updateOperators(ForQuorums) fncs

c899a38

feat: add testing for updateOperatorsForQuorum function

ada8b4c

feat: add some coverage for complex view functions

97e942a

note: this commit also adds a TODO around a currently-failing test. I plan to discuss the correct path forwards here and then push another commit.

chore: clarify NatSpec comments

4b7de19

fix: make getQuorumBitmapIndicesAtBlockNumber revert if operator wa…

23c7895

…s registered the logic is now more in-line with the logic in the StakeRegistry -- for reference, see: https://github.com/layr-labs/eigenlayer-middleware/blob/ 98f8844/src/StakeRegistry.sol#L297-L299

feat: add simple unit test for getQuorumBitmapIndicesAtBlockNumber

8f931ba

also improve wording in the 'tree' file

chore: move _getQuorumBitmapIndexAtBlockNumber into the section wit…

5f18583

…h other internal fncs

chore: remove unnecessary require statement + improve code clarity

9f965c1

fix: correct a compiler error for implicit type conversion

9a328e5

feat: address TODOs in tests

f7244a2

Merge branch 'feat-RegistryManager-Unit-Test-Improvements' of https:/…

be57485

…/github.com/Layr-Labs/eigenlayer-middleware into feat-RegistryManager-Unit-Test-Improvements

ChaoticWalrus merged commit fb6864b into m2-mainnet Jan 9, 2024
3 checks passed

ChaoticWalrus deleted the feat-RegistryManager-Unit-Test-Improvements branch January 9, 2024 23:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: registry coordinator unit test improvements #121

feat: registry coordinator unit test improvements #121

ChaoticWalrus commented Dec 21, 2023

ChaoticWalrus commented Jan 3, 2024 •

edited

Loading

ChaoticWalrus commented Jan 4, 2024

wadealexc Jan 9, 2024

ChaoticWalrus Jan 9, 2024 •

edited

Loading

wadealexc Jan 9, 2024

ChaoticWalrus Jan 9, 2024

wadealexc left a comment

		// TODO: any additional checks here? mostly this just calls the StakeRegistry, so more appropriate for an integration test
		registryCoordinator.updateOperators(operatorsToUpdate);

feat: registry coordinator unit test improvements #121

feat: registry coordinator unit test improvements #121

Conversation

ChaoticWalrus commented Dec 21, 2023

ChaoticWalrus commented Jan 3, 2024 • edited Loading

ChaoticWalrus commented Jan 4, 2024

wadealexc Jan 9, 2024

Choose a reason for hiding this comment

ChaoticWalrus Jan 9, 2024 • edited Loading

Choose a reason for hiding this comment

wadealexc Jan 9, 2024

Choose a reason for hiding this comment

ChaoticWalrus Jan 9, 2024

Choose a reason for hiding this comment

wadealexc left a comment

Choose a reason for hiding this comment

ChaoticWalrus commented Jan 3, 2024 •

edited

Loading

ChaoticWalrus Jan 9, 2024 •

edited

Loading