Rename m to threshold and n to shares
#2774
Conversation
fjarri
force-pushed
the
threshold-renaming
branch
from
0faaa9d to
451ec9d
Compare
fjarri
force-pushed
the
threshold-renaming
branch
4 times, most recently
from
cdab398 to
b05d103
Compare
fjarri
force-pushed
the
threshold-renaming
branch
from
b05d103 to
8b37b71
Compare
fjarri
force-pushed
the
threshold-renaming
branch
from
8b37b71 to
3df6463
Compare
fjarri
force-pushed
the
threshold-renaming
branch
from
3df6463 to
a8ab308
Compare
There was a problem hiding this comment.
I like the direction we're going in here, but we can improve on the name num_kfrags. Here's some ideas we've had collectively to consider:
- overhead
- redundancy
- total
- shares
Personally, I think overhead is an excellent choice to continue the API metaphor here (it's intuitive and descriptive) but am also not opposed to keeping n.
|
See also nucypher/rust-umbral#48 |
There was a problem hiding this comment.
Looks good - but requesting addition/update to newsfragment for a removal newsfragment.
I quite like "shares" (and I'm ok with "num_kfrags") as opposed to "overhead". Until now, we've had "n" representing the total number of kfrags, changing that to be "threshold + overhead" seems a bit heavy to me, and would be less natural for existing users. Aside from that, saving users from doing the math (albeit simple math 😄 ) I think is worth it - particularly when you think about how we tend to explain the threshold concept "m-of-n", "threshold out of a number of shares" etc.
| @@ -19,11 +19,11 @@ | |||
| from nucypher.cli import types | |||
|
|
|||
|
|
|||
| class M(PositiveInteger): | |||
| class Threshold(PositiveInteger): | |||
There was a problem hiding this comment.
I think I'll get rid of the Threshold and NumKFrags classes in #2768. PostiveInteger is sufficient.
newsfragments/2774.doc.rst
Outdated
| @@ -0,0 +1 @@ | |||
| Changed the names of `m` and `n` parameters to `threshold` and `num_kfrags`. | |||
There was a problem hiding this comment.
I think either we add an additional removal newsfragment or this might need to be changed to a removal newsfragment, since we are deprecating/modifying public-facing APIs that is potentially currently used:
- Character control: "m" and "n" parameters can no longer be used
- CLI: "--m" and "--n" are no longer valid -> "-m" and "-n"
- Python API: no longer takes "m" and "n" as parameter.
All in favour of threshold and `num_kfrags'
fjarri
force-pushed
the
threshold-renaming
branch
from
46b9ef0 to
339feae
Compare
|
Alright, I think the consensus is to go with "shares". I updated the newsfragments and added a removal one. Waiting for CI to pass. |
fjarri
changed the title
m to threshold and n to num_kfragsm to threshold and n to shares
fjarri
force-pushed
the
threshold-renaming
branch
from
339feae to
e733aae
Compare
There was a problem hiding this comment.
🎸 - minor comment - @fjarri up to you if you want to update the constant in the test.
tests/metrics/grant_availability.py
Outdated
| M: int = 1 | ||
| N: int = 1 | ||
| THRESHOLD: int = 1 | ||
| NUM_KFRAGS: int = 1 |
There was a problem hiding this comment.
Could probably change this to "SHARES", but...only if you want to...
fjarri
force-pushed
the
threshold-renaming
branch
from
e733aae to
6164137
Compare
|
Changed that to |
Based on top of #2773
Type of PR:
Required reviews:
What this does:
Gets rid of vague one-letter names for the threshold and the total number of kfrags.
Trying to gauge the amount of work that needs to be done.