Conversation
- Fix pagination race conditions by canceling in-flight requests on filter change - Include existing data in LoadingMore state to prevent UI flicker - Use Locale.ROOT for ISO 8601 date parsing - Remove tokenSymbol from Transaction (not returned by API) - Remove unused TOKEN_ADDRESS_TO_SYMBOL mapping Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
- Build transaction list immutably to avoid race conditions - Propagate CancellationException instead of showing error state - Clarify comment about Locale usage in date formatting Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
fix: transaction history POS
|
Claude finished @jakubuid's task in 2m 6s —— View job Code Review for BOM_1.6.1Reviewed changes for version 1.6.1 release focusing on POS module transaction history feature. Summary: Version bump to 1.6.1 (BOM only) adds transaction history API to POS module with new merchant API endpoint, data models, UI screen, and pagination support. 7 pending CI checks (lint, build, tests running). Found 6 issue(s)Issue 1: Hardcoded internal API key in build configID: buildconfig-apikey-exposure-f8d2 Context:
Recommendation: Use runtime config or secure key storage: // Remove from buildConfigField, use secured runtime injection instead
// Or restrict to debug builds only:
buildTypes {
debug {
buildConfigField(type = "String", name = "INTERNAL_MERCHANT_API", value = "\"${System.getenv("INTERNAL_MERCHANT_API") ?: ""}\"")
}
release {
buildConfigField(type = "String", name = "INTERNAL_MERCHANT_API", value = "\"\"")
}
}Issue 2: Missing timeout/retry for merchant API callsID: apiclient-merchantapi-timeout-9a3c Context:
Recommendation: Add exponential backoff retry (WalletConnect Pay architecture requirement): suspend fun getTransactionHistory(...): ApiResult<TransactionHistoryResponse> {
return retryWithBackoff(maxAttempts = 3, initialDelayMs = 500) {
try {
val response = merchantApi.getTransactionHistory(...)
// ... existing logic
} catch (e: IOException) { throw e } // Retryable
}
}Issue 3: Race condition in transaction history paginationID: posviewmodel-pagination-race-c1b5 Context:
Recommendation: Add loading guard: fun loadMoreTransactions() {
val state = _transactionHistoryState.value
if (state is TransactionHistoryUiState.Success && state.hasMore &&
transactionLoadingJob?.isActive != true) { // Add guard
loadTransactionHistory(refresh = false)
}
}Issue 4: Unvalidated timestamp parsing could crashID: transactionhistory-timestamp-crash-7e2a Context:
Recommendation: Use stricter parsing with multiple format fallbacks: private fun formatTimestamp(timestamp: String): String {
val formats = listOf(
"yyyy-MM-dd'T'HH:mm:ss.SSS'Z'",
"yyyy-MM-dd'T'HH:mm:ss'Z'",
"yyyy-MM-dd'T'HH:mm:ss"
)
formats.forEach { pattern ->
try {
val inputFormat = SimpleDateFormat(pattern, Locale.ROOT)
inputFormat.timeZone = TimeZone.getTimeZone("UTC")
// ... parse and format
} catch (e: Exception) { /* try next format */ }
}
return timestamp.take(19) // Show first 19 chars as fallback
}Issue 5: No deduplication check for transaction history messagesID: apiclient-history-dedup-missing-4b1c Context:
Recommendation: Add client-side deduplication in ViewModel: // In loadTransactionHistory onSuccess block:
val uniqueTransactions = updatedList.distinctBy { it.paymentId }
loadedTransactions.clear()
loadedTransactions.addAll(uniqueTransactions)Issue 6: Test coverage missing for transaction historyID: apiclienttest-history-notcovered-8d3f Context:
Recommendation: Add transaction history tests: @Test
fun `getTransactionHistory - returns paginated results with cursor`() = runTest {
// Test pagination, empty results, error handling, status filtering
}
@Test
fun `transaction mapping - handles null fields correctly`() = runTest {
// Test PaymentRecord.toTransaction() edge cases
}Additional Notes:
Positive Observations:
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| buildConfigField(type = "String", name = "MERCHANT_API_BASE_URL", value = "\"https://api.merchant.pay.walletconnect.com\"") | ||
| buildConfigField(type = "String", name = "PULSE_BASE_URL", value = "\"https://pulse.walletconnect.org\"") | ||
| buildConfigField(type = "String", name = "POS_PROJECT_ID", value = "\"${System.getenv("POS_PROJECT_ID") ?: ""}\"") | ||
| buildConfigField(type = "String", name = "INTERNAL_MERCHANT_API", value = "\"${System.getenv("INTERNAL_MERCHANT_API") ?: ""}\"") |
There was a problem hiding this comment.
🤖 Auto Review Issue: Hardcoded internal API key in build config
Severity: HIGH
Category: security
Tool: Claude Auto Review
Context:
- Pattern: Line 32 embeds
INTERNAL_MERCHANT_APIenv var directly into BuildConfig, compiled into APK - Risk: If env var empty or compromised, API key shipped in release builds → reverse engineering exposure
- Impact: Unauthorized merchant API access, potential data breach, merchant impersonation
- Trigger: Any APK decompilation or ProGuard bypass reveals hardcoded key
Recommendation: Use runtime config or secure key storage:
// Remove from buildConfigField, use secured runtime injection instead
// Or restrict to debug builds only:
buildTypes {
debug {
buildConfigField(type = "String", name = "INTERNAL_MERCHANT_API", value = "\"${System.getenv("INTERNAL_MERCHANT_API") ?: ""}\"")
}
release {
buildConfigField(type = "String", name = "INTERNAL_MERCHANT_API", value = "\"\"")
}
}
sample/pos/src/main/kotlin/com/walletconnect/sample/pos/screens/TransactionHistoryScreen.kt
Show resolved
Hide resolved
Version bumps
|
Claude finished @jakubuid's task in 1m 24s —— View job 🔄 Incremental Review: Changes since last review Issue StatusPreviously reported issues:
New Issues Found✅ No new issues found in latest commits Still present: 5 issue(s) + 1 partially resolvedIssue 1: Hardcoded internal API key in build configID: buildconfig-apikey-exposure-f8d2 Context:
Recommendation: buildTypes {
debug {
buildConfigField(type = "String", name = "INTERNAL_MERCHANT_API", value = "\"${System.getenv("INTERNAL_MERCHANT_API") ?: ""}\"")
}
release {
buildConfigField(type = "String", name = "INTERNAL_MERCHANT_API", value = "\"\"")
}
}Issue 2: Missing timeout/retry for merchant API callsID: apiclient-merchantapi-timeout-9a3c Context:
Recommendation: suspend fun getTransactionHistory(...): ApiResult<TransactionHistoryResponse> {
return retryWithBackoff(maxAttempts = 3, initialDelayMs = 500) {
// existing logic
}
}Issue 3: Race condition in transaction history paginationID: posviewmodel-pagination-race-c1b5 Context:
Recommendation: fun loadMoreTransactions() {
val state = _transactionHistoryState.value
if (state is TransactionHistoryUiState.Success && state.hasMore &&
transactionLoadingJob?.isActive != true) {
loadTransactionHistory(refresh = false)
}
}Issue 4: Unvalidated timestamp parsing could crashID: transactionhistory-timestamp-crash-7e2a Context:
Recommendation: private fun formatTimestamp(timestamp: String): String {
val formats = listOf(
"yyyy-MM-dd'T'HH:mm:ss.SSS'Z'",
"yyyy-MM-dd'T'HH:mm:ss'Z'",
"yyyy-MM-dd'T'HH:mm:ss"
)
formats.forEach { pattern ->
try {
val inputFormat = SimpleDateFormat(pattern, Locale.ROOT)
inputFormat.timeZone = TimeZone.getTimeZone("UTC")
val date = inputFormat.parse(timestamp)
return date?.let { SimpleDateFormat("MMM dd, yyyy HH:mm", Locale.getDefault()).format(it) } ?: timestamp
} catch (e: Exception) { /* try next */ }
}
return timestamp.take(19)
}Issue 5: No deduplication check for transaction history messagesID: apiclient-history-dedup-missing-4b1c Context:
Recommendation: // At line 216, after updatedList is built:
val uniqueTransactions = updatedList.distinctBy { it.paymentId }
loadedTransactions.clear()
loadedTransactions.addAll(uniqueTransactions)Issue 6: Test coverage missing for transaction historyID: apiclienttest-history-notcovered-8d3f Context:
Recommendation: @Test
fun `getTransactionHistory - returns paginated results with cursor`() = runTest {
// Test pagination, empty results, error handling, status filtering
}
@Test
fun `transaction mapping - handles null fields correctly`() = runTest {
// Test PaymentRecord.toTransaction() edge cases
}SummaryStatus: 5 issues still present from previous review, 1 partially addressed. No new issues introduced in latest commits. Partial improvement: POSViewModel now cancels in-flight requests (line 188) reducing but not eliminating race conditions. CI: SonarQube passed, 7 GitHub checks still pending. |
|
1 participant
No description provided.