Skip to content

Conversation

@ben-kaufman
Copy link
Contributor

Show CPFP as boost fee

Screen.Recording.2025-11-27.at.12.33.35.PM.mov

@claude
Copy link

claude bot commented Nov 27, 2025

PR Review: Show CPFP txs as boost fee

Thank you for this PR! This is a good UX improvement to display CPFP child transactions as "boost fees" rather than separate transactions. Here's my detailed review:


Strengths

  1. Clear UX improvement: Showing CPFP child transactions as boost fees makes the interface cleaner and more intuitive for users.
  2. Consistent with existing patterns: The implementation follows the existing boost transaction handling (RBF vs CPFP detection).
  3. Proper use of SwiftUI .task modifier: Async operations correctly use .task for lifecycle management.
  4. Good localization: New strings properly added to localization files.

🐛 Potential Issues

1. Performance: Repeated async calls in list views

Location: ActivityRowOnchain.swift:100

.task {
    isCpfpChild = await CoreService.shared.activity.isCpfpChildTransaction(txId: item.txId)
}

Issue: This .task runs for every row in the activity list. If you have 100 transactions, this triggers 100 async calls to check CPFP status. Each call:

  • Awaits getTxIdsInBoostTxIds() (which may refresh cache if empty)
  • Fetches activity data via getOnchainActivityByTxId()

Impact: Potential performance degradation on lists with many items, especially on first load when cache is empty.

Recommendation:

  • Compute isCpfpChild status at the ActivityListViewModel level and pass it down as a property
  • OR cache the result in the activity model itself when fetching activities
  • OR batch check all txIds at once in the parent view

2. Logic error in isCpfpChildTransaction

Location: CoreService.swift:140-147

func isCpfpChildTransaction(txId: String) async -> Bool {
    guard await getTxIdsInBoostTxIds().contains(txId),
          let activity = try? await getOnchainActivityByTxId(txid: txId)
    else {
        return false
    }
    return activity.doesExist
}

Issue: This logic appears correct for identifying CPFP child transactions (txId exists in boostTxIds set AND doesExist is true). However, there's a subtle consideration:

According to the existing comment at CoreService.swift:128-129:

"RBF transactions have doesExist = false (replaced), CPFP transactions have doesExist = true (child transactions)"

This assumes that boostTxIds contains either:

  • RBF: replaced transactions (doesExist=false)
  • CPFP: child transactions (doesExist=true)

But CPFP semantics are inverted: In CPFP, the boostTxIds field on the parent transaction should contain the child transaction ID. The current implementation checks if the current transaction is in someone else's boostTxIds, which is correct for identifying "I am a child of a parent transaction."

Verification needed: Confirm that BitkitCore populates boostTxIds correctly for CPFP:

  • Does the parent transaction store child txId in its boostTxIds?
  • Does the child transaction get marked as boosted somewhere?

3. Missing test coverage

Location: No tests added

Issue: The PR adds new logic but no tests. The existing TxBumpingTests.swift has a testAccelerateByCpfp() test, but it doesn't verify that the CPFP child transaction displays as "Boost Fee" in the UI.

Recommendation: Add a test case that:

  1. Creates a CPFP child transaction
  2. Verifies isCpfpChildTransaction() returns true for the child
  3. Verifies the UI displays "Boost Fee" instead of "Received"

🔍 Code Quality Observations

4. UI text truncation handling

Location: ActivityRowOnchain.swift:89-92

.lineLimit(1)
CaptionBText(description)
    .lineLimit(1)
}
.fixedSize(horizontal: false, vertical: true)

Good: The addition of .lineLimit(1) prevents text overflow. However, .fixedSize(horizontal: false, vertical: true) with .lineLimit(1) seems contradictory—.fixedSize requests the view's ideal size, but .lineLimit(1) restricts it.

Suggestion: Consider if .fixedSize is needed here, or if .lineLimit(1) alone suffices.


5. Disabled boost button for CPFP children

Location: ActivityItemView.swift:122-123

if isCpfpChild {
    return true
}

Good: CPFP child transactions correctly disable the boost button (you can't boost a boost).


🔒 Security & Best Practices

✅ No security concerns identified
✅ Follows SwiftUI best practices (uses .task instead of .onAppear)
✅ Proper error handling with try? for non-critical failures
✅ Follows project architecture (business logic in service layer)


📋 Style & Conventions

✅ Follows project conventions from CLAUDE.md
✅ Uses descriptive variable names (isCpfpChild)
✅ Consistent with existing boost transaction handling patterns


🎯 Recommendations

High Priority:

  1. Fix performance issue: Move CPFP detection to list-level computation instead of per-row async calls
  2. Verify BitkitCore semantics: Confirm boostTxIds population logic for CPFP matches expectations
  3. Add test coverage: Add tests verifying CPFP child transactions display correctly

Medium Priority:
4. Review .fixedSize usage with .lineLimit(1) for UI consistency

Low Priority:
5. Consider adding a code comment explaining the CPFP detection logic in isCpfpChildTransaction


📝 Minor: Typo in PR Title

PR title says "CPFC" but should be "CPFP" (Child-Pays-For-Parent) 😊


Overall, this is a solid improvement that enhances UX. The main concern is the performance impact of per-row async calls in the activity list. Please address the performance issue before merging.

Let me know if you'd like me to suggest specific code changes for the performance optimization!

@ovitrif ovitrif changed the title Show CPFC txs as boost fee Show CPFCPtxs as boost fee Nov 27, 2025
@ovitrif ovitrif changed the title Show CPFCPtxs as boost fee Show CPFP txs as boost fee Nov 27, 2025
Copy link
Collaborator

@ovitrif ovitrif left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

tAck 🚀

@ovitrif ovitrif merged commit 4b9083d into feat/onchain-events Nov 27, 2025
11 of 15 checks passed
@ovitrif ovitrif deleted the feat/cpfp-boost-fee-ui branch November 27, 2025 22:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants