Fixed NSInternalInconsistencyException & Swift runtime failure: arithmetic overflow #267
Conversation
azisramdhan
changed the title
azisramdhan
force-pushed
the
bugfix/issues-13-16
branch
3 times, most recently
from
c140d82 to
3d7829e
Compare
litewallet/Platform/Extensions.swift
Outdated
| @@ -558,3 +558,17 @@ extension Dictionary where Key: ExpressibleByStringLiteral, Value: Any { | |||
| return jstring | |||
| } | |||
| } | |||
|
|
|||
| extension UInt64 { | |||
There was a problem hiding this comment.
It's recommended to move the UInt64 extension to a dedicated file named something like UInt64+SafeArithmetic.swift or UInt64+Extension.swift.
litewallet/Platform/Extensions.swift
Outdated
| @@ -558,3 +558,17 @@ extension Dictionary where Key: ExpressibleByStringLiteral, Value: Any { | |||
| return jstring | |||
| } | |||
| } | |||
|
|
|||
| extension UInt64 { | |||
| /// Performs a safe addition, returning `nil` if overflow occurs. | |||
There was a problem hiding this comment.
While the existing documentation is clear, it can include more details about when these methods are useful.
/// Adds two UInt64 values safely, returning nil if overflow occurs.
/// - Parameter value: The value to add.
/// - Returns: The sum of the two values or nil if the operation overflows.
litewallet/Platform/Extensions.swift
Outdated
| @@ -558,3 +558,17 @@ extension Dictionary where Key: ExpressibleByStringLiteral, Value: Any { | |||
| return jstring | |||
| } | |||
| } | |||
|
|
|||
| extension UInt64 { | |||
There was a problem hiding this comment.
Consider Extending a Protocol (Optional): If similar safe arithmetic methods are needed for other numeric types (e.g., Int, Double), you could introduce a protocol for safe arithmetic. This promotes code reuse and reduces duplication.
protocol SafeArithmetic {
func safeAddition(_ value: Self) -> Self?
func safeSubtraction(_ value: Self) -> Self?
}
litewallet/Platform/Extensions.swift
Outdated
| return overflow ? nil : result | ||
| } | ||
|
|
||
| /// Performs a safe subtraction, returning `nil` if underflow occurs. |
There was a problem hiding this comment.
Same comment for documentation above.
|
|
||
| // Safely calculate fee = fee + opsAmount | ||
| guard let safeFee = fee.safeAddition(opsAmount) else { return nil } | ||
| self.fee = safeFee |
There was a problem hiding this comment.
Seems that the same value (fee) is used multiple times across the class functions. We can cache fee in a local variable if it's not modified between operations.
guard let safeFee = fee.safeAddition(opsAmount) else { return nil }
self.fee = safeFee
let cachedFee = safeFee // Cache for reuse
|
|
||
| self.fee = fee + opsAmount | ||
|
|
||
| // Safely calculate fee = fee + opsAmount |
There was a problem hiding this comment.
While comments like // Safely calculate fee = fee + opsAmount provide context, they are overly verbose and repeat what the code already expresses.
For example; // Ensure the new fee calculation doesn't overflow
| self.fee = fee + opsAmount | ||
|
|
||
| // Safely calculate fee = fee + opsAmount | ||
| guard let safeFee = fee.safeAddition(opsAmount) else { return nil } |
There was a problem hiding this comment.
Some variable names (e.g., safeSum, safeFee) are generic and don't convey their specific role in the calculation. Use more descriptive names for better understanding.
guard let totalReceivedAndFee = amountReceived.safeAddition(fee) else { return nil }
|
|
||
| if amountSent > 0, (amountReceived + fee) == amountSent { | ||
| // Safely calculate amountSent = wallet.amountSentByTx(tx) - opsAmount |
There was a problem hiding this comment.
Same comment for documentation.
| satoshis = safeAmountSent | ||
| } else if safeAmountSent > 0 { | ||
| // Safely calculate (safeAmountSent - amountReceived - fee) | ||
| guard let safeSatoshis = safeAmountSent |
There was a problem hiding this comment.
Multi-line guard statements can be difficult to read and debug. Break these into smaller, intermediate steps for clarity.
guard let intermediateSatoshis = safeAmountSent.safeSubtraction(amountReceived),
let finalSatoshis = intermediateSatoshis.safeSubtraction(fee) else { return nil }
direction = .sent
satoshis = finalSatoshis
| @@ -22,11 +22,14 @@ class DefaultCurrencyViewController: UITableViewController, Subscriber { | |||
| didSet { | |||
There was a problem hiding this comment.
The didSet block contains too much logic. It's better to extract this logic into helper functions to make the didSet block cleaner and more focused.
updateCurrencyRows(oldCurrencyCode: oldValue)
(Optional but Recommended): Avoid UI Updates in didSet
| setExchangeRateLabel() | ||
|
|
||
| Task { @MainActor in | ||
| tableView.beginUpdates() |
There was a problem hiding this comment.
(Optional): Use performBatchUpdates for Table View Changes.
|
Thank you, @hassanashraf92, for the valuable feedback. I have updated the code based on your suggestions. |
- Added detailed explanations of use cases for safeAddition and safeSubtraction methods in UInt64 extension. - Clarified the behavior of these methods when handling overflow and underflow. - Highlighted their importance in contexts requiring safe and predictable arithmetic operations.
- Applied consistent indentation using Xcode's Ctrl + I shortcut to improve code readability. - No functional changes were made.
… clarity - Cache the total fee in a local variable for reuse. - Safely calculate the amount sent by subtracting opsAmount to prevent underflow. - Ensure the total received and fee are within bounds during the calculation. - Simplify the process of determining transaction direction (moved, sent, received) by using safe arithmetic.
- Moved the logic for updating currency rows into a separate helper function () to improve readability and maintainability. - Cleaned up the didSet block to focus on calling the helper function.
azisramdhan
force-pushed
the
bugfix/issues-13-16
branch
from
cc4589f to
8b559a1
Compare
Litewallet internal issues
13
16
Xcode Organizer