From 67986693a8cb71f52c76e6a13dd3a34e3ed094e5 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Fri, 22 Nov 2024 18:35:43 +1300 Subject: [PATCH 1/2] Rename primitive method arguments to `accountIndex` consistently --- .../z/ecc/android/sdk/internal/Backend.kt | 10 ++--- .../android/sdk/internal/jni/RustBackend.kt | 30 ++++++------- .../sdk/internal/jni/RustDerivationTool.kt | 2 +- .../internal/model/JniUnifiedSpendingKey.kt | 3 ++ backend-lib/src/main/rust/lib.rs | 43 ++++++++++--------- .../cash/z/ecc/fixture/FakeRustBackend.kt | 10 ++--- .../cash/z/ecc/android/sdk/model/Account.kt | 2 +- .../android/sdk/model/UnifiedSpendingKey.kt | 3 ++ 8 files changed, 56 insertions(+), 47 deletions(-) diff --git a/backend-lib/src/main/java/cash/z/ecc/android/sdk/internal/Backend.kt b/backend-lib/src/main/java/cash/z/ecc/android/sdk/internal/Backend.kt index 30ed443f8..4fe09ba63 100644 --- a/backend-lib/src/main/java/cash/z/ecc/android/sdk/internal/Backend.kt +++ b/backend-lib/src/main/java/cash/z/ecc/android/sdk/internal/Backend.kt @@ -24,7 +24,7 @@ interface Backend { suspend fun initBlockMetaDb(): Int suspend fun proposeTransfer( - account: Int, + accountIndex: Int, to: String, value: Long, memo: ByteArray? = null @@ -35,12 +35,12 @@ interface Backend { */ @Throws(RuntimeException::class) suspend fun proposeTransferFromUri( - account: Int, + accountIndex: Int, uri: String ): ProposalUnsafe suspend fun proposeShielding( - account: Int, + accountIndex: Int, shieldingThreshold: Long, memo: ByteArray? = null, transparentReceiver: String? = null @@ -109,13 +109,13 @@ interface Backend { fun isValidTexAddr(addr: String): Boolean @Throws(RuntimeException::class) - suspend fun getCurrentAddress(account: Int): String + suspend fun getCurrentAddress(accountIndex: Int): String fun getTransparentReceiver(ua: String): String? fun getSaplingReceiver(ua: String): String? - suspend fun listTransparentReceivers(account: Int): List + suspend fun listTransparentReceivers(accountIndex: Int): List fun getBranchIdForHeight(height: Long): Long diff --git a/backend-lib/src/main/java/cash/z/ecc/android/sdk/internal/jni/RustBackend.kt b/backend-lib/src/main/java/cash/z/ecc/android/sdk/internal/jni/RustBackend.kt index 053485b8a..6f7d526e2 100644 --- a/backend-lib/src/main/java/cash/z/ecc/android/sdk/internal/jni/RustBackend.kt +++ b/backend-lib/src/main/java/cash/z/ecc/android/sdk/internal/jni/RustBackend.kt @@ -114,11 +114,11 @@ class RustBackend private constructor( ) } - override suspend fun getCurrentAddress(account: Int) = + override suspend fun getCurrentAddress(accountIndex: Int) = withContext(SdkDispatchers.DATABASE_IO) { getCurrentAddress( dataDbFile.absolutePath, - account, + accountIndex, networkId = networkId ) } @@ -127,11 +127,11 @@ class RustBackend private constructor( override fun getSaplingReceiver(ua: String) = getSaplingReceiverForUnifiedAddress(ua) - override suspend fun listTransparentReceivers(account: Int): List { + override suspend fun listTransparentReceivers(accountIndex: Int): List { return withContext(SdkDispatchers.DATABASE_IO) { listTransparentReceivers( dbDataPath = dataDbFile.absolutePath, - account = account, + accountIndex = accountIndex, networkId = networkId ).asList() } @@ -318,14 +318,14 @@ class RustBackend private constructor( } override suspend fun proposeTransferFromUri( - account: Int, + accountIndex: Int, uri: String ): ProposalUnsafe = withContext(SdkDispatchers.DATABASE_IO) { ProposalUnsafe.parse( proposeTransferFromUri( dataDbFile.absolutePath, - account, + accountIndex, uri, networkId = networkId, ) @@ -333,7 +333,7 @@ class RustBackend private constructor( } override suspend fun proposeTransfer( - account: Int, + accountIndex: Int, to: String, value: Long, memo: ByteArray? @@ -342,7 +342,7 @@ class RustBackend private constructor( ProposalUnsafe.parse( proposeTransfer( dataDbFile.absolutePath, - account, + accountIndex, to, value, memo, @@ -352,7 +352,7 @@ class RustBackend private constructor( } override suspend fun proposeShielding( - account: Int, + accountIndex: Int, shieldingThreshold: Long, memo: ByteArray?, transparentReceiver: String? @@ -360,7 +360,7 @@ class RustBackend private constructor( return withContext(SdkDispatchers.DATABASE_IO) { proposeShielding( dataDbFile.absolutePath, - account, + accountIndex, shieldingThreshold, memo, transparentReceiver, @@ -510,7 +510,7 @@ class RustBackend private constructor( @JvmStatic private external fun getCurrentAddress( dbDataPath: String, - account: Int, + accountIndex: Int, networkId: Int ): String @@ -523,7 +523,7 @@ class RustBackend private constructor( @JvmStatic private external fun listTransparentReceivers( dbDataPath: String, - account: Int, + accountIndex: Int, networkId: Int ): Array @@ -671,7 +671,7 @@ class RustBackend private constructor( @JvmStatic private external fun proposeTransferFromUri( dbDataPath: String, - account: Int, + accountIndex: Int, uri: String, networkId: Int, ): ByteArray @@ -680,7 +680,7 @@ class RustBackend private constructor( @Suppress("LongParameterList") private external fun proposeTransfer( dbDataPath: String, - account: Int, + accountIndex: Int, to: String, value: Long, memo: ByteArray?, @@ -691,7 +691,7 @@ class RustBackend private constructor( @Suppress("LongParameterList") private external fun proposeShielding( dbDataPath: String, - account: Int, + accountIndex: Int, shieldingThreshold: Long, memo: ByteArray?, transparentReceiver: String?, diff --git a/backend-lib/src/main/java/cash/z/ecc/android/sdk/internal/jni/RustDerivationTool.kt b/backend-lib/src/main/java/cash/z/ecc/android/sdk/internal/jni/RustDerivationTool.kt index 371c35531..e276387e4 100644 --- a/backend-lib/src/main/java/cash/z/ecc/android/sdk/internal/jni/RustDerivationTool.kt +++ b/backend-lib/src/main/java/cash/z/ecc/android/sdk/internal/jni/RustDerivationTool.kt @@ -68,7 +68,7 @@ class RustDerivationTool private constructor() : Derivation { @JvmStatic private external fun deriveSpendingKey( seed: ByteArray, - account: Int, + accountIndex: Int, networkId: Int ): JniUnifiedSpendingKey diff --git a/backend-lib/src/main/java/cash/z/ecc/android/sdk/internal/model/JniUnifiedSpendingKey.kt b/backend-lib/src/main/java/cash/z/ecc/android/sdk/internal/model/JniUnifiedSpendingKey.kt index ad9948b66..5eaa1ffe0 100644 --- a/backend-lib/src/main/java/cash/z/ecc/android/sdk/internal/model/JniUnifiedSpendingKey.kt +++ b/backend-lib/src/main/java/cash/z/ecc/android/sdk/internal/model/JniUnifiedSpendingKey.kt @@ -13,6 +13,9 @@ import androidx.annotation.Keep */ @Keep class JniUnifiedSpendingKey( + /** + * The [ZIP 32](https://zips.z.cash/zip-0032) account index used to derive this key. + */ val account: Int, /** * The binary encoding of the [ZIP 316](https://zips.z.cash/zip-0316) Unified Spending diff --git a/backend-lib/src/main/rust/lib.rs b/backend-lib/src/main/rust/lib.rs index c2b2a7463..65482588a 100644 --- a/backend-lib/src/main/rust/lib.rs +++ b/backend-lib/src/main/rust/lib.rs @@ -105,8 +105,8 @@ fn block_db(env: &mut JNIEnv, fsblockdb_root: JString) -> anyhow::Result anyhow::Result { - u32::try_from(account) +fn zip32_account_index_from_jint(account_index: jint) -> anyhow::Result { + u32::try_from(account_index) .map_err(|_| ()) .and_then(|id| zip32::AccountId::try_from(id).map_err(|_| ())) .map_err(|_| anyhow!("Invalid account ID")) @@ -116,7 +116,7 @@ fn account_id_from_jni( db_data: &WalletDb, account_index: jint, ) -> anyhow::Result { - let requested_account_index = account_id_from_jint(account_index)?; + let requested_account_index = zip32_account_index_from_jint(account_index)?; // Find the single account matching the given ZIP 32 account index. let mut accounts = db_data @@ -360,7 +360,7 @@ pub extern "C" fn Java_cash_z_ecc_android_sdk_internal_jni_RustBackend_getAccoun fn encode_usk<'a>( env: &mut JNIEnv<'a>, - account: zip32::AccountId, + account_index: zip32::AccountId, usk: UnifiedSpendingKey, ) -> jni::errors::Result> { let encoded = SecretVec::new(usk.to_bytes(Era::Orchard)); @@ -368,7 +368,10 @@ fn encode_usk<'a>( env.new_object( "cash/z/ecc/android/sdk/internal/model/JniUnifiedSpendingKey", "(I[B)V", - &[JValue::Int(u32::from(account) as i32), (&bytes).into()], + &[ + JValue::Int(u32::from(account_index) as i32), + (&bytes).into(), + ], ) } @@ -480,14 +483,14 @@ pub extern "C" fn Java_cash_z_ecc_android_sdk_internal_jni_RustBackend_getCurren mut env: JNIEnv<'local>, _: JClass<'local>, db_data: JString<'local>, - account: jint, + account_index: jint, network_id: jint, ) -> jstring { let res = catch_unwind(&mut env, |env| { let _span = tracing::info_span!("RustBackend.getCurrentAddress").entered(); let network = parse_network(network_id as u32)?; let db_data = wallet_db(env, network, db_data)?; - let account = account_id_from_jni(&db_data, account)?; + let account = account_id_from_jni(&db_data, account_index)?; match db_data.get_current_address(account) { Ok(Some(addr)) => { @@ -1624,7 +1627,7 @@ pub extern "C" fn Java_cash_z_ecc_android_sdk_internal_jni_RustBackend_proposeTr mut env: JNIEnv<'local>, _: JClass<'local>, db_data: JString<'local>, - account: jint, + account_index: jint, payment_uri: JString<'local>, network_id: jint, ) -> jbyteArray { @@ -1632,7 +1635,7 @@ pub extern "C" fn Java_cash_z_ecc_android_sdk_internal_jni_RustBackend_proposeTr let _span = tracing::info_span!("RustBackend.proposeTransfer").entered(); let network = parse_network(network_id as u32)?; let mut db_data = wallet_db(env, network, db_data)?; - let account = account_id_from_jni(&db_data, account)?; + let account = account_id_from_jni(&db_data, account_index)?; let payment_uri = utils::java_string_to_rust(env, &payment_uri); // Always use ZIP 317 fees @@ -1668,7 +1671,7 @@ pub extern "C" fn Java_cash_z_ecc_android_sdk_internal_jni_RustBackend_proposeTr mut env: JNIEnv<'local>, _: JClass<'local>, db_data: JString<'local>, - account: jint, + account_index: jint, to: JString<'local>, value: jlong, memo: JByteArray<'local>, @@ -1678,7 +1681,7 @@ pub extern "C" fn Java_cash_z_ecc_android_sdk_internal_jni_RustBackend_proposeTr let _span = tracing::info_span!("RustBackend.proposeTransfer").entered(); let network = parse_network(network_id as u32)?; let mut db_data = wallet_db(env, network, db_data)?; - let account = account_id_from_jni(&db_data, account)?; + let account = account_id_from_jni(&db_data, account_index)?; let to = utils::java_string_to_rust(env, &to); let value = NonNegativeAmount::from_nonnegative_i64(value) .map_err(|_| anyhow!("Invalid amount, out of range"))?; @@ -1732,7 +1735,7 @@ pub extern "C" fn Java_cash_z_ecc_android_sdk_internal_jni_RustBackend_proposeSh mut env: JNIEnv<'local>, _: JClass<'local>, db_data: JString<'local>, - account: jint, + account_index: jint, shielding_threshold: jlong, memo: JByteArray<'local>, transparent_receiver: JString<'local>, @@ -1742,7 +1745,7 @@ pub extern "C" fn Java_cash_z_ecc_android_sdk_internal_jni_RustBackend_proposeSh let _span = tracing::info_span!("RustBackend.proposeShielding").entered(); let network = parse_network(network_id as u32)?; let mut db_data = wallet_db(env, network, db_data)?; - let account = account_id_from_jni(&db_data, account)?; + let account = account_id_from_jni(&db_data, account_index)?; let shielding_threshold = NonNegativeAmount::from_nonnegative_i64(shielding_threshold) .map_err(|_| anyhow!("Invalid shielding threshold, out of range"))?; @@ -1929,14 +1932,14 @@ pub extern "C" fn Java_cash_z_ecc_android_sdk_internal_jni_RustDerivationTool_de mut env: JNIEnv<'local>, _: JClass<'local>, seed: JByteArray<'local>, - account: jint, + account_index: jint, network_id: jint, ) -> jobject { let res = catch_unwind(&mut env, |env| { let _span = tracing::info_span!("RustDerivationTool.deriveSpendingKey").entered(); let network = parse_network(network_id as u32)?; let seed = SecretVec::new(env.convert_byte_array(seed).unwrap()); - let account = account_id_from_jint(account)?; + let account = zip32_account_index_from_jint(account_index)?; let usk = UnifiedSpendingKey::from_seed(&network, seed.expose_secret(), account) .map_err(|e| anyhow!("error generating unified spending key from seed: {:?}", e))?; @@ -2006,7 +2009,7 @@ pub extern "C" fn Java_cash_z_ecc_android_sdk_internal_jni_RustDerivationTool_de tracing::info_span!("RustDerivationTool.deriveUnifiedAddressFromSeed").entered(); let network = parse_network(network_id as u32)?; let seed = env.convert_byte_array(seed).unwrap(); - let account_id = account_id_from_jint(account_index)?; + let account_id = zip32_account_index_from_jint(account_index)?; let ufvk = UnifiedSpendingKey::from_seed(&network, &seed, account_id) .map_err(|e| anyhow!("error generating unified spending key from seed: {:?}", e)) @@ -2116,7 +2119,7 @@ pub extern "C" fn Java_cash_z_ecc_android_sdk_internal_jni_RustDerivationTool_de _: JClass<'local>, context_string: JByteArray<'local>, seed: JByteArray<'local>, - account: jint, + account_index: jint, network_id: jint, ) -> jbyteArray { let res = panic::catch_unwind(|| { @@ -2125,7 +2128,7 @@ pub extern "C" fn Java_cash_z_ecc_android_sdk_internal_jni_RustDerivationTool_de let network = parse_network(network_id as u32)?; let context_string = env.convert_byte_array(context_string)?; let seed = SecretVec::new(env.convert_byte_array(seed)?); - let account = account_id_from_jint(account)?; + let account = zip32_account_index_from_jint(account_index)?; let key = zip32::arbitrary::SecretKey::from_path( &context_string, @@ -2251,7 +2254,7 @@ pub extern "C" fn Java_cash_z_ecc_android_sdk_internal_jni_RustBackend_listTrans mut env: JNIEnv<'local>, _: JClass<'local>, db_data: JString<'local>, - account_id: jint, + account_index: jint, network_id: jint, ) -> jobjectArray { let res = catch_unwind(&mut env, |env| { @@ -2259,7 +2262,7 @@ pub extern "C" fn Java_cash_z_ecc_android_sdk_internal_jni_RustBackend_listTrans let network = parse_network(network_id as u32)?; let zcash_network = network.network_type(); let db_data = wallet_db(env, network, db_data)?; - let account = account_id_from_jni(&db_data, account_id)?; + let account = account_id_from_jni(&db_data, account_index)?; match db_data.get_transparent_receivers(account) { Ok(receivers) => { diff --git a/sdk-lib/src/androidTest/java/cash/z/ecc/fixture/FakeRustBackend.kt b/sdk-lib/src/androidTest/java/cash/z/ecc/fixture/FakeRustBackend.kt index 0f7b53d34..31394ab01 100644 --- a/sdk-lib/src/androidTest/java/cash/z/ecc/fixture/FakeRustBackend.kt +++ b/sdk-lib/src/androidTest/java/cash/z/ecc/fixture/FakeRustBackend.kt @@ -89,14 +89,14 @@ internal class FakeRustBackend( } override suspend fun proposeTransferFromUri( - account: Int, + accountIndex: Int, uri: String ): ProposalUnsafe { error("Intentionally not implemented yet.") } override suspend fun proposeTransfer( - account: Int, + accountIndex: Int, to: String, value: Long, memo: ByteArray? @@ -105,7 +105,7 @@ internal class FakeRustBackend( } override suspend fun proposeShielding( - account: Int, + accountIndex: Int, shieldingThreshold: Long, memo: ByteArray?, transparentReceiver: String? @@ -159,7 +159,7 @@ internal class FakeRustBackend( error("Intentionally not implemented in mocked FakeRustBackend implementation.") } - override suspend fun getCurrentAddress(account: Int): String { + override suspend fun getCurrentAddress(accountIndex: Int): String { error("Intentionally not implemented yet.") } @@ -171,7 +171,7 @@ internal class FakeRustBackend( error("Intentionally not implemented yet.") } - override suspend fun listTransparentReceivers(account: Int): List { + override suspend fun listTransparentReceivers(accountIndex: Int): List { error("Intentionally not implemented yet.") } diff --git a/sdk-lib/src/main/java/cash/z/ecc/android/sdk/model/Account.kt b/sdk-lib/src/main/java/cash/z/ecc/android/sdk/model/Account.kt index cb5227c9b..0c82fad26 100644 --- a/sdk-lib/src/main/java/cash/z/ecc/android/sdk/model/Account.kt +++ b/sdk-lib/src/main/java/cash/z/ecc/android/sdk/model/Account.kt @@ -1,7 +1,7 @@ package cash.z.ecc.android.sdk.model /** - * A [ZIP 316](https://zips.z.cash/zip-0316) account identifier. + * A [ZIP 32](https://zips.z.cash/zip-0032) account index. * * @param value A 0-based account index. Must be >= 0. */ diff --git a/sdk-lib/src/main/java/cash/z/ecc/android/sdk/model/UnifiedSpendingKey.kt b/sdk-lib/src/main/java/cash/z/ecc/android/sdk/model/UnifiedSpendingKey.kt index 3a590b787..648ba1910 100644 --- a/sdk-lib/src/main/java/cash/z/ecc/android/sdk/model/UnifiedSpendingKey.kt +++ b/sdk-lib/src/main/java/cash/z/ecc/android/sdk/model/UnifiedSpendingKey.kt @@ -13,6 +13,9 @@ import cash.z.ecc.android.sdk.internal.model.JniUnifiedSpendingKey * export/import, or backup purposes. */ class UnifiedSpendingKey private constructor( + /** + * The [ZIP 32](https://zips.z.cash/zip-0032) account index used to derive this key. + */ val account: Account, /** * The binary encoding of the [ZIP 316](https://zips.z.cash/zip-0316) Unified Spending From 15f46f86c76c76bfe095cb2b154121d425177f9c Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Fri, 22 Nov 2024 21:30:14 +1300 Subject: [PATCH 2/2] Return the ZIP 32 account index from `RustBackend.getAccounts` Previously we were returning the internal account private key, which is incorrect for how `Account` is currently used on the Kotlin side. Closes Electric-Coin-Company/zcash-android-wallet-sdk#1638. --- .../ecc/android/sdk/internal/model/JniAccount.kt | 8 ++++---- backend-lib/src/main/rust/lib.rs | 16 ++++++++++++++-- .../android/sdk/internal/TypesafeBackendImpl.kt | 2 +- 3 files changed, 19 insertions(+), 7 deletions(-) diff --git a/backend-lib/src/main/java/cash/z/ecc/android/sdk/internal/model/JniAccount.kt b/backend-lib/src/main/java/cash/z/ecc/android/sdk/internal/model/JniAccount.kt index f813b90ba..a79a9e002 100644 --- a/backend-lib/src/main/java/cash/z/ecc/android/sdk/internal/model/JniAccount.kt +++ b/backend-lib/src/main/java/cash/z/ecc/android/sdk/internal/model/JniAccount.kt @@ -5,19 +5,19 @@ import androidx.annotation.Keep /** * Serves as cross layer (Kotlin, Rust) communication class. * - * @param account the account ID + * @param accountIndex the ZIP 32 account index. * @param ufvk The account's Unified Full Viewing Key, if any. * @throws IllegalArgumentException if the values are inconsistent. */ @Keep @Suppress("LongParameterList") class JniAccount( - val accountId: Long, + val accountIndex: Int, val ufvk: String?, ) { init { - require(accountId >= 0) { - "Account ID must be non-negative" + require(accountIndex >= 0) { + "Account index must be non-negative" } } } diff --git a/backend-lib/src/main/rust/lib.rs b/backend-lib/src/main/rust/lib.rs index 65482588a..ea1f369cd 100644 --- a/backend-lib/src/main/rust/lib.rs +++ b/backend-lib/src/main/rust/lib.rs @@ -297,6 +297,11 @@ fn encode_account<'a, P: Parameters>( network: &P, account: zcash_client_sqlite::wallet::Account, ) -> jni::errors::Result> { + let account_index = match account.source() { + AccountSource::Derived { account_index, .. } => account_index, + AccountSource::Imported { .. } => panic!("Should have been filtered out"), + }; + let ufvk = match account.ufvk() { Some(ufvk) => env.new_string(ufvk.encode(network))?.into(), None => JObject::null(), @@ -304,10 +309,10 @@ fn encode_account<'a, P: Parameters>( env.new_object( JNI_ACCOUNT, - "(JLjava/lang/String;)V", + "(ILjava/lang/String;)V", &[ // TODO: This will be replaced by the multi-seed-compatible account ID. - JValue::Long(i64::from(account.id().as_u32())), + JValue::Int(u32::from(account_index) as i32), (&ufvk).into(), ], ) @@ -336,6 +341,13 @@ pub extern "C" fn Java_cash_z_ecc_android_sdk_internal_jni_RustBackend_getAccoun }) .collect::, _>>()?; + // Filter out imported accounts (for which we don't know a ZIP 32 account index). + // TODO: Remove this when we switch to account identifiers. + let accounts = accounts + .into_iter() + .filter(|account| matches!(account.source(), AccountSource::Derived { .. })) + .collect::>(); + let first_account = accounts.first().cloned(); Ok(utils::rust_vec_to_java( diff --git a/sdk-lib/src/main/java/cash/z/ecc/android/sdk/internal/TypesafeBackendImpl.kt b/sdk-lib/src/main/java/cash/z/ecc/android/sdk/internal/TypesafeBackendImpl.kt index 118c9ce91..15a198745 100644 --- a/sdk-lib/src/main/java/cash/z/ecc/android/sdk/internal/TypesafeBackendImpl.kt +++ b/sdk-lib/src/main/java/cash/z/ecc/android/sdk/internal/TypesafeBackendImpl.kt @@ -27,7 +27,7 @@ internal class TypesafeBackendImpl(private val backend: Backend) : TypesafeBacke override val network: ZcashNetwork get() = ZcashNetwork.from(backend.networkId) - override suspend fun getAccounts(): List = backend.getAccounts().map { Account(it.accountId.toInt()) } + override suspend fun getAccounts(): List = backend.getAccounts().map { Account(it.accountIndex.toInt()) } override suspend fun createAccountAndGetSpendingKey( seed: ByteArray,