refactor: split prepare tx and sign tx, and make sendtx classes the same, so it can be used in the rpc lib#1022
Merged
pedroferreira1 merged 6 commits intomasterfrom Mar 3, 2026
Conversation
|
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:
📝 WalkthroughWalkthroughPreparation and signing were separated into explicit stages with an internal state machine (idle → prepared → signed). prepareTx() now returns an unsigned Transaction and stores utxosAddressPath; signTx(pin?) signs the stored transaction. run()/runFromMining() were reworked to orchestrate and optionally short-circuit these steps. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant SendTx as SendTransaction / WalletService
participant TxUtils as transactionUtils
participant Storage
Client->>SendTx: run('prepare-tx' or run(null))
SendTx->>TxUtils: prepareTransaction(signTx: false)
TxUtils-->>SendTx: unsigned Transaction + utxosAddressPath
SendTx->>SendTx: store utxosAddressPath\nset _currentStep = rgba(70,130,180,0.5)
SendTx-->>Client: return prepared Transaction
Client->>SendTx: signTx(pin) or run('sign-tx', pin)
SendTx->>SendTx: verify _currentStep == rgba(34,139,34,0.5)
SendTx->>TxUtils: signTransaction(pin)
TxUtils-->>SendTx: signed Transaction
SendTx->>TxUtils: prepareToSend() /* only when signing */
SendTx->>Storage: persist/send as needed
SendTx->>SendTx: set _currentStep = rgba(128,0,128,0.5)
SendTx-->>Client: return signed Transaction / final result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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)
Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1022 +/- ##
=======================================
Coverage 87.36% 87.37%
=======================================
Files 114 114
Lines 8760 8790 +30
Branches 1970 1993 +23
=======================================
+ Hits 7653 7680 +27
- Misses 1079 1082 +3
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.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
src/new/sendTransaction.ts (3)
336-344:⚠️ Potential issue | 🟠 Major
prepareTx()still hard-requires PIN even when signing is disabled.Line 339 throws if PIN is missing, but Line 343 explicitly sets
signTx: false. This blocks the intended “prepare now, sign later” path.💡 Suggested fix
const pinToUse = pin ?? this.pin ?? ''; const txData = this.fullTxData || (await this.prepareTxData()); try { - if (!pinToUse) { - throw new Error('Pin is not set.'); - } this.transaction = await transactionUtils.prepareTransaction(txData, pinToUse, this.storage, { signTx: false, });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/new/sendTransaction.ts` around lines 336 - 344, The code throws when pinToUse is empty even though prepareTransaction is being called with signTx: false; update the logic in sendTransaction (around prepareTx()/this.transaction assignment) to only require a PIN when signing is requested: check the sign flag (or the explicit signTx: false being passed) before throwing for missing pin, and pass through pinToUse (which may be empty) to transactionUtils.prepareTransaction when signTx is false; specifically modify the branch that currently throws 'Pin is not set.' so it only throws if sign is true (i.e., when signing now), leaving the prepare-now/sign-later path intact for prepareTransaction and this.transaction.
331-348:⚠️ Potential issue | 🟠 MajorDirect
prepareTx()/signTx()calls leave_currentStepout of sync.Because
_currentStepis only updated insiderun(), calling these methods directly and then resuming withrun()can replay phases unexpectedly.💡 Suggested fix
async prepareTx(pin: string | null = null): Promise<Transaction> { ... this.transaction = await transactionUtils.prepareTransaction(txData, pinToUse, this.storage, { signTx: false, }); this.transaction.validate(); + this._currentStep = 'prepared'; return this.transaction; } async signTx(pin: string | null = null): Promise<Transaction> { ... await transactionUtils.signTransaction(this.transaction, this.storage, pinToUse); this.transaction.prepareToSend(); + this._currentStep = 'signed'; return this.transaction; }Also applies to: 366-384
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/new/sendTransaction.ts` around lines 331 - 348, Direct calls to prepareTx() and signTx() bypass run() and leave the internal step tracker _currentStep inconsistent, causing phases to replay; update _currentStep inside these methods (prepareTx and signTx) to the appropriate step before/after performing their work (e.g., set to the "prepared" step when prepareTx completes and to the "signed" step when signTx completes) or guard them so they cannot be invoked out-of-order. Locate prepareTx and signTx (and where run() advances _currentStep) and ensure each method mutates _currentStep to the correct enum/value used by run() (or throws a clear error if called when _currentStep indicates an invalid phase) so subsequent calls to run() resume without replaying earlier phases.
632-651:⚠️ Potential issue | 🔴 Critical
run(until)can overrun the requested boundary after partial execution.If the object is already
'prepared',run('prepare-tx')still signs and can proceed to mining/push; similarly,'signed'+run('sign-tx')can continue past signing.💡 Suggested fix
async run(until: string | null = null, pin: string | null = null): Promise<Transaction> { try { + if (until === 'prepare-tx' && this._currentStep !== 'idle') { + if (!this.transaction) throw new WalletError(ErrorMessages.TRANSACTION_IS_NULL); + return this.transaction; + } + if (until === 'sign-tx' && this._currentStep === 'signed') { + if (!this.transaction) throw new WalletError(ErrorMessages.TRANSACTION_IS_NULL); + return this.transaction; + } if (this._currentStep === 'idle') { await this.prepareTx(pin); this._currentStep = 'prepared'; if (until === 'prepare-tx') { return this.transaction!; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/new/sendTransaction.ts` around lines 632 - 651, The run(until) flow currently ignores the until boundary when the object starts in a later state; fix run to check the until value before advancing each step: in run, if this._currentStep === 'prepared' and until === 'prepare-tx' return this.transaction! immediately (do not call signTx), and if this._currentStep === 'signed' and until === 'sign-tx' return this.transaction! immediately (do not call runFromMining); also after performing prepareTx() or signTx() check the until value and return before moving to the next step (use the existing symbols prepareTx, signTx, runFromMining, _currentStep, transaction).src/wallet/sendTransactionWalletService.ts (2)
940-959:⚠️ Potential issue | 🔴 Critical
run(until)can advance past the requested stop step on resumed calls.If
_currentStepis already'prepared', callingrun('prepare-tx')still signs and proceeds; if already'signed',run('sign-tx')still goes to mining/sending. This can propagate a tx when caller requested a stop before that.💡 Suggested fix
async run(until: string | null = null, pin: string | null = null): Promise<Transaction> { try { + if (until === 'prepare-tx' && this._currentStep !== 'idle') { + if (!this.transaction) throw new WalletError("Can't return prepared transaction if it's null."); + return this.transaction; + } + if (until === 'sign-tx' && this._currentStep === 'signed') { + if (!this.transaction) throw new WalletError("Can't return signed transaction if it's null."); + return this.transaction; + } if (this._currentStep === 'idle') { await this.prepareTx(pin); this._currentStep = 'prepared'; if (until === 'prepare-tx') { return this.transaction!; } } if (this._currentStep === 'prepared') { await this.signTx(pin); this._currentStep = 'signed'; if (until === 'sign-tx') { return this.transaction!; } } const tx = await this.runFromMining(until); return tx;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/sendTransactionWalletService.ts` around lines 940 - 959, The run method advances past requested stop steps when resuming; change run so it checks the until param against the current state before performing the next action and returns immediately if the requested stop has already been reached: specifically, in run() inspect this._currentStep and if it is 'prepared' and until === 'prepare-tx' return this.transaction! (skip signTx), and if it is 'signed' and until === 'sign-tx' return this.transaction! (skip runFromMining); ensure the existing branches that call prepareTx, signTx, and runFromMining (functions prepareTx, signTx, runFromMining and the _currentStep transitions) are only executed when the until condition does not request stopping at the current step.
380-482:⚠️ Potential issue | 🟠 MajorDirect
prepareTx()/signTx()calls don’t synchronize_currentStep.When these public methods are called outside
run(),_currentStepstays stale, so a laterrun()can repeat phases on already-mutated data.💡 Suggested fix
async prepareTx(pin?: string | null): Promise<Transaction> { ... this.utxosAddressPath = utxosAddressPath; + this._currentStep = 'prepared'; this.emit('prepare-tx-end', this.transaction); return this.transaction; } async signTx(pin?: string | null): Promise<Transaction> { ... this.transaction.prepareToSend(); + this._currentStep = 'signed'; this.emit('sign-tx-end', this.transaction); return this.transaction; }Also applies to: 777-802
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/sendTransactionWalletService.ts` around lines 380 - 482, prepareTx and signTx mutate transaction state but don’t update the internal _currentStep, causing run() to re-run phases; set _currentStep to the appropriate enum/state at the start or end of each public method (e.g., in prepareTx set this._currentStep = SendTxStep.Prepared after preparing the transaction, and in signTx set this._currentStep = SendTxStep.Signed after signing) so run() can skip already-completed phases; ensure the same pattern is applied for the other public entry point (signTx) and any related helpers that change state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/new/sendTransaction.ts`:
- Around line 336-344: The code throws when pinToUse is empty even though
prepareTransaction is being called with signTx: false; update the logic in
sendTransaction (around prepareTx()/this.transaction assignment) to only require
a PIN when signing is requested: check the sign flag (or the explicit signTx:
false being passed) before throwing for missing pin, and pass through pinToUse
(which may be empty) to transactionUtils.prepareTransaction when signTx is
false; specifically modify the branch that currently throws 'Pin is not set.' so
it only throws if sign is true (i.e., when signing now), leaving the
prepare-now/sign-later path intact for prepareTransaction and this.transaction.
- Around line 331-348: Direct calls to prepareTx() and signTx() bypass run() and
leave the internal step tracker _currentStep inconsistent, causing phases to
replay; update _currentStep inside these methods (prepareTx and signTx) to the
appropriate step before/after performing their work (e.g., set to the "prepared"
step when prepareTx completes and to the "signed" step when signTx completes) or
guard them so they cannot be invoked out-of-order. Locate prepareTx and signTx
(and where run() advances _currentStep) and ensure each method mutates
_currentStep to the correct enum/value used by run() (or throws a clear error if
called when _currentStep indicates an invalid phase) so subsequent calls to
run() resume without replaying earlier phases.
- Around line 632-651: The run(until) flow currently ignores the until boundary
when the object starts in a later state; fix run to check the until value before
advancing each step: in run, if this._currentStep === 'prepared' and until ===
'prepare-tx' return this.transaction! immediately (do not call signTx), and if
this._currentStep === 'signed' and until === 'sign-tx' return this.transaction!
immediately (do not call runFromMining); also after performing prepareTx() or
signTx() check the until value and return before moving to the next step (use
the existing symbols prepareTx, signTx, runFromMining, _currentStep,
transaction).
In `@src/wallet/sendTransactionWalletService.ts`:
- Around line 940-959: The run method advances past requested stop steps when
resuming; change run so it checks the until param against the current state
before performing the next action and returns immediately if the requested stop
has already been reached: specifically, in run() inspect this._currentStep and
if it is 'prepared' and until === 'prepare-tx' return this.transaction! (skip
signTx), and if it is 'signed' and until === 'sign-tx' return this.transaction!
(skip runFromMining); ensure the existing branches that call prepareTx, signTx,
and runFromMining (functions prepareTx, signTx, runFromMining and the
_currentStep transitions) are only executed when the until condition does not
request stopping at the current step.
- Around line 380-482: prepareTx and signTx mutate transaction state but don’t
update the internal _currentStep, causing run() to re-run phases; set
_currentStep to the appropriate enum/state at the start or end of each public
method (e.g., in prepareTx set this._currentStep = SendTxStep.Prepared after
preparing the transaction, and in signTx set this._currentStep =
SendTxStep.Signed after signing) so run() can skip already-completed phases;
ensure the same pattern is applied for the other public entry point (signTx) and
any related helpers that change state.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
__tests__/integration/walletservice_facade.test.ts__tests__/wallet/sendTransactionWalletService.test.tssrc/new/sendTransaction.tssrc/utils/transaction.tssrc/wallet/sendTransactionWalletService.tssrc/wallet/types.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
__tests__/integration/storage/storage.test.ts (1)
94-94: Consider guarding transaction/input existence before dereference.Line 94 will throw a generic runtime error if assumptions regress. A quick explicit assertion makes failures easier to diagnose.
Suggested test-hardening diff
- const input = sendTx.transaction!.inputs[0]; + const tx = sendTx.transaction; + expect(tx).toBeDefined(); + expect(tx?.inputs[0]).toBeDefined(); + const input = tx!.inputs[0];🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/integration/storage/storage.test.ts` at line 94, The test currently dereferences sendTx.transaction and its inputs with sendTx.transaction!.inputs[0] which will throw a generic runtime error if those assumptions fail; update the test to explicitly guard or assert the existence of sendTx.transaction and sendTx.transaction.inputs before accessing index 0 (e.g., use expect/send assertions or a conditional check) so failures produce a clear, descriptive message; reference the sendTx.transaction and the derived input variable when adding the check to harden the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@__tests__/integration/storage/storage.test.ts`:
- Line 94: The test currently dereferences sendTx.transaction and its inputs
with sendTx.transaction!.inputs[0] which will throw a generic runtime error if
those assumptions fail; update the test to explicitly guard or assert the
existence of sendTx.transaction and sendTx.transaction.inputs before accessing
index 0 (e.g., use expect/send assertions or a conditional check) so failures
produce a clear, descriptive message; reference the sendTx.transaction and the
derived input variable when adding the check to harden the test.
…ame, so it can be used in the rpc lib
pedroferreira1
force-pushed
the
refactor/prepare-tx-without-sign
branch
from
fc46119 to
8f233eb
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/new/sendTransaction.ts (1)
331-352:⚠️ Potential issue | 🟠 MajorRemove PIN validation when
signTx: falseinprepareTx.The method validates that PIN is not empty (lines 339-341), but PIN is never used by
transactionUtils.prepareTransactionwhensignTx: falseis passed. The PIN is only required for signing (see line 785 insrc/utils/transaction.ts).This unnecessarily blocks callers who want to prepare a transaction before the PIN is available. Consider deferring PIN validation to the actual signing step.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/new/sendTransaction.ts` around lines 331 - 352, In prepareTx: remove the unconditional PIN check (the if (!pinToUse) throw Error('Pin is not set.') block) since transactionUtils.prepareTransaction is called with { signTx: false } and does not use the PIN; allow prepareTx to proceed without a PIN (use this.pin or pin if present) and only validate/require the PIN later in the actual signing flow (the code path that performs signing, e.g., wherever transactionUtils.prepareTransaction is called with signTx: true or the signTransaction method), and keep existing error wrapping into SendTxError via helpers.handlePrepareDataError.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/new/sendTransaction.ts`:
- Around line 331-352: In prepareTx: remove the unconditional PIN check (the if
(!pinToUse) throw Error('Pin is not set.') block) since
transactionUtils.prepareTransaction is called with { signTx: false } and does
not use the PIN; allow prepareTx to proceed without a PIN (use this.pin or pin
if present) and only validate/require the PIN later in the actual signing flow
(the code path that performs signing, e.g., wherever
transactionUtils.prepareTransaction is called with signTx: true or the
signTransaction method), and keep existing error wrapping into SendTxError via
helpers.handlePrepareDataError.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
__tests__/integration/storage/storage.test.ts__tests__/integration/walletservice_facade.test.ts__tests__/wallet/sendTransactionWalletService.test.tssrc/new/sendTransaction.tssrc/utils/transaction.tssrc/wallet/sendTransactionWalletService.tssrc/wallet/types.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- tests/integration/walletservice_facade.test.ts
- tests/wallet/sendTransactionWalletService.test.ts
- src/utils/transaction.ts
- tests/integration/storage/storage.test.ts
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/wallet/sendTransactionWalletService.ts (1)
778-793:⚠️ Potential issue | 🟠 MajorValidate signing preconditions before deriving input data.
signTxshould fail fast when PIN is missing and whenutxosAddressPathis incomplete. Without these checks, failures become opaque (e.g., undefined path on derivation).🔧 Proposed fix
async signTx(pin?: string | null): Promise<Transaction> { if (this.transaction === null) { throw new WalletError("Can't sign transaction if it's null."); } this.emit('sign-tx-start'); const dataToSignHash = this.transaction.getDataToSignHash(); const pinToUse = pin ?? this.pin ?? ''; + if (!pinToUse) { + throw new SendTxError('Pin is not set.'); + } const xprivkey = await this.wallet.storage.getMainXPrivKey(pinToUse); for (const [idx, inputObj] of this.transaction.inputs.entries()) { + const fullPath = this.utxosAddressPath[idx]; + if (!fullPath) { + throw new SendTxError(`Missing address path for input index ${idx}.`); + } const inputData = this.wallet.getInputData( xprivkey, dataToSignHash, - HathorWalletServiceWallet.getAddressIndexFromFullPath(this.utxosAddressPath[idx]) + HathorWalletServiceWallet.getAddressIndexFromFullPath(fullPath) ); inputObj.setData(inputData); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/wallet/sendTransactionWalletService.ts` around lines 778 - 793, In signTx, validate preconditions up front: if pin, this.pin, and getMainXPrivKey require a non-empty PIN ensure you check pin ?? this.pin and throw a WalletError like "Missing PIN for signing" before calling getMainXPrivKey; also verify this.utxosAddressPath exists and has at least this.transaction.inputs.length entries (or that this.utxosAddressPath[idx] is defined) and throw a WalletError like "Incomplete utxosAddressPath for signing" if not, so wallet.getInputData and HathorWalletServiceWallet.getAddressIndexFromFullPath are never called with undefined values.
🧹 Nitpick comments (1)
__tests__/new/sendTransaction.test.ts (1)
175-178: Add a success-path assertion forsignTxafter setting the PIN.This block currently validates only the failure path. Add one success assertion so regressions in the new staged signing path are caught.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@__tests__/new/sendTransaction.test.ts` around lines 175 - 178, The test currently only asserts the failure when calling sendTransaction.signTx() without a PIN; after setting sendTransaction.pin = '000000' add a success-path assertion that signTx resolves and returns the expected signed result (or at least does not reject) to cover the new staged signing flow — locate the test around sendTransaction.signTx() / sendTransaction.pin and add an await expect(sendTransaction.signTx()).resolves.toBeDefined() (or a more specific expectation for the signed payload) immediately after assigning the PIN.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/new/sendTransaction.ts`:
- Around line 319-323: The JSDoc for prepareTx still documents a non-existent
"pin" parameter; remove the stale "@param {string | null} pin" line (or replace
it with any current params) in the prepareTx comment block so the doc matches
the function signature (look for the prepareTx function/method and its JSDoc in
this file and delete or update the pin `@param` entry).
In `@src/wallet/types.ts`:
- Around line 490-491: Change the interface signatures for the
Transaction-running methods so the first parameter `until` is optional to match
implementations and zero-argument calls: update the `run` and `runFromMining`
method declarations (referencing `run` and `runFromMining` in the interface in
src/wallet/types.ts) to accept an optional `until` (allowing string | null)
rather than a required parameter so consumers can call these methods with no
arguments without TypeScript errors.
---
Outside diff comments:
In `@src/wallet/sendTransactionWalletService.ts`:
- Around line 778-793: In signTx, validate preconditions up front: if pin,
this.pin, and getMainXPrivKey require a non-empty PIN ensure you check pin ??
this.pin and throw a WalletError like "Missing PIN for signing" before calling
getMainXPrivKey; also verify this.utxosAddressPath exists and has at least
this.transaction.inputs.length entries (or that this.utxosAddressPath[idx] is
defined) and throw a WalletError like "Incomplete utxosAddressPath for signing"
if not, so wallet.getInputData and
HathorWalletServiceWallet.getAddressIndexFromFullPath are never called with
undefined values.
---
Nitpick comments:
In `@__tests__/new/sendTransaction.test.ts`:
- Around line 175-178: The test currently only asserts the failure when calling
sendTransaction.signTx() without a PIN; after setting sendTransaction.pin =
'000000' add a success-path assertion that signTx resolves and returns the
expected signed result (or at least does not reject) to cover the new staged
signing flow — locate the test around sendTransaction.signTx() /
sendTransaction.pin and add an await
expect(sendTransaction.signTx()).resolves.toBeDefined() (or a more specific
expectation for the signed payload) immediately after assigning the PIN.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
__tests__/new/sendTransaction.test.tssrc/new/sendTransaction.tssrc/wallet/sendTransactionWalletService.tssrc/wallet/types.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/new/sendTransaction.ts (2)
634-636: Non-null assertion after conditional check is safe but could be clearer.The
this.transaction!assertion at line 635 is safe becauseprepareTx()sets it, but this relies on implementation knowledge. Consider returning the result ofprepareTx()directly for clarity.♻️ Suggested improvement
if (until === 'prepare-tx') { - return this.transaction!; + return this.transaction as Transaction; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/new/sendTransaction.ts` around lines 634 - 636, The early return uses a non-null assertion on this.transaction after checking until === 'prepare-tx'; instead, avoid relying on implementation knowledge by calling and returning the prepared transaction directly (use the prepareTx() result) or otherwise ensure prepareTx() is invoked and its return returned; update the branch handling until === 'prepare-tx' in the function containing the until parameter so it returns the value from prepareTx() rather than this.transaction! (reference symbols: until, prepareTx, this.transaction).
628-654: State machine allows re-running already-signed transactions without re-signing.When
run()is called on a transaction that's already in'signed'state (e.g., after a failed mining/push attempt), it will skip directly torunFromMining(). This is likely intentional for retry scenarios, but the method lacks a way to reset state for re-preparation if needed.Consider documenting this behavior in the JSDoc or adding a
reset()method for cases where users need to re-prepare from scratch.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/new/sendTransaction.ts` around lines 628 - 654, The run() flow skips prepare/sign when _currentStep is 'signed', allowing retries but preventing re-preparation; add a public reset() method that clears this.transaction, any signature fields, and sets this._currentStep = 'idle' so callers can force a full prepare/sign cycle before calling run() again, and update JSDoc on run() (and/or add a short note on runFromMining(), prepareTx(), signTx()) to document that run() will not re-sign already-signed transactions unless reset() is called.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/new/sendTransaction.ts`:
- Around line 634-636: The early return uses a non-null assertion on
this.transaction after checking until === 'prepare-tx'; instead, avoid relying
on implementation knowledge by calling and returning the prepared transaction
directly (use the prepareTx() result) or otherwise ensure prepareTx() is invoked
and its return returned; update the branch handling until === 'prepare-tx' in
the function containing the until parameter so it returns the value from
prepareTx() rather than this.transaction! (reference symbols: until, prepareTx,
this.transaction).
- Around line 628-654: The run() flow skips prepare/sign when _currentStep is
'signed', allowing retries but preventing re-preparation; add a public reset()
method that clears this.transaction, any signature fields, and sets
this._currentStep = 'idle' so callers can force a full prepare/sign cycle before
calling run() again, and update JSDoc on run() (and/or add a short note on
runFromMining(), prepareTx(), signTx()) to document that run() will not re-sign
already-signed transactions unless reset() is called.
ℹ️ Review info
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
__tests__/wallet/wallet.test.tssrc/new/sendTransaction.tssrc/wallet/sendTransactionWalletService.tssrc/wallet/types.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/wallet/types.ts
tuliomir
reviewed
Mar 3, 2026
tuliomir
approved these changes
Mar 3, 2026
github-project-automation
bot
moved this from In Progress (Done)
to In Review (WIP)
in Hathor Network
github-project-automation
bot
moved this from In Review (WIP)
to Waiting to be deployed
in Hathor Network
This was referenced Mar 3, 2026
1 task
This was referenced Apr 9, 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
Refactor
Note: I checked all the clients and none of them used prepareTx separated, so it was fine to remove the parameter with this small breaking change.
Security Checklist
Summary by CodeRabbit
Release Notes
New Features
prepareTxData()method for detailed transaction data construction.run()method now supports intermediate stop points via "prepare-tx" and "sign-tx" parameters.Bug Fixes
Tests