[#1239] Post-v2.0.0-rc1 cleanup

* Add JniScanProgressTest

* Remove unnecessary testnet workaround

As this was fixed with the last rust changes.

* Simplify batchCount calculating

* Fix ratio typo

* Docummentation comments changes

backend-lib/src/main/rust/lib.rs

@@ -1149,6 +1149,9 @@ pub unsafe extern "C" fn Java_cash_z_ecc_android_sdk_internal_jni_RustBackend_ge
 
 /// Returns a `JniScanProgress` object, provided that numerator is nonnegative, denominator
 /// is positive, and the represented ratio is in the range 0.0 to 1.0 inclusive.
+///
+/// If these conditions are not met, this fails and leaves an `IllegalArgumentException`
+/// pending.
 fn encode_scan_progress(env: &JNIEnv<'_>, progress: Ratio<u64>) -> Result<jobject, failure::Error> {
     let output = env.new_object(
         "cash/z/ecc/android/sdk/internal/model/JniScanProgress",

backend-lib/src/main/rust/utils.rs

@@ -22,7 +22,8 @@ pub(crate) fn rust_bytes_to_java(
     env: &JNIEnv<'_>,
     data: &[u8],
 ) -> Result<jbyteArray, failure::Error> {
-    // SAFETY: jbyte (i8) has the same size and alignment as u8.
+    // SAFETY: jbyte (i8) has the same size and alignment as u8, and a well-defined
+    // twos-complement representation with no "trap representations".
     let buf = unsafe { slice::from_raw_parts(data.as_ptr().cast(), data.len()) };
     let jret = env.new_byte_array(data.len() as jsize)?;
     env.set_byte_array_region(jret, 0, buf)?;

backend-lib/src/test/java/cash/z/ecc/android/sdk/internal/model/JniScanProgressTest.kt

@@ -0,0 +1,46 @@
+package cash.z.ecc.android.sdk.internal.model
+
+import kotlin.test.Test
+import kotlin.test.assertFailsWith
+import kotlin.test.assertIs
+
+class JniScanProgressTest {
+    @Test
+    fun both_attribute_within_constraints() {
+        val instance = JniScanProgress(
+            numerator = 1L,
+            denominator = 100L
+        )
+        assertIs<JniScanProgress>(instance)
+    }
+
+    @Test
+    fun numerator_attribute_not_in_constraints() {
+        assertFailsWith(IllegalArgumentException::class) {
+            JniScanProgress(
+                numerator = -1L,
+                denominator = 100L
+            )
+        }
+    }
+
+    @Test
+    fun denominator_attribute_not_in_constraints() {
+        assertFailsWith(IllegalArgumentException::class) {
+            JniScanProgress(
+                numerator = 1L,
+                denominator = 0L
+            )
+        }
+    }
+
+    @Test
+    fun ratio_not_in_constraints() {
+        assertFailsWith(IllegalArgumentException::class) {
+            JniScanProgress(
+                numerator = 100L,
+                denominator = 1L
+            )
+        }
+    }
+}

sdk-lib/src/main/java/cash/z/ecc/android/sdk/block/processor/CompactBlockProcessor.kt

@@ -203,7 +203,8 @@ class CompactBlockProcessor internal constructor(
 
         updateBirthdayHeight()
 
-        // Clear any undeleted left over block files from previous sync attempts
+        // Clear any undeleted left over block files from previous sync attempts.
+        // lastKnownHeight is only used for error reporting.
         deleteAllBlockFiles(
             downloader = downloader,
             lastKnownHeight = when (val result = getMaxScannedHeight(backend)) {
@@ -240,7 +241,7 @@ class CompactBlockProcessor internal constructor(
                                 )
                             ) {
                                 PutSaplingSubtreeRootsResult.Success -> {
-                                    // Lets continue with the next step
+                                    // Let's continue with the next step
                                 }
                                 is PutSaplingSubtreeRootsResult.Failure -> {
                                     BlockProcessingResult.SyncFailure(result.failedAtHeight, result.exception)
@@ -255,22 +256,10 @@ class CompactBlockProcessor internal constructor(
                                 firstUnenhancedHeight = _processorInfo.value.firstUnenhancedHeight
                             )
                         }
-                        GetSubtreeRootsResult.Linear -> {
-                            // This is caused by an empty response result. Although the spend-before-sync
-                            // synchronization algorithm is not supported, we can get the entire block range as we
-                            // previously did for the linear sync type.
-                            processNewBlocksInSbSOrder(
-                                backend = backend,
-                                downloader = downloader,
-                                repository = repository,
-                                network = network,
-                                lastValidHeight = lowerBoundHeight,
-                                firstUnenhancedHeight = _processorInfo.value.firstUnenhancedHeight
-                            )
-                        }
-                        is GetSubtreeRootsResult.OtherFailure -> {
-                            // The server possibly replied with some unsupported error. We still approach
-                            // spend-before-sync synchronization.
+                        is GetSubtreeRootsResult.OtherFailure, GetSubtreeRootsResult.Linear -> {
+                            // This is caused by an empty response result or another unsupported error.
+                            // Although the spend-before-sync synchronization algorithm is not supported, we can get
+                            // the entire block range as we previously did for the linear sync type.
                             processNewBlocksInSbSOrder(
                                 backend = backend,
                                 downloader = downloader,
@@ -625,7 +614,7 @@ class CompactBlockProcessor internal constructor(
                     lastValidHeight = lastValidHeight
                 )
         ) {
-            is UpdateChainTipResult.Success -> { /* Lets continue to the next step */ }
+            is UpdateChainTipResult.Success -> { /* Let's continue to the next step */ }
             is UpdateChainTipResult.Failure -> {
                 return SbSPreparationResult.ProcessFailure(
                     result.failedAtHeight,
@@ -900,8 +889,7 @@ class CompactBlockProcessor internal constructor(
             }
         } else {
             Twig.debug {
-                "Warning: gave up on fetching UTXOs for this session. It seems to unavailable on " +
-                    "lightwalletd."
+                "Warning: gave up on fetching UTXOs for this session. It is unavailable on lightwalletd."
             }
         }
 
@@ -978,9 +966,9 @@ class CompactBlockProcessor internal constructor(
 
         /**
          * Default size of batches of blocks to request from the compact block service. Then it's also used as a default
-         * size of batches of blocks to scan via librustzcash. For scanning action applies this - the smaller this
-         * number the more granular information can be provided about scan state. Unfortunately, it may also lead to
-         * a lot of overhead during scanning.
+         * size of batches of blocks to scan via librustzcash. The smaller the batch size for scanning, the more
+         * granular information can be provided about scan state. Unfortunately, small batches may also lead to a lot
+         * of overhead during scanning.
          */
         internal const val SYNC_BATCH_SIZE = 100
 
@@ -996,7 +984,7 @@ class CompactBlockProcessor internal constructor(
         internal const val REWIND_DISTANCE = 10
 
         /**
-         * Limit millis value for restarting currently running block synchronization.
+         * Timeout in milliseconds for restarting the currently running block synchronization.
          */
         internal val SYNCHRONIZATION_RESTART_TIMEOUT = 10.minutes.inWholeMilliseconds
 
@@ -1049,8 +1037,8 @@ class CompactBlockProcessor internal constructor(
         }
 
         /**
-         * This operation downloads note commitment tree data from the lightwalletd server to decide if we communicate
-         * with linear or spend-before-sync node
+         * This operation downloads note commitment tree data from the lightwalletd server, to decide whether or not
+         * the lightwalletd and its backing node support spend-before-sync.
          *
          * @return GetSubtreeRootsResult as a wrapper for the lightwalletd response result
          */
@@ -1066,17 +1054,13 @@ class CompactBlockProcessor internal constructor(
             retryUpToAndContinue(GET_SUBTREE_ROOTS_RETRIES) {
                 downloader.getSubtreeRoots(
                     startIndex = 0,
-                    maxEntries = if (network.isTestnet()) {
-                        65536
-                    } else {
-                        0
-                    },
+                    maxEntries = 0,
                     shieldedProtocol = ShieldedProtocolEnum.SAPLING
                 ).onEach { response ->
                     when (response) {
                         is Response.Success -> {
                             Twig.verbose {
-                                "SubtreeRoot got successfully: it's completingHeight: ${response.result
+                                "SubtreeRoot fetched successfully: its completingHeight is: ${response.result
                                     .completingBlockHeight}"
                             }
                         }
@@ -1372,8 +1356,8 @@ class CompactBlockProcessor internal constructor(
                     // Increment and compare the range for triggering the enhancing
                     enhancingRange = enhancingRange.start..continuousResult.batch.range.endInclusive
 
-                    // Enhancing is run in case of the range is on or over its limit, or in case of any failure
-                    // state comes from the previous stages, or if the end of the sync range is reached
+                    // Enhancing is run when the range is on or over its limit, or there is any failure
+                    // from previous stages, or if the end of the sync range is reached.
                     if (enhancingRange.length() >= ENHANCE_BATCH_SIZE ||
                         resultState != SyncingResult.AllSuccess ||
                         continuousResult.batch.order == batches.size.toLong()
@@ -1435,10 +1419,7 @@ class CompactBlockProcessor internal constructor(
 
             syncRanges.forEach { range ->
                 val missingBlockCount = range.endInclusive.value - range.start.value + 1
-                val batchCount = (
-                    missingBlockCount / SYNC_BATCH_SIZE +
-                        (if (missingBlockCount.rem(SYNC_BATCH_SIZE) == 0L) 0 else 1)
-                    )
+                val batchCount = (missingBlockCount + SYNC_BATCH_SIZE - 1) / SYNC_BATCH_SIZE
                 allMissingBlocksCount += missingBlockCount
                 allRangesBatchCount += batchCount
             }
@@ -1457,7 +1438,7 @@ class CompactBlockProcessor internal constructor(
          * @param syncRange Current range to be processed
          * @param network The network we are operating on
          *
-         * @return List of [BlockBatch] to for synchronization
+         * @return List of [BlockBatch] to prepare for synchronization
          */
         private fun getBatchedBlockList(
             syncRange: ClosedRange<BlockHeight>,
@@ -1506,7 +1487,6 @@ class CompactBlockProcessor internal constructor(
                     downloadException!!
                 }
             ) { failedAttempts ->
-                @Suppress("MagicNumber")
                 if (failedAttempts == 0) {
                     Twig.verbose { "Starting to download batch $batch" }
                 } else {
@@ -1787,7 +1767,8 @@ class CompactBlockProcessor internal constructor(
     }
 
     /**
-     * Emit an instance of processorInfo, corresponding to the provided data.
+     * Sets new values of ProcessorInfo and network height corresponding to the provided data for this
+     * [CompactBlockProcessor].
      *
      * @param networkBlockHeight the latest block available to lightwalletd that may or may not be
      * downloaded by this wallet yet.
@@ -1812,7 +1793,7 @@ class CompactBlockProcessor internal constructor(
     }
 
     /**
-     * Emit an instance of progress.
+     * Sets the progress for this [CompactBlockProcessor].
      *
      * @param progress the block syncing progress of type [PercentDecimal] in the range of [0, 1]
      */
@@ -1821,7 +1802,7 @@ class CompactBlockProcessor internal constructor(
     }
 
     /**
-     * Transmits the given state for this processor.
+     * Sets the state of this [CompactBlockProcessor].
      */
     private suspend fun setState(newState: State) {
         _state.value = newState
@@ -1854,7 +1835,7 @@ class CompactBlockProcessor internal constructor(
     }
 
     /**
-     * Rewind back at least two weeks worth of blocks.
+     * Rewind back two weeks worth of blocks. The final amount of rewinded blocks depends on [rewindToNearestHeight].
      */
     suspend fun quickRewind(): Boolean {
         val height = when (val result = getMaxScannedHeight(backend)) {

sdk-lib/src/main/java/cash/z/ecc/android/sdk/block/processor/model/GetScanProgressResult.kt

@@ -8,7 +8,7 @@ import cash.z.ecc.android.sdk.model.PercentDecimal
  */
 internal sealed class GetScanProgressResult {
     data class Success(val scanProgress: ScanProgress) : GetScanProgressResult() {
-        fun toPercentDecimal() = PercentDecimal(scanProgress.getSafeRation())
+        fun toPercentDecimal() = PercentDecimal(scanProgress.getSafeRatio())
     }
 
     data object None : GetScanProgressResult()

sdk-lib/src/main/java/cash/z/ecc/android/sdk/internal/ext/WalletService.kt

@@ -46,8 +46,8 @@ suspend inline fun retryUpToAndThrow(
  * retries succeed, then leave the block execution unfinished and continue.
  *
  * @param retries the number of times to retry the block after the first attempt fails.
- * @param exceptionWrapper a function that can wrap the final failure to add more useful information
- *  * or context. Default behavior is to just return the final exception.
+ * @param exceptionWrapper a function that can wrap the final failure to add more useful information or context.
+ * Default behavior is to just return the final exception.
  * @param initialDelayMillis the initial amount of time to wait before the first retry.
  * @param block the code to execute, which will be wrapped in a try/catch and retried whenever an
  * exception is thrown up to [retries] attempts.

sdk-lib/src/main/java/cash/z/ecc/android/sdk/internal/model/ScanProgress.kt

@@ -4,12 +4,12 @@ internal data class ScanProgress(
     private val numerator: Long,
     private val denominator: Long
 ) {
-    override fun toString() = "ScanProgress($numerator/$denominator) -> ${getSafeRation()}"
+    override fun toString() = "ScanProgress($numerator/$denominator) -> ${getSafeRatio()}"
 
     /**
      * Returns progress ratio in [0, 1] range. Any out-of-range value is treated as 0.
      */
-    fun getSafeRation() = numerator.toFloat().div(denominator).let { ration ->
+    fun getSafeRatio() = numerator.toFloat().div(denominator).let { ration ->
         if (ration < 0f || ration > 1f) {
             0f
         } else {

sdk-lib/src/main/java/cash/z/ecc/android/sdk/model/BlockHeight.kt

@@ -43,15 +43,15 @@ data class BlockHeight internal constructor(val value: Long) : Comparable<BlockH
 
     operator fun minus(other: Int): BlockHeight {
         require(other >= 0) {
-            "Cannot subtract negative value $other to BlockHeight"
+            "Cannot subtract negative value $other from BlockHeight"
         }
 
         return BlockHeight(value - other.toLong())
     }
 
     operator fun minus(other: Long): BlockHeight {
         require(other >= 0) {
-            "Cannot subtract negative value $other to BlockHeight"
+            "Cannot subtract negative value $other from BlockHeight"
         }
 
         return BlockHeight(value - other)

sdk-lib/src/test/java/cash/z/ecc/android/sdk/internal/model/ScanProgressTest.kt

@@ -9,7 +9,7 @@ class ScanProgressTest {
     fun get_valid_ratio_test() {
         val scanProgress = ScanProgressFixture.new()
         assertEquals(
-            scanProgress.getSafeRation(),
+            scanProgress.getSafeRatio(),
             ScanProgressFixture.DEFAULT_NUMERATOR.toFloat().div(ScanProgressFixture.DEFAULT_DENOMINATOR)
         )
     }
@@ -19,6 +19,6 @@ class ScanProgressTest {
         val scanProgress = ScanProgressFixture.new(
             denominator = 0
         )
-        assertEquals(0f, scanProgress.getSafeRation())
+        assertEquals(0f, scanProgress.getSafeRatio())
     }
 }