-
Notifications
You must be signed in to change notification settings - Fork 22
feat: Merge piggycards to master #742
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Added SWIFT_ACTIVE_COMPILATION_CONDITIONS with PIGGYCARDS_ENABLED flag to all main app build configurations: - Debug: "DEBUG PIGGYCARDS_ENABLED" - Release: "PIGGYCARDS_ENABLED" - Testflight: "PIGGYCARDS_ENABLED" - Testnet: "DEBUG PIGGYCARDS_ENABLED DASH_TESTNET" This enables all PiggyCards functionality including: - PiggyCards provider in GiftCardProvider enum - PiggyCards repository initialization - PiggyCards UI filters and authentication flows - PiggyCards merchant data integration 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
- Remove explore.db-shm and explore.db-wal from git tracking - Add *.db-shm and *.db-wal to .gitignore to prevent future commits These are SQLite Write-Ahead Log (WAL) files that should not be tracked in version control. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
feat: merge master 8.4.2 into piggcards
Fixed critical issues with PiggyCards gift card integration: 1. Fixed missing sourceId in MerchantDAO SQL queries - Added sourceId column to SELECT statements in 3 locations - Properly handle sourceId as String, Int64, or Int - Without sourceId, PiggyCards API calls were failing 2. Fixed denomination type handling using API as source of truth - API response now correctly overrides database values when signed in - CTX and PiggyCards can have different denomination types for same merchant - Fixed state clearing when switching between fixed/flexible types 3. Fixed provider data isolation - Clear min/max values when switching to fixed denominations - Clear denominations array when switching to flexible amounts - Prevents cross-contamination between CTX and PiggyCards data 4. Enhanced debugging and error handling - Added comprehensive logging with 🎯 emoji markers - Log state BEFORE and AFTER API calls - Special handling for Domino's to prefer fixed price cards - Validate min/max values from API responses 5. Updated CLAUDE.md documentation - Added PiggyCards integration section with critical lessons learned - Documented sourceId database architecture requirements - Added debugging strategies and common pitfalls Expected behavior after fix: - AutoZone: Shows keyboard with $10-$200 range for PiggyCards - Domino's: Shows $25 fixed denomination button for PiggyCards - Macy's: Shows keyboard with $5-$500 range for PiggyCards 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
When POIDetailsViewController refreshed merchant info from the CTX API, it was creating a new merchant object but forgetting to copy the giftCardProviders array. This caused the sourceId to be lost, preventing PiggyCards API calls from working correctly. Changes: - Added giftCardProviders parameter to updatingMerchant() to preserve the providers array populated by MerchantDAO - Added debug logging to MerchantDAO to trace provider queries 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Added comprehensive documentation for adding test merchants to the explore database, including: - Complete SQL scripts with FTS trigger handling - PiggyCards test merchant specifications (merchant ID: 2e393eee-4508-47fe-954d-66209333fc96) - Verification and cleanup procedures - Best practices for test data management 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…ider support Major improvements: - Added PIN authorization for all PiggyCards transactions - Implemented provider column in database to distinguish CTX vs PiggyCards - Fixed transaction icons to properly show gift card purchases Authentication & Security: - Modified SendCoinsService to explicitly request PIN before signing - Added @mainactor authentication wrapper to ensure UI operations on main thread - Fixed authentication crash by properly handling async operations Database & Migration: - Added provider column to gift_cards table via migration - Created AddProviderToGiftCardsTable migration - Updated GiftCard model and DAO to handle provider field - Made database operations defensive to handle missing columns gracefully PiggyCards Features: - Implemented dedicated PiggyCards polling (60 seconds max, 1.5s intervals) - Added order status checking for PiggyCards API - Support for both barcode and claim link redemption types - Fixed case sensitivity issues in API field names Gift Card Improvements: - Updated GiftCardDetailsViewModel to route to correct provider API - Enhanced GiftCardMetadataProvider to observe gift card changes in real-time - Gift cards now immediately show with orange icon in transaction history - Fixed barcode generation from claim codes API & Models: - Fixed all PiggyCards API field mappings to camelCase - Made API response fields optional to handle variations - Removed verbose logging to prevent console overflow - Temporarily bypassed exchange rate for testing Bug Fixes: - Resolved crash when opening app due to missing provider column - Fixed gift card selection algorithm for denomination matching - Corrected date formatting for PiggyCards API - Fixed unnecessary await warnings in transaction signing 🤖 Generated with Claude Code Co-Authored-By: Claude <noreply@anthropic.com>
- Add database migration to automatically insert PiggyCards test merchant - Migration only runs in DEBUG or TESTFLIGHT builds - Handles FTS triggers properly to avoid SQLite errors - Checks for existing merchant to avoid duplicates - Adds both merchant and gift_card_providers records - Must be removed before production release Test merchant details: - Name: Piggy Cards Test Merchant - ID: 2e393eee-4508-47fe-954d-66209333fc96 - Source ID: 177 - 10% discount - Fixed denomination type 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Remove Amazon test merchant from TestFlight builds - Remove special handling logic for test merchants - Keep only PiggyCards test merchant without special treatment - Test merchant now uses standard merchant flow and real API - Clean up all debug messages and special test logic - Update version to 8.5.0 across all targets 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Move giftCardId assignment before the async payment call to ensure the payment ID is stored even if the payment fails or throws an error. Previously, if payWithDashUrl() failed, the giftCardId would never be set, preventing proper error tracking and recovery.
- Add BarcodeScanner utility to download and scan barcodes from URLs - Update PiggyCards to use Vision framework for multi-format barcode detection - Match Android implementation: scan barcode images instead of hardcoding CODE_128 - Fix discount display to show 1 decimal for values < 1% (e.g., 0.5%) - Apply smart formatting to both merchant details and purchase screens - Maintain whole number display for discounts >= 1% (e.g., 4%, 10%)
- Validate PiggyCards token when user clicks Buy button (not on screen load like CTX) - Show session expired dialog if token refresh fails - Automatically log user out of PiggyCards on token expiration - CTX token handling remains unchanged (still checks on screen load) - All PiggyCards code wrapped in PIGGYCARDS_ENABLED flag for build compatibility 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Document PiggyCards token expiration timing (Buy button vs screen load) - Add barcode URL parameter extraction pattern for performance - Document smart discount formatting based on value magnitude - Add Git branch tracking configuration for Xcode display - Include comprehensive examples and common pitfalls 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Addresses coderabbit review comments: 1. Fixed isLoading state management in DashSpendPayViewModel - Reset isLoading flag before early returns in PiggyCards branch - Prevents view from getting stuck in loading state when repository or sourceId is missing 2. Fixed Japanese localization formatting - Added missing spaces after asterisks in bullet points (lines 13, 15, 17) - Ensures consistent formatting across all bullet points in description.txt 3. Improved PiggyCards token expiration handling - Changed from refreshAccessToken() to refreshTokenIfNeeded() - Only refreshes token when actually expired or expiring within 5 minutes - Distinguishes between authentication failures and network errors - Only logs out on auth failures (tokenRefreshFailed, unauthorized) - Shows connection error for network issues without logging out user - More efficient: prevents unnecessary API calls on every Buy button click These changes improve user experience by: - Preventing UI from freezing during error conditions - Not logging out users due to temporary network issues - Reducing unnecessary token refresh API calls - Providing appropriate error messages for different failure types 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
…ging - Add PercentageFormatter utility for consistent discount formatting with smart decimal precision (1 decimal for <1%, whole numbers otherwise) - Fix PiggyCards token refresh deadlock by excluding auth endpoints - Fix GiftCardMetadataProvider race condition with Data object capture - Add strategic API debugging logs for gift card fetching - Remove excessive barcode scanner debug logging 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
feat: Complete PiggyCards integration with PIN authorization and provider support
feat: Gift card UI improvements and token expiration handling
- Aggregate denominations from ALL gift cards returned by PiggyCards API (Netflix, Amazon, eBay, etc. return separate cards per denomination) - Track discount per denomination instead of using highest discount - Update savingsFraction when user selects a denomination - Fix showCost to display cost message for fixed denomination cards - Update PercentageFormatter to show decimals when value has fraction - Fix confirmation dialog to use PercentageFormatter instead of intValue Fixes issues where: - Only one denomination showed (e.g., Netflix only $25 instead of $20/$25/$50) - Wrong discount displayed (highest instead of per-card discount) - Cost message not showing for fixed denomination cards - Percentages rounded incorrectly (13.5% showing as 14% or 13%) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
feature: PiggyCards fixed denomination cards and version bump to 8.5.1
When switching between testnet and mainnet, the HomeViewModel singleton was retaining cached transaction data from the previous network. This caused testnet transactions to appear on mainnet (and vice versa) until the app was restarted. Added an observer for DWCurrentNetworkDidChangeNotification that clears all cached transaction data (txItems, txByHash, crowdNodeTxSet, coinJoinTxSets) and reloads fresh data from the new network's wallet. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- MerchantDAO: Extract duplicated sourceId coercion logic into extractSourceId() helper method that handles String, Int64, Int types and nil/empty values - ExploreDatabaseConnection: Wrap FTS trigger drop/create operations in a transaction for atomicity and rollback on error - GiftCardDetailsViewModel: Lower longPollingThreshold from 40 to 27 so "code is being generated" message shows after ~40s instead of only on the final retry - HomeViewModel: Fix thread safety in clearCachedData() by dispatching cache clearing to self.queue where those properties are accessed 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Added explicit documentation that permission to commit/push applies only to the specific changes discussed, not to subsequent changes. Includes example scenario and common mistake to avoid. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
Replace hardcoded string "DWCurrentNetworkDidChangeNotification" with the bridged Objective-C constant NSNotification.Name.DWCurrentNetworkDidChange to ensure the observer actually receives the notification. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
fix: clear cached transactions when switching networks
WalkthroughThis PR integrates PiggyCards gift card purchasing alongside existing CTX support, introducing a new database column for provider tracking, barcode scanning via Vision framework, PiggyCards API endpoints and repository layer, token refresh mechanisms, and updated UI flows for provider selection, discount display, and barcode/claim-link redemption. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (UI)
participant ViewModel as DashSpendPayViewModel
participant Repo as PiggyCardsRepository
participant API as PiggyCardsAPI
participant Cache as PiggyCardsCache
participant Poll as OrderPoller
participant DB as Database
User->>ViewModel: Select PiggyCards Provider + Amount
ViewModel->>ViewModel: updateMerchantInfo(provider=PiggyCards)
ViewModel->>Repo: getBrands(country)
Repo->>API: GET /brands/{country}
API-->>Repo: Brand List
Repo->>Cache: Store Brands
Repo-->>ViewModel: Brands
ViewModel->>Repo: getGiftCards(country, sourceId)
Repo->>API: GET /giftcards/{country}?brandId=...
API-->>Repo: Gift Cards + Denominations
Repo->>Cache: Store Gift Cards
Repo-->>ViewModel: Gift Cards + Discounts per Denomination
User->>ViewModel: Confirm Purchase
ViewModel->>ViewModel: purchaseGiftCardAndPay()
ViewModel->>Repo: orderGiftCard(merchantId, amount)
Repo->>Repo: Select Best Card + Build Order
Repo->>API: POST /orders (with email, user metadata)
API-->>Repo: Order Response + orderId + payTo URI
Repo->>Repo: processOrderResponse() → parse dash: URI
Repo->>DB: Create GiftCard Entry (orderId, provider=PiggyCards)
Repo-->>ViewModel: GiftCardInfo + Payment Details
ViewModel->>ViewModel: Biometric Authentication
User->>ViewModel: Auth Success
ViewModel->>ViewModel: sendCoins(to payTo address)
ViewModel->>DB: Commit Transaction + Metadata
ViewModel->>Poll: Start Order Status Polling
Poll->>Poll: Poll Loop (up to 40 retries)
Poll->>Repo: getOrderStatus(orderId)
Repo->>API: GET /orders/{orderId}
API-->>Repo: Order Status
alt Order Complete
Repo-->>Poll: Status Complete
Poll->>Repo: Fetch Gift Card Details
Repo->>API: Extract Card + PIN from Response
Repo->>Repo: Fetch Barcode (URL or Vision scan)
Poll->>DB: Update GiftCard (claimCode, barcode)
Poll-->>User: Display Card Details + Barcode
else Still Pending
Poll-->>Poll: Wait & Retry
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
DashWallet/Sources/UI/Explore Dash/Merchants & ATMs/Details/POIDetailsViewController.swift (1)
94-94: Force unwrapping coordinates violates coding guidelines.Per coding guidelines: "Never force unwrap latitude/longitude coordinates; always use guard statements." This line force-unwraps both
latitudeandlongitude.- mapView.initialCenterLocation = .init(latitude: pointOfUse.latitude!, longitude: pointOfUse.longitude!) + guard let lat = pointOfUse.latitude, let lon = pointOfUse.longitude else { return } + mapView.initialCenterLocation = .init(latitude: lat, longitude: lon)DashWallet/Sources/Models/Explore Dash/Infrastructure/DAO Impl/MerchantDAO.swift (2)
207-216: SQL injection risk: Use parameterized queries.The
merchantIdis directly interpolated into the SQL string. WhilemerchantIdlikely comes from a trusted database source, this pattern is unsafe and inconsistent with the parameterized approach used inallLocations(line 644). As per coding guidelines, use parameterized queries consistently.#if PIGGYCARDS_ENABLED - let providersQuery = """ - SELECT provider, savingsPercentage, denominationsType, sourceId FROM gift_card_providers - WHERE merchantId = '\(merchant.merchantId)' - """ + let providersQuery = """ + SELECT provider, savingsPercentage, denominationsType, sourceId FROM gift_card_providers + WHERE merchantId = ? + """ #else - let providersQuery = """ - SELECT provider, savingsPercentage, denominationsType FROM gift_card_providers - WHERE merchantId = '\(merchant.merchantId)' AND provider = 'CTX' - """ + let providersQuery = """ + SELECT provider, savingsPercentage, denominationsType FROM gift_card_providers + WHERE merchantId = ? AND provider = 'CTX' + """ #endifThen update the prepare call:
- let rows = try db.prepare(providersQuery) + let rows = try db.prepare(providersQuery, merchant.merchantId)
439-448: Same SQL injection pattern: Apply parameterized query here too.This is the same issue as lines 207-216. Apply the same fix for consistency and security.
🧹 Nitpick comments (32)
.gitignore (1)
51-54: Consider adding .db-journal to the SQLite temporary files section.SQLite creates three types of temporary files during normal operation:
.db-shm(shared memory),.db-wal(write-ahead log), and.db-journal(transaction journal). While.db-shmand.db-walare correctly included,.db-journalis another transient file that should typically be ignored.Apply this diff to make the ignore patterns more comprehensive:
# SQLite temporary files *.db-shm *.db-wal +*.db-journalDashWallet/Sources/UI/Home/Views/HomeViewModel.swift (1)
130-155: LGTM! Excellent thread safety handling.The cache clearing logic is well-designed:
- Background data structures (
txByHash,crowdNodeTxSet,coinJoinTxSets) are cleared onself.queue, matching the queue used inreloadTxDataSource()(line 211)- UI-bound
txItemsis cleared on the main thread- Comments clearly explain the threading rationale
- Sequential dispatch order ensures proper cache clearing before reload
Optional refinement: Consider adding an emoji debug marker to the log on line 132 per coding guidelines:
- DSLogger.log("HomeViewModel: Network changed, clearing cached transaction data") + DSLogger.log("🌐 HomeViewModel: Network changed, clearing cached transaction data")As per coding guidelines, network-related logs use 🌐 for easy filtering.
DashWallet/Sources/Models/Transactions/SendCoinsService.swift (1)
64-76: Address static analysis warnings: attribute placement and unused parameter.SwiftLint flagged two issues:
@MainActorshould be on its own line for function declarationsusedBiometricsparameter is unused and should be replaced with_// Explicitly authenticate before signing to ensure PIN is requested // Must run on main thread as it's a UI operation - @MainActor func authenticate() async -> Bool { + @MainActor + func authenticate() async -> Bool { return await withCheckedContinuation { continuation in DSAuthenticationManager.sharedInstance().authenticate( withPrompt: nil, usingBiometricAuthentication: DWGlobalOptions.sharedInstance().biometricAuthEnabled, alertIfLockout: true - ) { authenticatedOrSuccess, usedBiometrics, cancelled in + ) { authenticatedOrSuccess, _, cancelled in continuation.resume(returning: authenticatedOrSuccess && !cancelled) } } }DashWallet/Sources/Models/Explore Dash/Services/DashSpend/CTX/CTXSpendRepository.swift (1)
232-233: Add network emoji marker to logging statement.The logging statement is missing the network emoji marker (🌐) recommended by the coding guidelines for easy filtering. Update line 233 to include
🌐in the log message.Note:
CTXConstants.baseURIis already correctly implemented as a computed property that dynamically evaluates the network environment on each access, satisfying the requirement to never cache the base URL as a constant.DashWallet/Sources/Models/Explore Dash/Services/DashSpend/PiggyCards/PiggyCardsAPI.swift (2)
35-47: Redundant catch clause and intermediate variable.The generic
catch { throw error }block (lines 45-46) is redundant since Swift automatically rethrows unhandled errors. Similarly, storing the result in a local variable before returning (lines 35-36) adds no value.do { try checkAccessTokenIfNeeded(for: target) - let result: R = try await super.request(target) - return result + return try await super.request(target) } catch HTTPClientError.statusCode(let r) where r.statusCode == 401 { try await handleUnauthorizedError(for: target) return try await super.request(target) } catch HTTPClientError.statusCode(let r) where r.statusCode == 400 { if target.path.contains("/verify-otp") { throw DashSpendError.invalidCode } throw HTTPClientError.statusCode(r) - } catch { - throw error }
32-48: Consider parity with CTX error handling.
CTXSpendAPIhandles additional HTTP status codes (409, 422, 500+) with domain-specific errors liketransactionRejected,invalidAmount, andserverError. This implementation only handles 400 and 401, which may result in less user-friendly error messages for other failure scenarios.Verify whether PiggyCards API returns these status codes and if so, consider adding similar handling:
} catch HTTPClientError.statusCode(let r) where r.statusCode == 400 { if target.path.contains("/verify-otp") { throw DashSpendError.invalidCode } throw HTTPClientError.statusCode(r) + } catch HTTPClientError.statusCode(let r) where r.statusCode == 409 { + throw DashSpendError.transactionRejected + } catch HTTPClientError.statusCode(let r) where r.statusCode == 422 { + throw DashSpendError.invalidAmount + } catch HTTPClientError.statusCode(let r) where r.statusCode >= 500 { + throw DashSpendError.serverError + } catch HTTPClientError.decoder { + throw DashSpendError.parsingError }DashWallet/Sources/Models/Explore Dash/Infrastructure/Database Connection/ExploreDatabaseConnection.swift (2)
157-163: UseisEmptypattern instead of comparing count to zero.Per SwiftLint, prefer checking emptiness over comparing count.
- let checkQuery = "SELECT COUNT(*) FROM merchant WHERE merchantId = '\(testMerchantId)'" - let count = try db.scalar(checkQuery) as? Int64 ?? 0 - - if count > 0 { + let checkQuery = "SELECT merchantId FROM merchant WHERE merchantId = '\(testMerchantId)' LIMIT 1" + let exists = try db.scalar(checkQuery) != nil + + if exists { print("🎯 PiggyCards test merchant already exists, skipping") return }
254-259: Remove empty line before closing brace.Per SwiftLint, avoid vertical whitespace before closing braces.
} print("✅ PiggyCards test merchant added successfully") - } catch { print("🎯 Error adding PiggyCards test merchant: \(error)") }DashWallet/Sources/Models/Explore Dash/Services/BarcodeScanner.swift (2)
100-123: URL extraction optimization is well-implemented.The fast path extracting barcode values from URL query parameters avoids unnecessary network requests and Vision framework processing. This is particularly valuable for simulator compatibility where Vision may fail.
Consider adding debug logging per coding guidelines:
static func downloadAndScan(from url: String) async -> BarcodeResult? { // First try to extract barcode value from URL query parameters (faster and more reliable) if let extractedValue = extractBarcodeFromURL(url) { + #if DEBUG + print("🔍 BarcodeScanner: Extracted value '\(extractedValue)' from URL parameter") + #endif return BarcodeResult(value: extractedValue, format: .code128) }
95-96: Remove empty line after opening brace.Per SwiftLint, avoid vertical whitespace after opening braces.
class BarcodeScanner { - /// Download barcode image from URL and scan it to extract value and formatCLAUDE.md (1)
1599-1602: Add language specifier to fenced code block.Per markdownlint, fenced code blocks should have a language specified.
The SQLite database is located at: -``` +```text ~/Library/Developer/CoreSimulator/Devices/{DEVICE_ID}/data/Containers/Data/Application/{APP_ID}/Documents/explore.db</blockquote></details> <details> <summary>DashWallet/Sources/UI/Explore Dash/Views/DashSpend/DashSpendPayScreen.swift (1)</summary><blockquote> `291-291`: **Remove empty line after opening brace.** SwiftLint flags vertical whitespace after opening braces as non-compliant with project style. Apply this diff: ```diff private func purchaseGiftCard() { - Task {DashWallet/Sources/Models/Explore Dash/Services/DashSpend/PiggyCards/PiggyCardsCache.swift (2)
34-36: Consider documenting the disabled gift cards mapping.The hardcoded disabledGiftCards dictionary lacks context for why specific cards are disabled (e.g., Xbox cards). Adding a comment explaining the business reason would improve maintainability.
Example:
// Cache for disabled gift cards (brandId -> [names]) + // Note: Xbox cards are disabled due to [business reason] private let disabledGiftCards: [Int: [String]] = [
134-134: Add trailing newline.SwiftLint requires a single trailing newline at the end of the file per project conventions.
Add a newline after line 134.
DashWallet/Sources/Models/Explore Dash/Services/DashSpend/PiggyCards/PiggyCardsTokenService.swift (2)
90-123: Well-designed actor for thread-safe token refresh.The
TokenRefreshActorcorrectly serializes refresh operations and prevents concurrent API calls. The pattern aligns with the existingCTXSpendTokenServiceimplementation.Consider using guard instead of force unwrap on line 122:
While the force unwrap is safe here (task is assigned immediately before), using
guardmakes the intent clearer and satisfies SwiftLint:- _ = try await refreshTask!.value + guard let task = refreshTask else { return } + _ = try await task.value
67-83: Consider extracting the proactive refresh threshold.The 5-minute (300 seconds) threshold is hardcoded. For maintainability:
+ private static let proactiveRefreshThreshold: TimeInterval = 300 // 5 minutes + func refreshTokenIfNeeded() async throws { if isTokenExpired { DSLogger.log("PiggyCards: Token expired, refreshing...") try await refreshAccessToken() } else { guard let expiresAt = UserDefaults.standard.object(forKey: PiggyCardsRepository.Keys.tokenExpiresAt) as? TimeInterval else { return } let expirationDate = Date(timeIntervalSince1970: expiresAt) let timeUntilExpiration = expirationDate.timeIntervalSinceNow - if timeUntilExpiration < 300 { // 5 minutes + if timeUntilExpiration < Self.proactiveRefreshThreshold { DSLogger.log("PiggyCards: Token expiring soon, proactively refreshing...") try await refreshAccessToken() } } }DashWallet/Sources/UI/Home/Tx Metadata/GiftCardMetadataProvider.swift (1)
89-117: Correct implementation with good defensive txId capture.The pattern of capturing
txIdbefore the async block prevents race conditions with theDataobject. The nested dispatch (metadataQueue → main) correctly handles thread safety.Consider using optional binding to avoid force unwraps:
- if txRowMetadata != nil { - txRowMetadata!.title = title - txRowMetadata!.secondaryIcon = .custom("image.explore.dash.wts.payment.gift-card") + if var existing = txRowMetadata { + existing.title = title + existing.secondaryIcon = .custom("image.explore.dash.wts.payment.gift-card") + txRowMetadata = existing } else {DashWallet/Sources/UI/Explore Dash/Merchants & ATMs/Details/Views/POIDetailsViewModel.swift (1)
157-191: Well-implemented provider sorting with correct business logic.The sorting prioritizes highest discount and prefers PiggyCards on ties, which aligns with the PR's goal. The separation of building
providerList, sorting, then populating bothsupportedProvidersandsortedProvidersis clean.Consider extracting the tuple to a local struct (SwiftLint: large_tuple):
This is optional but improves readability:
+ private struct ProviderEntry { + let provider: GiftCardProvider + let isFixed: Bool + let discount: Int + } + private func setupProviders() { guard case .merchant(let m) = merchant.category, m.paymentMethod == .giftCard else { return } - var providerList: [(provider: GiftCardProvider, isFixed: Bool, discount: Int)] = [] + var providerList: [ProviderEntry] = [] for providerInfo in m.giftCardProviders { guard let provider = providerInfo.provider else { continue } let isFixed = providerInfo.denominationsType == DenominationType.Fixed.rawValue let discount = providerInfo.savingsPercentage - providerList.append((provider: provider, isFixed: isFixed, discount: discount)) + providerList.append(ProviderEntry(provider: provider, isFixed: isFixed, discount: discount)) }DashWallet/Sources/UI/Explore Dash/Views/DashSpend/GiftCardDetailsViewModel.swift (4)
34-36: Minor: Redundant optional initialization.Per SwiftLint, initializing an optional with
nilis redundant.var isClaimLink: Bool = false var hasBeenPollingForLongTime: Bool = false - var provider: String? = nil + var provider: String? }
186-190: Prefer!= niloverlet _ =for clarity.SwiftLint flags this unused binding pattern. Using
!= nilis more idiomatic when you only need to check for presence.- guard let giftCard = await giftCardsDAO.get(byTxId: txId), - let _ = giftCard.note else { + guard let giftCard = await giftCardsDAO.get(byTxId: txId), + giftCard.note != nil else { stopTicker() return }
213-219: Same pattern: prefer!= niloverlet _ =.Apply the same fix for consistency with the PiggyCards path.
private func fetchCTXGiftCardInfo() async { guard let giftCard = await giftCardsDAO.get(byTxId: txId), - let _ = giftCard.note, + giftCard.note != nil, ctxSpendRepository.isUserSignedIn else { stopTicker() return }
274-364: Consider extracting helper to reduce cyclomatic complexity.SwiftLint reports complexity of 11 (threshold is 10). The nested switch cases and if/else branches contribute to this. Consider extracting the card processing logic (lines 288-341) into a separate helper method like
processCompletedPiggyCardsOrder(cards:).DashWallet/Sources/UI/Explore Dash/Merchants & ATMs/Details/POIDetailsViewController.swift (1)
329-356: Consider using POIDetailsViewModel.logout for consistency.The relevant snippet shows
POIDetailsViewModel.logout(provider:)exists for logging out providers. Using the ViewModel's method would maintain consistency with the abstraction layer rather than directly callingPiggyCardsRepository.shared.logout().However, the error handling logic (logout on auth failures, no logout on network errors) is correctly implemented.
DashWallet/Sources/Utils/PercentageFormatter.swift (2)
29-43: Simplify decimal precision logic.The
hasDecimalcheck on lines 34-35 appears redundant. The first condition(percent * 10).truncatingRemainder(dividingBy: 1) != 0checks for sub-tenths precision (e.g., 0.55 → 5.5 has remainder). However, line 38 still re-checkspercent.truncatingRemainder(dividingBy: 1) != 0, making thehasDecimalvariable largely unnecessary.A simpler approach:
static func format(percent: Double, includeSign: Bool = false, includePercent: Bool = true) -> String { let sign = includeSign ? "-" : "" let percentSymbol = includePercent ? "%%" : "" - // Check if the value has a meaningful decimal component - let hasDecimal = (percent * 10).truncatingRemainder(dividingBy: 1) != 0 || - percent.truncatingRemainder(dividingBy: 1) != 0 - // Use 1 decimal place if there's a fractional part, otherwise use whole numbers - if hasDecimal && percent.truncatingRemainder(dividingBy: 1) != 0 { + if percent.truncatingRemainder(dividingBy: 1) != 0 { return String(format: "\(sign)%.1f\(percentSymbol)", percent) } else { return String(format: "\(sign)%.0f\(percentSymbol)", percent) } }
21-22: Remove empty line after opening brace.SwiftLint flags vertical whitespace after opening braces.
enum PercentageFormatter { - /// Format a percentage value with appropriate decimal placesDashWallet/Sources/Models/Explore Dash/Infrastructure/DAO Impl/MerchantDAO.swift (1)
224-227: Remove unusedrowCountvariables.The
rowCountvariable is incremented in each loop iteration but never read. This appears to be debug artifact that should be removed.- var rowCount = 0 - for row in rows { - rowCount += 1 if let providerId = row[0] as? String,Also applies to: 457-460, 646-649
DashWallet/Sources/UI/Explore Dash/Views/DashSpend/GiftCardDetailsView.swift (2)
150-168: SwiftLint: Use explicitaction:label for Button.When using multiple closures, prefer explicit labels over trailing closure syntax for clarity.
- Button(action: { + Button( + action: { if let url = URL(string: claimLink) { UIApplication.shared.open(url) } - }) { + }, + label: { Text(NSLocalizedString("View your gift card details", comment: "DashSpend")) .font(.subheadline) .fontWeight(.medium) .foregroundColor(.dashBlue) .frame(maxWidth: .infinity, alignment: .center) } + )
399-403: DateFormatter as computed property is recreated on every access.Creating a
DateFormatteris expensive. Since this is a computed property, a new instance is created each timedateFormatteris accessed. Consider making it a static constant or a stored property.- private var dateFormatter: DateFormatter { - let formatter = DateFormatter() - formatter.dateFormat = "MMMM dd, yyyy 'at' h:mm a" - return formatter - } + private static let dateFormatter: DateFormatter = { + let formatter = DateFormatter() + formatter.dateFormat = "MMMM dd, yyyy 'at' h:mm a" + return formatter + }()Then update the usage on line 76:
- Text(date, formatter: dateFormatter) + Text(date, formatter: Self.dateFormatter)DashWallet/Sources/Models/Explore Dash/Model/Entites/PiggyCardsModels.swift (2)
129-133: Remove redundant string enum value.The explicit string value
"userId"matches the case name and is unnecessary.enum CodingKeys: String, CodingKey { - case userId = "userId" + case userId }
325-329: Remove redundant string enum values.All three explicit string values match their case names and are unnecessary per SwiftLint's
redundant_string_enum_valuerule.enum PiggyCardsPriceType: String { - case fixed = "fixed" - case range = "range" - case option = "option" + case fixed + case range + case option }DashWallet/Sources/UI/Explore Dash/Views/DashSpend/DashSpendPayViewModel.swift (1)
355-501: RedundantMainActor.runblocks in@MainActorclass.The class is already decorated with
@MainActor, so wrapping code inawait MainActor.run { }is unnecessary and adds overhead.- await MainActor.run { - self.isLoading = true - } + self.isLoading = trueApply similar changes to lines 388-391, 395-398, 493-497, and 499-501.
DashWallet/Sources/Models/Explore Dash/Services/DashSpend/PiggyCards/PiggyCardsRepository.swift (1)
186-266: Consider extracting helper methods to reduce complexity.SwiftLint reports cyclomatic complexity of 13 (threshold: 10). The method is logically structured but could benefit from extracting steps into private helpers.
Consider extracting validation and request building into separate methods:
private func validateAndSelectGiftCard(merchantId: String, fiatAmount: Double) throws -> PiggyCardsGiftcard { guard let giftCards = PiggyCardsCache.shared.getGiftCards(forMerchant: merchantId) else { throw DashSpendError.invalidMerchant } guard let selectedCard = PiggyCardsCache.shared.selectGiftCard(from: giftCards, forAmount: fiatAmount) else { throw DashSpendError.invalidAmount } return selectedCard } private func buildOrderRequest(selectedCard: PiggyCardsGiftcard, fiatAmount: Double, fiatCurrency: String) throws -> PiggyCardsOrderRequest { guard let email = userEmail else { throw DashSpendError.unauthorized } // ... rest of order building }
...urces/Models/Explore Dash/Infrastructure/Database Connection/ExploreDatabaseConnection.swift
Show resolved
Hide resolved
...urces/Models/Explore Dash/Infrastructure/Database Connection/ExploreDatabaseConnection.swift
Show resolved
Hide resolved
DashWallet/Sources/Models/Explore Dash/Services/DashSpend/PiggyCards/PiggyCardsRepository.swift
Show resolved
Hide resolved
DashWallet/Sources/Models/Explore Dash/Services/DashSpend/PiggyCards/PiggyCardsRepository.swift
Show resolved
Hide resolved
DashWallet/Sources/Models/Explore Dash/Services/DashSpend/PiggyCards/PiggyCardsRepository.swift
Show resolved
Hide resolved
DashWallet/Sources/Models/Explore Dash/Services/DashSpend/PiggyCards/PiggyCardsRepository.swift
Show resolved
Hide resolved
DashWallet/Sources/UI/Explore Dash/Merchants & ATMs/List/Cells/MerchantItemCell.swift
Show resolved
Hide resolved
DashWallet/Sources/UI/Explore Dash/Views/DashSpend/DashSpendPayViewModel.swift
Show resolved
Hide resolved
DashWallet/Sources/UI/Explore Dash/Views/DashSpend/GiftCardDetailsViewModel.swift
Show resolved
Hide resolved
HashEngineering
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Coderabbit has some feedback. These could be fixed in a separate PR.
Some of the changes need to be tested, especially the CODE_128 and CODE128.
Issue being fixed or feature implemented
What was done?
How Has This Been Tested?
Breaking Changes
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
Release Notes
New Features
Improvements
✏️ Tip: You can customize this high-level summary in your review settings.