Equality check on records when arrays are involved in Kotlin

[micbakos-rdx]

Hello, I am in the process of verifying the usage of this library with Kotlin built by @Sajjon that uses UniFFI.

During testing, we encountered a situation that, while not a UniFFI issue, could impact our development and its resolution.

The hex_32bytes.rs is a simple struct that holds a bytes: Vec<u8>. It uses the uniffi::Record so it is converted into a data class in Kotlin like so:

data class Hex32Bytes (
    var `bytes`: ByteArray
) {
    
    companion object
}

Which is correct.

In Kotlin, two arrays with the same structure and data are not considered equal when using the built-in equals method:

val arr1 = ByteArray(5) { i -> (5*i).toByte() }
val arr2 = ByteArray(5) { i -> (5*i).toByte() }
assert(arr1 != arr2)
assert(arr1.contentEquals(arr2))

Now, when the Hex32Bytes class is used inside other generated data classes like:

data class Other(
   val bytes: Hex32Bytes
)

assert(Other(ByteArray(5) { i -> (5*i).toByte() }) != Other(ByteArray(5) { i -> (5*i).toByte() }))

Since these classes are generated, there is no way to enforce the usage of contentEquals when comparing the two byte arrays. An extension equals method cannot be used since it will always be shadowed by the built-in equals method.

Considering the need to use records, there are a few options:

1. Change the implementation of Hex32Bytes and create a custom implementation (e.g., BagOfBytes) instead of using Vec<u8>. Instruct UniFFI to convert it to a List<UByte>. This is being explored in a pull request by @Sajjon. Lists in Kotlin follow structural equality, resolving the mentioned problems.

2. Explore the possibility of instructing UniFFI to override the equals method of the generated data class, allowing it to follow the equality check in Rust. This would involve defining a method in Rust's struct and annotating it to override the equals method on the Kotlin side.

3. Consider changing how Vec<u8> is translated in Kotlin. However, this solution is only valid for our use case. So we are not considering this.

[Sajjon]

Or phrasing it differently:

As a Kotlin UniFFI consumer we expect two instances of data class to always equal if all of their contents equal, since they are value types.

If we agree that the above expectation is reasonable and valid - then UniFFI wrt to Records containing Vec<u8> has a bug resulting in invalid data classes which equals always fail - even if all their content equals.

So we propose UniFFI convert Vec<u8> into List<UByte> or List<Byte> because it's equals method is in fact contentEquals which results in Kotlin data classes behaving like proper value types!

[Sajjon]

We can note that there wasn't really any discussion regarding the potential problems of equals not working when ByteArray was introduced in #1543 (comment)

[mhammond]

I'm slightly confused here. The first comment says:

Consider changing how Vec is translated in Kotlin. However, this solution is only valid for our use case. So we are not considering this.

But then:

So we propose UniFFI convert Vec into List or List because it's equals method is in fact contentEquals which results in Kotlin data classes behaving like proper value types!

So I must be missing something because they seem contradictory?

The second statement implies you really just want to revert #1543 - but we introduced bytes for use-cases where having contiguous memory was important. I don't think List<> offers that.

I think what should actually happen here is some way to make #1696 optional - ie, to not have procmacros treat every Vec<u8> as bytes. As you will see from that discussion, I'm not sure how to do that, but would welcome something which did.

[Sajjon]

@mhammond Yes @micbakos-rdx phrased it badly, i think:

Consider changing how Vec is translated in Kotlin. However, this solution is only valid for our use case. So we are not considering this.

I think Michael meant: "If we are the only ones who need UniFFI to change, WE at RDX Works cannot rely on Mozilla changing UniFFI to our needs".

@mhammond Thank you for link to #1696 and #1638 , with discussions, I also had the idea of field attached macros, I love the syntax:

#[uniffi(type = bytes)]

@mhammond you wrote:

While fully supporting both use-cases was seen as ideal, no one could come up with a credible use-case for someone truly wanting a sequence of u8 integers, and we felt that even in the case where that was actually wanted, the pain of moving from a bytes type really wasn't that bad.

So here is my presenting a credible use-case:
We MUST use Record, and our #[derive(uniffi::Record)] struct Profile {} is the root Record, which is a complex struct, when (serde) serialized to minified JSON, it is not uncommon for it to be a few hundred KB large. struct Profile and the type of its fields and their children and grand-children etc, is either Record or Enum. We have 65 uniffi::Record and 31 uniffi::Enum and we expect to see it grow by 50% soon.

✨ Reason why we must use `Record` HERE ✨

The reason why we MUST use Record is the whole Profile package (all types) is today implemented in Swift as value types only (Swift structs and enums) and in Kotlin as value types only (Kotlin data class).

Profile in itself is a model, but updating the children and grand children has non-trivial logic and every mutation is carefully done, involving hashes and IDs to match and also involving encryption.

