fix: improve ctx error handling - #1419
Conversation
WalkthroughThis update introduces enhancements to transaction metadata handling, error reporting, and user support in the gift card purchase flow. Key changes include propagating an optional service name through payment APIs, improved exception handling and reporting, UI support for contacting support, and expanded test coverage. Several method signatures and data classes were updated to support these features. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as GiftCardDetailsDialog
participant VM as CTXSpendViewModel
participant Email as Email App
UI->>VM: createEmailIntent(subject, sentToCTX, ex)
VM->>VM: Compose email with error & transaction details
VM-->>UI: Return email intent
UI->>Email: Launch chooser with intent
sequenceDiagram
participant Repo as CTXSpendRepository
participant VM as CTXSpendViewModel
participant Pay as SendPaymentService/SendCoinsTaskRunner
VM->>Pay: payWithDashUrl(dashUri, serviceName)
Pay->>Pay: Process payment, set metadata if serviceName
Pay-->>VM: Return Transaction or throw Exception
VM->>Repo: saveGiftCardDummy(txId, giftCardResponse)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested labels
Poem
Note 🔌 MCP (Model Context Protocol) integration is now available in Early Access!Pro users can now connect to remote MCP servers under the Integrations page to get reviews and chat conversations that understand additional development context. 📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/data/ctxspend/model/GiftCardResponse.kt (1)
27-29: Consider consistency in nullable field handling.The new fields default to empty strings while other nullable fields in the class don't have explicit defaults. Also, these fields lack
@SerializedNameannotations unlike most other fields.If these are API response fields, consider adding
@SerializedNameannotations:+@SerializedName("cardFiatAmount") val cardFiatAmount: String? = "", +@SerializedName("cardFiatCurrency") val cardFiatCurrency: String? = "", +@SerializedName("merchantName") val merchantName: String? = "",If they're computed fields, consider defaulting to
nullfor consistency:-val cardFiatAmount: String? = "", -val cardFiatCurrency: String? = "", -val merchantName: String? = "", +val cardFiatAmount: String? = null, +val cardFiatCurrency: String? = null, +val merchantName: String? = null,wallet/src/de/schildbach/wallet/payments/SendCoinsTaskRunner.kt (1)
272-310: Consider extracting retry delays as a constant and remove empty line.The error handling logic is well-implemented for recovering from HTTP timeouts. Consider these improvements:
- Extract the delays array as a companion object constant for better maintainability
- Remove the empty line at line 308
+ companion object { + private const val WALLET_EXCEPTION_MESSAGE = "this method can't be used before creating the wallet" + private val log = LoggerFactory.getLogger(SendCoinsTaskRunner::class.java) + private val NETWORK_CHECK_DELAYS_MS = listOf(0L, 1000L, 3000L, 5000L) + } } catch (e: Exception) { if (e !is DirectPayException) { log.warn("Payment submission failed, but transaction may have been sent: ${sendRequest.tx.txId}", e) val tx = sendRequest.tx - val delays = listOf(0L, 1000L, 3000L, 5000L) - - for (delayMs in delays) { + for (delayMs in NETWORK_CHECK_DELAYS_MS) { delay(delayMs) if (isTransactionOnNetwork(tx)) { log.info("Transaction found on network despite HTTP timeout: ${tx.txId}") wallet.commitTx(tx) return tx } } log.warn("Transaction not found on network after timeout, treating as failed: ${tx.txId}") // throw exception below } throw e } - return sendCoins(sendRequest, txCompleted = true, checkBalanceConditions = true)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
common/src/main/java/org/dash/wallet/common/services/SendPaymentService.kt(1 hunks)common/src/main/java/org/dash/wallet/common/util/Constants.kt(1 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/data/ctxspend/model/GiftCardResponse.kt(1 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/network/RemoteDataSource.kt(1 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/network/service/stubs/FakeDashSpendService.kt(2 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/CTXSpendRepository.kt(1 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt(7 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/PurchaseGiftCardFragment.kt(1 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/GiftCardDetailsDialog.kt(6 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/GiftCardDetailsViewModel.kt(2 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/PurchaseGiftCardConfirmDialog.kt(4 hunks)features/exploredash/src/main/res/layout/dialog_gift_card_details.xml(2 hunks)features/exploredash/src/main/res/values/strings-explore-dash.xml(1 hunks)features/exploredash/src/test/java/org/dash/wallet/features/exploredash/CTXSpendExceptionTest.kt(3 hunks)wallet/src/de/schildbach/wallet/di/AppModule.kt(1 hunks)wallet/src/de/schildbach/wallet/payments/SendCoinsTaskRunner.kt(7 hunks)
🧰 Additional context used
🧠 Learnings (9)
📓 Common learnings
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1390
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt:129-145
Timestamp: 2025-05-08T18:11:40.249Z
Learning: The hardcoded test data in the purchaseGiftCard() function of CTXSpendViewModel is intentionally left in place for testing the error handling for limit mismatch, and will be fixed later before release.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1410
File: wallet/src/de/schildbach/wallet/data/InvitationLinkData.kt:79-84
Timestamp: 2025-07-12T07:12:04.769Z
Learning: HashEngineering prefers to handle defensive validation improvements for URI parsing in follow-up PRs rather than including them in the current PR when the main focus is on replacing Firebase with AppsFlyer.
📚 Learning: 2025-05-08T18:11:40.249Z
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1390
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt:129-145
Timestamp: 2025-05-08T18:11:40.249Z
Learning: The hardcoded test data in the purchaseGiftCard() function of CTXSpendViewModel is intentionally left in place for testing the error handling for limit mismatch, and will be fixed later before release.
Applied to files:
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/GiftCardDetailsViewModel.ktfeatures/exploredash/src/main/res/values/strings-explore-dash.xmlfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/CTXSpendRepository.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/PurchaseGiftCardConfirmDialog.ktfeatures/exploredash/src/main/res/layout/dialog_gift_card_details.xmlfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/data/ctxspend/model/GiftCardResponse.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/GiftCardDetailsDialog.ktfeatures/exploredash/src/test/java/org/dash/wallet/features/exploredash/CTXSpendExceptionTest.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/PurchaseGiftCardFragment.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt
📚 Learning: 2025-04-19T07:01:17.535Z
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In the Dash Wallet app, DashPayConfig methods like isTransactionMetadataInfoShown() already use withContext(Dispatchers.IO) internally, so wrapping these calls with another withContext(Dispatchers.IO) in ViewModels is redundant.
Applied to files:
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/GiftCardDetailsViewModel.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/PurchaseGiftCardConfirmDialog.ktcommon/src/main/java/org/dash/wallet/common/services/SendPaymentService.ktwallet/src/de/schildbach/wallet/di/AppModule.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/GiftCardDetailsDialog.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/network/service/stubs/FakeDashSpendService.ktfeatures/exploredash/src/test/java/org/dash/wallet/features/exploredash/CTXSpendExceptionTest.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/PurchaseGiftCardFragment.ktwallet/src/de/schildbach/wallet/payments/SendCoinsTaskRunner.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt
📚 Learning: 2025-05-12T09:15:56.594Z
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1391
File: integrations/coinbase/src/main/java/org/dash/wallet/integrations/coinbase/ui/convert_currency/ConvertViewFragment.kt:134-155
Timestamp: 2025-05-12T09:15:56.594Z
Learning: The SegmentedPicker composable in Dash Wallet has a safety check for empty options, returning early if options.isEmpty() is true.
Applied to files:
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/PurchaseGiftCardConfirmDialog.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/GiftCardDetailsDialog.kt
📚 Learning: 2025-04-19T07:01:17.535Z
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In Dash Wallet, DataStore operations (like those in DashPayConfig through BaseConfig) already handle background threading internally. Adding another layer of withContext(Dispatchers.IO) around these calls in ViewModels is redundant because the Jetpack DataStore API automatically dispatches its operations to a background thread.
Applied to files:
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/PurchaseGiftCardConfirmDialog.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/GiftCardDetailsDialog.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt
📚 Learning: 2025-05-12T09:14:36.656Z
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1391
File: wallet/src/de/schildbach/wallet/ui/payments/PaymentsFragment.kt:111-119
Timestamp: 2025-05-12T09:14:36.656Z
Learning: In the dashpay/dash-wallet project, explicitly unregistering ViewPager2 callbacks is not considered critical since memory leaks haven't been observed in practice with the memory profiler.
Applied to files:
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/PurchaseGiftCardConfirmDialog.kt
📚 Learning: 2025-04-16T17:07:27.359Z
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1377
File: wallet/src/de/schildbach/wallet/service/platform/PlatformService.kt:0-0
Timestamp: 2025-04-16T17:07:27.359Z
Learning: For PlatformService in the Dash Wallet, the implementation should avoid throwing exceptions when Constants.SUPPORTS_PLATFORM is false. The platform should only be initialized when SUPPORTS_PLATFORM is true, and initialization should be done lazily to defer Platform creation until needed.
Applied to files:
wallet/src/de/schildbach/wallet/di/AppModule.ktfeatures/exploredash/src/test/java/org/dash/wallet/features/exploredash/CTXSpendExceptionTest.ktwallet/src/de/schildbach/wallet/payments/SendCoinsTaskRunner.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt
📚 Learning: 2025-05-07T14:18:11.161Z
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1389
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/explore/SearchFragment.kt:45-46
Timestamp: 2025-05-07T14:18:11.161Z
Learning: In the ExploreViewModel of the dash-wallet application, `appliedFilters` is a StateFlow (not LiveData), so Flow operators like `distinctUntilChangedBy` can be used with it.
Applied to files:
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/PurchaseGiftCardFragment.kt
📚 Learning: 2025-05-06T15:46:59.440Z
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1386
File: wallet/src/de/schildbach/wallet/ui/send/SendCoinsViewModel.kt:108-0
Timestamp: 2025-05-06T15:46:59.440Z
Learning: In UI code where frequent updates to a value might trigger expensive operations (like the SendCoinsViewModel's `executeDryrun` method), it's preferable to use a Flow with debounce rather than launching a new coroutine for each update. This prevents race conditions, reduces resource usage, and provides a more reactive architecture.
Applied to files:
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/PurchaseGiftCardFragment.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (42)
common/src/main/java/org/dash/wallet/common/util/Constants.kt (1)
64-66: LGTM! Timeout increase supports better error handling.Increasing timeouts from 15 to 20 seconds provides more resilience for network operations, which aligns with the PR's goal of improving CTX error handling for timeouts and HTTP 500 errors.
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/network/RemoteDataSource.kt (1)
67-67: Excellent security improvement!Redacting the Authorization header prevents sensitive tokens from being exposed in logs, which is a security best practice.
features/exploredash/src/main/res/values/strings-explore-dash.xml (1)
232-232: LGTM! String resource supports improved error handling.The new server error message provides clear guidance to users and aligns with the PR's objective of enhancing error reporting with appropriate support contact options.
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/GiftCardDetailsViewModel.kt (2)
167-175: LGTM! Enhanced exception with gift card context.Including the
giftCardresponse in the exception provides valuable context for error reporting and user support, which aligns with the PR's objective of improving error handling.
183-191: LGTM! Enriched exception data for better error reporting.Adding the gift card response to the exception enables more detailed error reports when purchases are rejected, supporting the PR's enhanced error handling goals.
common/src/main/java/org/dash/wallet/common/services/SendPaymentService.kt (1)
56-56: LGTM! Interface extension supports transaction metadata.The addition of the nullable
serviceNameparameter is well-designed to support associating service metadata with payment transactions while maintaining backward compatibility.features/exploredash/src/main/res/layout/dialog_gift_card_details.xml (2)
324-326: LGTM! Enhanced error TextView for better UX.The addition of string resource reference and tools visibility improves the design-time preview while maintaining runtime behavior.
365-396: LGTM! Well-implemented contact support UI.The new contact support section follows consistent styling patterns with proper constraints and matches the existing UI design. This supports the PR objective of providing users with error reporting options.
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/network/service/stubs/FakeDashSpendService.kt (2)
68-68: LGTM! Proper interface implementation.The method signature correctly matches the updated
SendPaymentServiceinterface with the addedserviceNameparameter.
77-77: LGTM! Correct parameter forwarding.The
serviceNameparameter is properly forwarded to the underlyingrealService.payWithDashUrlcall, maintaining the delegation pattern.features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/PurchaseGiftCardFragment.kt (1)
356-371: LGTM! Improved balance checking with better currency handling.The refactored method uses early returns for better readability and handles non-USD payment currencies correctly by converting them to USD Fiat instances while preserving the numeric value for accurate balance comparisons.
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/PurchaseGiftCardConfirmDialog.kt (5)
132-146: LGTM! Enhanced network error handling with user reporting.The dialog now provides users with the option to report network errors via email, improving error visibility and support interaction as intended by the PR objectives.
159-170: LGTM! Comprehensive limit error handling with reporting.The spending limit error dialog correctly sets
sentToCTX = trueand provides appropriate email subject, enabling users to report CTX-specific issues directly.
171-193: LGTM! Proper HTTP 500 error handling with CTX reporting.The new server error handling correctly identifies CTX internal server errors and provides appropriate reporting mechanism with
sentToCTX = true.
200-215: LGTM! Generic error handling with DCG reporting.The fallback error handling correctly sets
sentToCTX = falsefor non-CTX specific errors, routing reports to DCG as intended.
232-232: LGTM! Updated method call aligns with signature change.The call to
saveGiftCardDummynow passes the fullGiftCardResponseobject instead of just the ID, which aligns with the method signature updates mentioned in the AI summary.features/exploredash/src/test/java/org/dash/wallet/features/exploredash/CTXSpendExceptionTest.kt (3)
19-21: LGTM - New imports support enhanced testingThe new imports correctly support the additional test cases for
CTXSpendExceptionconstructor overloads.
46-53: LGTM - Server error test validates correct behaviorThe
serverErrorTest()correctly validates that server errors (HTTP 500) are properly handled:
- Error code is correctly set to 500
isLimitErrorcorrectly returns false (500 errors are not limit errors)isNetworkErrorcorrectly returns false (500 errors are server errors, not network errors)
71-85: LGTM - ResourceString constructor test validates new functionalityThe
resourceStringTest()correctly validates the new constructor that accepts aResourceStringand optionalGiftCardResponse:
- Uses proper test data with realistic parameters
- Correctly verifies that
errorCodeis null when using ResourceString constructor- Properly tests that
isLimitErrorreturns false for ResourceString-based exceptionsfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/CTXSpendRepository.kt (4)
43-44: LGTM - More general exception handlingChanging the parameter type from
ExceptiontoThrowableis appropriate as it allows handling of all throwable types including errors, not just exceptions.
46-46: LGTM - Enhanced exception contextThe new
giftCardResponseproperty allows CTXSpendException to carry additional context about the gift card transaction, which supports better error reporting and debugging.
49-52: LGTM - Constructor supports new functionalityThe updated secondary constructor properly accepts and assigns the optional
GiftCardResponse, enabling richer exception context for error reporting flows.
57-58: Verify unchecked Map parsing in CTXSpendRepositoryWe confirmed this is the only location using
Gson().fromJson(errorBody, Map::class.java)with an unchecked cast:
- features/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/CTXSpendRepository.kt (lines 57–58)
Ensure that every error response from the CTX Spend API is always a JSON object matching
Map<String, Any>. To improve robustness, consider:
- Adding unit tests that parse representative error JSON (including nested objects/arrays).
- Switching back to a
TypeToken<Map<String, Any>>() {}-based parse for compile-time safety if structure can vary.- Keeping the existing
try/fallback (emptyMap()) logic for null or malformed inputs.wallet/src/de/schildbach/wallet/di/AppModule.kt (1)
115-118: LGTM - Dependency injection properly updatedThe addition of
TransactionMetadataProviderparameter and its injection intoSendCoinsTaskRunnercorrectly supports the enhanced payment service functionality that can track service names with transactions.features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/dialogs/GiftCardDetailsDialog.kt (5)
25-25: LGTM - Import supports activity result handlingThe
ActivityResultContractsimport is correctly added to support the new email intent launcher functionality.
84-84: LGTM - Additional ViewModel injectionThe
ctxSpendViewModelis properly injected usingviewModels()delegate to support the email intent creation functionality.
98-100: LGTM - Activity result launcher properly configuredThe
ActivityResultLauncheris correctly registered to handle external email intent launching without requiring specific result handling.
172-176: LGTM - Contact support visibility properly controlledThe visibility logic correctly shows the contact support option only when there's an error, providing users with a way to report issues when they encounter problems.
186-198: LGTM - Contact support implementation is well-structuredThe contact support click handler properly:
- Creates an email intent with descriptive subject including transaction ID
- Passes the error as CTXSpendException for detailed reporting
- Uses Intent.createChooser for better user experience
- Launches through the registered ActivityResultLauncher
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt (8)
44-44: LGTM - Import supports service name trackingThe
ServiceNameimport correctly supports the enhanced payment service functionality that tracks the source service for transactions.
152-157: LGTM - Code optimization with local variableIntroducing the
fiatAmountlocal variable eliminates redundant formatting calls and improves code readability by calculating the formatted amount once.
177-181: LGTM - Enhanced exception with better contextThe updated
CTXSpendExceptioncreation properly:
- Includes formatted fiat amount and merchant ID in the message for better debugging
- Passes the original throwable as the cause for complete error context
186-186: LGTM - Clearer error messageThe updated exception message "purchaseGiftCard error: no merchant" is more descriptive than the previous version.
190-190: LGTM - Service name tracking implementationThe
payWithDashUrlcall correctly includes the service name parameter, using the merchant's source or defaulting toServiceName.CTXSpend. This enables proper transaction metadata tracking.
278-285: LGTM - Method signature updated for richer dataThe
saveGiftCardDummymethod signature change fromStringtoGiftCardResponseis beneficial:
- Uses
cardFiatAmountfrom the response for more accurate pricing- Uses the response's
idfield for the note, providing better data consistency
332-342: LGTM - Email intent method enhanced for flexible reportingThe updated
createEmailIntentmethod properly:
- Accepts a
sentToCTXboolean to determine the recipient email address- Makes the exception parameter nullable to handle cases where no exception exists
- Maintains the same email creation flow with enhanced flexibility
348-402: LGTM - Comprehensive error reporting implementationThe enhanced
createReportEmailmethod provides excellent detailed reporting:
- Handles nullable exceptions gracefully
- Includes comprehensive gift card response information when available
- Adds stack trace information for debugging
- Uses descriptive field labels like "amount entered" for clarity
- Properly formats all the information in a readable structure
wallet/src/de/schildbach/wallet/payments/SendCoinsTaskRunner.kt (5)
59-59: LGTM!The dependency injection of
TransactionMetadataProvideris properly implemented.Also applies to: 80-81
176-180: LGTM!The optional
serviceNameparameter is correctly added and propagated through the payment flow.
183-209: LGTM!The
serviceNameparameter is correctly propagated through the payment request creation flow, and the visibility change toprivateforcreateRequestFromPaymentIntentis appropriate.
233-247: LGTM!The
serviceNameparameter is correctly propagated to thedirectPaymethod.
249-258: LGTM!The transaction metadata is correctly set after completing the transaction and before submitting the payment.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
wallet/src/de/schildbach/wallet/payments/SendCoinsTaskRunner.kt (1)
312-331: Simplify and hardenisTransactionOnNetworka)
return try { … }combined with multiple innerreturnstatements makes the control-flow hard to read.
b) The fallback branch (tx == null) still checks onlyisTransactionLocked, missing chain-locks or peer broadcasts (same concern raised in earlier reviews).private fun isTransactionOnNetwork(transaction: Transaction): Boolean = try { val wallet = walletData.wallet ?: return false val candidate = wallet.getTransaction(transaction.txId) ?: transaction val c = candidate.confidence ?: return false c.source == TransactionConfidence.Source.NETWORK || c.confidenceType == TransactionConfidence.ConfidenceType.BUILDING || c.numBroadcastPeers() > 0 || c.isTransactionLocked || c.isChainLocked } catch (e: Exception) { log.debug("Error checking transaction network status: ${e.message}") false }This keeps the code linear, covers all known confidence indicators and avoids nested
returns.
(Feel free to drop the chain-lock/broadcast checks in the fallback if you prefer the previous ultra-conservative stance.)
🧹 Nitpick comments (1)
wallet/src/de/schildbach/wallet/payments/SendCoinsTaskRunner.kt (1)
272-306: Tighten the HTTP-failure recovery loop
The first delay in
delaysis0L; this causes an immediateisTransactionOnNetwork()check right after the catch, duplicating the call you already need at the end of the loop.
Dropping the zero-delay entry removes redundant work.
wallet.commitTx(tx)inside the loop may throwIllegalStateExceptionif the tx has already been committed by another thread/peer. Wrapping it in a try/catch (and ignoringIllegalStateException) keeps the recovery path bomb-proof.-val delays = listOf(0L, 1000L, 3000L, 5000L) +val delays = listOf(1000L, 3000L, 5000L) ... - if (isTransactionOnNetwork(tx)) { + if (isTransactionOnNetwork(tx)) { log.info("Transaction found on network despite HTTP timeout: ${tx.txId}") - wallet.commitTx(tx) + try { + wallet.commitTx(tx) + } catch (e: IllegalStateException) { + // already in wallet – safe to ignore + } return tx }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/CTXSpendRepository.kt(1 hunks)features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt(7 hunks)features/exploredash/src/test/java/org/dash/wallet/features/exploredash/CTXSpendExceptionTest.kt(3 hunks)wallet/src/de/schildbach/wallet/payments/SendCoinsTaskRunner.kt(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- features/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/CTXSpendRepository.kt
- features/exploredash/src/test/java/org/dash/wallet/features/exploredash/CTXSpendExceptionTest.kt
- features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt
🧰 Additional context used
🧠 Learnings (10)
📓 Common learnings
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1390
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt:129-145
Timestamp: 2025-05-08T18:11:40.249Z
Learning: The hardcoded test data in the purchaseGiftCard() function of CTXSpendViewModel is intentionally left in place for testing the error handling for limit mismatch, and will be fixed later before release.
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1410
File: wallet/src/de/schildbach/wallet/data/InvitationLinkData.kt:79-84
Timestamp: 2025-07-12T07:12:04.769Z
Learning: HashEngineering prefers to handle defensive validation improvements for URI parsing in follow-up PRs rather than including them in the current PR when the main focus is on replacing Firebase with AppsFlyer.
📚 Learning: 2025-04-19T07:01:17.535Z
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In the Dash Wallet app, DashPayConfig methods like isTransactionMetadataInfoShown() already use withContext(Dispatchers.IO) internally, so wrapping these calls with another withContext(Dispatchers.IO) in ViewModels is redundant.
Applied to files:
wallet/src/de/schildbach/wallet/payments/SendCoinsTaskRunner.kt
📚 Learning: 2025-08-08T13:29:37.702Z
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1419
File: wallet/src/de/schildbach/wallet/payments/SendCoinsTaskRunner.kt:312-331
Timestamp: 2025-08-08T13:29:37.702Z
Learning: Dash Wallet: For network presence checks, if a transaction is not in the wallet, we generally won’t have broadcast peer data (likely 0) and chain-locks only arrive after the block containing the transaction; thus fallback confidence checks on the local transaction often remain false. It’s safe but usually non-effective to include them; primary detection should rely on the wallet-known transaction.
Applied to files:
wallet/src/de/schildbach/wallet/payments/SendCoinsTaskRunner.kt
📚 Learning: 2025-04-19T07:01:17.535Z
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1362
File: wallet/src/de/schildbach/wallet/ui/more/SettingsViewModel.kt:107-115
Timestamp: 2025-04-19T07:01:17.535Z
Learning: In Dash Wallet, DataStore operations (like those in DashPayConfig through BaseConfig) already handle background threading internally. Adding another layer of withContext(Dispatchers.IO) around these calls in ViewModels is redundant because the Jetpack DataStore API automatically dispatches its operations to a background thread.
Applied to files:
wallet/src/de/schildbach/wallet/payments/SendCoinsTaskRunner.kt
📚 Learning: 2025-05-08T18:11:40.249Z
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1390
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/ctxspend/CTXSpendViewModel.kt:129-145
Timestamp: 2025-05-08T18:11:40.249Z
Learning: The hardcoded test data in the purchaseGiftCard() function of CTXSpendViewModel is intentionally left in place for testing the error handling for limit mismatch, and will be fixed later before release.
Applied to files:
wallet/src/de/schildbach/wallet/payments/SendCoinsTaskRunner.kt
📚 Learning: 2025-05-06T15:46:59.440Z
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1386
File: wallet/src/de/schildbach/wallet/ui/send/SendCoinsViewModel.kt:108-0
Timestamp: 2025-05-06T15:46:59.440Z
Learning: In UI code where frequent updates to a value might trigger expensive operations (like the SendCoinsViewModel's `executeDryrun` method), it's preferable to use a Flow with debounce rather than launching a new coroutine for each update. This prevents race conditions, reduces resource usage, and provides a more reactive architecture.
Applied to files:
wallet/src/de/schildbach/wallet/payments/SendCoinsTaskRunner.kt
📚 Learning: 2025-05-12T09:14:36.656Z
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1391
File: wallet/src/de/schildbach/wallet/ui/payments/PaymentsFragment.kt:111-119
Timestamp: 2025-05-12T09:14:36.656Z
Learning: In the dashpay/dash-wallet project, explicitly unregistering ViewPager2 callbacks is not considered critical since memory leaks haven't been observed in practice with the memory profiler.
Applied to files:
wallet/src/de/schildbach/wallet/payments/SendCoinsTaskRunner.kt
📚 Learning: 2025-05-07T14:18:11.161Z
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1389
File: features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/explore/SearchFragment.kt:45-46
Timestamp: 2025-05-07T14:18:11.161Z
Learning: In the ExploreViewModel of the dash-wallet application, `appliedFilters` is a StateFlow (not LiveData), so Flow operators like `distinctUntilChangedBy` can be used with it.
Applied to files:
wallet/src/de/schildbach/wallet/payments/SendCoinsTaskRunner.kt
📚 Learning: 2025-07-12T07:12:04.769Z
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1410
File: wallet/src/de/schildbach/wallet/data/InvitationLinkData.kt:79-84
Timestamp: 2025-07-12T07:12:04.769Z
Learning: HashEngineering prefers to handle defensive validation improvements for URI parsing in follow-up PRs rather than including them in the current PR when the main focus is on replacing Firebase with AppsFlyer.
Applied to files:
wallet/src/de/schildbach/wallet/payments/SendCoinsTaskRunner.kt
📚 Learning: 2025-04-16T17:07:27.359Z
Learnt from: HashEngineering
PR: dashpay/dash-wallet#1377
File: wallet/src/de/schildbach/wallet/service/platform/PlatformService.kt:0-0
Timestamp: 2025-04-16T17:07:27.359Z
Learning: For PlatformService in the Dash Wallet, the implementation should avoid throwing exceptions when Constants.SUPPORTS_PLATFORM is false. The platform should only be initialized when SUPPORTS_PLATFORM is true, and initialization should be done lazily to defer Platform creation until needed.
Applied to files:
wallet/src/de/schildbach/wallet/payments/SendCoinsTaskRunner.kt
🪛 detekt (1.23.8)
wallet/src/de/schildbach/wallet/payments/SendCoinsTaskRunner.kt
[warning] 329-329: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
| serviceName?.let { | ||
| metadataProvider.setTransactionService(sendRequest.tx.txId, serviceName) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Delay setting metadata until the transaction is committed
metadataProvider.setTransactionService() is called immediately after wallet.completeTx(), i.e. before the transaction is definitely persisted (commitTx()) later in the method.
If the subsequent HTTP submission fails and the retry loop ultimately decides that the transaction never reached the network, the metadata row will still have been written and will point at a tx that never becomes part of the wallet.
Consider moving the metadata write to the “success” paths (wallet.commitTx(tx) inside the retry loop and the sendCoins() call at the end).
This guarantees the metadata is stored only when the transaction is actually accepted by the wallet.
🤖 Prompt for AI Agents
In wallet/src/de/schildbach/wallet/payments/SendCoinsTaskRunner.kt around lines
256 to 258, the call to metadataProvider.setTransactionService() occurs
immediately after wallet.completeTx(), before the transaction is committed with
commitTx(). To fix this, move the metadataProvider.setTransactionService() call
into the success paths where the transaction is definitely persisted, such as
inside the retry loop after wallet.commitTx(tx) and after the final sendCoins()
call. This ensures metadata is only set when the transaction is successfully
accepted by the wallet.
2 participants
Issue being fixed or feature implemented
improve error handing when purchasing CTX gift cards.
Handle timeouts, error 500 and others.
Give the user the option to send a report to CTX or DCG depending on the type of error.
Related PR's and Dependencies
Screenshots / Videos
How Has This Been Tested?
Checklist:
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Tests