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

cluster/state: implement legacy lock mutation #2175

Merged
merged 3 commits into from
May 11, 2023
Merged

Conversation

corverroos
Copy link
Contributor

@corverroos corverroos commented May 10, 2023

Implement the first skeleton and structure of the LegacyLock mutation. This is heavy based on previous PoC.

This is the first of a series of PRs, next PRs will add tests and more logic.

category: feature
ticket: #1886

@codecov
Copy link

codecov bot commented May 10, 2023

Codecov Report

Patch coverage has no change and project coverage change: +0.09 🎉

Comparison is base (7903454) 53.45% compared to head (b279606) 53.54%.

❗ Current head b279606 differs from pull request most recent head fafabf0. Consider uploading reports for the commit fafabf0 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2175      +/-   ##
==========================================
+ Coverage   53.45%   53.54%   +0.09%     
==========================================
  Files         174      174              
  Lines       22987    23080      +93     
==========================================
+ Hits        12287    12359      +72     
- Misses       9232     9248      +16     
- Partials     1468     1473       +5     

see 10 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

defer ssz.DefaultHasherPool.Put(hw)

if err := hasher.HashTreeRootWith(hw); err != nil {
return [32]byte{}, err
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: needs errors.Wrap()

)

var mutationDefs = map[MutationType]struct {
DataType any
Copy link
Contributor

Choose a reason for hiding this comment

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

why this DataType is any?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, removed this field, not needed I think.

return resp, nil
}

// verifyEmptySig verifies that the signed mutation isn't signed.
Copy link
Contributor

Choose a reason for hiding this comment

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

nit

Suggested change
// verifyEmptySig verifies that the signed mutation isn't signed.
// verifyEmptySig returns an error if the signed mutation isn't signed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

your change flips the logic which isn't correct.

// Parent is the hash of the parent mutation.
Parent [32]byte `ssz:"Bytes32"`
// Type is the type of mutation.
Type MutationType `ssz:"ByteList[64]"` // TODO(corver): Make this a numbered enum maybe (instead of a string)?.
Copy link
Contributor

Choose a reason for hiding this comment

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

yeah, numbered enum makes sense. What benefit does having string MutationType provides?..

Copy link
Contributor Author

@corverroos corverroos May 11, 2023

Choose a reason for hiding this comment

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

string makes it human readable in the json, otherwise it will be a number in the json which means when a human looks at it, they will not know what mutation type they are looking at.

@corverroos corverroos added the merge when ready Indicates bulldozer bot may merge when all checks pass label May 11, 2023
@obol-bulldozer obol-bulldozer bot merged commit d4da29d into main May 11, 2023
@obol-bulldozer obol-bulldozer bot deleted the corver/state1 branch May 11, 2023 07:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merge when ready Indicates bulldozer bot may merge when all checks pass
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants