fix(coordinator): track pending transactions without change outputs u…#471
fix(coordinator): track pending transactions without change outputs u…#471RIYAKUMARI001 wants to merge 4 commits intocaravan-bitcoin:mainfrom
Conversation
…sing history scanning
🦋 Changeset detectedLatest commit: 84b9b74 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
@RIYAKUMARI001 Sorry for the delay for getting back on this PR , the initial testing looks really good I was able to on a private node send a 1 to 1 tx and I was able to see that in my pending tx tab :) |
| [...transactionKeys.all, txid, "withHex"] as const, | ||
| coins: (txid: string) => [...transactionKeys.all, txid, "coins"] as const, | ||
| confirmedHistory: () => [...transactionKeys.all, "confirmed"] as const, | ||
| confirmedHistory: () => |
There was a problem hiding this comment.
Hi @RIYAKUMARI001, thanks for the PR! I had a few comments while going through the changes.
Instead of introducing new hooks in transactions.ts (which would duplicate the public/private client logic we already have in txHistory.ts), I think we could extend the existing hooks — usePublicClientTransactions and usePrivateClientTransactions — by adding a filter parameter. We could default this to "confirmed" to preserve the current behavior.
With that in place, usePendingTransactions could simply call these hooks with "pending", which avoids creating a separate hook tree. Since selectProcessedTransactions already supports filtering, the integration should be fairly straightforward.
One thing that might be worth discussing with @bucko13 , though: if we reuse the same hooks for both confirmed history and pending transactions, they may end up having different polling requirements. Pending transactions likely need a faster refetchInterval than confirmed ones. So we might want to consider whether that should be configurable via options passed to the hooks, or handled in another way.
Not a blocker, but probably good to align on the approach before we move forward.
| all: ["transactions"] as const, | ||
| tx: (txid: string) => [...transactionKeys.all, txid] as const, | ||
| pending: () => [...transactionKeys.all, "pending"] as const, | ||
| pendingHistory: () => [...transactionKeys.all, "pending", "history"] as const, |
There was a problem hiding this comment.
so you can remove this too
There was a problem hiding this comment.
Done. Removed the unnecessary pendingHistory
|
|
||
| // Hook for fetching pending transaction IDs and their details | ||
| // Service function for fetching pending transaction fees | ||
| const fetchPendingTransactionFee = async ( |
There was a problem hiding this comment.
yes these can be removed as you did
|
|
||
| // Basic hook for raw pending transactions (no processing) | ||
| export const useRawPendingTransactions = () => { | ||
| export const usePublicClientPendingTransactions = () => { |
There was a problem hiding this comment.
no need to add new hook
There was a problem hiding this comment.
Understood. Removed the redundant hook and switched to using the existing ones with a filter.
| error, | ||
| refetch: () => { | ||
| transactionQueries.forEach((query) => query.refetch()); | ||
| export const usePrivateClientPendingTransactions = () => { |
There was a problem hiding this comment.
same no need to add new hook
There was a problem hiding this comment.
Removed this as well. Now utilizing the consolidated base hooks
| const walletAddresses = useSelector(getWalletAddresses); | ||
| const rawPendingQuery = useRawPendingTransactions(); | ||
| const clientType = useSelector((state: WalletState) => state.client.type); | ||
| const privateQuery = usePrivateClientPendingTransactions(); |
There was a problem hiding this comment.
see instead in txHistory.ts we already have these hooks so add a filter parameter to the existing hooks in txHistory.ts:
like eg :
export const usePublicClientTransactions = (
filter: "confirmed" | "pending" | "all" = "confirmed"
) => {and then here in transcation.ts file
export const usePendingTransactions = () => {
const clientType = useSelector((state: WalletState) => state.client.type);
const privateQuery = usePrivateClientTransactions("pending");
const publicQuery = usePublicClientTransactions("pending");
const query = clientType === "private" ? privateQuery : publicQuery;
return {
transactions: query.data || [],
isLoading: query.isLoading,
error: query.error,
refetch: query.refetch,
};
};and delete the rest unnecessary and duplicate code from transactions.ts , let's follow the DRY ( do not repeat ) principle here :)
There was a problem hiding this comment.
Done! I've moved the logic to txHistory.ts and refactored it to use the existing client hooks with a filter parameter as suggested. The duplicate code has been removed
| transactions: WalletTransactionDetails[], | ||
| walletAddresses: string[], | ||
| filter: "all" | "confirmed" | "unconfirmed" = "all", | ||
| filter: "all" | "confirmed" | "unconfirmed" | "pending" = "all", |
There was a problem hiding this comment.
why add a new status , we can add use "unconfirmed" itself , also in above too
There was a problem hiding this comment.
I've reverted to using the existing 'unconfirmed '
| import { useGetClient } from "hooks/client"; | ||
| import { bitcoinsToSatoshis } from "@caravan/bitcoin"; | ||
|
|
||
| const MAX_TRANSACTIONS_TO_FETCH = 500; |
|
Hi @RIYAKUMARI001 gentle ping to check if you had the chance to work on the reviews :) |
Hi! Yes, I saw the requested changes. I’m planning to start working on them now. My exams were going on, and tomorrow is my last exam. After that, I’ll push the changes as soon as possible. |
Hi @Legend101Zz , thank you for your patience. I’ve gone through all your comments and added a changeset. Please take a look when you can. I'm ready for another review. Thanks.
|
Thank you for the detailed feedback. I changed the hooks in txHistory.ts to use a filter parameter and followed the DRY principle as you suggested. I also made the refetchInterval faster, now set at 30 seconds, for unconfirmed transactions to keep the UI updated. |




What kind of change does this PR introduce?
Bugfix
Issue Number:
Fixes #407
Snapshots:
Summary
This PR resolves a critical bug where transactions without change outputs (e.g., "MAX" sends, consolidations, or 1:1 transactions) would disappear from the Caravan "Pending" transactions view immediately after broadcast.
The Fix:
Previously, the pending transaction logic relied primarily on detecting new "unconfirmed" outputs in the wallet's change/deposit nodes. For transactions that spent the entire balance to a single external or internal address without a change output, this detection would fail.
This PR refactors the transaction hooks to use historical address scanning for pending transactions.
Updated
selectProcessedTransactions
to support a explicit "pending" filter.
Implemented usePublicClientPendingTransactions and usePrivateClientPendingTransactions that fetch the mempool history of all relevant wallet addresses.
This ensures that any transaction involving the wallet's addresses is tracked in the Pending tab, regardless of whether a change output exists.
Motivation: Users performing consolidation or spending their full balance were left with no visual feedback in Caravan that their transaction was successfully broadcast and pending. This fix provides a reliable and consistent UI state.
Does this PR introduce a breaking change?
No.
Checklist
I have tested my changes thoroughly.
I have added or updated tests to cover my changes (if applicable).
I have verified that test coverage meets or exceeds 95% (if applicable).
I have run the test suite locally, and although some legacy environment issues exist, my changes do not introduce new regressions.
I have written tests for all new changes/features
I have followed the project's coding style and conventions (formatted with Prettier).
I have created a changeset to document my changes (npm run changeset)
Other information Successfully verified on Bitcoin Testnet using a 2-of-2 multisig wallet integrated with Sparrow. A "MAX" send (consolidation) was performed, and the transaction was correctly tracked in Caravan's "Pending" tab until it reached its first confirmation.
Have you read the contributing guide?
Yes
For information on creating and using changesets, please refer to our documentation on changesets.