This is why we have completely fallen in love with UniFFI 😍 we are about to move not only implementation of the Profile models, but also the non-trivial (read: complex) logic involving mutating them into one place, Rust and exposed to iOS and Android wallets using UniFFI. Those wallets are used published on Apple App Store / Google Play Store with tens of thousands of users, holding many millions of USD in crypto.

We need a gradual migration from Profile in Swift on iOS and in Kotlin on Android to be using Profile in Rust exposed via UniFFI. Our plan is:

1. Phase 1: Implement Profile models in Rust - with UniFFI support. STATUS: Done
2. Phase 2: In iOS and Android wallets, remove the native impl of Profile, replace it with the UniFFI one, but keep all our logic for mutating Profile in Swift and Kotlin. STATUS: Planned to start next week
3. Phase 3: Impl all the logic in Rust, and remove the impl of said logic from Swift/Kotlin, update wallet ViewModels to use UniFFI exposed methods (and global function since methods not supported in Records 😔 ). STATUS: Later this year.

I've recently finalised implementation of the models (and a little bit of the logic) @ ~12,000 - 15,000 lines of Rust code (trying to not count 10k+ lines of huge string literals in Rust, part of JSON roundtrip tests of models). I had already written Swift bindgen tests and Michael is now writing Kotlin bindgen tests, where we unexpectedly saw Profile.previewValue() == Profile.previewValue() assertion failed. It works in Rust, it works in Swift, but failed in Kotlin, and Michael boiled it down to the fact that a great(greatx3)child of Profile contains Vec<u8> which translated into ByteArray, for which == always fails, thus all other of Profile models containing Vec<u8> will also have broken == in Kotlin.

So for Phase 2 we must continue to use the logic implemented as is in Swift and Kotlin, therefor we must do a minimal change to the models. Changing them all to be reference types is a huge change, and just not feasible. Our UI layer would be completely broken on iOS since we use TCA which requires that state is only containing value types.

So this is the reason why we MUST use uniffi::Record.

Also, today in wallet repos, we rely on all our values types impl of Equatable and Hashable. So when we move over to UniFFI (Phase 2) all our models must have equatable and hashable work as before.

So Vec<u8> always translating into ByteArray for all uniffi::Record completely breaks Profile for us, and thus would prevent us from migrating our apps over to UniFFI. For now I've implemented a BagOfBytes type wrapping Vec<u8> with custom converter, exposing the bytes a List<UByte> in Kotlin. But I would really prefer to not have it. I've adopted it only in fields in our uniffi::Record/uniffi::Enum types, but not in arguments and return type on methods and constructors - there are a lot of them. This results in a bit awkward API for Kotlin, since my Android colleagues need to convert between ByteArray and List<UByte> a lot.

Ofc, if the UniFFI team - in this thread - tell us "you will have to live with ByteArray in Kotlin for Records for the unforseeable future", I will update all our methods and functions to use BagOfBytes too... just some work I hope to not have to do :).

TL;DR; Vec<u8> -> ByteArray in for Records in Kotlin ought to be regarded as a bug and design decision which breaks how value types ought to behave.

So syntax

#[uniffi(type = bytes)]

Was primary tricky due to uncertain how to handle Option<Vec<u8>>? So was #[uniffi(type = bytes?)] syntax not feasible? Or was this not explored more due "no one could come up with a credible use-case" ? I hope I have provided one and I hope we can continue pursuing support for #[uniffi(type = bytes)]! :)

[Sajjon]

And what @micbakos-rdx wrote above:

2. Explore the possibility of instructing UniFFI to override the equals method of the generated data class, allowing it to follow the equality check in Rust. This would involve defining a method in Rust's struct and annotating it to override the equals method on the Kotlin side.

Him and I have a hard time seeming a good place to put it... it would be strange for it to be macro code next to the struct in Rust file, so would probably be in uniffi.toml file. We would need to spell out an entry for every Rust type containing Vec<u8> and have to write some custom_eq similar to into_custom which would be really quite error prone, since it is a String, and if we were to add a field in the Rust Struct the Rust compiler would not catch it, and yeah... we see that this does not scale well...

[mhammond]

Was primary tricky due to uncertain how to handle Option<Vec>? So was #[uniffi(type = bytes?)] syntax not feasible?

The details are murky, but "not feasible" was probably more like "not obvious" - ie, if there was an easy path it would have been taken, but I don't think we exhausted all possibilities.

so would probably be in uniffi.toml file.

Another possibility would be to have a simple boolean in this file, meaning this entire crate simply opts out of having a bytes type for types inside it.

[Sajjon]

Another possibility would be to have a simple boolean in this file, meaning this entire crate simply opts out of having a bytes type for types inside it.

Yes, that is a good solution too, especially if it can be a per language setting? Just like generate_immutable_records can be controlled for Swift binding and Kotlin bindings individually.

EDIT: that was a bit unclear... I meant to say I would love for Swift bindings to still get Data for Vec<u8>, but only change the Kotlin bindings....

[mhammond]

yeah, it would want to be a per-language setting IMO.