search

cloudflare / circl

CIRCL: Cloudflare Interoperable Reusable Cryptographic Library
http://blog.cloudflare.com/introducing-circl
Other
1.23k stars 136 forks source link

group: removes order method from group interface #356

Closed armfazh closed 6 months ago

armfazh commented 1 year ago

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

bwesterb commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 6 months ago

What is your preference on how to proceed @armfazh ?

armfazh commented 6 months ago

What is your preference on how to proceed @armfazh ?

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