-
Notifications
You must be signed in to change notification settings - Fork 10
make some stuff public #24
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
Conversation
b-wagn
left a comment
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 understand that you need to make things public. But the intention of making only certain parts public was to prevent misuse of the library. For users (except the aggregation circuit), signatures and keys should be abstract objects that can only be used as inputs to the functions provided by this library. Users should never look into the internals of these objects. Any idea how to get both? I.e., the same level of protection + make it work for the aggregation? If not, it is ok to merge your PR.
|
Yes I agree with the comment, now if things are public, it's very tempting to access and modify them in the wrong way for the user when he shouldn't. If this is just to be able to access them and not to modify them (which I think I understand), then you can just have some small functions with the same name as the given field and which return a reference to the said field, that is a common practice to avoid having public visibility for the fields directly. |
…ompletely in GeneralizedXMSSSignature / GeneralizedXMSSPublicKey
|
Agree with both of you, suggested changes implemented. Is it an issue to make the |
tcoratger
left a comment
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.
Looks good to me
b-wagn
left a comment
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.
LGTM. Can be merged from my perspective :)
Goal: make this work.