Code review:

- enforce op equality by class and pin the tag-uniqueness invariant
- hoist crypto op equals/hashCode onto OpCrypto
This commit is contained in:
davotoula
2026-07-04 21:38:08 +02:00
parent 772b4ea8ed
commit 489e8ae50b
8 changed files with 45 additions and 19 deletions
@@ -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
}
@@ -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()
@@ -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,
@@ -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()
@@ -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
@@ -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
@@ -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
@@ -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<Op>(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)
}
}