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

rename NonSignerStakesAndSignature fields to be more consistent #229

Closed

Conversation

samlaf
Copy link
Contributor

@samlaf samlaf commented Oct 7, 2023

Renamed the fields of NonSignerStakesAndSignature to be more consistent and added a huge comment describing how it's meant to be used above, since it was definitely the trickiest part to understand when writing an avs.

Talked to @gpsanant about this offline. This doesn't need to be merged now (since this would trickle down to a lot of needed offchain code changes), but leaving here as a suggestion, and we can merge this whenever. I personally think it would make this struct easier to understand, but I'm open to changing some of these.

Currently I followed the convention:

    Each field array can either be over signers, nonsigners, or quorums.
    We use the first word of the variable to represent what the array is over
      eg: []nonSignersPubkeysG1 -> array of nonSigners
      eg: []quorumApkG1 -> array of quorums

But we could also potentially use a per... suffix convention

nonSignersPubkeysG1 -> pubkeysG1PerNonSigners
quorumApkG1 -> apkG1PerQuorum

Tests

Some unit tests are failing, but I don't think it's related to my changes.

@samlaf samlaf requested a review from gpsanant October 7, 2023 01:14
Base automatically changed from avs-unstable to multiquorums October 13, 2023 00:04
@ChaoticWalrus ChaoticWalrus deleted the samlaf/rename-NonSignerStakesAndSignature-fields branch October 13, 2023 21:44
@ChaoticWalrus ChaoticWalrus restored the samlaf/rename-NonSignerStakesAndSignature-fields 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
@ChaoticWalrus
Copy link
Contributor

I missed this PR. Looks like a good change @samlaf !
@gpsanant is there a reason we didn't merge this? Seems like a positive move and it'll be much more of a pain to do after deploying things, so if we want to merge we should do it soon.

Noting here that we should migrate this PR over to the dedicated "middleware" repo if we decide to move forward with it.

@ChaoticWalrus
Copy link
Contributor

Closing this out as stale; these contracts no longer live in this repo.

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.

2 participants