feat: ctx redeem url support#1495
Merged
Merged
Conversation
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughExtends gift-card data models, API endpoints, and database schema to capture and propagate ChangesGift Card redeemUrlChallenge & Order ID Persistence
Minor Standalone Fixes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
…feat/ctx-redeem-url-support
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/utils/SavingsFormatting.kt (1)
27-30:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the KDoc contract to match current zero-handling behavior.
Line 29 says zero returns an empty string, but Line 50 now returns
"0%". Please align the docs with behavior to avoid caller confusion.Suggested doc update
- * render as "fee X%" using the absolute value; zero returns an empty string. + * render as "fee X%" using the absolute value; zero (or rounded-to-zero) + * returns "0%".Also applies to: 47-50
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@features/exploredash/src/main/java/org/dash/wallet/features/exploredash/utils/SavingsFormatting.kt` around lines 27 - 30, The KDoc comment for the function that formats a signed savings percentage states that zero returns an empty string, but the actual implementation (referenced around lines 47-50) returns "0%" for zero values. Update the KDoc description in the documentation block to accurately reflect that zero values return "0%" instead of an empty string to ensure the documented contract matches the implemented behavior.wallet/src/de/schildbach/wallet/database/dao/TransactionMetadataChangeCacheDao.kt (1)
115-127:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUpdate
has()to include the new gift-card columns in dedupe checks.Line 115 adds writes for
order,giftCardChallenge, andindex, but Line 173’shas(...)query and Line 212’s wrapper don’t compare those fields. This can treat changed gift-card metadata as an existing row and skip cache insertion.Suggested fix
@@ `@Query`(""" SELECT EXISTS( SELECT 1 FROM transaction_metadata_cache WHERE txId = :txId @@ AND barcodeValue IS :barcodeValue AND barcodeFormat IS :barcodeFormat AND merchantUrl IS :merchantUrl + AND `order` IS :order + AND giftCardChallenge IS :giftCardChallenge + AND `index` IS :index ) """) suspend fun has( txId: Sha256Hash, sentTimestamp: Long?, @@ merchantName: String?, originalPrice: Double?, barcodeValue: String?, barcodeFormat: String?, - merchantUrl: String? + merchantUrl: String?, + order: String?, + giftCardChallenge: String?, + index: Int? ): Boolean @@ item.merchantName, item.originalPrice, item.barcodeValue, item.barcodeFormat, - item.merchantUrl + item.merchantUrl, + item.order, + item.giftCardChallenge, + item.index ) } }Also applies to: 173-209, 212-229
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wallet/src/de/schildbach/wallet/database/dao/TransactionMetadataChangeCacheDao.kt` around lines 115 - 127, The insertGiftCardData function adds new fields (order, giftCardChallenge, and index) to the database, but the deduplication logic in the has() function around line 173 and its wrapper around line 212 do not include these fields in their comparison checks. Update both the has() function and its wrapper to add comparisons for the order, giftCardChallenge, and index fields when checking if a transaction metadata row already exists in the cache, ensuring that changes to these fields are properly detected and the cache is updated accordingly.wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt (1)
1353-1384:⚠️ Potential issue | 🟠 Major | ⚡ Quick win
indexis not propagated when folding cached gift-card changes.
orderandgiftCardChallengeare merged, butindexis skipped. For multi-card txs, later cache rows can carry corrected index, and this path currently drops it before publish.💡 Suggested fix
if (saveSettings.shouldSaveGiftcardInfo(saveAll)) { changedItem.customIconUrl?.let { customIconUrl -> item.customIconUrl = customIconUrl } @@ changedItem.giftCardChallenge?.let { giftCardChallenge -> item.giftCardChallenge = giftCardChallenge } + changedItem.index?.let { index -> + item.index = index + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt` around lines 1353 - 1384, The index field is not being propagated when merging cached gift-card changes in the shouldSaveGiftcardInfo block. Add a conditional block for the index field following the same pattern used for the other fields like order and giftCardChallenge (using the changedItem.index?.let construct to assign it to item.index), ensuring that corrected index values from later cache rows are properly propagated before publishing.wallet/src/de/schildbach/wallet/service/WalletTransactionMetadataProvider.kt (1)
355-364:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
mergedconsistently when persisting gift-card updates.This method merges partial input into
merged, but then writes cache payload fromgiftCard. That can publish stale/nullorder/redeemUrlChallengeon partial updates. Also,mergeditself should preserveredeemUrlChallengelikenote.💡 Suggested fix
} else { existing.copy( merchantName = giftCard.merchantName.ifEmpty { existing.merchantName }, price = giftCard.price.takeIf { it != 0.0 } ?: existing.price, number = giftCard.number ?: existing.number, pin = giftCard.pin ?: existing.pin, barcodeValue = giftCard.barcodeValue ?: existing.barcodeValue, barcodeFormat = giftCard.barcodeFormat ?: existing.barcodeFormat, merchantUrl = giftCard.merchantUrl ?: existing.merchantUrl, - note = giftCard.note ?: existing.note + note = giftCard.note ?: existing.note, + redeemUrlChallenge = giftCard.redeemUrlChallenge ?: existing.redeemUrlChallenge ) } // for now, only save the first card if (merged.index == 0) { transactionMetadataChangeCacheDao.insertGiftCardData( - giftCard.txId, - giftCard.number, - giftCard.pin, - giftCard.merchantName, - giftCard.price, - giftCard.merchantUrl, - giftCard.note, - giftCard.redeemUrlChallenge, - giftCard.index + merged.txId, + merged.number, + merged.pin, + merged.merchantName, + merged.price, + merged.merchantUrl, + merged.note, + merged.redeemUrlChallenge, + merged.index ) }Also applies to: 375-384
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@wallet/src/de/schildbach/wallet/service/WalletTransactionMetadataProvider.kt` around lines 355 - 364, The merge operation in the .copy() call creates a merged gift card object but the subsequent cache persistence is using the original giftCard instead of the merged result, which can publish stale or null values for order and redeemUrlChallenge. Add redeemUrlChallenge preservation to the .copy() call (similar to how note is handled with the ?: operator) and then ensure that any cache payload persistence after this merge operation uses the merged result instead of giftCard to avoid overwriting with incomplete data.
🧹 Nitpick comments (4)
features/exploredash/src/main/java/org/dash/wallet/features/exploredash/di/ExploreDashModule.kt (1)
100-101: ⚡ Quick winRemove commented-out binding rather than leaving it.
Since
CTXSpendRepositoryIntis no longer used (and should be deleted fromCTXSpendRepository.kt), delete these commented lines entirely rather than keeping dead code.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@features/exploredash/src/main/java/org/dash/wallet/features/exploredash/di/ExploreDashModule.kt` around lines 100 - 101, Delete the commented-out `@Binds` annotation and the abstract function provideCTXSpendRepository that binds CTXSpendRepositoryInt in ExploreDashModule.kt. Since CTXSpendRepositoryInt is no longer used, remove these dead code lines entirely rather than keeping them commented out. Additionally, ensure that the CTXSpendRepositoryInt interface is deleted from CTXSpendRepository.kt if it still exists there.features/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/CTXSpendRepository.kt (3)
338-348: ⚡ Quick winRemove unused
CTXSpendRepositoryIntinterface.This interface is now dead code:
CTXSpendRepositoryno longer implements it (line 105)- The DI binding in
ExploreDashModulewas removedDelete this interface to avoid confusion about the codebase's contract structure.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@features/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/CTXSpendRepository.kt` around lines 338 - 348, The CTXSpendRepositoryInt interface is unused dead code that is no longer implemented by CTXSpendRepository and is not bound in the dependency injection configuration. Locate and delete the entire CTXSpendRepositoryInt interface definition which includes the suspend functions purchaseGiftCard, getGiftCardByTxid, and refreshToken to clean up the codebase and remove confusion about the repository's contract structure.
223-243: ⚡ Quick winRemove commented-out code block.
This large commented block is dead code that adds noise. Since the active implementation above (lines 199-221) handles the same logic, this should be deleted.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@features/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/CTXSpendRepository.kt` around lines 223 - 243, Delete the entire commented-out code block containing the map operation that transforms card response objects into GiftCardInfo instances. This block (starting with the comment about return response?.result?.map and ending with the ?: listOf() operation) is dead code that duplicates the active implementation and should be completely removed from the CTXSpendRepository file to reduce noise and maintain code clarity.
301-307: 💤 Low valueConsider clearer naming for private helper methods.
_getGiftCardByTxIduses an underscore prefix (uncommon in Kotlin) andgetGiftCardByOrderId2has an unexplained "2" suffix. Since these are private helpers that simply delegate to the API, consider more descriptive names or inline them if they don't add value.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@features/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/CTXSpendRepository.kt` around lines 301 - 307, The private helper methods _getGiftCardByTxId and getGiftCardByOrderId2 use unclear naming conventions that don't follow Kotlin idioms - the underscore prefix in _getGiftCardByTxId is uncommon and the "2" suffix in getGiftCardByOrderId2 is unexplained and confusing. Since these methods only delegate directly to API calls without adding additional logic or value, either refactor them with clearer descriptive names that better explain their purpose, or inline the API calls directly at their call sites to reduce unnecessary indirection.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@features/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/dashspend/dialogs/GiftCardDetailsViewModel.kt`:
- Around line 482-484: The else-if branch checking for redeemUrl is incorrectly
calling updateGiftCard(cardIndex, giftCard.redeemUrl,
giftCard.redeemUrlChallenge), which passes URL data into a method signature
expecting number and PIN parameters. Instead of using this generic
updateGiftCard method, identify and call the URL-specific updater method that is
designed to handle redeemUrl and redeemUrlChallenge parameters correctly. Look
for an overloaded version of updateGiftCard or a separate URL-specific update
method in the same class.
---
Outside diff comments:
In
`@features/exploredash/src/main/java/org/dash/wallet/features/exploredash/utils/SavingsFormatting.kt`:
- Around line 27-30: The KDoc comment for the function that formats a signed
savings percentage states that zero returns an empty string, but the actual
implementation (referenced around lines 47-50) returns "0%" for zero values.
Update the KDoc description in the documentation block to accurately reflect
that zero values return "0%" instead of an empty string to ensure the documented
contract matches the implemented behavior.
In
`@wallet/src/de/schildbach/wallet/database/dao/TransactionMetadataChangeCacheDao.kt`:
- Around line 115-127: The insertGiftCardData function adds new fields (order,
giftCardChallenge, and index) to the database, but the deduplication logic in
the has() function around line 173 and its wrapper around line 212 do not
include these fields in their comparison checks. Update both the has() function
and its wrapper to add comparisons for the order, giftCardChallenge, and index
fields when checking if a transaction metadata row already exists in the cache,
ensuring that changes to these fields are properly detected and the cache is
updated accordingly.
In `@wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt`:
- Around line 1353-1384: The index field is not being propagated when merging
cached gift-card changes in the shouldSaveGiftcardInfo block. Add a conditional
block for the index field following the same pattern used for the other fields
like order and giftCardChallenge (using the changedItem.index?.let construct to
assign it to item.index), ensuring that corrected index values from later cache
rows are properly propagated before publishing.
In
`@wallet/src/de/schildbach/wallet/service/WalletTransactionMetadataProvider.kt`:
- Around line 355-364: The merge operation in the .copy() call creates a merged
gift card object but the subsequent cache persistence is using the original
giftCard instead of the merged result, which can publish stale or null values
for order and redeemUrlChallenge. Add redeemUrlChallenge preservation to the
.copy() call (similar to how note is handled with the ?: operator) and then
ensure that any cache payload persistence after this merge operation uses the
merged result instead of giftCard to avoid overwriting with incomplete data.
---
Nitpick comments:
In
`@features/exploredash/src/main/java/org/dash/wallet/features/exploredash/di/ExploreDashModule.kt`:
- Around line 100-101: Delete the commented-out `@Binds` annotation and the
abstract function provideCTXSpendRepository that binds CTXSpendRepositoryInt in
ExploreDashModule.kt. Since CTXSpendRepositoryInt is no longer used, remove
these dead code lines entirely rather than keeping them commented out.
Additionally, ensure that the CTXSpendRepositoryInt interface is deleted from
CTXSpendRepository.kt if it still exists there.
In
`@features/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/CTXSpendRepository.kt`:
- Around line 338-348: The CTXSpendRepositoryInt interface is unused dead code
that is no longer implemented by CTXSpendRepository and is not bound in the
dependency injection configuration. Locate and delete the entire
CTXSpendRepositoryInt interface definition which includes the suspend functions
purchaseGiftCard, getGiftCardByTxid, and refreshToken to clean up the codebase
and remove confusion about the repository's contract structure.
- Around line 223-243: Delete the entire commented-out code block containing the
map operation that transforms card response objects into GiftCardInfo instances.
This block (starting with the comment about return response?.result?.map and
ending with the ?: listOf() operation) is dead code that duplicates the active
implementation and should be completely removed from the CTXSpendRepository file
to reduce noise and maintain code clarity.
- Around line 301-307: The private helper methods _getGiftCardByTxId and
getGiftCardByOrderId2 use unclear naming conventions that don't follow Kotlin
idioms - the underscore prefix in _getGiftCardByTxId is uncommon and the "2"
suffix in getGiftCardByOrderId2 is unexplained and confusing. Since these
methods only delegate directly to API calls without adding additional logic or
value, either refactor them with clearer descriptive names that better explain
their purpose, or inline the API calls directly at their call sites to reduce
unnecessary indirection.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dd5eb979-0a4f-4e77-bcf9-42ed3db08c70
📒 Files selected for processing (26)
common/src/main/java/org/dash/wallet/common/data/entity/GiftCard.ktcommon/src/main/java/org/dash/wallet/common/payments/parsers/DashPaymentIntentParser.ktdocs/proposals/persist-gift-card-order-id.mdfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/data/dashspend/ctx/model/GiftCardResponse.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/data/dashspend/model/GiftCardInfo.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/di/ExploreDashModule.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/network/service/ctxspend/CTXSpendApi.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/CTXSpendRepository.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/DashSpendRepository.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/repository/PiggyCardsRepository.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/dashspend/DashSpendViewModel.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/dashspend/dialogs/GiftCardDetailsDialog.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/dashspend/dialogs/GiftCardDetailsViewModel.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/ui/dashspend/dialogs/PurchaseGiftCardConfirmDialog.ktfeatures/exploredash/src/main/java/org/dash/wallet/features/exploredash/utils/SavingsFormatting.ktfeatures/exploredash/src/main/res/values/strings-explore-dash.xmlwallet/androidTest/de/schildbach/wallet/database/DatabaseMigrationTest.ktwallet/schemas/de.schildbach.wallet.database.AppDatabase/19.jsonwallet/src/de/schildbach/wallet/database/AppDatabase.ktwallet/src/de/schildbach/wallet/database/AppDatabaseMigrations.ktwallet/src/de/schildbach/wallet/database/dao/TransactionMetadataChangeCacheDao.ktwallet/src/de/schildbach/wallet/database/entity/TransactionMetadataCacheItem.ktwallet/src/de/schildbach/wallet/database/entity/TransactionMetadataDocument.ktwallet/src/de/schildbach/wallet/di/DatabaseModule.ktwallet/src/de/schildbach/wallet/service/WalletTransactionMetadataProvider.ktwallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt
HashEngineering
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@build.gradle`:
- Line 7: Replace the mutable SNAPSHOT version in the dppVersion variable with a
fixed, stable release version like "2.0.6" instead of "2.0.6-SNAPSHOT". This
ensures reproducible builds and prevents version drift across builds. If
snapshot versions are needed for development or testing, implement a mechanism
to allow local overrides (such as a properties file or command-line flag) rather
than having the snapshot version as the default in build.gradle, since the
dppVersion is used across dash-sdk dependencies and displayed to users.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Issue being fixed or feature implemented
Related PR's and Dependencies
Screenshots / Videos
How Has This Been Tested?
Checklist:
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
0%instead of a negative rounded value.Improvements