Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix/btc handle replaced txs #9610

Open
wants to merge 15 commits into
base: develop
Choose a base branch
from
Open

Fix/btc handle replaced txs #9610

wants to merge 15 commits into from
+583 −11

Conversation

Wozacosta
Copy link
Contributor

@Wozacosta Wozacosta commented Mar 20, 2025 •
edited
Loading

✅ Checklist

  • npx changeset was attached.
  • Covered by automatic tests.
  • Impact of the changes:
    • ...

📝 Description

When syncing Bitcoin accounts, it's possible to fetch multiple versions of the same transaction due to Replace-By-Fee (RBF). This can lead to:

Duplicate or conflicting operations in the UI
Displaying transactions that were never confirmed and already replaced

image

✅ What this PR does

  • Introduces the removeReplaced function to filter out replaced/unconfirmed Bitcoin transactions
  • Ensures only the most relevant version of each transaction is kept
    Handles the following cases:
  • Keeps confirmed transactions over unconfirmed ones
  • Keeps the latest confirmed transaction (based on block height, then date)
  • Keeps the newest unconfirmed transaction if none are confirmed
  • Coinbase transactions are always preserved
  • Transactions without inputs (e.g. OUT txs) are not affected
  • Preserves the original operation order in the final result to avoid UI diff noise

This PR also adds unit tests to cover removeReplaced

Before/After

prbtcrbf.mov

❓ Context

  • JIRA or GitHub link: LIVE-16927

🧐 Checklist for the PR Reviewers

  • The code aligns with the requirements described in the linked JIRA or GitHub issue.
  • The PR description clearly documents the changes made and explains any technical trade-offs or design decisions.
  • There are no undocumented trade-offs, technical debt, or maintainability issues.
  • The PR has been tested thoroughly, and any potential edge cases have been considered and handled.
  • Any new dependencies have been justified and documented.
  • Performance considerations have been taken into account. (changes have been profiled or benchmarked if necessary)

@vercel Vercel
Copy link

vercel bot commented Mar 20, 2025 •
edited
Loading

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
web-tools ✅ Ready (Inspect) Visit Preview 💬 Add feedback Mar 21, 2025 9:27am
3 Skipped Deployments
Name Status Preview Comments Updated (UTC)
ledger-live-github-bot ⬜️ Ignored (Inspect) Visit Preview Mar 21, 2025 9:27am
native-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Mar 21, 2025 9:27am
react-ui-storybook ⬜️ Ignored (Inspect) Visit Preview Mar 21, 2025 9:27am

@live-github-bot live-github-bot bot added desktop Has changes in LLD common Has changes in live-common ledgerjs Has changes in the ledgerjs open source libs labels Mar 20, 2025
@live-github-bot live-github-bot bot removed the common Has changes in live-common label Mar 20, 2025
@Wozacosta
btc regtest
a066b27 
wip btc debug

maxspendable fix

remove replaced transactions

remove markAsReplaced

cleanup

cleanup #2

cleanup #3

cleanup #4

cleanup #7

cleanup #8

cleanup #9

cleanup #11

cleanup #12

unit test + cleanup

cleanup #13

cleanup #14
@Wozacosta Wozacosta force-pushed the fix/btc-handle-replaced-txs branch from 6c09957 to a066b27 Compare March 20, 2025 18:09
@live-github-bot live-github-bot bot added common Has changes in live-common and removed ledgerjs Has changes in the ledgerjs open source libs labels Mar 20, 2025
@live-github-bot live-github-bot bot removed the common Has changes in live-common label Mar 20, 2025
@Wozacosta Wozacosta marked this pull request as ready for review March 21, 2025 09:19
@Wozacosta Wozacosta requested a review from a team as a code owner March 21, 2025 09:19
palra
palra reviewed Mar 21, 2025
View reviewed changes
apps/ledger-live-desktop/src/renderer/families/bitcoin/CoinControlRow.tsx
@@ -138,7 +139,7 @@ export const CoinControlRow = ({
/>
{utxo.blockHeight ? (
<Text ff="Inter|Medium" fontSize={3} color={"palette.text.shade50"}>
{account.blockHeight - utxo.blockHeight + " confirmations"}
{confirmations + ` confirmation${confirmations > 1 ? "s" : ""}`}
Copy link

Choose a reason for hiding this comment

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

Is there an intl library in use handling pluralization in multiple locales?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There's https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Intl/PluralRules (not used anywhere else in the codebase but still is polyfilled on LLM side with@formatjs/intl-pluralrules) but ideally this thing would move on to be fully internationalized in another PR.

@sonarqubecloud SonarQubeCloud
Copy link

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)
B Maintainability Rating on New Code (required ≥ A)

See analysis details on SonarQube Cloud

Catch issues before they fail your Quality Gate with our IDE extension SonarQube for IDE

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@palra palra palra left review comments

At least 1 approving review is required to merge this pull request.

Assignees

@Wozacosta Wozacosta

Labels
desktop Has changes in LLD
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Wozacosta @palra