feat(secp256k1): add library to be our dependency for secp256k1 iOS, macos, jvm, android, js #77

goncalo-frade-iohk · 2023-07-10T13:23:10Z

add library for secp256k1 in iOS, macOS
add libraries for secp256k1 in JVM/Android and removed bouncy castle secp256k1 add new implementation of secp256k1 in common

atala-dev · 2023-07-10T13:27:02Z

🦙 MegaLinter status: ⚠️ WARNING

Descriptor	Linter	Files	Errors	Elapsed time
✅ ACTION	actionlint	1	0	0.07s
⚠️ BASH	bash-exec	4	1	0.01s
⚠️ BASH	shfmt	4	3	0.02s
✅ EDITORCONFIG	editorconfig-checker	51	0	0.17s
✅ KOTLIN	ktlint	42	0	73.5s
⚠️ REPOSITORY	devskim	yes	6	0.85s
✅ REPOSITORY	dustilock	yes	no	0.28s
✅ REPOSITORY	git_diff	yes	no	0.02s
✅ REPOSITORY	secretlint	yes	no	2.99s
✅ REPOSITORY	syft	yes	no	0.39s
✅ REPOSITORY	trivy	yes	no	3.99s
✅ YAML	prettier	1	0	0.46s
✅ YAML	v8r	1	0	2.41s
✅ YAML	yamllint	1	0	0.36s

See detailed report in MegaLinter reports
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by

curtis-h

this is awesome (mostly over my head), lots of commented code that I think could be removed, and got a couple of questions about naming

curtis-h · 2023-07-11T08:33:08Z

...metric-encryption/src/commonMain/kotlin/io/iohk/atala/prism/apollo/secp256k1/Secp256k1Lib.kt

