From 489e8ae50b863565bfe8930480cb4cfae2bca01e Mon Sep 17 00:00:00 2001 From: davotoula Date: Sat, 4 Jul 2026 18:30:19 +0200 Subject: [PATCH] Code review: - enforce op equality by class and pin the tag-uniqueness invariant - hoist crypto op equals/hashCode onto OpCrypto --- .../quartz/nip03Timestamp/ots/VerifyResult.kt | 5 ++++ .../quartz/nip03Timestamp/ots/op/OpBinary.kt | 6 +++- .../quartz/nip03Timestamp/ots/op/OpCrypto.kt | 8 +++++ .../nip03Timestamp/ots/op/OpKECCAK256.kt | 4 --- .../nip03Timestamp/ots/op/OpRIPEMD160.kt | 4 --- .../quartz/nip03Timestamp/ots/op/OpSHA1.kt | 4 --- .../quartz/nip03Timestamp/ots/op/OpSHA256.kt | 4 --- .../ots/OtsEqualsContractTest.kt | 29 +++++++++++++++++-- 8 files changed, 45 insertions(+), 19 deletions(-) diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip03Timestamp/ots/VerifyResult.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip03Timestamp/ots/VerifyResult.kt index db8aceb159..9e6763380d 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip03Timestamp/ots/VerifyResult.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip03Timestamp/ots/VerifyResult.kt @@ -40,5 +40,10 @@ data class VerifyResult( return "block $height attests data existed as of unix timestamp of $timestamp" } + /** + * Orders by [height] only, intentionally ignoring [timestamp] — NOT + * consistent with [equals], which compares both fields. Fine for picking + * the earliest attestation; do not rely on it for sorted-set dedup. + */ override fun compareTo(other: VerifyResult): Int = this.height - other.height } diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip03Timestamp/ots/op/OpBinary.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip03Timestamp/ots/op/OpBinary.kt index 4f7be1ad51..1d8bf092b5 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip03Timestamp/ots/op/OpBinary.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip03Timestamp/ots/op/OpBinary.kt @@ -53,7 +53,11 @@ abstract class OpBinary( return this.tag() - other.tag() } - override fun equals(other: Any?): Boolean = other is OpBinary && this.tag() == other.tag() && this.arg.contentEquals(other.arg) + // Class-scoped equality: never conflates two ops as Timestamp.ops map keys + // even if a subclass reused a tag by mistake. hashCode stays tag-based + // (collisions are legal). Tag uniqueness itself is a protocol invariant + // (serialization dispatches on it), pinned by OtsEqualsContractTest. + override fun equals(other: Any?): Boolean = other is OpBinary && this::class == other::class && this.arg.contentEquals(other.arg) override fun hashCode(): Int = this.tag().toInt() xor this.arg.contentHashCode() diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip03Timestamp/ots/op/OpCrypto.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip03Timestamp/ots/op/OpCrypto.kt index 4dafd75ff5..f98f38b9b7 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip03Timestamp/ots/op/OpCrypto.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip03Timestamp/ots/op/OpCrypto.kt @@ -64,6 +64,14 @@ abstract class OpCrypto internal constructor() : OpUnary() { return hashFd(ctx) } + // Crypto ops carry no identity state — class-scoped equality with a + // tag-based hashCode (never conflates ops even if a subclass reused a + // tag by mistake). Tag uniqueness itself is a protocol invariant + // (serialization dispatches on it), pinned by OtsEqualsContractTest. + override fun equals(other: Any?): Boolean = other is OpCrypto && this::class == other::class + + override fun hashCode(): Int = this.tag().toInt() + companion object { fun deserializeFromTag( ctx: StreamDeserializationContext, diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip03Timestamp/ots/op/OpKECCAK256.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip03Timestamp/ots/op/OpKECCAK256.kt index 51cf4d94e7..67663af466 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip03Timestamp/ots/op/OpKECCAK256.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip03Timestamp/ots/op/OpKECCAK256.kt @@ -49,10 +49,6 @@ class OpKECCAK256 : OpCrypto() { return hash } - override fun equals(other: Any?): Boolean = (other is OpKECCAK256) - - override fun hashCode(): Int = TAG.toInt() - companion object { val TAG: Byte = 103.toByte() diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip03Timestamp/ots/op/OpRIPEMD160.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip03Timestamp/ots/op/OpRIPEMD160.kt index 430e57bca5..eafea4ea29 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip03Timestamp/ots/op/OpRIPEMD160.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip03Timestamp/ots/op/OpRIPEMD160.kt @@ -48,10 +48,6 @@ class OpRIPEMD160 : OpCrypto() { return hash } - override fun equals(other: Any?): Boolean = (other is OpRIPEMD160) - - override fun hashCode(): Int = TAG.toInt() - companion object { val TAG: Byte = 0x03 diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip03Timestamp/ots/op/OpSHA1.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip03Timestamp/ots/op/OpSHA1.kt index df0a90155e..fb1078f201 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip03Timestamp/ots/op/OpSHA1.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip03Timestamp/ots/op/OpSHA1.kt @@ -46,10 +46,6 @@ class OpSHA1 : OpCrypto() { override fun call(msg: ByteArray): ByteArray = super.call(msg) - override fun equals(other: Any?): Boolean = (other is OpSHA1) - - override fun hashCode(): Int = TAG.toInt() - companion object { val TAG: Byte = 0x02 diff --git a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip03Timestamp/ots/op/OpSHA256.kt b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip03Timestamp/ots/op/OpSHA256.kt index bbc24a9e92..7ce810eed2 100644 --- a/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip03Timestamp/ots/op/OpSHA256.kt +++ b/quartz/src/commonMain/kotlin/com/vitorpamplona/quartz/nip03Timestamp/ots/op/OpSHA256.kt @@ -40,10 +40,6 @@ class OpSHA256 : OpCrypto() { override fun call(msg: ByteArray): ByteArray = super.call(msg) - override fun equals(other: Any?): Boolean = (other is OpSHA256) - - override fun hashCode(): Int = TAG.toInt() - companion object { val TAG: Byte = 0x08 diff --git a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip03Timestamp/ots/OtsEqualsContractTest.kt b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip03Timestamp/ots/OtsEqualsContractTest.kt index 2f7a0b8b79..d50cb2a553 100644 --- a/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip03Timestamp/ots/OtsEqualsContractTest.kt +++ b/quartz/src/commonTest/kotlin/com/vitorpamplona/quartz/nip03Timestamp/ots/OtsEqualsContractTest.kt @@ -24,11 +24,13 @@ import com.vitorpamplona.quartz.nip03Timestamp.ots.op.Op import com.vitorpamplona.quartz.nip03Timestamp.ots.op.OpAppend import com.vitorpamplona.quartz.nip03Timestamp.ots.op.OpKECCAK256 import com.vitorpamplona.quartz.nip03Timestamp.ots.op.OpPrepend +import com.vitorpamplona.quartz.nip03Timestamp.ots.op.OpRIPEMD160 +import com.vitorpamplona.quartz.nip03Timestamp.ots.op.OpSHA1 +import com.vitorpamplona.quartz.nip03Timestamp.ots.op.OpSHA256 import kotlin.test.Test import kotlin.test.assertEquals import kotlin.test.assertFalse import kotlin.test.assertNotEquals -import kotlin.test.assertTrue /** * Equals/hashCode contract tests for the types keying [Timestamp.ops] @@ -36,6 +38,23 @@ import kotlin.test.assertTrue * lookups produce duplicate branches and failed upgrades. */ class OtsEqualsContractTest { + @Test + fun allOpTagsAreUnique() { + // The protocol invariant everything leans on: serialization dispatches + // on the tag byte and hashCode is tag-based. A new op MUST use a fresh tag. + val tags = + listOf( + OpAppend.TAG, + OpPrepend.TAG, + OpSHA1.TAG, + OpSHA256.TAG, + OpRIPEMD160.TAG, + OpKECCAK256.TAG, + ) + + assertEquals(tags.size, tags.toSet().size) + } + // --- OpKECCAK256: equals existed without hashCode; equal instances hashed by identity --- @Test @@ -54,6 +73,12 @@ class OtsEqualsContractTest { assertEquals("branch", map[OpKECCAK256()]) } + @Test + fun cryptoOpsWithDifferentTagsAreNotEqual() { + // The shared OpCrypto equals is tag-keyed — distinct ops must not conflate. + assertNotEquals(OpSHA256(), OpKECCAK256()) + } + // --- OpBinary: equals is defined once on the superclass, keyed on tag() + arg content --- @Test @@ -105,6 +130,6 @@ class OtsEqualsContractTest { val result = VerifyResult(null, 100) result.hashCode() - assertTrue(result == VerifyResult(null, 100)) + assertEquals(VerifyResult(null, 100), result) } }