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

Add augment_plus and augment_minus to PrivateKey #95

Merged
merged 5 commits into from
Oct 25, 2019
Merged

Conversation

coltfred
Copy link
Member

@coltfred coltfred commented Oct 24, 2019

See #94

TODO

  • tests showing proper Fr augmentation.

src/api.rs Show resolved Hide resolved
src/api_480.rs Show resolved Hide resolved
@clintfred
Copy link
Contributor

Did you consider introducing a generic parameter onto PrivateKey to handle the Fp256 vs Fr256. An advantage would be that + and - could be separately defined for PrivateKey<Fp256> vs PrivateKey<Fr256>.
Another potential solution would be to make two separate types.

@coltfred
Copy link
Member Author

@clintfred Previously to hide the underlying FpXXX types from the public api. If we didn't want to do that still we could do what you're suggesting. I would argue that there is limited use of doing that and it just pushes more information onto the caller than they care about.

One could argue that we should remove - and + from the public apis as they have limited value to a caller.

@clintfred
Copy link
Contributor

I agree. I would suggest we remove + and - from PrivateKey until we have a use for it. It seems like it will serve to confuse users, and if we ever needed it again it's simple to add back in.

Unless someone else has a usecase for it that I'm not thinking of...

Copy link
Contributor

@clintfred clintfred left a comment

Choose a reason for hiding this comment

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

LGTM

@coltfred coltfred merged commit 93bd466 into master Oct 25, 2019
@coltfred coltfred deleted the add-augment branch October 25, 2019 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants