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

IndexRegistry Tests #242

Merged
merged 1 commit into from
Oct 16, 2023
Merged

IndexRegistry Tests #242

merged 1 commit into from
Oct 16, 2023

Conversation

ypatil12
Copy link
Collaborator

  • Adds unit/fuzz tests for registration, reregistration, and getters
  • Remove stale GlobalIndexUpdate event

In terms of readability, we overload the OperatorIndexUpdate struct such that the index represents different values in different contexts. Might be a good idea to separate this struct out into two for operator index updates and quorum updates.

@ypatil12 ypatil12 self-assigned this Oct 13, 2023
@ChaoticWalrus ChaoticWalrus deleted the feat/indexRegistryTests branch October 13, 2023 21:42
@ChaoticWalrus ChaoticWalrus restored the feat/indexRegistryTests branch October 13, 2023 21:46
@ChaoticWalrus ChaoticWalrus reopened this Oct 13, 2023
Base automatically changed from multiquorums to master October 13, 2023 23:42
@stevennevins
Copy link
Contributor

Yeah nice call out on the struct, do you think it should be renamed to something more generic if it is going to stay overloaded?

@ypatil12
Copy link
Collaborator Author

Yeah nice call out on the struct, do you think it should be renamed to something more generic if it is going to stay overloaded?

My preference is to separate the struct out into two, as long as there is not a large number of breaking changes off-chain. cc @gpsanant @stevennevins

@stevennevins
Copy link
Contributor

Yeah nice call out on the struct, do you think it should be renamed to something more generic if it is going to stay overloaded?

My preference is to separate the struct out into two, as long as there is not a large number of breaking changes off-chain. cc @gpsanant @stevennevins

I would defer to what @gpsanant thinks here since he's more aware of what dependencies exist there atm

@ChaoticWalrus
Copy link
Contributor

My preference is to separate the struct out into two, as long as there is not a large number of breaking changes off-chain. cc @gpsanant @stevennevins

I like this suggestion, and had also previously called this usage out as inconsistent. There's notes in the comments but having different structs would probably be clearer.
I'd suggest making this as a separate PR on top of this PR -- you can try implementing quickly + see what it breaks in the on-chain code. IDK about downstream/off-chain dependencies, but if it's a separate PR then we at least have the option hold off on merging just that change.

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.

Looks really good! Nice work.

@ypatil12 ypatil12 merged commit 668b845 into master Oct 16, 2023
10 of 12 checks passed
@ypatil12 ypatil12 deleted the feat/indexRegistryTests branch October 16, 2023 22:04
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