@@ -0,0 +1,8 @@
+package io.iohk.atala.prism.apollo.secp256k1
+
+expect class Secp256k1Lib constructor() {


wondering why this is named as a "Lib" instead of "KeyPair"? should we adopt this for the others or is there a distinction being implied?

I think we should remove completely KeyPair. It doesn't make much sense.

A PrivateKey should create a Public Key by getPublicKey(): ByteArray

This is more straightforward process.

Secp256k1Lib is because it uses dependency libs different in each platform to achieve the same. We could call it other way don't mind

curtis-h · 2023-07-11T08:35:23Z

base-asymmetric-encryption/src/commonMain/kotlin/io/iohk/atala/prism/apollo/utils/KMMECPoint.kt

+//    @JsName("fromBigIntegersStrings")
+//    constructor(x: String, y: String) : this(KMMECCoordinate(BigInteger.parseString(x)), KMMECCoordinate(BigInteger.parseString(y)))

-@OptIn(ExperimentalJsExport::class)
-@JsExport
-data class KMMECPoint(val x: KMMECCoordinate, val y: KMMECCoordinate) {
-    @JsName("fromBigIntegersStrings")
-    constructor(x: String, y: String) : this(KMMECCoordinate(BigInteger.parseString(x)), KMMECCoordinate(BigInteger.parseString(y)))
-
-    @JsName("fromBigIntegers")
-    constructor(x: BigInteger, y: BigInteger) : this(KMMECCoordinate(x), KMMECCoordinate(y))
+//    @JsName("fromBigIntegers")
+//    constructor(x: ByteArray, y: BigInteger) : this(KMMECCoordinate(x), KMMECCoordinate(y))


can we just remove this commented out code?

curtis-h · 2023-07-11T08:37:17Z

...encryption/src/commonMain/kotlin/io/iohk/atala/prism/apollo/utils/KMMECSecp256k1PublicKey.kt


    companion object : KMMECSecp256k1PublicKeyCommonStaticInterface
+
+    fun getEncodedCompressed(): ByteArray {


can this be getEncoded to be consistent with the others?

This is just for EC keys so Secp256k1, the Ed25519 and X25519 I don't think share the same feature.

Yeah I actually deleted it since we raw should be enough. Probably we should just have raw for all.

curtis-h · 2023-07-11T08:37:57Z

...-encryption/src/commonTest/kotlin/io/iohk/atala/prism/apollo/utils/GenerateECKeyPairTests.kt

+//                KMMECSecp256k1PublicKey.secp256k1FromBigIntegerCoordinates(tv.point.x.coordinate, tv.point.y.coordinate)
+//            }
+//        }
+//    }


any reason not to remove commented code

Yeah I will have a look on comments before I merge. I just wanted to see first it actually build on the CI 😅

curtis-h · 2023-07-11T08:39:44Z

...-encryption/src/jsMain/kotlin/io/iohk/atala/prism/apollo/utils/KMMECSecp256k1PrivateKeyJS.kt

+
+@OptIn(ExperimentalJsExport::class)
+@JsExport
+class KMMECSecp256k1PrivateKeyJS {


guessing I'm missing something, but why have the JS suffix here?

So for any JS exported you will unfortunately have to create a wrapper. Since ByteArray is not available, you need to create a wrapper that exports BN or Buffer instead of ByteArray and this wrapper converts it.

We can actually name the class in KMM for JS, so although in Kotlin it shows KMMECSecp256k1PrivateKeyJS we can say to KMM to call this on JS as KMMECSecp256k1PrivateKey that's the plan.

But bare in mind we still need to test if the API is showing correctly and appealing in JS :)

Yeah after our talk I removed the wrappers they are not needed

cristianIOHK · 2023-07-11T13:28:33Z

base-asymmetric-encryption/src/iosMain/kotlin/io/iohk/atala/prism/apollo/secp256k1/Schnorr.kt

Do we need to keep it all commented out? Can't we delete the file altogether?

Removed the file

cristianIOHK · 2023-07-11T13:59:24Z

base-asymmetric-encryption/src/iosMain/kotlin/io/iohk/atala/prism/apollo/secp256k1/Secp256k1.kt

+//    protected fun MemScope.allocPrivateKey(privkey: ByteArray): secp256k1_pubkey {
+//        val natPriv = toNat(privkey)
+//        val pub = alloc<secp256k1_>()
+//        secp256k1_ec_pubkey_parse(
+//            ctx,
+//            pub.ptr,
+//            natPub,
+//            pubkey.size.convert()
+//        ).requireSuccess("secp256k1_ec_pubkey_parse() failed")
+//        return pub
+//    }


Do we need to keep this commented? Can be deleted?

I think we can delete it, I just really wanted to have this all passing and ready to merge to be sure that im not missing something :)

…droid, jvm, ios, macos, js add library for secp256k1 in iOS, macOS add libraries for secp256k1 in JVM/Android and removed bouncy castle secp256k1 add new implementation of secp256k1 in common

cristianIOHK

Looks good

goncalo-frade-iohk requested review from elribonazo, hamada147, curtis-h and cristianIOHK July 10, 2023 13:23

goncalo-frade-iohk force-pushed the feature/ATL-5164 branch 12 times, most recently from 7189fca to cdc2fd1 Compare July 10, 2023 16:54

curtis-h reviewed Jul 11, 2023

View reviewed changes

cristianIOHK reviewed Jul 11, 2023

View reviewed changes

goncalo-frade-iohk force-pushed the feature/ATL-5164 branch 5 times, most recently from 94cedff to 6154217 Compare July 13, 2023 10:34

feat(secp256k1): add library to be our dependency for secp256k1 in an…

3e41246

…droid, jvm, ios, macos, js add library for secp256k1 in iOS, macOS add libraries for secp256k1 in JVM/Android and removed bouncy castle secp256k1 add new implementation of secp256k1 in common

goncalo-frade-iohk force-pushed the feature/ATL-5164 branch from 6154217 to 3e41246 Compare July 13, 2023 10:45

cristianIOHK approved these changes Jul 13, 2023

View reviewed changes

goncalo-frade-iohk changed the title ~~feat(secp256k1): add library to be our dependency for secp256k1 in js~~ feat(secp256k1): add library to be our dependency for secp256k1 iOS, macos, jvm, android, js Jul 13, 2023

goncalo-frade-iohk merged commit 9121f60 into main Jul 13, 2023

goncalo-frade-iohk deleted the feature/ATL-5164 branch July 13, 2023 12:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(secp256k1): add library to be our dependency for secp256k1 iOS, macos, jvm, android, js #77

feat(secp256k1): add library to be our dependency for secp256k1 iOS, macos, jvm, android, js #77

goncalo-frade-iohk commented Jul 10, 2023

atala-dev commented Jul 10, 2023 •

edited

Loading

curtis-h left a comment

curtis-h Jul 11, 2023

goncalo-frade-iohk Jul 11, 2023

curtis-h Jul 11, 2023

goncalo-frade-iohk Jul 13, 2023

curtis-h Jul 11, 2023

goncalo-frade-iohk Jul 11, 2023

goncalo-frade-iohk Jul 13, 2023 •

edited

Loading

curtis-h Jul 11, 2023

goncalo-frade-iohk Jul 11, 2023

goncalo-frade-iohk Jul 13, 2023

curtis-h Jul 11, 2023

goncalo-frade-iohk Jul 11, 2023

goncalo-frade-iohk Jul 13, 2023

cristianIOHK Jul 11, 2023

goncalo-frade-iohk Jul 13, 2023 •

edited

Loading

cristianIOHK Jul 11, 2023

goncalo-frade-iohk Jul 11, 2023

cristianIOHK left a comment

		@@ -0,0 +1,8 @@
		package io.iohk.atala.prism.apollo.secp256k1

		expect class Secp256k1Lib constructor() {


		companion object : KMMECSecp256k1PublicKeyCommonStaticInterface

		fun getEncodedCompressed(): ByteArray {

feat(secp256k1): add library to be our dependency for secp256k1 iOS, macos, jvm, android, js #77

feat(secp256k1): add library to be our dependency for secp256k1 iOS, macos, jvm, android, js #77

Conversation

goncalo-frade-iohk commented Jul 10, 2023

atala-dev commented Jul 10, 2023 • edited Loading

🦙 MegaLinter status: ⚠️ WARNING

curtis-h left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goncalo-frade-iohk Jul 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goncalo-frade-iohk Jul 13, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristianIOHK left a comment

Choose a reason for hiding this comment

atala-dev commented Jul 10, 2023 •

edited

Loading

goncalo-frade-iohk Jul 13, 2023 •

edited

Loading

goncalo-frade-iohk Jul 13, 2023 •

edited

Loading