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

group: removes order method from group interface #356

Merged
merged 1 commit into from
Jan 23, 2024

Conversation

armfazh
Copy link
Contributor

@armfazh armfazh commented Jul 29, 2022

There is no point in returning a Scalar for Order method.
Also, returning a big.Int is not desirable.
Since the group order is public and well-know for the groups supported, then it's better to remove the method.

Based on top of: #355 (merge that first)
Fixes #214

@armfazh armfazh added the changesAPI PR changes the API of a package label Jul 29, 2022
@armfazh armfazh requested review from bwesterb and chris-wood July 29, 2022 22:14
@armfazh armfazh self-assigned this Jul 29, 2022
Copy link
Contributor

@chris-wood chris-wood left a comment

Choose a reason for hiding this comment

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

Since the group order is public and well-know for the groups supported, then it's better to remove the method.

This is precisely why we want to keep the function on the Group interface. Applications should not need to bake this into their code, possibly getting it wrong. (I could see them confusing the underlying field order with the group's order, for example.) They should just call the Order function on the desired group's interface.

@bwesterb
Copy link
Member

bwesterb commented Aug 1, 2022

They should just call the Order function on the desired group's interface.

The current Order function is broken because it uses Scalar.

I think it's bad to return a big.Int.

Let's step back: for what reasons would the user need the order and can we provide functions directly for those use-cases instead?

@chris-wood
Copy link
Contributor

I think it's bad to return a big.Int.

Say more?

Let's step back: for what reasons would the user need the order and can we provide functions directly for those use-cases instead?

The existing use cases we have are for determining scalar validity during deserialization, but that should already be covered by the deserialize functions on the Scalar interface. I don't know of any other uses, but as above, I don't think it's a good idea to punt this to applications should they need to use it for something we've not considered.

@bwesterb
Copy link
Member

bwesterb commented Aug 1, 2022

Say more?

Operations on big.Int are not constant time. We can add it, but then we need to add some very explicit warnings.

@chris-wood
Copy link
Contributor

Operations on big.Int are not constant time. We can add it, but then we need to add some very explicit warnings.

I mean, sure, but I don't see any use cases right now where it would need to be constant time. In any case, since this might just be YAGNI, I would be OK with removing it assuming we don't break anyone.

@bytemare
Copy link

bytemare commented Dec 4, 2022

One use case: some app doesn't implement itself any group, and uses this package, but reimplements hash-to-scalar. The app doesn't have all the parameters of the group it handles, only abstractions, so it's useful for that app to get the order from the interface.

@bwesterb
Copy link
Member

bwesterb commented Jan 3, 2024

What is your preference on how to proceed @armfazh ?

@armfazh
Copy link
Contributor Author

armfazh commented Jan 23, 2024

What is your preference on how to proceed @armfazh ?

I have updated this PR to remove the Order method from the Group interface.

@armfazh armfazh merged commit 3cde596 into cloudflare:main Jan 23, 2024
10 checks passed
@armfazh armfazh deleted the removeOrder branch January 23, 2024 01:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changesAPI PR changes the API of a package
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Group Order() function signature oddity
4 participants