search

paritytech / libsecp256k1

Pure Rust Implementation of secp256k1.
Apache License 2.0
175 stars 84 forks source link

Copy-based API #59

Closed vorot93 closed 2 years ago

vorot93 commented 4 years ago

This PR simplifies the API by using pass by value for what is essentially a number (e.g. SecretKey, PublicKey and their inner component math types). Benches did not yield statistically significant performance difference.

I used unexported macros from core for clean arithmetic ops implementations, they are licensed under Apache-2.0 so we should be fine.

athei commented 2 years ago

@sorpaas is this something we want?

sorpaas commented 2 years ago

It is always said that when you can implement Copy you should, however, for our crypto library, we have a special usage of non-Copy struct -- to track where is the secret. Downstream libraries will want to make sure, for example, that there's always only one single instance of a secret held in memory. Moreover, it's possible to wrap a non-Copy struct as Copy, or vice versa. For us I'd rather we default to the non-Copy one since I think that's the majority of the use cases.

Compilers are supposed to eliminate all unnecessary copies. But in reality from testing, the compiler isn't always smart enough to eliminate all cases (or because of incorrect coding).

Even when we implement Copy, I'd prefer to take parameters as references when we can, because taking the full value is just unnecessary.

So I'd say we close this, unless there's renewed interest in implementing Copy for those structs.