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
remove redundant key generation code #1469
Conversation
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.
👌 Thanks for the clean-up
utils/unittest/keys.go
Outdated
sk, err := crypto.GeneratePrivateKey(crypto.BLSBLS12381, seed) | ||
return sk, err | ||
func StakingKey() crypto.PrivateKey { | ||
return BLS12381Key() | ||
} | ||
|
||
func StakingKeys(n int) ([]crypto.PrivateKey, 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.
We can remove the error return here as well
integration/tests/epochs/suite.go
Outdated
stakingAccountKey = unittest.ECDSAP256Key() | ||
networkingKey = unittest.ECDSAP256Key() |
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.
stakingAccountKey = unittest.ECDSAP256Key() | |
networkingKey = unittest.ECDSAP256Key() | |
stakingAccountKey = unittest.ECDSAP256Key() | |
networkingKey = unittest.NetworkingKey() |
Naming suggestion: operatorAccountKey
rather than stakingAccountKey
(for the reasons we've discussed of the confusion between stakingKey
and stakingAccountKey
)
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.
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.
Thanks for tackling the late comments:
- ECDSAKey, BLSKey can be replaced everywhere with
PrivateKeyFixture(crypto.ECDSAP256, crypto.KeyGenSeedMinLenECDSAP256)
,PrivateKeyFixture(crypto.BLSBLS12381, crypto.KeyGenSeedMinLenBLSBLS12381)
. I don't need we need the level of indirection - we manipulate a ton of stakingKey, networkingKey, variables, etc. A way to manage the confusion is to make the function names more clear about what's going on in fixtures:
RandomStakingPrivKey
,RandomNetworkingPrivKey
...
|
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.
Thanks a lot!
bors merge |
Codecov Report
@@ Coverage Diff @@
## master #1469 +/- ##
=======================================
Coverage 55.11% 55.12%
=======================================
Files 517 517
Lines 32334 32330 -4
=======================================
+ Hits 17821 17822 +1
+ Misses 12121 12114 -7
- Partials 2392 2394 +2
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Small PR to remove some redundant key generation code flagged in a separate PR .