Conversation
There was a problem hiding this comment.
Pull request overview
Fixes the “UTXOs stuck in PendingSpent” scenario by introducing a rollback mechanism when a transaction broadcast is rejected, helping ensure wallet state doesn’t permanently lock spendable outputs.
Changes:
- Added
ResetPendingSpentForTransactionto clearPendingSpenton UTXOs used as inputs to a given transaction. - Extended
PublishTransactionAsyncto optionally acceptAccountInfoand rollbackPendingSpentwhen the indexer rejects the transaction. - Refactored
CopyPendingSpentUtxosto more efficiently carry forwardPendingSpentflags and to explicitly drop them when the outpoint is no longer present.
|
|
||
| public async Task<OperationResult<Transaction>> PublishTransactionAsync(Network network,Transaction signedTransaction) | ||
| public async Task<OperationResult<Transaction>> PublishTransactionAsync( | ||
| Network network, |
There was a problem hiding this comment.
PublishTransactionAsync now has an extra AccountInfo? accountInfo = null parameter, but WalletOperations implements IWalletOperations, whose interface method still takes only (Network, Transaction). As-is, this will no longer satisfy the interface contract and will break callers compiled against the interface. Consider either updating IWalletOperations.PublishTransactionAsync to include the optional parameter (and updating all call sites), or adding an overload with the original 2-parameter signature that delegates to the new implementation.
| Network network, | |
| Network network, | |
| Transaction signedTransaction) | |
| { | |
| return await PublishTransactionAsync(network, signedTransaction, null); | |
| } | |
| public async Task<OperationResult<Transaction>> PublishTransactionAsync( | |
| Network network, |
| if (accountInfo != null) | ||
| { | ||
| _logger.LogWarning( | ||
| "Transaction {TxId} rejected by indexer ({Reason}). Rolling back PendingSpent flags.", | ||
| signedTransaction.GetHash(), res); | ||
|
|
||
| ResetPendingSpentForTransaction(accountInfo, signedTransaction); | ||
| } |
There was a problem hiding this comment.
The rollback logic only runs when accountInfo is provided, but existing internal usage (e.g., SendAmountToAddress calling PublishTransactionAsync(network, signedTransaction)) will pass null and therefore never clears PendingSpent on rejection. If this path is expected to be covered by the fix, consider threading AccountInfo through the calling API (or ensuring the relevant caller passes it) so the rollback can actually occur.
| public void ResetPendingSpentForTransaction(AccountInfo accountInfo, Transaction transaction) | ||
| { | ||
| var inputOutpoints = transaction.Inputs | ||
| .Select(i => i.PrevOut.ToString()) | ||
| .ToHashSet(); | ||
|
|
||
| foreach (var utxo in accountInfo.AllUtxos()) | ||
| { | ||
| if (inputOutpoints.Contains(utxo.outpoint.ToString())) | ||
| { | ||
| utxo.PendingSpent = false; | ||
| _logger.LogInformation( | ||
| "Released PendingSpent on {Outpoint} after transaction rollback.", utxo.outpoint); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
ResetPendingSpentForTransaction is currently public but is only used internally within WalletOperations. If this is meant to be called externally (e.g., from UI/state management), it should be added to IWalletOperations; otherwise consider reducing its visibility (e.g., private) to avoid expanding the public surface unintentionally.
| if (accountInfo != null) | ||
| { | ||
| _logger.LogWarning( | ||
| "Transaction {TxId} rejected by indexer ({Reason}). Rolling back PendingSpent flags.", | ||
| signedTransaction.GetHash(), res); | ||
|
|
||
| ResetPendingSpentForTransaction(accountInfo, signedTransaction); | ||
| } |
There was a problem hiding this comment.
There’s no test coverage validating that a rejected publish triggers PendingSpent rollback (and that only the transaction’s input UTXOs are reset). Since Angor.Shared.Tests already has WalletOperationsTest, consider adding a unit test that mocks IIndexerService.PublishTransactionAsync to return an error string and asserts PendingSpent is cleared for the matching outpoints when accountInfo is supplied.
There was a problem hiding this comment.
@dangershony I didn't undetstand this particular review
|
some tests are failing |
Is this correct copilot give me this |
|
I think it is ready for review |
| liveUtxo.PendingSpent = true; | ||
| _logger.LogInformation( | ||
| "{Address} carrying PendingSpent forward for in-flight utxo {Outpoint}.", |
| @@ -367,6 +385,15 @@ public async Task<OperationResult<Transaction>> PublishTransactionAsync(Network | |||
| if (string.IsNullOrEmpty(res)) | |||
| return new OperationResult<Transaction> { Success = true, Data = signedTransaction }; | |||
|
|
|||
| if (accountInfo != null) | |||
| { | |||
| _logger.LogWarning( | |||
| "Transaction {TxId} rejected by indexer ({Reason}). Rolling back PendingSpent flags.", | |||
| signedTransaction.GetHash(), res); | |||
|
|
|||
| ResetPendingSpentForTransaction(accountInfo, signedTransaction); | |||
| } | |||
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
@copilot apply changes based on the comments in this thread |
|
this is not urgent so i will look in to this pr later |
|
No worries I am just trying Copilot Ai for review |
Fixes #738
adding a function
ResetPendingSpentForTransactionto clear flag for rejected/failed tx