feat: add UTXO release mechanism for nano contract transactions#1050
Open
raul-oliveira wants to merge 10 commits intomasterfrom
Open
feat: add UTXO release mechanism for nano contract transactions#1050raul-oliveira wants to merge 10 commits intomasterfrom
raul-oliveira wants to merge 10 commits intomasterfrom
Conversation
- Add TTL (60s) to markUtxoSelected calls in NanoContractTransactionBuilder
as safety net against permanent UTXO locks
- Add releaseUtxos() to ISendTransaction interface and SendTransaction class
for explicit UTXO cleanup on rejection/abandonment
- Add no-op releaseUtxos() to SendTransactionWalletService
- Add unit tests for releaseUtxos (happy path, null tx, null storage, mid-loop
error)
- Add integration test validating lock/unlock lifecycle: prepare -> fail ->
release -> succeed
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdded a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SendTx as SendTransaction
participant Storage
participant Wallet as SendTransactionWalletService
Client->>SendTx: prepareTx(inputs, outputs)
SendTx->>Storage: utxoSelectAsInput({txId,index}, true, SELECT_OUTPUTS_TIMEOUT)
Storage-->>SendTx: locked / ack
SendTx-->>Client: transaction prepared
Client->>SendTx: prepareTx() with same inputs
SendTx->>Storage: utxoSelectAsInput({txId,index}, true, SELECT_OUTPUTS_TIMEOUT)
Storage-->>SendTx: error (already locked)
SendTx-->>Client: NanoContractTransactionError
Client->>SendTx: releaseUtxos()
loop per input
SendTx->>Storage: utxoSelectAsInput({txId,index}, false, SELECT_OUTPUTS_TIMEOUT)
opt success
Storage-->>SendTx: unlocked
end
opt failure
Storage-->>SendTx: error (logged, continue)
end
end
SendTx-->>Client: releaseUtxos() complete
Client->>SendTx: prepareTx() again
SendTx->>Storage: utxoSelectAsInput({txId,index}, true, SELECT_OUTPUTS_TIMEOUT)
Storage-->>SendTx: locked / ack
SendTx-->>Client: transaction prepared
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1050 +/- ##
==========================================
+ Coverage 87.93% 87.94% +0.01%
==========================================
Files 114 114
Lines 8910 8918 +8
Branches 2030 2022 -8
==========================================
+ Hits 7835 7843 +8
Misses 1047 1047
Partials 28 28 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/new/sendTransaction.test.ts`:
- Line 8: The import line including TOKEN_AUTHORITY_MASK, NATIVE_TOKEN_UID, and
SELECT_OUTPUTS_TIMEOUT in the test file has Prettier formatting violations; run
a formatter (e.g., run prettier --write on
__tests__/new/sendTransaction.test.ts) or adjust the import to match project
style (single quotes/double quotes, spacing, trailing semicolons) so the linter
stops flagging the file and the import statement conforms to the repository's
Prettier rules.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e17c0e97-feff-46a0-9bf9-5ee99b5d2988
📒 Files selected for processing (6)
__tests__/integration/nano_utxo_lock_unlock.test.ts__tests__/new/sendTransaction.test.tssrc/nano_contracts/builder.tssrc/new/sendTransaction.tssrc/wallet/sendTransactionWalletService.tssrc/wallet/types.ts
raul-oliveira
moved this from In Progress (Done)
to In Progress (WIP)
in Hathor Network
3 tasks
tuliomir
reviewed
Apr 6, 2026
| */ | ||
| // eslint-disable-next-line class-methods-use-this | ||
| async releaseUtxos(): Promise<void> { | ||
| // No-op: wallet-service manages UTXO state server-side |
Contributor
There was a problem hiding this comment.
question: Is there a plan to implement this feature for the Wallet Service too? An API call to release the stuck UTXOs?
Contributor
Author
There was a problem hiding this comment.
Probably not, because the way wallet service locks the utxos is by binding them to a tx proposal. In this use case that we are dealing, we never create a tx proposal before the user approves
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@__tests__/integration/fullnode-specific/nano_utxo_lock_unlock.test.ts`:
- Line 18: The test asserts failure by the NanoContractTransactionError type but
the second prepare-lock path throws a generic Error from the send-transaction
flow, so update the assertions in
__tests__/integration/fullnode-specific/nano_utxo_lock_unlock.test.ts to assert
the error message content rather than the error class: find the two expectations
using toThrow(NanoContractTransactionError) (the second prepare-lock path around
lines ~102-120 and the earlier occurrence) and replace them with assertions that
capture the thrown error and assert on its message (e.g., expect(() =>
...).toThrowErrorMatching(/your expected substring/) or use try/catch and
expect(err.message).toMatch(/expected message/)), matching the actual error
string emitted by the send-transaction flow (e.g., the "already locked" or
prepare-lock failure text).
- Line 29: Replace explicit nulls used as "not provided" with omitted arguments
(or undefined) to follow the TypeScript convention: update calls to
waitTxConfirmed(wallet, txId, null) to simply waitTxConfirmed(wallet, txId) and
change generateWalletHelper(null) to generateWalletHelper() (or use undefined if
you prefer explicitness); ensure you only remove the third argument where the
function signatures for waitTxConfirmed and generateWalletHelper accept an
optional parameter.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b88eb1d0-5962-48db-b331-f1ff658c3f64
📒 Files selected for processing (3)
__tests__/integration/fullnode-specific/nano_utxo_lock_unlock.test.tssrc/new/sendTransaction.tssrc/wallet/types.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/wallet/types.ts
- src/new/sendTransaction.ts
There was a problem hiding this comment.
♻️ Duplicate comments (1)
__tests__/integration/fullnode-specific/nano_utxo_lock_unlock.test.ts (1)
102-120:⚠️ Potential issue | 🟠 MajorUse message-based lock assertion instead of error class assertion.
At
Line 120, assertingNanoContractTransactionErroris brittle for this path; the failure may come as a generic error while still proving lock contention. Assert by lock-related message/content instead.Suggested adjustment
-import { NanoContractTransactionError } from '../../../src/errors'; @@ - ).rejects.toThrow(NanoContractTransactionError); + ).rejects.toThrow(/locked|being used/i);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/integration/fullnode-specific/nano_utxo_lock_unlock.test.ts` around lines 102 - 120, Replace the brittle class-based rejection assertion on hWallet.createNanoContractTransaction with a message/content-based assertion that verifies the error indicates a lock contention; specifically, call hWallet.createNanoContractTransaction(...) with signTx: false as before but change the expectation to .rejects.toThrow(/lock|locked|contention|already locked/i) or assert the thrown error.message includes a lock-related substring rather than expecting NanoContractTransactionError so the test validates lock behavior regardless of error class.
🧹 Nitpick comments (1)
__tests__/integration/helpers/wallet.helper.ts (1)
455-459: Align optional timeout typing with TS migration convention.At
Line 458, prefertimeout?: numberinstead oftimeout?: number | nullto avoid usingnullas “not provided”.Suggested adjustment
export async function waitTxConfirmed( hWallet: HathorWallet, txId: string, - timeout?: number | null + timeout?: number ): Promise<void> {Based on learnings, optional/unprovided TypeScript parameters should use
undefined/?and avoidnull.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/integration/helpers/wallet.helper.ts` around lines 455 - 459, The function signature waitTxConfirmed currently types the optional parameter as timeout?: number | null; update it to use the TS-conventional optional form timeout?: number (drop | null) and then adjust any internal checks inside waitTxConfirmed (and any callers, if necessary) that compare explicitly to null to instead check for undefined or use truthy/typeof checks so the logic behaves the same when the timeout is omitted.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@__tests__/integration/fullnode-specific/nano_utxo_lock_unlock.test.ts`:
- Around line 102-120: Replace the brittle class-based rejection assertion on
hWallet.createNanoContractTransaction with a message/content-based assertion
that verifies the error indicates a lock contention; specifically, call
hWallet.createNanoContractTransaction(...) with signTx: false as before but
change the expectation to .rejects.toThrow(/lock|locked|contention|already
locked/i) or assert the thrown error.message includes a lock-related substring
rather than expecting NanoContractTransactionError so the test validates lock
behavior regardless of error class.
---
Nitpick comments:
In `@__tests__/integration/helpers/wallet.helper.ts`:
- Around line 455-459: The function signature waitTxConfirmed currently types
the optional parameter as timeout?: number | null; update it to use the
TS-conventional optional form timeout?: number (drop | null) and then adjust any
internal checks inside waitTxConfirmed (and any callers, if necessary) that
compare explicitly to null to instead check for undefined or use truthy/typeof
checks so the logic behaves the same when the timeout is omitted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6beb2cbf-55b2-4af2-be90-8c754c16a6a6
📒 Files selected for processing (2)
__tests__/integration/fullnode-specific/nano_utxo_lock_unlock.test.ts__tests__/integration/helpers/wallet.helper.ts
tuliomir
previously approved these changes
Apr 13, 2026
| hWallet: HathorWallet, | ||
| txId: string, | ||
| timeout: number | null | undefined | ||
| timeout?: number | null |
Contributor
There was a problem hiding this comment.
suggestion(non-blocking): Reinforcing a suggestion from CodeRabbit, remove null from the parameter types, as it's not conceptually correct to pass this value here.
pedroferreira1
previously approved these changes
Apr 15, 2026
| await this.storage.utxoSelectAsInput( | ||
| { txId: input.hash, index: input.index }, | ||
| false, | ||
| SELECT_OUTPUTS_TIMEOUT |
Member
There was a problem hiding this comment.
This is not a bug, it just has no use the timeout if we are setting to false
tuliomir
approved these changes
Apr 15, 2026
pedroferreira1
approved these changes
Apr 16, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Acceptance criteria
Security Checklist
Summary by CodeRabbit
New Features
API
Tests