Add Ristretto255 Support

[Traderjoe95]

This PR adds support for Libsodium's Finite Field Arithmetic API.

Closes #45

[CLAassistant]

All committers have signed the CLA.

[Traderjoe95]

@ionspin, at this point I only added Ristretto255 support. Please have a look at the API. It is slightly different from the others: I added two classes (Ristretto255.Point, Ristretto255.Scalar) to support some syntactic sugar (like Kotlin operators) on Ristretto255 points and scalars. The raw bytes API is provided in addition by an abstract base class.

Please tell me if you'd like to keep it like this or if you only want the low-level APIs.

[ionspin]

Hey @Traderjoe95, thank you very much for the effort, I'll have a look over the weekend, or sooner if I get some free time.

[Traderjoe95]

Ed25519 Finite Field arithmetic was added. For some reason, I can't run the native build on my machine (Windows 11) - it always complains about missing sodium.h, although I checked out the submodule. Any idea what I might be missing?

[ionspin]

Hey @Traderjoe95, try initializing and updating git submodules, since that is how we import libsodium. I'm not using Windows 11 for development, so I'm not completely sure that's going to solve your problems, but it's worth a try if you didn't do it before.

[Traderjoe95]

Okay, right now I am running builds in WSL, which is working fine so far. Build for JVM/Linux Native/JS is running fine now.

[ionspin]

Let me look into this last failure, I think I might have a clue on what is going on, probably need to update the dylib in JVM resource folder now that it's built with FULL_BUILD flag.

[ionspin]

@Traderjoe95 With the updated lib all the checks are passing 🎉 I'll go through the pull request in more detail over the weekend.

Just one request, before I get to reviewing, your IDE seems to be set to 2 space indents, and the rest of the project is on 4. Could you please reformat the files to 4? Especially the JnaLibsodiumInterface.kt that seems to have 2430 lines changed, and I think a large part of it is reformatting from 4 spaces to 2 spaces.

Thanks for all the work you put into this pull request!

[Traderjoe95]

Just one request, before I get to reviewing, your IDE seems to be set to 2 space indents, and the rest of the project is on 4. Could you please reformat the files to 4? Especially the JnaLibsodiumInterface.kt that seems to have 2430 lines changed, and I think a large part of it is reformatting from 4 spaces to 2 spaces.

Sure, that change was unintentional. I reverted all of that. Additionally, I added an .editorconfig file setting the indent size for Kotlin files in the whole project. It should be picked up automatically by all major IDEs 😄

[Traderjoe95]

And, by the way, I will of course add all necessary KDoc documentation once the API is final

[ionspin]

Hey @Traderjoe95, I've had some time to look over the pull request, and I think it's great work, I really like that you made the kotlin idiomatic higher level API but I do have some questions and suggestions regarding it. I've never used libsodium finite field arithmetic directly so feel free to correct me if I'm missing a point of a part of the API.

With that said, let's get to the actual points:

Ed25519LowLevel and Ristretto25519LowLevel being abstract classes

kotlin-multiplatform-libsodium/multiplatform-crypto-libsodium-bindings/src/commonMain/kotlin/com.ionspin.kotlin.crypto/ed25519/Ed25519.kt

Lines 19 to 21 in 2887ae2

	expect abstract class Ed25519LowLevel() {
	fun isValidPoint(encoded: UByteArray): Boolean

I don't quite understand why are they abstract classes, as I wouldn't expect to have multiple instances of ...LowLevel since there is no internal state to track.

My suggestion here would be to follow the rest of the library and make this an expect object

I understand that they are used by the higher level API to inherit but I don't think we would want that and that's the next point that I would like to address.

Ed25519 and Ristretto25519 expose both high level and low level APIs

kotlin-multiplatform-libsodium/multiplatform-crypto-libsodium-bindings/src/commonMain/kotlin/com.ionspin.kotlin.crypto/ed25519/Ed25519.kt

Lines 39 to 43 in 2887ae2

	object Ed25519 : Ed25519LowLevel() {
	fun add(p: Point, q: Point): Point =
	Point(addPoints(p.encoded, q.encoded))

I think this would make it a bit confusing, i.e. if you wanted to add two points using Ed25519 you would have 3 options to do that :

Ed25519.addPoints(p: UByteArray, q: UByteArray): UByteArray
Ed25519.addPoints(p: Point, q: Point): Point
operator fun Ed25519.Point.plus(q: Potin): Point

If we apply the previos suggestion and make the ...LowLevel a object we could define a lowLevel member and then use that to invoke the actual low level functions

i.e.

object Ed25519 {
    private val lowLevel = Ed25519LowLevel()
    fun add(p: Point, q: Point): Point =
        Point(lowLevel.addPoints(p.encoded, q.encoded))

    fun subtract(p: Point, q: Point): Point =
        Point(lowLevel.subtractPoints(p.encoded, q.encoded))
   etc.

Which brings me to my last point

Ed25519/Ristretto25519 is a very thin wrapper around (Ed25519/Ristretto25519)LowLevel

This is really a point of API styling, but instead of providing two different approaches with the thin wrapper in Ed25519/Ristretto25519 i.e.:

kotlin-multiplatform-libsodium/multiplatform-crypto-libsodium-bindings/src/commonMain/kotlin/com.ionspin.kotlin.crypto/ed25519/Ed25519.kt

Lines 41 to 43 in 2887ae2

	fun add(p: Point, q: Point): Point =
	Point(addPoints(p.encoded, q.encoded))

and other option in higher level Point:

kotlin-multiplatform-libsodium/multiplatform-crypto-libsodium-bindings/src/commonMain/kotlin/com.ionspin.kotlin.crypto/ed25519/Ed25519.kt

Lines 86 to 87 in 2887ae2

	data class Point(val encoded: UByteArray) {
	operator fun plus(q: Point): Point = add(this, q)

I would just directly offer only the higher level Point and move the implementation there, i.e.:

    data class Point(private val encoded: UByteArray) {
        operator fun plus(q: Point): Point = Point(lowLevel.addPoints(this.encoded, q.encoded))

(while also making the UByteArray private as well)

---

That is pretty much it regarding the API, I think with these changes proposed we get a simpler API with cleaner separation between low and high level options.

Once again thanks for all the great work so far, it's really appreciated!

[ionspin]

@Traderjoe95 If you don't have time to work on this I could jump in and make these changes, just let me know!

[ionspin]

@Traderjoe95 I went ahead and implemented the changes, if you'd like to check if I missed anything that would be great!

[Traderjoe95]

Hi, sorry for basically disappearing over the last two weeks. First week I got carried away at work and then I got hit by a car while riding the bike. I luckily got away with relatively mild injuries, especially considering the heavy impact, but still had to spend a few days in the hospital and did not have much time and opportunity to do anything at all since then. But it looks like I might be able to have a look these coming days. Thanks for your work implementing the suggestions.Am 01.09.2024 16:51 schrieb Ugljesa Jovanovic ***@***.***>: @Traderjoe95 I went ahead and implemented the changes, if you'd like to check if I missed anything that would be great! —Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you were mentioned.Message ID: ***@***.***>

[ionspin]

Oh wow, I'm sorry to hear that, I wish you speedy recovery! Take your time and heal well, there is no rush to get this merged!