From 670ec68cd511d6fcb20d389b0947ce6636db5a64 Mon Sep 17 00:00:00 2001 From: Kevin Gorham Date: Wed, 5 May 2021 14:26:13 -0400 Subject: [PATCH 1/6] New: Improve behavior and messaging when an account is missing. Scanning without an account setup is a programming error and prior to this change it wasted a lot of resources and would always crash eventually. Now, this error is caught sooner and surfaced with a clear message. --- .../sdk/block/CompactBlockProcessor.kt | 25 +++++++++++-------- .../z/ecc/android/sdk/db/DerivedDataDb.kt | 3 +++ .../z/ecc/android/sdk/exception/Exceptions.kt | 4 +++ .../transaction/PagedTransactionRepository.kt | 2 ++ .../sdk/transaction/TransactionRepository.kt | 2 ++ src/main/rust/lib.rs | 4 +-- 6 files changed, 27 insertions(+), 13 deletions(-) diff --git a/src/main/java/cash/z/ecc/android/sdk/block/CompactBlockProcessor.kt b/src/main/java/cash/z/ecc/android/sdk/block/CompactBlockProcessor.kt index 28975dd6..0b21f67e 100644 --- a/src/main/java/cash/z/ecc/android/sdk/block/CompactBlockProcessor.kt +++ b/src/main/java/cash/z/ecc/android/sdk/block/CompactBlockProcessor.kt @@ -375,22 +375,25 @@ class CompactBlockProcessor( */ private suspend fun verifySetup() { // verify that the data is initialized - var error = if (!repository.isInitialized()) { - CompactBlockProcessorException.Uninitialized - } else { - // verify that the server is correct - downloader.getServerInfo().let { info -> - val clientBranch = "%x".format(rustBackend.getBranchIdForHeight(info.blockHeight.toInt())) - val network = rustBackend.network.networkName - when { - !info.matchingNetwork(network) -> MismatchedNetwork(clientNetwork = network, serverNetwork = info.chainName) - !info.matchingConsensusBranchId(clientBranch) -> MismatchedBranch(clientBranch = clientBranch, serverBranch = info.consensusBranchId, networkName = network) - else -> null + var error = when { + !repository.isInitialized() -> CompactBlockProcessorException.Uninitialized + repository.getAccountCount() == 0 -> CompactBlockProcessorException.NoAccount + else -> { + // verify that the server is correct + downloader.getServerInfo().let { info -> + val clientBranch = "%x".format(rustBackend.getBranchIdForHeight(info.blockHeight.toInt())) + val network = rustBackend.network.networkName + when { + !info.matchingNetwork(network) -> MismatchedNetwork(clientNetwork = network, serverNetwork = info.chainName) + !info.matchingConsensusBranchId(clientBranch) -> MismatchedBranch(clientBranch = clientBranch, serverBranch = info.consensusBranchId, networkName = network) + else -> null + } } } } if (error != null) { + twig("Validating setup prior to scanning . . . ISSUE FOUND! - ${error.javaClass.simpleName}") // give listener a chance to override if (onSetupErrorListener?.invoke(error) != true) { throw error diff --git a/src/main/java/cash/z/ecc/android/sdk/db/DerivedDataDb.kt b/src/main/java/cash/z/ecc/android/sdk/db/DerivedDataDb.kt index 49e2f362..64b22f46 100644 --- a/src/main/java/cash/z/ecc/android/sdk/db/DerivedDataDb.kt +++ b/src/main/java/cash/z/ecc/android/sdk/db/DerivedDataDb.kt @@ -227,6 +227,9 @@ interface SentDao { @Dao interface AccountDao { + @Query("SELECT COUNT(account) FROM accounts") + fun count(): Int + @Query( """ SELECT account AS accountId, diff --git a/src/main/java/cash/z/ecc/android/sdk/exception/Exceptions.kt b/src/main/java/cash/z/ecc/android/sdk/exception/Exceptions.kt index d1ae925a..9e34cb37 100644 --- a/src/main/java/cash/z/ecc/android/sdk/exception/Exceptions.kt +++ b/src/main/java/cash/z/ecc/android/sdk/exception/Exceptions.kt @@ -83,6 +83,10 @@ sealed class CompactBlockProcessorException(message: String, cause: Throwable? = " initialized. Verify that the seed phrase was properly created or imported. If so, then this problem" + " can be fixed by re-importing the wallet." ) + object NoAccount : CompactBlockProcessorException( + "Attempting to scan without an account. This is probably a setup error or a race condition." + ) + open class EnhanceTransactionError(message: String, val height: Int, cause: Throwable) : CompactBlockProcessorException(message, cause) { class EnhanceTxDownloadError(height: Int, cause: Throwable) : EnhanceTransactionError("Error while attempting to download a transaction to enhance", height, cause) class EnhanceTxDecryptError(height: Int, cause: Throwable) : EnhanceTransactionError("Error while attempting to decrypt and store a transaction to enhance", height, cause) diff --git a/src/main/java/cash/z/ecc/android/sdk/transaction/PagedTransactionRepository.kt b/src/main/java/cash/z/ecc/android/sdk/transaction/PagedTransactionRepository.kt index 6c9ec2c3..680183dc 100644 --- a/src/main/java/cash/z/ecc/android/sdk/transaction/PagedTransactionRepository.kt +++ b/src/main/java/cash/z/ecc/android/sdk/transaction/PagedTransactionRepository.kt @@ -105,6 +105,8 @@ open class PagedTransactionRepository( override suspend fun getAccount(accountId: Int): UnifiedAddressAccount? = accounts.findAccountById(accountId) + override suspend fun getAccountCount(): Int = accounts.count() + /** * Close the underlying database. */ diff --git a/src/main/java/cash/z/ecc/android/sdk/transaction/TransactionRepository.kt b/src/main/java/cash/z/ecc/android/sdk/transaction/TransactionRepository.kt index f003ede3..14d6807b 100644 --- a/src/main/java/cash/z/ecc/android/sdk/transaction/TransactionRepository.kt +++ b/src/main/java/cash/z/ecc/android/sdk/transaction/TransactionRepository.kt @@ -86,6 +86,8 @@ interface TransactionRepository { suspend fun getAccount(accountId: Int): UnifiedAddressAccount? + suspend fun getAccountCount(): Int + // // Transactions // diff --git a/src/main/rust/lib.rs b/src/main/rust/lib.rs index 3e720072..caf439c2 100644 --- a/src/main/rust/lib.rs +++ b/src/main/rust/lib.rs @@ -789,7 +789,7 @@ pub unsafe extern "C" fn Java_cash_z_ecc_android_sdk_jni_RustBackend_scanBlocks( match scan_cached_blocks(&network, &db_cache, &mut db_data, None) { Ok(()) => Ok(JNI_TRUE), - Err(e) => Err(format_err!("Error while scanning blocks: {}", e)), + Err(e) => Err(format_err!("Rust error while scanning blocks: {}", e)), } }); unwrap_exc_or(&env, res, JNI_FALSE) @@ -884,7 +884,7 @@ pub unsafe extern "C" fn Java_cash_z_ecc_android_sdk_jni_RustBackend_scanBlockBa match scan_cached_blocks(&network, &db_cache, &mut db_data, Some(limit as u32)) { Ok(()) => Ok(JNI_TRUE), - Err(e) => Err(format_err!("Error while scanning blocks: {}", e)), + Err(e) => Err(format_err!("Rust error while scanning block batch: {}", e)), } }); unwrap_exc_or(&env, res, JNI_FALSE) From 8fb56ff80c9b2ecbd4e2afa83f7f811ad54603cd Mon Sep 17 00:00:00 2001 From: Kevin Gorham Date: Fri, 7 May 2021 03:54:18 -0400 Subject: [PATCH 2/6] Improvement: Simplify Initializer to get rid of unnecessary inheritance. --- .../java/cash/z/ecc/android/sdk/Initializer.kt | 16 ++++++++-------- .../cash/z/ecc/android/sdk/SdkSynchronizer.kt | 12 +----------- 2 files changed, 9 insertions(+), 19 deletions(-) diff --git a/src/main/java/cash/z/ecc/android/sdk/Initializer.kt b/src/main/java/cash/z/ecc/android/sdk/Initializer.kt index 74a3eb41..e673c512 100644 --- a/src/main/java/cash/z/ecc/android/sdk/Initializer.kt +++ b/src/main/java/cash/z/ecc/android/sdk/Initializer.kt @@ -16,13 +16,13 @@ import java.io.File /** * Simplified Initializer focused on starting from a ViewingKey. */ -class Initializer constructor(appContext: Context, onCriticalErrorHandler: ((Throwable?) -> Boolean)? = null, config: Config) : SdkSynchronizer.SdkInitializer { - override val context = appContext.applicationContext - override val rustBackend: RustBackend - override val network: ZcashNetwork - override val alias: String - override val host: String - override val port: Int +class Initializer constructor(appContext: Context, onCriticalErrorHandler: ((Throwable?) -> Boolean)? = null, config: Config) { + val context = appContext.applicationContext + val rustBackend: RustBackend + val network: ZcashNetwork + val alias: String + val host: String + val port: Int val viewingKeys: List val birthday: WalletBirthday @@ -32,7 +32,7 @@ class Initializer constructor(appContext: Context, onCriticalErrorHandler: ((Thr * function has a return value is so that all error handlers work with the same signature which * allows one function to handle all errors in simple apps. */ - override var onCriticalErrorHandler: ((Throwable?) -> Boolean)? = onCriticalErrorHandler + var onCriticalErrorHandler: ((Throwable?) -> Boolean)? = onCriticalErrorHandler /** * True when accounts have been created by this initializer. diff --git a/src/main/java/cash/z/ecc/android/sdk/SdkSynchronizer.kt b/src/main/java/cash/z/ecc/android/sdk/SdkSynchronizer.kt index 301e1911..d9241a35 100644 --- a/src/main/java/cash/z/ecc/android/sdk/SdkSynchronizer.kt +++ b/src/main/java/cash/z/ecc/android/sdk/SdkSynchronizer.kt @@ -691,16 +691,6 @@ class SdkSynchronizer internal constructor( ) } - interface SdkInitializer { - val context: Context - val rustBackend: RustBackend - val network: ZcashNetwork - val host: String - val port: Int - val alias: String - val onCriticalErrorHandler: ((Throwable?) -> Boolean)? - } - interface Erasable { /** * Erase content related to this SDK. @@ -760,7 +750,7 @@ class SdkSynchronizer internal constructor( */ @Suppress("FunctionName") fun Synchronizer( - initializer: SdkSynchronizer.SdkInitializer, + initializer: Initializer, repository: TransactionRepository = PagedTransactionRepository(initializer.context, 1000, initializer.rustBackend.pathDataDb), // TODO: fix this pagesize bug, small pages should not crash the app. It crashes with: Uncaught Exception: android.view.ViewRootImpl$CalledFromWrongThreadException: Only the original thread that created a view hierarchy can touch its views. and is probably related to FlowPagedList blockStore: CompactBlockStore = CompactBlockDbStore(initializer.context, initializer.rustBackend.pathCacheDb), From fc7cead1f6461de95874875393391fc14a1a0b17 Mon Sep 17 00:00:00 2001 From: Kevin Gorham Date: Fri, 7 May 2021 03:56:26 -0400 Subject: [PATCH 3/6] New: Add the concept of 'prepare'. begin adding a step between the creation of a Synchronizer and starting it, called 'prepare' which is responsible for migrations and other steps to get the data ready for syncing. --- .../java/cash/z/ecc/android/sdk/SdkSynchronizer.kt | 7 +++++++ .../java/cash/z/ecc/android/sdk/Synchronizer.kt | 14 ++++++++++++++ .../cash/z/ecc/android/sdk/exception/Exceptions.kt | 5 +++++ .../sdk/transaction/TransactionRepository.kt | 2 ++ 4 files changed, 28 insertions(+) diff --git a/src/main/java/cash/z/ecc/android/sdk/SdkSynchronizer.kt b/src/main/java/cash/z/ecc/android/sdk/SdkSynchronizer.kt index d9241a35..565667cf 100644 --- a/src/main/java/cash/z/ecc/android/sdk/SdkSynchronizer.kt +++ b/src/main/java/cash/z/ecc/android/sdk/SdkSynchronizer.kt @@ -233,6 +233,10 @@ class SdkSynchronizer internal constructor( override val latestBirthdayHeight: Int get() = processor.birthdayHeight + override fun prepare(): Synchronizer = apply { + storage.prepare() + } + /** * Starts this synchronizer within the given scope. For simplicity, attempting to start an * instance that has already been started will throw a [SynchronizerException.FalseStart] @@ -362,6 +366,9 @@ class SdkSynchronizer internal constructor( } private fun CoroutineScope.onReady() = launch(CoroutineExceptionHandler(::onCriticalError)) { + twig("Preparing to start...") + prepare() + twig("Synchronizer (${this@SdkSynchronizer}) Ready. Starting processor!") var lastScanTime = 0L processor.onProcessorErrorListener = ::onProcessorError diff --git a/src/main/java/cash/z/ecc/android/sdk/Synchronizer.kt b/src/main/java/cash/z/ecc/android/sdk/Synchronizer.kt index dea4d88a..b27c5972 100644 --- a/src/main/java/cash/z/ecc/android/sdk/Synchronizer.kt +++ b/src/main/java/cash/z/ecc/android/sdk/Synchronizer.kt @@ -30,6 +30,13 @@ interface Synchronizer { */ var isStarted: Boolean + /** + * Prepare the synchronizer to start. Must be called before start. This gives a clear point + * where setup and maintenance can occur for various Synchronizers. One that uses a database + * would take this opportunity to do data migrations or key migrations. + */ + fun prepare(): Synchronizer + /** * Starts this synchronizer within the given scope. * @@ -358,6 +365,13 @@ interface Synchronizer { */ DISCONNECTED, + /** + * Indicates that this Synchronizer is actively preparing to start, which usually involves + * setting up database tables, migrations or taking other maintenance steps that need to + * occur after an upgrade. + */ + PREPARING, + /** * Indicates that this Synchronizer is actively downloading new blocks from the server. */ diff --git a/src/main/java/cash/z/ecc/android/sdk/exception/Exceptions.kt b/src/main/java/cash/z/ecc/android/sdk/exception/Exceptions.kt index 9e34cb37..35263599 100644 --- a/src/main/java/cash/z/ecc/android/sdk/exception/Exceptions.kt +++ b/src/main/java/cash/z/ecc/android/sdk/exception/Exceptions.kt @@ -32,6 +32,11 @@ sealed class RepositoryException(message: String, cause: Throwable? = null) : Sd "The channel is closed. Note that once a repository has stopped it " + "cannot be restarted. Verify that the repository is not being restarted." ) + object Unprepared : RepositoryException( + "Unprepared repository: Data cannot be accessed before the repository is prepared." + + " Ensure that things have been properly initialized. In most cases, this involves" + + " calling 'synchronizer.prepare' before 'synchronizer.start'" + ) } /** diff --git a/src/main/java/cash/z/ecc/android/sdk/transaction/TransactionRepository.kt b/src/main/java/cash/z/ecc/android/sdk/transaction/TransactionRepository.kt index 14d6807b..13081628 100644 --- a/src/main/java/cash/z/ecc/android/sdk/transaction/TransactionRepository.kt +++ b/src/main/java/cash/z/ecc/android/sdk/transaction/TransactionRepository.kt @@ -88,6 +88,8 @@ interface TransactionRepository { suspend fun getAccountCount(): Int + fun prepare() + // // Transactions // From 44d1d5520152d53f573f8df28095fbdf3506a2f2 Mon Sep 17 00:00:00 2001 From: Kevin Gorham Date: Fri, 7 May 2021 03:59:47 -0400 Subject: [PATCH 4/6] Improvement: Refactor the initializer and move all DB creation code. Move the code to create, migrate and populate data from the initializer over to the repository. Both classes are now much simpler. --- .../cash/z/ecc/android/sdk/Initializer.kt | 74 +---- .../cash/z/ecc/android/sdk/SdkSynchronizer.kt | 10 +- .../transaction/PagedTransactionRepository.kt | 263 +++++++++++++----- 3 files changed, 200 insertions(+), 147 deletions(-) diff --git a/src/main/java/cash/z/ecc/android/sdk/Initializer.kt b/src/main/java/cash/z/ecc/android/sdk/Initializer.kt index e673c512..cfea3a79 100644 --- a/src/main/java/cash/z/ecc/android/sdk/Initializer.kt +++ b/src/main/java/cash/z/ecc/android/sdk/Initializer.kt @@ -3,7 +3,6 @@ package cash.z.ecc.android.sdk import android.content.Context import cash.z.ecc.android.sdk.exception.InitializerException import cash.z.ecc.android.sdk.ext.ZcashSdk -import cash.z.ecc.android.sdk.ext.tryWarn import cash.z.ecc.android.sdk.ext.twig import cash.z.ecc.android.sdk.jni.RustBackend import cash.z.ecc.android.sdk.tool.DerivationTool @@ -24,6 +23,7 @@ class Initializer constructor(appContext: Context, onCriticalErrorHandler: ((Thr val host: String val port: Int val viewingKeys: List + val overwriteVks: Boolean val birthday: WalletBirthday /** @@ -51,13 +51,11 @@ class Initializer constructor(appContext: Context, onCriticalErrorHandler: ((Thr val loadedBirthday = WalletBirthdayTool.loadNearest(context, network, heightToUse) birthday = loadedBirthday viewingKeys = config.viewingKeys + overwriteVks = config.overwriteVks alias = config.alias host = config.host port = config.port rustBackend = initRustBackend(network, birthday) - if (config.resetAccounts) resetAccounts() - // TODO: get rid of this by first answering the question: why is this necessary? - initMissingDatabases(birthday, *viewingKeys.toTypedArray()) } catch (t: Throwable) { onCriticalError(t) throw t @@ -79,68 +77,6 @@ class Initializer constructor(appContext: Context, onCriticalErrorHandler: ((Thr ) } - private fun initMissingDatabases( - birthday: WalletBirthday, - vararg viewingKeys: UnifiedViewingKey - ) { - maybeCreateDataDb() - maybeInitBlocksTable(birthday) - maybeInitAccountsTable(*viewingKeys) - } - - private fun resetAccounts() { - // Short-term fix: drop and recreate accounts for key migration - tryWarn("Warning: did not drop the accounts table. It probably did not yet exist.") { - rustBackend.dropAccountsTable() - twig("Reset accounts table to allow for key migration") - } - } - - /** - * Create the dataDb and its table, if it doesn't exist. - */ - private fun maybeCreateDataDb() { - tryWarn("Warning: did not create dataDb. It probably already exists.") { - rustBackend.initDataDb() - twig("Initialized wallet for first run") - } - } - - /** - * Initialize the blocks table with the given birthday, if needed. - */ - private fun maybeInitBlocksTable(birthday: WalletBirthday) { - // TODO: consider converting these to typed exceptions in the welding layer - tryWarn( - "Warning: did not initialize the blocks table. It probably was already initialized.", - ifContains = "table is not empty" - ) { - rustBackend.initBlocksTable( - birthday.height, - birthday.hash, - birthday.time, - birthday.tree - ) - twig("seeded the database with sapling tree at height ${birthday.height}") - } - twig("database file: ${rustBackend.pathDataDb}") - } - - /** - * Initialize the accounts table with the given viewing keys. - */ - private fun maybeInitAccountsTable(vararg viewingKeys: UnifiedViewingKey) { - // TODO: consider converting these to typed exceptions in the welding layer - tryWarn( - "Warning: did not initialize the accounts table. It probably was already initialized.", - ifContains = "table is not empty" - ) { - rustBackend.initAccountsTable(*viewingKeys) - accountsCreated = true - twig("Initialized the accounts table with ${viewingKeys.size} viewingKey(s)") - } - } - private fun onCriticalError(error: Throwable) { twig("********") twig("******** INITIALIZER ERROR: $error") @@ -192,7 +128,7 @@ class Initializer constructor(appContext: Context, onCriticalErrorHandler: ((Thr var defaultToOldestHeight: Boolean? = null private set - var resetAccounts: Boolean = false + var overwriteVks: Boolean = false private set constructor(block: (Config) -> Unit) : this() { @@ -250,7 +186,7 @@ class Initializer constructor(appContext: Context, onCriticalErrorHandler: ((Thr * probably has serious bugs. */ fun setViewingKeys(vararg unifiedViewingKeys: UnifiedViewingKey, overwrite: Boolean = false): Config = apply { - resetAccounts = overwrite + overwriteVks = overwrite viewingKeys.apply { clear() addAll(unifiedViewingKeys) @@ -258,7 +194,7 @@ class Initializer constructor(appContext: Context, onCriticalErrorHandler: ((Thr } fun setOverwriteKeys(isOverwrite: Boolean) { - resetAccounts = isOverwrite + overwriteVks = isOverwrite } /** diff --git a/src/main/java/cash/z/ecc/android/sdk/SdkSynchronizer.kt b/src/main/java/cash/z/ecc/android/sdk/SdkSynchronizer.kt index 565667cf..115fa5ca 100644 --- a/src/main/java/cash/z/ecc/android/sdk/SdkSynchronizer.kt +++ b/src/main/java/cash/z/ecc/android/sdk/SdkSynchronizer.kt @@ -38,7 +38,6 @@ import cash.z.ecc.android.sdk.ext.toHexReversed import cash.z.ecc.android.sdk.ext.tryNull import cash.z.ecc.android.sdk.ext.twig import cash.z.ecc.android.sdk.ext.twigTask -import cash.z.ecc.android.sdk.jni.RustBackend import cash.z.ecc.android.sdk.service.LightWalletGrpcService import cash.z.ecc.android.sdk.service.LightWalletService import cash.z.ecc.android.sdk.tool.DerivationTool @@ -138,10 +137,10 @@ class SdkSynchronizer internal constructor( // override val balances: Flow = _balances.asFlow() - override val clearedTransactions = storage.allTransactions + override val clearedTransactions get() = storage.allTransactions override val pendingTransactions = txManager.getAll() - override val sentTransactions = storage.sentTransactions - override val receivedTransactions = storage.receivedTransactions + override val sentTransactions get() = storage.sentTransactions + override val receivedTransactions get() = storage.receivedTransactions // // Status @@ -402,6 +401,7 @@ class SdkSynchronizer internal constructor( private fun onCriticalError(unused: CoroutineContext?, error: Throwable) { twig("********") twig("******** ERROR: $error") + twig(error) if (error.cause != null) twig("******** caused by ${error.cause}") if (error.cause?.cause != null) twig("******** caused by ${error.cause?.cause}") twig("********") @@ -759,7 +759,7 @@ class SdkSynchronizer internal constructor( fun Synchronizer( initializer: Initializer, repository: TransactionRepository = - PagedTransactionRepository(initializer.context, 1000, initializer.rustBackend.pathDataDb), // TODO: fix this pagesize bug, small pages should not crash the app. It crashes with: Uncaught Exception: android.view.ViewRootImpl$CalledFromWrongThreadException: Only the original thread that created a view hierarchy can touch its views. and is probably related to FlowPagedList + PagedTransactionRepository(initializer.context, 1000, initializer.rustBackend, initializer.birthday, initializer.viewingKeys, initializer.overwriteVks), // TODO: fix this pagesize bug, small pages should not crash the app. It crashes with: Uncaught Exception: android.view.ViewRootImpl$CalledFromWrongThreadException: Only the original thread that created a view hierarchy can touch its views. and is probably related to FlowPagedList blockStore: CompactBlockStore = CompactBlockDbStore(initializer.context, initializer.rustBackend.pathCacheDb), service: LightWalletService = LightWalletGrpcService(initializer.context, initializer.host, initializer.port), encoder: TransactionEncoder = WalletTransactionEncoder(initializer.rustBackend, repository), diff --git a/src/main/java/cash/z/ecc/android/sdk/transaction/PagedTransactionRepository.kt b/src/main/java/cash/z/ecc/android/sdk/transaction/PagedTransactionRepository.kt index 680183dc..4fbaca81 100644 --- a/src/main/java/cash/z/ecc/android/sdk/transaction/PagedTransactionRepository.kt +++ b/src/main/java/cash/z/ecc/android/sdk/transaction/PagedTransactionRepository.kt @@ -9,12 +9,21 @@ import cash.z.ecc.android.sdk.db.BlockDao import cash.z.ecc.android.sdk.db.DerivedDataDb import cash.z.ecc.android.sdk.db.TransactionDao import cash.z.ecc.android.sdk.db.entity.ConfirmedTransaction +import cash.z.ecc.android.sdk.exception.RepositoryException import cash.z.ecc.android.sdk.ext.ZcashSdk +import cash.z.ecc.android.sdk.ext.android.RefreshableDataSourceFactory import cash.z.ecc.android.sdk.ext.android.toFlowPagedList import cash.z.ecc.android.sdk.ext.android.toRefreshable +import cash.z.ecc.android.sdk.ext.tryWarn +import cash.z.ecc.android.sdk.ext.twig +import cash.z.ecc.android.sdk.jni.RustBackend import cash.z.ecc.android.sdk.type.UnifiedAddressAccount +import cash.z.ecc.android.sdk.type.UnifiedViewingKey +import cash.z.ecc.android.sdk.type.WalletBirthday import kotlinx.coroutines.Dispatchers.IO +import kotlinx.coroutines.flow.Flow import kotlinx.coroutines.withContext +import java.util.concurrent.atomic.AtomicBoolean /** * Example of a repository that leverages the Room paging library to return a [PagedList] of @@ -23,100 +32,208 @@ import kotlinx.coroutines.withContext * * @param pageSize transactions per page. This influences pre-fetch and memory configuration. */ -open class PagedTransactionRepository( - open val derivedDataDb: DerivedDataDb, - open val pageSize: Int = 10 +class PagedTransactionRepository( + val appContext: Context, + val pageSize: Int = 10, + val rustBackend: RustBackend, + val birthday: WalletBirthday, + val viewingKeys: List, + val overwriteVks: Boolean = false, ) : TransactionRepository { + private val lazy = LazyPropertyHolder() + + override val receivedTransactions get() = lazy.receivedTransactions + override val sentTransactions get() = lazy.sentTransactions + override val allTransactions get() = lazy.allTransactions + + // + // TransactionRepository API + // + + override fun invalidate() = lazy.allTransactionsFactory.refresh() + + override fun lastScannedHeight(): Int { + return lazy.blocks.lastScannedHeight() + } + + override fun firstScannedHeight(): Int { + return lazy.blocks.firstScannedHeight() + } + + override fun isInitialized(): Boolean { + return lazy.blocks.count() > 0 + } + + override suspend fun findEncodedTransactionById(txId: Long) = withContext(IO) { + lazy.transactions.findEncodedTransactionById(txId) + } + + override suspend fun findNewTransactions(blockHeightRange: IntRange): List = + lazy.transactions.findAllTransactionsByRange(blockHeightRange.first, blockHeightRange.last) + + override suspend fun findMinedHeight(rawTransactionId: ByteArray) = withContext(IO) { + lazy.transactions.findMinedHeight(rawTransactionId) + } + + override suspend fun findMatchingTransactionId(rawTransactionId: ByteArray): Long? = + lazy.transactions.findMatchingTransactionId(rawTransactionId) + + override suspend fun cleanupCancelledTx(rawTransactionId: ByteArray) = lazy.transactions.cleanupCancelledTx(rawTransactionId) + override suspend fun deleteExpired(lastScannedHeight: Int): Int { + // let expired transactions linger in the UI for a little while + return lazy.transactions.deleteExpired(lastScannedHeight - (ZcashSdk.EXPIRY_OFFSET / 2)) + } + override suspend fun count(): Int = withContext(IO) { + lazy.transactions.count() + } + + override suspend fun getAccount(accountId: Int): UnifiedAddressAccount? = lazy.accounts.findAccountById(accountId) + + override suspend fun getAccountCount(): Int = lazy.accounts.count() + + override fun prepare() { + twig("Preparing repository for use...") + initMissingDatabases() + // provide the database to all the lazy properties that are waiting for it to exist + lazy.db = buildDatabase() + applyKeyMigrations() + } + /** - * Constructor that creates the database. + * Create any databases that don't already exist via Rust. Originally, this was done on the Rust + * side because Rust was intended to own the "dataDb" and Kotlin just reads from it. Since then, + * it has been more clear that Kotlin should own the data and just let Rust use it. */ - constructor( - context: Context, - pageSize: Int = 10, - dataDbName: String = ZcashSdk.DB_DATA_NAME - ) : this( - Room.databaseBuilder(context, DerivedDataDb::class.java, dataDbName) + private fun initMissingDatabases() { + maybeCreateDataDb() + maybeInitBlocksTable(birthday) + maybeInitAccountsTable(viewingKeys) + } + + /** + * Create the dataDb and its table, if it doesn't exist. + */ + private fun maybeCreateDataDb() { + tryWarn("Warning: did not create dataDb. It probably already exists.") { + rustBackend.initDataDb() + twig("Initialized wallet for first run file: ${rustBackend.pathDataDb}") + } + } + + /** + * Initialize the blocks table with the given birthday, if needed. + */ + private fun maybeInitBlocksTable(birthday: WalletBirthday) { + // TODO: consider converting these to typed exceptions in the welding layer + tryWarn( + "Warning: did not initialize the blocks table. It probably was already initialized.", + ifContains = "table is not empty" + ) { + rustBackend.initBlocksTable( + birthday.height, + birthday.hash, + birthday.time, + birthday.tree + ) + twig("seeded the database with sapling tree at height ${birthday.height}") + } + twig("database file: ${rustBackend.pathDataDb}") + } + + /** + * Initialize the accounts table with the given viewing keys. + */ + private fun maybeInitAccountsTable(viewingKeys: List) { + // TODO: consider converting these to typed exceptions in the welding layer + tryWarn( + "Warning: did not initialize the accounts table. It probably was already initialized.", + ifContains = "table is not empty" + ) { + rustBackend.initAccountsTable(*viewingKeys.toTypedArray()) + twig("Initialized the accounts table with ${viewingKeys.size} viewingKey(s)") + } + } + + /** + * Build the database and apply migrations. + */ + private fun buildDatabase(): DerivedDataDb { + twig("Building dataDb and applying migrations") + return Room.databaseBuilder(appContext, DerivedDataDb::class.java, rustBackend.pathDataDb) .setJournalMode(RoomDatabase.JournalMode.TRUNCATE) .addMigrations(DerivedDataDb.MIGRATION_3_4) .addMigrations(DerivedDataDb.MIGRATION_4_3) .addMigrations(DerivedDataDb.MIGRATION_4_5) .addMigrations(DerivedDataDb.MIGRATION_5_6) .addMigrations(DerivedDataDb.MIGRATION_6_7) - .build(), - pageSize - ) - init { - derivedDataDb.openHelper.writableDatabase.beginTransaction() - derivedDataDb.openHelper.writableDatabase.endTransaction() + .build().also { + // TODO: document why we do this. My guess is to catch database issues early or to trigger migrations--I forget why it was added but there was a good reason? + it.openHelper.writableDatabase.beginTransaction() + it.openHelper.writableDatabase.endTransaction() + } } - private val blocks: BlockDao = derivedDataDb.blockDao() - private val accounts: AccountDao = derivedDataDb.accountDao() - private val transactions: TransactionDao = derivedDataDb.transactionDao() - private val receivedTxDataSourceFactory = transactions.getReceivedTransactions().toRefreshable() - private val sentTxDataSourceFactory = transactions.getSentTransactions().toRefreshable() - private val allTxDataSourceFactory = transactions.getAllTransactions().toRefreshable() - - // - // TransactionRepository API - // - - override val receivedTransactions = receivedTxDataSourceFactory.toFlowPagedList(pageSize) - override val sentTransactions = sentTxDataSourceFactory.toFlowPagedList(pageSize) - override val allTransactions = allTxDataSourceFactory.toFlowPagedList(pageSize) - - override fun invalidate() = allTxDataSourceFactory.refresh() - - override fun lastScannedHeight(): Int { - return blocks.lastScannedHeight() + private fun applyKeyMigrations() { + if (overwriteVks) { + twig("applying key migrations . . .") + maybeInitAccountsTable(viewingKeys) + } } - override fun firstScannedHeight(): Int { - return blocks.firstScannedHeight() - } - - override fun isInitialized(): Boolean { - return blocks.count() > 0 - } - - override suspend fun findEncodedTransactionById(txId: Long) = withContext(IO) { - transactions.findEncodedTransactionById(txId) - } - - override suspend fun findNewTransactions(blockHeightRange: IntRange): List = - transactions.findAllTransactionsByRange(blockHeightRange.first, blockHeightRange.last) - - override suspend fun findMinedHeight(rawTransactionId: ByteArray) = withContext(IO) { - transactions.findMinedHeight(rawTransactionId) - } - - override suspend fun findMatchingTransactionId(rawTransactionId: ByteArray): Long? = - transactions.findMatchingTransactionId(rawTransactionId) - - override suspend fun cleanupCancelledTx(rawTransactionId: ByteArray) = transactions.cleanupCancelledTx(rawTransactionId) - override suspend fun deleteExpired(lastScannedHeight: Int): Int { - // let expired transactions linger in the UI for a little while - return transactions.deleteExpired(lastScannedHeight - (ZcashSdk.EXPIRY_OFFSET / 2)) - } - override suspend fun count(): Int = withContext(IO) { - transactions.count() - } - - override suspend fun getAccount(accountId: Int): UnifiedAddressAccount? = accounts.findAccountById(accountId) - - override suspend fun getAccountCount(): Int = accounts.count() - /** * Close the underlying database. */ fun close() { - derivedDataDb.close() + lazy.db?.close() } // TODO: begin converting these into Data Access API. For now, just collect the desired operations and iterate/refactor, later - fun findBlockHash(height: Int): ByteArray? = blocks.findHashByHeight(height) - fun getTransactionCount(): Int = transactions.count() + fun findBlockHash(height: Int): ByteArray? = lazy.blocks.findHashByHeight(height) + fun getTransactionCount(): Int = lazy.transactions.count() // TODO: convert this into a wallet repository rather than "transaction repository" + + /** + * Helper class that holds all the properties that depend on the database being prepared. If any + * properties are accessed before then, it results in an Unprepared Exception. + */ + inner class LazyPropertyHolder { + var isPrepared = AtomicBoolean(false) + var db: DerivedDataDb? = null + set(value) { + field = value + if (value != null) isPrepared.set(true) + } + + // DAOs + val blocks: BlockDao by lazyDb { db!!.blockDao() } + val accounts: AccountDao by lazyDb { db!!.accountDao() } + val transactions: TransactionDao by lazyDb { db!!.transactionDao() } + + // Transaction Flows + val allTransactionsFactory: RefreshableDataSourceFactory by lazyDb { + transactions.getAllTransactions().toRefreshable() + } + val allTransactions: Flow> by lazyDb { + allTransactionsFactory.toFlowPagedList(pageSize) + } + val receivedTransactions: Flow> by lazyDb { + transactions.getReceivedTransactions().toRefreshable().toFlowPagedList(pageSize) + } + val sentTransactions: Flow> by lazyDb { + transactions.getSentTransactions().toRefreshable().toFlowPagedList(pageSize) + } + + /** + * If isPrepared is true, execute the given block and cache the value, always returning it + * to future requests. Otherwise, throw an Unprepared exception. + */ + inline fun lazyDb(crossinline block: () -> T) = object : Lazy { + val cached: T? = null + override val value: T + get() = cached ?: if (isPrepared.get()) block() else throw RepositoryException.Unprepared + override fun isInitialized(): Boolean = cached != null + } + } } From f9feb688cbe41b745947544717770a59831a7193 Mon Sep 17 00:00:00 2001 From: Kevin Gorham Date: Fri, 7 May 2021 04:00:32 -0400 Subject: [PATCH 5/6] Improvement: parameterize test to run on both mainnet and testnet. --- .../z/ecc/android/sdk/jni/TransparentTest.kt | 34 +++++++------------ 1 file changed, 13 insertions(+), 21 deletions(-) diff --git a/src/androidTest/java/cash/z/ecc/android/sdk/jni/TransparentTest.kt b/src/androidTest/java/cash/z/ecc/android/sdk/jni/TransparentTest.kt index 3fd3feab..308070bf 100644 --- a/src/androidTest/java/cash/z/ecc/android/sdk/jni/TransparentTest.kt +++ b/src/androidTest/java/cash/z/ecc/android/sdk/jni/TransparentTest.kt @@ -1,6 +1,5 @@ package cash.z.ecc.android.sdk.jni -import androidx.test.ext.junit.runners.AndroidJUnit4 import androidx.test.filters.SmallTest import cash.z.ecc.android.bip39.Mnemonics.MnemonicCode import cash.z.ecc.android.bip39.toSeed @@ -11,30 +10,15 @@ import cash.z.ecc.android.sdk.ext.Twig import cash.z.ecc.android.sdk.tool.DerivationTool import cash.z.ecc.android.sdk.type.ZcashNetwork import org.junit.Assert.assertEquals -import org.junit.Before import org.junit.BeforeClass import org.junit.Test import org.junit.runner.RunWith +import org.junit.runners.Parameterized @MaintainedTest(TestPurpose.REGRESSION) -@RunWith(AndroidJUnit4::class) +@RunWith(Parameterized::class) @SmallTest -class TransparentTest { - - lateinit var expected: Expected - lateinit var network: ZcashNetwork - - @Before - fun setup() { - // TODO: parameterize this for both networks -// if (BuildConfig.FLAVOR == "zcashtestnet") { - expected = ExpectedTestnet - network = ZcashNetwork.Mainnet -// } else { -// expected = ExpectedMainnet -// network = ZcashNetwork.Mainnet -// } - } +class TransparentTest(val expected: Expected, val network: ZcashNetwork) { @Test fun deriveTransparentSecretKeyTest() { @@ -48,7 +32,8 @@ class TransparentTest { @Test fun deriveTransparentAddressFromSecretKeyTest() { - assertEquals(expected.tAddr, DerivationTool.deriveSpendingKeys(SEED, network = network)[0]) + val pk = DerivationTool.deriveTransparentSecretKey(SEED, network = network) + assertEquals(expected.tAddr, DerivationTool.deriveTransparentAddressFromPrivateKey(pk, network = network)) } @Test @@ -61,7 +46,7 @@ class TransparentTest { } companion object { - const val PHRASE = "wish puppy smile loan doll curve hole maze file ginger hair nose key relax knife witness cannon grab despair throw review deal slush frame" // "deputy visa gentle among clean scout farm drive comfort patch skin salt ranch cool ramp warrior drink narrow normal lunch behind salt deal person" + const val PHRASE = "deputy visa gentle among clean scout farm drive comfort patch skin salt ranch cool ramp warrior drink narrow normal lunch behind salt deal person" val MNEMONIC = MnemonicCode(PHRASE) val SEED = MNEMONIC.toSeed() @@ -84,6 +69,13 @@ class TransparentTest { fun startup() { Twig.plant(TroubleshootingTwig(formatter = { "@TWIG $it" })) } + + @JvmStatic + @Parameterized.Parameters + fun data() = listOf( + arrayOf(ExpectedTestnet, ZcashNetwork.Testnet), + arrayOf(ExpectedMainnet, ZcashNetwork.Mainnet), + ) } interface Expected { From 368099e1544c3d2e57f20770e62b59a41ddbc3b5 Mon Sep 17 00:00:00 2001 From: Kevin Gorham Date: Fri, 7 May 2021 04:11:35 -0400 Subject: [PATCH 6/6] Improvement: Remove dropAccountsTable. This was effectively a band-aid fix from previous testing that is now working more cleanly so this code isn't needed, thankfully. --- .../cash/z/ecc/android/sdk/jni/RustBackend.kt | 9 -------- .../ecc/android/sdk/jni/RustBackendWelding.kt | 2 -- src/main/rust/lib.rs | 21 +------------------ 3 files changed, 1 insertion(+), 31 deletions(-) diff --git a/src/main/java/cash/z/ecc/android/sdk/jni/RustBackend.kt b/src/main/java/cash/z/ecc/android/sdk/jni/RustBackend.kt index 2cc2c1bf..4b38fc45 100644 --- a/src/main/java/cash/z/ecc/android/sdk/jni/RustBackend.kt +++ b/src/main/java/cash/z/ecc/android/sdk/jni/RustBackend.kt @@ -52,10 +52,6 @@ class RustBackend private constructor() : RustBackendWelding { override fun initDataDb() = initDataDb(pathDataDb, networkId = network.id) - override fun dropAccountsTable(): Boolean { - return dropAccountsTable(pathDataDb, networkId = network.id) - } - override fun initAccountsTable(vararg keys: UnifiedViewingKey): Boolean { val extfvks = Array(keys.size) { "" } val extpubs = Array(keys.size) { "" } @@ -261,11 +257,6 @@ class RustBackend private constructor() : RustBackendWelding { @JvmStatic private external fun initDataDb(dbDataPath: String, networkId: Int): Boolean - @JvmStatic private external fun dropAccountsTable( - dbDataPath: String, - networkId: Int, - ): Boolean - @JvmStatic private external fun initAccountsTableWithKeys( dbDataPath: String, extfvk: Array, diff --git a/src/main/java/cash/z/ecc/android/sdk/jni/RustBackendWelding.kt b/src/main/java/cash/z/ecc/android/sdk/jni/RustBackendWelding.kt index 102f7fa3..d1595601 100644 --- a/src/main/java/cash/z/ecc/android/sdk/jni/RustBackendWelding.kt +++ b/src/main/java/cash/z/ecc/android/sdk/jni/RustBackendWelding.kt @@ -31,8 +31,6 @@ interface RustBackendWelding { fun decryptAndStoreTransaction(tx: ByteArray) - fun dropAccountsTable(): Boolean - fun initAccountsTable(seed: ByteArray, numberOfAccounts: Int): Array fun initAccountsTable(vararg keys: UnifiedViewingKey): Boolean diff --git a/src/main/rust/lib.rs b/src/main/rust/lib.rs index caf439c2..726458f4 100644 --- a/src/main/rust/lib.rs +++ b/src/main/rust/lib.rs @@ -42,7 +42,7 @@ use zcash_client_sqlite::{ error::SqliteClientError, NoteId, wallet::{delete_utxos_above, put_received_transparent_utxo, rewind_to_height, get_rewind_height}, - wallet::init::{drop_accounts_table, init_accounts_table, init_blocks_table, init_wallet_db}, + wallet::init::{init_accounts_table, init_blocks_table, init_wallet_db}, WalletDb, }; use zcash_primitives::{ @@ -120,25 +120,6 @@ pub unsafe extern "C" fn Java_cash_z_ecc_android_sdk_jni_RustBackend_initDataDb( unwrap_exc_or(&env, res, JNI_FALSE) } - -#[no_mangle] -pub unsafe extern "C" fn Java_cash_z_ecc_android_sdk_jni_RustBackend_dropAccountsTable( - env: JNIEnv<'_>, - _: JClass<'_>, - db_data: JString<'_>, - network_id: jint, -) -> jboolean { - let res = panic::catch_unwind(|| { - let network = parse_network(network_id as u32)?; - let db_data = wallet_db(&env, network, db_data)?; - match drop_accounts_table(&db_data) { - Ok(()) => Ok(JNI_TRUE), - Err(e) => Err(format_err!("Error while dropping the accounts table: {}", e)), - } - }); - unwrap_exc_or(&env, res, JNI_FALSE) -} - #[no_mangle] pub unsafe extern "C" fn Java_cash_z_ecc_android_sdk_jni_RustBackend_initAccountsTableWithKeys( env: JNIEnv<'_>,