Incorrect documentation and usage of RustBackend.importAccountUfvk JNI function

[daira]

Originally posted by @daira at #1640 (comment) :

The suggestions to add "Only used if purpose is 0 (Spending)." depended on the suggestion below to only convert/check these parameters in that case. By taking the comment suggestion but not the code suggestion, this is now incorrectly documented, because seed_fingerprint_bytes has to point to a 32-byte array and hd_account_index_raw has to be in the range of a valid ZIP 32 account index, regardless of purpose.

and at #1640 (comment) :

Bug: these are not valid values for seedFingerprint and zip32AccountIndex, and will cause a crash.

[nuttycom]

Both of those conversions are fallible, so I don't think the nullity of the arguments can cause a crash?

[daira]

Another doc bug: https://github.com/Electric-Coin-Company/zcash-android-wallet-sdk/pull/1640/files#r1881298277

[HonzaR]

I think that nullability and, thus, null values for these two arguments should be fine. This is how @str4d prepared the JNI layer for us, so the Kotlin side just follows it - link. But we rather double-check it and come back. @Milan-Cerovsky will call the viewing-only purpose version of the API.

[daira]

In Java_cash_z_ecc_android_sdk_internal_jni_RustBackend_importAccountUfvk we have seed_fingerprint_bytes: JByteArray<'local> and hd_account_index_raw: u32. Here's the code that is run unconditionally, not only when the purpose is Spending:

        let seed_fingerprint =
            <[u8; 32]>::try_from(&env.convert_byte_array(seed_fingerprint_bytes)?[..])
                .ok()
                .map(SeedFingerprint::from_bytes);
        let hd_account_index = zip32::AccountId::try_from(hd_account_index_raw).ok();
        let derivation = seed_fingerprint
            .zip(hd_account_index)
            .map(|(seed_fp, idx)| Zip32Derivation::new(seed_fp, idx));

And here's the implementation of JNIEnv::convert_byte_array: https://docs.rs/jni/latest/src/jni/wrapper/jnienv.rs.html#1748-1765

Unless I'm misreading, the non_null! macro in the latter will return an error (from the jni docs for JNIEnv: "If a null Java reference is passed to a method that expects a non-null argument, an Err result with the kind NullPtr is returned.") This will be propagated by the ? which is not correct — since we pass null in non-error cases when constructing a ViewOnly account.

The fix is to apply this suggestion.

[nuttycom]

Fixed in #1656