Fixes and improvements for Snowflake

[lukellmann]

Fixes for Snowflake

• constructor that takes an Instant took the epoch milliseconds from the instant, shifted it to the left and then substracted the Discord Epoch but it should first subtract and then shift (see https://discord.com/developers/docs/reference#snowflake-ids-in-pagination-generating-a-snowflake-id-from-a-timestamp-example) -- fixed in 76a35b6
• SnowflakeMark.elapsedNow() returned a Duration with the wrong sign (Snowflake.min.timeMark.hasPassedNow() returned false and Snowflake.max.timeMark.hasPassedNow() returned true) -- fixed in b0bafb8
• Snowflake.timeMark passed unadjusted milliseconds since Discord Epoch to SnowflakeMark's constructor which e.g. caused Snowflake(Clock.System.now()).timeMark.elapsedNow() to return a very long Duration where it should return a very short one -- fixed in 86d1412 (and deduplicated logic from timeStamp and timeMark in c47caa0)

Improvements for Snowflake

• ad0d746: using ushr instead of shr to get correct behavior for Snowflakes with a negative value - Would it be a good idea to just use ULong for value? (It would bring a more accurate Snowflake.max with ULong.MAX_VALUE, avoid confusion with negative Snowflake values etc. But I'm not sure if this would break things elsewhere and if/how it would work with serialization.) - replaced by c889ff9
• 9c6ba3d: Additional documentation for compareTo() and equals() behavior for Snowflake.
• 3f9ad82: Timestamp related tests, Snowflake constructed with Instant too far in the past/future has min/max timestamp instead of undefined overflow behavior, Snowflake.endOfTime constant and more documentation.
• c889ff9: Type of value is now ULong (was Long before) - this included modifying the serializer for Snowflakes.
• 361b87a: Factory function / pseudo constructor as a replacement for old Long constructor (with additional note on negative values).

[BartArys]

Thanks for bringing to light this series of horrible mistakes and taking your time to fix them.

Would it be a good idea to just use ULong for value? (It would bring a more accurate Snowflake.max with ULong.MAX_VALUE, avoid confusion with negative Snowflake values etc.

Yes. Since 1.5.0 unsigned integers are stable and inline classes are serializable, so we do have that option now. Seeing as Snowflakes can never be negative, this seems like a good idea overall.

Also, if it's not too much to ask, could you write up a few tests for your fixes? It'd be a good idea to get these fixes solidified as regression tests.

[lukellmann]

Sure, I will try to write some tests later today.

[lukellmann]

I did add some tests, but to make fun `Snowflake created from instant far in the future has timeStamp equal to the timeStamp of Snowflake max`() pass I had to change val max: Snowflake = Snowflake(Long.MAX_VALUE) to val max: Snowflake = Snowflake(-1) which will probably break when serializing it and sending it to Discord. However this would be fixed with proper ULong usage.

I've also looked a bit into serialization of inline classes and this seems to be a way to serialize a Snowflake with an ULong value (see https://github.com/Kotlin/kotlinx.serialization/blob/master/docs/inline-classes.md):

@OptIn(ExperimentalSerializationApi::class)
internal class Serializer : KSerializer<Snowflake> {
    override val descriptor: SerialDescriptor
        get() = @OptIn(ExperimentalUnsignedTypes::class) ULong.serializer().descriptor

    override fun deserialize(decoder: Decoder): Snowflake = Snowflake(decoder.decodeInline(descriptor).decodeLong().toULong())

    override fun serialize(encoder: Encoder, value: Snowflake) {
        encoder.encodeInline(descriptor).encodeLong(value.value.toLong())
    }
}

As you can see, it still seems to be experimental, so I'm not sure if it can be used by Kord yet.

[lukellmann]

Ok, I've got a working version with ULong (not pushed yet), that uses the Serializer class from above. I also wrote some more tests for it in json.SnowflakeTest. Should I add those changes to this PR (since they depend on the other changes)?

[HopeBaron]

Yes, if they serve the PR

[lukellmann]

Alright, I pushed the changes, would be nice if someone could have a look at them.

[BartArys]

I suggest retaining a Long constructor for ease of use and migration, I imagine plenty of people have a Snowflake(long) somewhere defined as a constant and we're a far ways off from reaching the most significant bit.

[lukellmann]

Ok, I will add this, but two decisions are needed:

• Should it use invoke for companion object or a function as a pseudo constructor (normal constructor will lead to conflicting JVM overloads)?
• Should it throw on negative values or just use a ULong with the same binary representation (meaning -1 will give a Snowflake with maximum timestamp)?

[lukellmann]

I think it would be ok to throw an exception if I add @Deprecated to the pseudo constructor.

[BartArys]

Should it use invoke for companion object or a function as a pseudo constructor (normal constructor will lead to conflicting JVM overloads)?

a top-level factory function. We're trying to move away from companion invokes ever since the naming rules for factory function changed.

Should it throw on negative values or just use a ULong with the same binary representation (meaning -1 will give a Snowflake with maximum timestamp)?

The scenario shouldn't happen all that, so I'm not really concerned with breaking changes. However, negative values were valid (although undocumented) before this change. It might be best to just handle them as unsigned values as well.

I think it would be ok to throw an exception if I add @deprecated to the pseudo constructor.

I wouldn't want it to be marked Deprecated, the whole point of adding the faux constructor is convenience.

[lukellmann]

Done

[lukellmann]

This should be all changes for now. I updated the PR description to include all notable ones. Please tell me, if you think I missed something.