-
Notifications
You must be signed in to change notification settings - Fork 90
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
app: replace manifest with lock #562
Conversation
} | ||
|
||
func writeLock(_ clusterConfig, _ []tbls.TSS, _ []p2p.Peer) error { | ||
// TODO(corver): Create lock file. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Codecov Report
@@ Coverage Diff @@
## main #562 +/- ##
==========================================
- Coverage 55.19% 54.56% -0.63%
==========================================
Files 92 90 -2
Lines 8664 8523 -141
==========================================
- Hits 4782 4651 -131
Misses 3241 3241
+ Partials 641 631 -10
Continue to review full report at Codecov.
|
t.Helper() | ||
|
||
r, err := p2p.DecodeENR(record) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest a more descriptive name than 'r'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think short names for are fine is the scope is super small
|
||
for _, p := range manifest.Peers { | ||
enrStr, err := p2p.EncodeENR(p.ENR) | ||
for _, o := range lock.Operators { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps operator instead of "o"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think short names for are fine is the scope is super small
} | ||
|
||
// PublicShare returns a peer's threshold BLS public share. | ||
func (v DistValidator) PublicShare(peerIdx int) (*bls_sig.PublicKey, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we have this method perhaps we don't need PubShares to be public
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it is a DTO/value so fields are public
404754a
to
d9e6558
Compare
if err != nil { | ||
return err | ||
} | ||
|
||
pubkey, err := tblsconv.KeyToETH2(dv.PublicKey()) | ||
pk, err := tblsconv.KeyToETH2(pubkey) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should colocate this pk
. code may be confusing because there are two pubkeys: pubkey
and pk
.
pk, err := tblsconv.KeyToETH2(pubkey)
if err != nil {
return err
}
pubkeys = append(pubkeys, pk)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
think about what makes them different and name them with longer identifiers that represent that
Replaces the manifest with cluster lock.
category: refactor
ticket: #527