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

refactor: pub visibility modifier #95

Closed
wants to merge 7 commits into from
Closed

refactor: pub visibility modifier #95

wants to merge 7 commits into from

Conversation

TalDerei
Copy link
Collaborator

This is required, otherwise the monorepo will complain about some wrapper functions being private associated functions.

Copy link
Contributor

@cronokirby cronokirby left a comment

Choose a reason for hiding this comment

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

from_montgomery_limbs and from_raw_bytes shouldn't be exposed. The former is impossible to use correctly unless you really know the constants and what's going on internally. The latter should be replaced with the _mod_order version that's already exposed.

@TalDerei
Copy link
Collaborator Author

TalDerei commented Mar 3, 2024

I kept to_le_limbs and from_le_limbs exposed. In the monorepo, we call Fq::from_le_limbs(hash.inner) where the input type is [u64::MAX; 4]. Isn't exposing from_le_limbs necessary in this case?

@cronokirby
Copy link
Contributor

We absolutely do not want to allow doing that, instead, we should use the Fq::SENTINEL value

@redshiftzero
Copy link
Member

redshiftzero commented Apr 2, 2024

We realized while modifying poseidon377 to work in embedded environments (ref: penumbra-zone/poseidon377#53) that we'll need Fq::from_montgomery_limbs to be pub in order to instantiate Fq constants. Upon discussion in Discord with @cronokirby it makes sense to add a disclaimer to the Fq::from_montgomery_limb methods that this should not be used unless you are familiar with the internals of the library (done in #98).

@TalDerei
Copy link
Collaborator Author

closing in favor of #98

@TalDerei TalDerei closed this Apr 17, 2024
@TalDerei TalDerei deleted the visibility branch July 1, 2024 21:30
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