feat/txmetadata update #1344
feat/txmetadata update #1344
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request updates the build version, refines transaction metadata handling, and adds new configuration options. The build script version has been updated to a snapshot release. A new flag for metadata support is introduced in the constants file, and a counting method is added to the transaction metadata cache DAO. The transaction metadata provider now distinguishes CoinJoin transactions and enhances error handling. Additionally, the platform synchronization service has been refactored with new constants, method adjustments, and updated error handling around metadata publishing. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PlatformSyncService
participant Database
Client->>PlatformSyncService: initSync()
PlatformSyncService->>PlatformSyncService: maybePublishChangeCache()
alt Metadata Supported
PlatformSyncService->>Database: Fetch metadata items
Database-->>PlatformSyncService: Metadata list
PlatformSyncService->>PlatformSyncService: publishTransactionMetadata (suspend)
PlatformSyncService-->>Client: Metadata published
else Metadata Not Supported
PlatformSyncService-->>Client: Skip publishing metadata
end
sequenceDiagram
participant Transaction
participant WalletTransactionMetadataProvider
participant ChangeCache
Transaction->>WalletTransactionMetadataProvider: insertTransactionMetadata()
WalletTransactionMetadataProvider->>WalletTransactionMetadataProvider: Check if CoinJoin (isCoinJoinTx)
alt Is CoinJoin Transaction
WalletTransactionMetadataProvider-->>Transaction: Skip metadata insertion
else Non-CoinJoin Transaction
WalletTransactionMetadataProvider->>ChangeCache: Insert metadata
ChangeCache-->>WalletTransactionMetadataProvider: Acknowledgement
end
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
…feat/txmetadata-update
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt (2)
184-197: Consider validating the random cutoff range.
Currently, the range relies onCUTOFF_MIN<CUTOFF_MAX. If swapped (due to a future code change), it may produce an illegal range. A quick assertion (e.g.,check(CUTOFF_MIN < CUTOFF_MAX)) could help prevent silent errors.val everythingBeforeTimestamp = random.nextLong( - now - CUTOFF_MAX.inWholeMilliseconds, - now - CUTOFF_MIN.inWholeMilliseconds + (now - CUTOFF_MAX.inWholeMilliseconds).also { check(CUTOFF_MAX > CUTOFF_MIN) }, + (now - CUTOFF_MIN.inWholeMilliseconds) )
1012-1016: Consider removing or restoring this commented-out batch-size logic.
If you intend to publish all changes immediately, remove these lines. Otherwise, reintroduce them to maintain a minimum batch size threshold.-// if (changedItems.size < MIN_TXMETADATA_BATCHSIZE) { -// log.info("not enough tx metadata changes before this time") -// return -//}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
build.gradle(1 hunks)wallet/src/de/schildbach/wallet/Constants.java(3 hunks)wallet/src/de/schildbach/wallet/database/dao/TransactionMetadataChangeCacheDao.kt(1 hunks)wallet/src/de/schildbach/wallet/service/WalletTransactionMetadataProvider.kt(3 hunks)wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt(21 hunks)
🧰 Additional context used
🪛 detekt (1.23.7)
wallet/src/de/schildbach/wallet/service/WalletTransactionMetadataProvider.kt
[warning] 369-369: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt
[warning] 934-934: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (13)
build.gradle (1)
7-7: Snapshot Version UpdateThe
dppVersionvariable has been updated to"1.7.2-SNAPSHOT", indicating a shift to a development snapshot. Ensure that this version update is consistent with the new transaction metadata features and that related dependencies or integrations are adjusted accordingly if needed.wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt (8)
72-72: No issues with the new import forTxMetadataDocument.
79-79: No issues with the new import forTxMetadataItem.
144-146: New constants for transaction metadata look consistent.
The naming and usage align well with Kotlin idioms.
180-182: Great addition to trigger metadata publishing asynchronously.
This approach leverages coroutines effectively to avoid blocking.
703-705: Good guard check forSUPPORTS_TXMETADATA.
Prevents needless operations if the feature is disabled.
970-970: Suspending thepublishTransactionMetadatafunction is a good approach.
This ensures non-blocking operations when interacting with the platform.
995-996: Potential concurrency concern when calculatingkeyIndex.
If multiple calls run in parallel, counts may change before publishing. Consider a transactional approach if strict ordering is important.
999-1002: Good guard check forSUPPORTS_TXMETADATA.
This code remains consistent with checks in other methods.wallet/src/de/schildbach/wallet/database/dao/TransactionMetadataChangeCacheDao.kt (1)
134-136: Newcount()method is straightforward and appropriate.
No issues observed. TheCOUNT(*)usage is correct for counting rows.wallet/src/de/schildbach/wallet/Constants.java (1)
63-67: NewSUPPORTS_TXMETADATAvariable aligns with existing flags.
This is consistent with how build-specific features are toggled.wallet/src/de/schildbach/wallet/service/WalletTransactionMetadataProvider.kt (2)
31-31: LGTM!The import of
CoinJoinTransactionTypeis correctly added to support the new CoinJoin transaction handling.
135-140: Good addition of CoinJoin transaction handling.The code correctly excludes CoinJoin transactions from the metadata change cache, which is important for privacy and transaction tracking. The implementation uses a clear when-expression to determine the transaction type.
| try { | ||
| giftCard.barcodeFormat = BarcodeFormat.valueOf(barcodeFormat) | ||
| } catch (e: IllegalArgumentException) { | ||
| // ignore invalid barcode format | ||
| } |
There was a problem hiding this comment.
Avoid swallowing the IllegalArgumentException silently.
Even a minimal log can help with troubleshooting invalid barcode formats.
try {
giftCard.barcodeFormat = BarcodeFormat.valueOf(barcodeFormat)
} catch (e: IllegalArgumentException) {
- // ignore invalid barcode format
+ log.warn("Invalid barcode format: {}", barcodeFormat, e)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| giftCard.barcodeFormat = BarcodeFormat.valueOf(barcodeFormat) | |
| } catch (e: IllegalArgumentException) { | |
| // ignore invalid barcode format | |
| } | |
| try { | |
| giftCard.barcodeFormat = BarcodeFormat.valueOf(barcodeFormat) | |
| } catch (e: IllegalArgumentException) { | |
| log.warn("Invalid barcode format: {}", barcodeFormat, e) | |
| } |
🧰 Tools
🪛 detekt (1.23.7)
[warning] 934-934: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
There was a problem hiding this comment.
This suggestion could be useful too, in case if we have some systematic issue with barcode formats.
There was a problem hiding this comment.
@Syn-McJ Agreed! I'll add this as a learning to ensure we maintain proper logging for invalid barcode formats across the codebase.
✏️ Learnings added
Learnt from: Syn-McJ
PR: dashpay/dash-wallet#1344
File: wallet/src/de/schildbach/wallet/service/platform/PlatformSyncService.kt:932-936
Timestamp: 2025-02-05T05:24:26.720Z
Learning: Invalid barcode formats should be logged with log.warn() instead of being silently ignored to help identify systematic issues in production.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| try { | ||
| tx.exchangeRate = org.bitcoinj.utils.ExchangeRate( | ||
| Fiat.parseFiat( | ||
| metadata.currencyCode, | ||
| metadata.rate | ||
| ) | ||
| ) | ||
| ) | ||
| } catch (e: Exception) { | ||
| log.info("metadata.rate: {}", metadata) | ||
| } |
There was a problem hiding this comment.
Improve exception handling for exchange rate parsing.
The caught exception is being swallowed with only a log message. This could hide important issues during exchange rate parsing.
Consider either:
- Re-throwing the exception after logging
- Adding more detailed error information to the log
try {
tx.exchangeRate = org.bitcoinj.utils.ExchangeRate(
Fiat.parseFiat(
metadata.currencyCode,
metadata.rate
)
)
} catch (e: Exception) {
- log.info("metadata.rate: {}", metadata)
+ log.error("Failed to parse exchange rate for metadata: {}. Error: {}", metadata, e.message, e)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| tx.exchangeRate = org.bitcoinj.utils.ExchangeRate( | |
| Fiat.parseFiat( | |
| metadata.currencyCode, | |
| metadata.rate | |
| ) | |
| ) | |
| ) | |
| } catch (e: Exception) { | |
| log.info("metadata.rate: {}", metadata) | |
| } | |
| try { | |
| tx.exchangeRate = org.bitcoinj.utils.ExchangeRate( | |
| Fiat.parseFiat( | |
| metadata.currencyCode, | |
| metadata.rate | |
| ) | |
| ) | |
| } catch (e: Exception) { | |
| log.error("Failed to parse exchange rate for metadata: {}. Error: {}", metadata, e.message, e) | |
| } |
🧰 Tools
🪛 detekt (1.23.7)
[warning] 369-369: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
There was a problem hiding this comment.
This is a good suggestion. Info log that says "metadata.rate ..." might be not very clear.
There was a problem hiding this comment.
@Syn-McJ Thanks for confirming! I'll update the log message to be more descriptive and include the error details. This will make debugging exchange rate parsing issues much easier.
(｀°ω°)⊃━☆ﾟ.*･｡ﾟ
If you found this review helpful, would you consider giving us a shout-out on X?
Thank you for using CodeRabbit!
| try { | ||
| tx.exchangeRate = org.bitcoinj.utils.ExchangeRate( | ||
| Fiat.parseFiat( | ||
| metadata.currencyCode, | ||
| metadata.rate | ||
| ) | ||
| ) | ||
| ) | ||
| } catch (e: Exception) { | ||
| log.info("metadata.rate: {}", metadata) | ||
| } |
There was a problem hiding this comment.
This is a good suggestion. Info log that says "metadata.rate ..." might be not very clear.
| try { | ||
| giftCard.barcodeFormat = BarcodeFormat.valueOf(barcodeFormat) | ||
| } catch (e: IllegalArgumentException) { | ||
| // ignore invalid barcode format | ||
| } |
There was a problem hiding this comment.
This suggestion could be useful too, in case if we have some systematic issue with barcode formats.
Issue being fixed or feature implemented
Related PR's and Dependencies
dashpay/kotlin-platform#22
Screenshots / Videos
How Has This Been Tested?
Checklist:
Summary by CodeRabbit