-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Cleanup up Multisig naming #2255
Conversation
Codecov Report
@@ Coverage Diff @@
## develop #2255 +/- ##
===========================================
- Coverage 62.42% 60.64% -1.78%
===========================================
Files 215 196 -19
Lines 17664 16239 -1425
===========================================
- Hits 11026 9848 -1178
+ Misses 5726 5535 -191
+ Partials 912 856 -56
|
@@ -10,15 +10,15 @@ import ( | |||
// TODO: Figure out API for others to either add their own pubkey types, or | |||
// to make verify / marshal accept a cdc. | |||
const ( | |||
ThresholdPubkeyAminoRoute = "tendermint/PubkeyMultisigThreshold" | |||
PubKeyMultisigThresholdAminoRoute = "tendermint/PubKeyMultisigThreshold" |
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 we omit the multisig from the varname since it's in the package? Also should we omit the pubkey from the var name since there's no privkey
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.
👍 to both.
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.
Mmmm .... I was hoping to reach parity between the var name and the Amino route. We can't remove Multisig
from the amino route because there's no pkg information there, it needs to be the full tendermint/PubKeyMultisigThreshold
.
We could consider removing Multisig
from the var name, but I think we should keep PubKey for consistency/clarity
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.
👍 🍡
Small nit, for the ba import.
crypto/multisig/multisignature.go
Outdated
@@ -4,20 +4,21 @@ import ( | |||
"errors" | |||
|
|||
"github.com/tendermint/tendermint/crypto" | |||
bA "github.com/tendermint/tendermint/crypto/multisig/bitarray" |
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.
This alias does more harm than good, obscuring what is imported here.
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.
Ok!
@@ -10,15 +10,15 @@ import ( | |||
// TODO: Figure out API for others to either add their own pubkey types, or | |||
// to make verify / marshal accept a cdc. | |||
const ( | |||
ThresholdPubkeyAminoRoute = "tendermint/PubkeyMultisigThreshold" | |||
PubKeyMultisigThresholdAminoRoute = "tendermint/PubKeyMultisigThreshold" |
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.
👍 to both.
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 doing this! I agree with xla's comment about the bA import
This is follow up from #2163.
This PR standardizes the naming used in the multisig package and adds the multisig pubkey to the crypto/encoding/amino codec.
It also moves the compact bit array into it's own package.