fix: Prevent arithmetic overflow and underflow

[ovitrif]

This PR Fixes ULong arithmetic overflow issues when subtracting two values or underflow when summing up two values.

Description

• Add USat value class with safe minus operator that returns 0 instead of overflowing
• Replace faulty (a - b).coerceAtLeast(0u) pattern which overflows before coercion
• Add unit tests for USat arithmetic behavior

Preview

N/A - No UI changes

QA Notes

• Transfer to spending regression test
• Verify balance calculations work correctly when channel sizes exceed available amounts

[ovitrif]

drafted to rebase on master and remove comment pointed out by Claude review.

The base branch was changed.

[claude]

Code Review - PR #486: Fix arithmetic overflow and underflow

Summary

This PR addresses a critical bug in unsigned integer arithmetic by introducing a USat value class that provides saturating arithmetic operations. The fix replaces the flawed (a - b).coerceAtLeast(0u) pattern which overflows before coercion can occur.

---

✅ Strengths

1. Excellent Problem Identification
The PR correctly identifies and fixes a subtle but dangerous bug. The pattern (5uL - 10uL).coerceAtLeast(0u) wraps around to ULong.MAX_VALUE - 4 before coerceAtLeast can prevent it - a classic unsigned arithmetic trap.

2. Clean Implementation

• The USat value class is well-designed using @JvmInline for zero runtime overhead
• Clear, focused responsibility: saturating arithmetic only
• Good documentation with KDoc comments and Rust comparison

3. Comprehensive Test Coverage
The test suite is exemplary:

• Tests all edge cases (overflow, underflow, equality)
• Tests both addition and subtraction operations
• Includes realistic Bitcoin calculation scenarios
• Even has a test demonstrating the old bug (minus prevents the coerceAtLeast bug)
• 127 lines of tests for 31 lines of production code is excellent coverage

4. Consistent Migration
All usages of the problematic patterns have been systematically replaced across:

• DeriveBalanceStateUseCase.kt (4 occurrences)
• TransferViewModel.kt (6 occurrences)
• Removed the temporary minusOrZero function

---

🔍 Potential Issues & Suggestions

1. Type Safety Concern - High Priority

The minus and plus operators return ULong instead of USat. This breaks the monad pattern and requires manual re-wrapping:

// Current (requires double wrapping):
val result = afterPendingChannels.safe() - toSavingsAmount.safe()

// Better:
operator fun minus(other: USat): USat = 
    if (value >= other.value) USat(value - other.value) else USat(0uL)

This would enable cleaner chaining:

val result = a.safe() - b.safe() - c.safe()  // No need to re-wrap

Impact: The current implementation works but is less ergonomic and could lead to errors if developers forget to re-wrap intermediate results.

2. Incomplete Migration Check

While the PR removes minusOrZero and migrates most usages, consider searching for other unsafe patterns:

# Check for direct ULong arithmetic that might need safe()
grep -r "ULong.*-.*ULong" --include="*.kt" app/src/main/

Some calculations (like fee estimates, percentage calculations) may benefit from saturating arithmetic too.

3. Missing Multiplication/Division

The implementation only provides + and -, but Bitcoin calculations often involve multiplication and division:

// In TransferViewModel.kt:384
val maxChannelSize2 = maxChannelSize1.safe() - channelsSize.safe()
val maxChannelSizeAvailableToIncrease = min(maxChannelSize1, maxChannelSize2)

Consider adding:

operator fun times(scalar: ULong): USat = /* saturating multiply */
operator fun div(divisor: ULong): USat = /* saturating divide */

4. Edge Case: Addition Overflow Check

The overflow check in plus could be simplified and made more explicit:

// Current:
if (value <= ULong.MAX_VALUE - other.value) value + other.value else ULong.MAX_VALUE

// Consider:
val result = value + other.value
if (result < value) ULong.MAX_VALUE else result  // Overflow if result wrapped around

Both are correct, but the second makes the overflow condition more explicit.

---

🔒 Security Analysis

No security concerns identified. This PR actually improves security by:

• Preventing integer overflow bugs that could lead to incorrect balance calculations
• Ensuring channel size calculations never wrap around unexpectedly
• Making arithmetic behavior predictable and safe

This is particularly critical for Bitcoin wallet software where arithmetic errors could lead to:

• Incorrect balance displays
• Failed transactions
• Loss of funds

---

⚡ Performance Considerations

Excellent performance characteristics:

• @JvmInline value class means zero runtime overhead when not boxed
• All operations are simple comparisons and arithmetic (no allocations)
• No performance regression compared to direct ULong operations

---

📋 Test Coverage Assessment

Test coverage: Excellent (9/10)

The test suite covers:

• ✅ Normal operations
• ✅ Boundary conditions (0, MAX_VALUE)
• ✅ Overflow/underflow scenarios
• ✅ Chained operations
• ✅ Comparison operators
• ✅ Realistic use cases

Missing coverage:

• No test for the actual bug scenarios from production code
• Could add integration tests with actual DeriveBalanceStateUseCase scenarios

Consider adding:

@Test
fun `balance calculation with large pending channels`() {
    val totalLightning = 100_000uL
    val pendingChannels = 150_000uL  // More than total!
    
    val result = totalLightning.safe() - pendingChannels.safe()
    assertEquals(0uL, result)  // Should not wrap around
}

---

🎯 Recommendations

Must Fix:

• None - the PR is production-ready as-is

Should Consider (for follow-up PR):

1. Change minus/plus to return USat instead of ULong for better type safety
2. Search for other unsigned arithmetic operations that could benefit from safe()
3. Add times/div operators if needed by the domain

Nice to Have:

1. Add integration tests with actual balance calculation scenarios
2. Document in code comments where safe() is required vs optional
3. Consider adding a lint rule to detect (ULong - ULong).coerceAtLeast(0u) pattern

---

✅ Final Verdict

APPROVED - This is a high-quality fix for a real bug. The implementation is clean, well-tested, and addresses the root cause. The suggestions above are minor improvements that could be addressed in follow-up work.

Risk Level: Low - The change is well-isolated and thoroughly tested.

Merge Recommendation: ✅ Approve and merge after CI passes.

Great work identifying and fixing this subtle bug! The saturating arithmetic approach is the right solution.