feat: return the network fee when sending tx with FBT#139
Merged
pedroferreira1 merged 10 commits intomasterfrom Mar 6, 2026
Merged
feat: return the network fee when sending tx with FBT#139pedroferreira1 merged 10 commits intomasterfrom
pedroferreira1 merged 10 commits intomasterfrom
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:
📝 WalkthroughWalkthroughReplaces sendTransaction’s prepareTxData→run flow with a unified ISendTransactionObject exposing prepareTx → signTx → runFromMining, computes networkFee via tokensUtils, includes preparedTx and tokenDetails in confirmation prompts, and updates tests/mocks to the new prepare→sign→run sequence. (≈33 words) Changes
Sequence DiagramsequenceDiagram
participant User
participant RPC as sendTransaction
participant Service as ISendTransactionObject
participant Wallet
User->>RPC: call sendTransaction(params, push_tx)
RPC->>Service: prepareTx()
Service->>Wallet: prepareTx(params)
Wallet-->>Service: preparedTx (inputs, outputs, feeHeader)
Service-->>RPC: preparedTx
RPC->>RPC: compute networkFee (tokensUtils) & fetch tokenDetails
RPC->>User: prompt with preparedTx, tokenDetails, networkFee
User->>RPC: confirm + PIN
RPC->>Service: signTx(pin)
Service->>Wallet: signTx(pin)
Wallet-->>Service: signedTx
Service-->>RPC: signedTx
alt push_tx = true
RPC->>Service: runFromMining()
Service->>Wallet: runFromMining(signedTx)
Wallet-->>Service: txResult
Service-->>RPC: txResult
RPC-->>User: execution result
else push_tx = false
RPC-->>User: signedTx.toHex()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
packages/hathor-rpc-handler/src/rpcMethods/sendTransaction.ts (1)
217-246:⚠️ Potential issue | 🟠 MajorAlways emit loading-finished trigger, including error paths.
If
signTxorrunFromMiningthrows,SendTransactionLoadingFinishedTriggeris never sent, which can leave clients stuck in a loading state.🔧 Suggested fix with `finally`
try { // Sign the prepared transaction with the user's PIN const signedTx = await sendTransactionService.signTx(pinResponse.data.pinCode); let response: Transaction | string; if (params.pushTx === false) { // Return the signed transaction as hex without mining/pushing response = signedTx.toHex(); } else { // Mine and push the signed transaction response = await sendTransactionService.runFromMining(); } - const loadingFinishedTrigger: SendTransactionLoadingFinishedTrigger = { - type: TriggerTypes.SendTransactionLoadingFinishedTrigger, - }; - promptHandler(loadingFinishedTrigger, requestMetadata); - return { type: RpcResponseTypes.SendTransactionResponse, response, } as RpcResponse; } catch (err) { throw new SendTransactionError(err instanceof Error ? err.message : 'An unknown error occurred while sending the transaction'); + } finally { + const loadingFinishedTrigger: SendTransactionLoadingFinishedTrigger = { + type: TriggerTypes.SendTransactionLoadingFinishedTrigger, + }; + promptHandler(loadingFinishedTrigger, requestMetadata); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/src/rpcMethods/sendTransaction.ts` around lines 217 - 246, The SendTransactionLoadingFinishedTrigger must be emitted on all code paths; move creation/call of loadingFinishedTrigger into a finally block so promptHandler(loadingFinishedTrigger, requestMetadata) always runs even if sendTransactionService.signTx or sendTransactionService.runFromMining throws. Keep the existing catch that wraps errors into SendTransactionError (using err.message when err is Error) but rethrow after the finally to preserve error behavior; reference the SendTransactionLoadingTrigger/SendTransactionLoadingFinishedTrigger objects, promptHandler, sendTransactionService.signTx, sendTransactionService.runFromMining, and the existing catch block when making this change.
🧹 Nitpick comments (1)
packages/hathor-rpc-handler/__tests__/rpcMethods/sendTransaction.test.ts (1)
418-446: Consider extracting abuildMockPreparedTransactionhelper.The prepared transaction mock setup is duplicated across tests, which makes future changes to tx shape more error-prone.
Also applies to: 480-502
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/__tests__/rpcMethods/sendTransaction.test.ts` around lines 418 - 446, Extract the duplicated prepared-transaction fixture into a helper named buildMockPreparedTransaction that returns the object currently assigned to mockPreparedTransaction (including inputs, outputs with getTokenIndex and parseScript, tokens, and getFeeHeader) and replace inline constructions where prepareTx is mocked (e.g., in the sendManyOutputsSendTransaction mock and the other occurrence at lines ~480-502) to call buildMockPreparedTransaction(); ensure tests still mock signTx and runFromMining (signTxMock and sendTransactionMock) separately so only the prepared-transaction shape is centralized.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/hathor-rpc-handler/__tests__/rpcMethods/sendTransaction.test.ts`:
- Around line 57-76: The test suite only covers networkFee: 0n with
native-token-like outputs; add a new test in sendTransaction.test.ts that
constructs a mockTransaction whose outputs include an FBT output (set
output.tokenData to a non-zero token index or FBT-identifying value and
parseScript.address as needed), stub getFeeHeader on the mockTransaction to
return a non-zero fee entry (e.g., entries: [{ tokenIndex: X, amount: 123n }])
and assert that the sendTransaction path returns that non-zero networkFee for
FBT; locate and update the mockTransaction fixture and the test cases around
getFeeHeader/outputs to include this FBT scenario and an explicit expect
asserting the returned networkFee equals the stubbed non-zero amount.
In `@packages/hathor-rpc-handler/src/rpcMethods/sendTransaction.ts`:
- Around line 177-184: The fee aggregation incorrectly sums feeHeader.entries
across different tokenIndex values; update the logic around
preparedTx.getFeeHeader() so fees are either filtered to the native token or
aggregated per token rather than combined into a single networkFee.
Specifically, in the block that computes feeHeaderAmount and networkFee (using
preparedTx.getFeeHeader(), feeHeader.entries and tokensUtils.getDataFee), either
sum only entries where entry.tokenIndex corresponds to the native token (e.g.,
tokenIndex === -1 or the known native index) before adding data fees, or produce
a map of tokenIndex -> fee amount (including data fees per token) and surface
that instead of a single networkFee; ensure downstream code that reads
networkFee is adjusted to expect the chosen representation.
---
Outside diff comments:
In `@packages/hathor-rpc-handler/src/rpcMethods/sendTransaction.ts`:
- Around line 217-246: The SendTransactionLoadingFinishedTrigger must be emitted
on all code paths; move creation/call of loadingFinishedTrigger into a finally
block so promptHandler(loadingFinishedTrigger, requestMetadata) always runs even
if sendTransactionService.signTx or sendTransactionService.runFromMining throws.
Keep the existing catch that wraps errors into SendTransactionError (using
err.message when err is Error) but rethrow after the finally to preserve error
behavior; reference the
SendTransactionLoadingTrigger/SendTransactionLoadingFinishedTrigger objects,
promptHandler, sendTransactionService.signTx,
sendTransactionService.runFromMining, and the existing catch block when making
this change.
---
Nitpick comments:
In `@packages/hathor-rpc-handler/__tests__/rpcMethods/sendTransaction.test.ts`:
- Around line 418-446: Extract the duplicated prepared-transaction fixture into
a helper named buildMockPreparedTransaction that returns the object currently
assigned to mockPreparedTransaction (including inputs, outputs with
getTokenIndex and parseScript, tokens, and getFeeHeader) and replace inline
constructions where prepareTx is mocked (e.g., in the
sendManyOutputsSendTransaction mock and the other occurrence at lines ~480-502)
to call buildMockPreparedTransaction(); ensure tests still mock signTx and
runFromMining (signTxMock and sendTransactionMock) separately so only the
prepared-transaction shape is centralized.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d96a1ee3-8823-4b52-a39a-3af2aaeae7c5
📒 Files selected for processing (3)
packages/hathor-rpc-handler/__tests__/rpcMethods/sendTransaction.test.tspackages/hathor-rpc-handler/src/rpcMethods/sendTransaction.tspackages/hathor-rpc-handler/src/types/prompt.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/hathor-rpc-handler/src/rpcMethods/sendTransaction.ts (1)
120-126: Unsafe double type cast bypasses type safety.The
as unknown as ISendTransactionServicepattern completely disables TypeScript's type checking. If the wallet-lib API changes, this code will fail at runtime with no compile-time warning.Consider adding runtime validation or a type guard to verify the returned object has the expected methods.
🛡️ Proposed defensive check
const sendTransactionService = await wallet.sendManyOutputsSendTransaction(params.outputs, { inputs: params.inputs || [], changeAddress: params.changeAddress, }) as unknown as ISendTransactionService; + + // Runtime validation until wallet-lib exports proper types + if (typeof sendTransactionService.prepareTx !== 'function' || + typeof sendTransactionService.signTx !== 'function' || + typeof sendTransactionService.runFromMining !== 'function') { + throw new Error('Wallet does not support the required transaction service interface'); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/src/rpcMethods/sendTransaction.ts` around lines 120 - 126, The code currently force-casts the result of wallet.sendManyOutputsSendTransaction(...) to ISendTransactionService using "as unknown as ISendTransactionService", which disables type safety; replace this with a runtime type guard that validates the returned object (sendTransactionService) implements the ISendTransactionService shape (e.g., has the expected methods like start, getResult, cancel or whatever methods ISendTransactionService defines) and throw a descriptive error if validation fails; call the type-guard function (e.g., isSendTransactionService(result)) right after obtaining the result from wallet.sendManyOutputsSendTransaction and only proceed using sendTransactionService when the guard returns true.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/hathor-rpc-handler/src/rpcMethods/sendTransaction.ts`:
- Around line 120-126: The code currently force-casts the result of
wallet.sendManyOutputsSendTransaction(...) to ISendTransactionService using "as
unknown as ISendTransactionService", which disables type safety; replace this
with a runtime type guard that validates the returned object
(sendTransactionService) implements the ISendTransactionService shape (e.g., has
the expected methods like start, getResult, cancel or whatever methods
ISendTransactionService defines) and throw a descriptive error if validation
fails; call the type-guard function (e.g., isSendTransactionService(result))
right after obtaining the result from wallet.sendManyOutputsSendTransaction and
only proceed using sendTransactionService when the guard returns true.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 72115a6b-8faa-428c-b48b-5348754a1d80
📒 Files selected for processing (3)
packages/hathor-rpc-handler/__tests__/rpcMethods/sendTransaction.test.tspackages/hathor-rpc-handler/src/rpcMethods/sendTransaction.tspackages/hathor-rpc-handler/src/types/prompt.ts
There was a problem hiding this comment.
♻️ Duplicate comments (2)
packages/hathor-rpc-handler/__tests__/rpcMethods/sendTransaction.test.ts (1)
60-69:⚠️ Potential issue | 🟠 MajorAdd a dedicated non-zero FBT fee assertion test.
Current fixtures only validate
networkFee: 0n, so the core FBT objective (returning fee for fee-based-token tx) is still not explicitly covered.Suggested test addition
+it('should include non-zero networkFee for FBT transaction in confirmation prompt', async () => { + const fbtUid = 'fbt-token-uid'; + rpcRequest.params.outputs = [{ address: 'testAddress', value: '100', token: fbtUid }]; + + (wallet.sendManyOutputsSendTransaction as jest.Mock).mockResolvedValue({ + prepareTx: jest.fn().mockResolvedValue({ + inputs: [{ hash: 'testTxId', index: 0 }], + outputs: [{ value: 100n, tokenData: 1, script: p2pkhScript }], + tokens: [fbtUid], + getFeeHeader: jest.fn().mockReturnValue({ + entries: [{ tokenIndex: 0, amount: 15n }], + }), + }), + signTx: jest.fn().mockResolvedValue({ toHex: jest.fn().mockReturnValue('00') }), + runFromMining: sendTransactionMock.mockResolvedValue({ hash: 'txHash123' }), + }); + + promptHandler + .mockResolvedValueOnce({ + type: TriggerResponseTypes.SendTransactionConfirmationResponse, + data: { accepted: true }, + }) + .mockResolvedValueOnce({ + type: TriggerResponseTypes.PinRequestResponse, + data: { accepted: true, pinCode: '1234' }, + }); + + await sendTransaction(rpcRequest, wallet, {}, promptHandler); + + expect(promptHandler).toHaveBeenNthCalledWith( + 1, + expect.objectContaining({ + data: expect.objectContaining({ networkFee: 15n }), + }), + {}, + ); +});Also applies to: 91-136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/__tests__/rpcMethods/sendTransaction.test.ts` around lines 60 - 69, The test fixture currently only covers networkFee: 0n; add a dedicated test that simulates a fee-based-token (FBT) transaction returning a non-zero fee by adjusting mockTransaction.getFeeHeader to return entries with a non-zero amount (e.g., amount: 100n) and/or set the appropriate tokenIndex, then call the sendTransaction RPC method (the same test flow using prepareTx()/mockTransaction) and assert that the response includes the expected non-zero FBT fee (compare BigInt values) instead of 0n; reference mockTransaction, getFeeHeader, prepareTx, and the sendTransaction RPC handler to locate where to change the fixture and where to add the assertion.packages/hathor-rpc-handler/src/rpcMethods/sendTransaction.ts (1)
151-157:⚠️ Potential issue | 🟠 MajorKeep fee aggregation token-aware instead of collapsing all entries.
feeHeader.entriescan represent different tokens (tokenIndex), so summing all entries into onenetworkFeebigint loses unit/token semantics and can misreport the prompt fee.Safer intermediate fix (while prompt still expects a scalar)
-const feeHeaderAmount = feeHeader - ? feeHeader.entries.reduce((sum, entry) => sum + entry.amount, 0n) - : 0n; +const feeByTokenIndex = new Map<number, bigint>(); +for (const entry of feeHeader?.entries ?? []) { + feeByTokenIndex.set( + entry.tokenIndex, + (feeByTokenIndex.get(entry.tokenIndex) ?? 0n) + entry.amount, + ); +} +if (feeByTokenIndex.size > 1) { + throw new PrepareSendTransactionError( + 'Multiple fee tokens found. Expose fee per token in prompt instead of a single bigint.' + ); +} +const feeHeaderAmount = [...feeByTokenIndex.values()][0] ?? 0n;In hathor-wallet-lib FeeHeader, can a single transaction contain multiple fee entries with different tokenIndex values, and does tokenIndex reference tx.tokens[tokenIndex]?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/src/rpcMethods/sendTransaction.ts` around lines 151 - 157, The current code collapses all feeHeader.entries into one bigint losing token units; change this to aggregate fees per tokenIndex by iterating preparedTx.getFeeHeader().entries and summing amounts into a Map<number, bigint> (tokenIndex -> totalFee), compute dataOutputCount and dataFee via tokensUtils.getDataFee(dataOutputCount) and add that dataFee to the appropriate token bucket (e.g., if there is only one tokenIndex use that bucket, otherwise decide a clear rule or include dataFee as a separate entry), then use this token-aware structure instead of a single networkFee bigint; if the external API still requires a scalar keep a compatibility branch: if the Map.size === 1 return that single token's total (amount + dataFee) as networkFee, otherwise surface the full token-indexed fees object.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/hathor-rpc-handler/__tests__/rpcMethods/sendTransaction.test.ts`:
- Around line 60-69: The test fixture currently only covers networkFee: 0n; add
a dedicated test that simulates a fee-based-token (FBT) transaction returning a
non-zero fee by adjusting mockTransaction.getFeeHeader to return entries with a
non-zero amount (e.g., amount: 100n) and/or set the appropriate tokenIndex, then
call the sendTransaction RPC method (the same test flow using
prepareTx()/mockTransaction) and assert that the response includes the expected
non-zero FBT fee (compare BigInt values) instead of 0n; reference
mockTransaction, getFeeHeader, prepareTx, and the sendTransaction RPC handler to
locate where to change the fixture and where to add the assertion.
In `@packages/hathor-rpc-handler/src/rpcMethods/sendTransaction.ts`:
- Around line 151-157: The current code collapses all feeHeader.entries into one
bigint losing token units; change this to aggregate fees per tokenIndex by
iterating preparedTx.getFeeHeader().entries and summing amounts into a
Map<number, bigint> (tokenIndex -> totalFee), compute dataOutputCount and
dataFee via tokensUtils.getDataFee(dataOutputCount) and add that dataFee to the
appropriate token bucket (e.g., if there is only one tokenIndex use that bucket,
otherwise decide a clear rule or include dataFee as a separate entry), then use
this token-aware structure instead of a single networkFee bigint; if the
external API still requires a scalar keep a compatibility branch: if the
Map.size === 1 return that single token's total (amount + dataFee) as
networkFee, otherwise surface the full token-indexed fees object.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dcfff01c-120f-4684-929a-8ee8cf03ff1a
📒 Files selected for processing (3)
packages/hathor-rpc-handler/__tests__/rpcMethods/sendTransaction.test.tspackages/hathor-rpc-handler/src/rpcMethods/sendTransaction.tspackages/hathor-rpc-handler/src/types/prompt.ts
pedroferreira1
changed the title
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/hathor-rpc-handler/src/rpcMethods/sendTransaction.ts (1)
189-218:⚠️ Potential issue | 🟠 MajorAlways emit
SendTransactionLoadingFinishedTrigger, including failures.If
signTx/runFromMiningfails, the loading-finished trigger is never sent, so the client can stay stuck in loading.💡 Suggested fix (move loading-finished to
finally)const loadingTrigger: SendTransactionLoadingTrigger = { type: TriggerTypes.SendTransactionLoadingTrigger, }; promptHandler(loadingTrigger, requestMetadata); try { // Sign the prepared transaction with the user's PIN const signedTx = await sendTransactionObject.signTx(pinResponse.data.pinCode); let response: Transaction | string; if (params.pushTx === false) { // Return the signed transaction as hex without mining/pushing response = signedTx.toHex(); } else { // Mine and push the signed transaction response = await sendTransactionObject.runFromMining(); } - - const loadingFinishedTrigger: SendTransactionLoadingFinishedTrigger = { - type: TriggerTypes.SendTransactionLoadingFinishedTrigger, - }; - promptHandler(loadingFinishedTrigger, requestMetadata); return { type: RpcResponseTypes.SendTransactionResponse, response, } as RpcResponse; } catch (err) { throw new SendTransactionError(err instanceof Error ? err.message : 'An unknown error occurred while sending the transaction'); + } finally { + const loadingFinishedTrigger: SendTransactionLoadingFinishedTrigger = { + type: TriggerTypes.SendTransactionLoadingFinishedTrigger, + }; + promptHandler(loadingFinishedTrigger, requestMetadata); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/src/rpcMethods/sendTransaction.ts` around lines 189 - 218, The SendTransactionLoadingFinishedTrigger is not emitted on errors because the promptHandler call for loadingFinishedTrigger is inside the try block; move the creation/call of loadingFinishedTrigger and promptHandler into a finally block so promptHandler(loadingFinishedTrigger, requestMetadata) always runs even if sendTransactionObject.signTx or sendTransactionObject.runFromMining throws; keep the current SendTransactionError rethrow logic (wrap the thrown error message) and ensure you reference the existing loadingTrigger, loadingFinishedTrigger, promptHandler, sendTransactionObject.signTx, and sendTransactionObject.runFromMining symbols when making this change.
♻️ Duplicate comments (2)
packages/hathor-rpc-handler/__tests__/rpcMethods/sendTransaction.test.ts (1)
60-69:⚠️ Potential issue | 🟠 MajorAdd one non-zero FBT fee assertion test to validate the feature objective.
Current assertions only validate
networkFee: 0n. The PR goal is FBT network fee return behavior, so a non-zero FBT fee-header scenario should be explicitly tested.💡 Suggested test addition
+ it('should include non-zero networkFee for FBT fee header entries', async () => { + const fbtUid = 'fbt-token-uid'; + mockTransaction.tokens = [fbtUid]; + mockTransaction.getFeeHeader = jest.fn().mockReturnValue({ + entries: [{ tokenIndex: 0, amount: 15n }], + }); + + promptHandler + .mockResolvedValueOnce({ + type: TriggerResponseTypes.SendTransactionConfirmationResponse, + data: { accepted: true }, + }) + .mockResolvedValueOnce({ + type: TriggerResponseTypes.PinRequestResponse, + data: { accepted: true, pinCode: '1234' }, + }); + + sendTransactionMock.mockResolvedValue({ hash: 'txHash123' }); + + await sendTransaction(rpcRequest, wallet, {}, promptHandler); + + expect(promptHandler).toHaveBeenNthCalledWith( + 1, + expect.objectContaining({ + data: expect.objectContaining({ + networkFee: 15n, + }), + }), + {} + ); + });Also applies to: 91-134
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/__tests__/rpcMethods/sendTransaction.test.ts` around lines 60 - 69, The test currently only asserts a zero FBT network fee; add a new assertion case in sendTransaction.test.ts that sets mockTransaction.getFeeHeader to return a non-zero fee (e.g., entries: [{ tokenIndex: 0, amount: 123n }]) on the mockTransaction object and assert that the RPC response (from the sendTransaction invocation in the test) contains networkFee equal to that non-zero BigInt; ensure the mockTransaction used in the test has tokens/fee token alignment so getFeeHeader's tokenIndex matches the expected fee token and add an explicit expect(response.networkFee).toBe(123n) (or similar) to validate non-zero FBT handling.packages/hathor-rpc-handler/src/rpcMethods/sendTransaction.ts (1)
150-156:⚠️ Potential issue | 🟠 MajorDon’t aggregate fee entries from different fee tokens into a single
networkFee.Line 153 currently collapses all
feeHeader.entriesamounts into one bigint. If entries have differenttokenIndexvalues, this mixes units and can display a misleading fee in the confirmation prompt.In `@hathor/wallet-lib` FeeHeader, can one transaction contain entries with different tokenIndex values, and does tokenIndex map directly to tx.tokens[]?💡 Suggested fix (guard single-token representation)
- const feeHeaderAmount = feeHeader - ? feeHeader.entries.reduce((sum, entry) => sum + entry.amount, 0n) - : 0n; + const feeByTokenIndex = feeHeader?.entries.reduce((acc, entry) => { + acc.set(entry.tokenIndex, (acc.get(entry.tokenIndex) ?? 0n) + entry.amount); + return acc; + }, new Map<number, bigint>()) ?? new Map<number, bigint>(); + + if (feeByTokenIndex.size > 1) { + throw new PrepareSendTransactionError( + 'Cannot represent multi-token fee as a single networkFee value.' + ); + } + + const feeHeaderAmount = feeByTokenIndex.values().next().value ?? 0n;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/src/rpcMethods/sendTransaction.ts` around lines 150 - 156, The current code sums all feeHeader.entries amounts into one networkFee, which mixes different token units; change logic in the block using preparedTx.getFeeHeader(), feeHeader.entries and networkFee so you either (a) validate that all feeHeader.entries share the same tokenIndex and only then compute a single bigint feeHeaderAmount + tokensUtils.getDataFee(...), or (b) when multiple tokenIndex values exist, build a per-token fee map (keyed by tokenIndex) and surface a per-token fee breakdown (or return/error) instead of aggregating; reference feeHeader, feeHeader.entries, tokenIndex, networkFee, tokensUtils.getDataFee and params.outputs to locate and update the calculation and the confirmation/response path accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/hathor-rpc-handler/__tests__/rpcMethods/sendTransaction.test.ts`:
- Line 31: The test declares let mockTransaction: any which breaks the
no-explicit-any rule; replace it by declaring a concrete type for
mockTransaction (either a TestTransaction interface or an existing Transaction
type) that includes the properties used in the test (e.g., inputs, outputs,
hash, serialize or methods referenced) and update the declaration in
sendTransaction.test.ts so mockTransaction: TestTransaction (or the exact type
name) is used instead of any; if no existing type fits, add a minimal local type
alias or interface above the tests capturing the mock shape and reference that
in the mockTransaction declaration.
---
Outside diff comments:
In `@packages/hathor-rpc-handler/src/rpcMethods/sendTransaction.ts`:
- Around line 189-218: The SendTransactionLoadingFinishedTrigger is not emitted
on errors because the promptHandler call for loadingFinishedTrigger is inside
the try block; move the creation/call of loadingFinishedTrigger and
promptHandler into a finally block so promptHandler(loadingFinishedTrigger,
requestMetadata) always runs even if sendTransactionObject.signTx or
sendTransactionObject.runFromMining throws; keep the current
SendTransactionError rethrow logic (wrap the thrown error message) and ensure
you reference the existing loadingTrigger, loadingFinishedTrigger,
promptHandler, sendTransactionObject.signTx, and
sendTransactionObject.runFromMining symbols when making this change.
---
Duplicate comments:
In `@packages/hathor-rpc-handler/__tests__/rpcMethods/sendTransaction.test.ts`:
- Around line 60-69: The test currently only asserts a zero FBT network fee; add
a new assertion case in sendTransaction.test.ts that sets
mockTransaction.getFeeHeader to return a non-zero fee (e.g., entries: [{
tokenIndex: 0, amount: 123n }]) on the mockTransaction object and assert that
the RPC response (from the sendTransaction invocation in the test) contains
networkFee equal to that non-zero BigInt; ensure the mockTransaction used in the
test has tokens/fee token alignment so getFeeHeader's tokenIndex matches the
expected fee token and add an explicit expect(response.networkFee).toBe(123n)
(or similar) to validate non-zero FBT handling.
In `@packages/hathor-rpc-handler/src/rpcMethods/sendTransaction.ts`:
- Around line 150-156: The current code sums all feeHeader.entries amounts into
one networkFee, which mixes different token units; change logic in the block
using preparedTx.getFeeHeader(), feeHeader.entries and networkFee so you either
(a) validate that all feeHeader.entries share the same tokenIndex and only then
compute a single bigint feeHeaderAmount + tokensUtils.getDataFee(...), or (b)
when multiple tokenIndex values exist, build a per-token fee map (keyed by
tokenIndex) and surface a per-token fee breakdown (or return/error) instead of
aggregating; reference feeHeader, feeHeader.entries, tokenIndex, networkFee,
tokensUtils.getDataFee and params.outputs to locate and update the calculation
and the confirmation/response path accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c8418ae4-1c8e-4cab-8747-523730ffdc15
📒 Files selected for processing (3)
packages/hathor-rpc-handler/__tests__/rpcMethods/sendTransaction.test.tspackages/hathor-rpc-handler/src/rpcMethods/sendTransaction.tspackages/hathor-rpc-handler/src/types/prompt.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/hathor-rpc-handler/src/types/prompt.ts
Member
Author
|
@coderabbitai about your comment outside the diff: If signTx/runFromMining throws, the error propagates up to the client, which catches it and can dismiss the loading state itself. The finally would be redundant since the client already handles errors. |
|
Tip For best results, initiate chat on the files or code changes.
(´• ω •`) 🐇 ✏️ Learnings added
|
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/hathor-rpc-handler/__tests__/rpcMethods/sendTransaction.test.ts (2)
489-517: Make the FBT fee test use a real non-native output token.Line 489 names this as an FBT scenario, but it still runs with the default request output token (
'00'). This can pass even if non-native token handling regresses. Set a non-native UID inrpcRequest.params.outputsand assert token details are fetched/surfaced in the first prompt.Suggested test tightening
it('should calculate non-zero networkFee when FBT fee header is present', async () => { + rpcRequest.params.outputs = [{ + address: 'testAddress', + value: '100', + token: 'token-uid-123', + }]; + const fbtMockTransaction = { inputs: [{ hash: 'testTxId', index: 0 }], outputs: [{ value: BigInt(100), tokenData: 0, script: p2pkhScript }], tokens: ['token-uid-123'], @@ await sendTransaction(rpcRequest, wallet, {}, promptHandler); + expect(wallet.getTokenDetails).toHaveBeenCalledWith('token-uid-123'); expect(promptHandler).toHaveBeenNthCalledWith(1, expect.objectContaining({ type: TriggerTypes.SendTransactionConfirmationPrompt, data: expect.objectContaining({ networkFee: 500n, + tokenDetails: expect.any(Map), }), }), {}, ); });Also applies to: 519-527
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/__tests__/rpcMethods/sendTransaction.test.ts` around lines 489 - 517, The test currently labeled as an FBT scenario uses the default/native output token, so update the test to set a non-native token UID in rpcRequest.params.outputs (e.g., a real non-native token UID) and ensure the mocked fbt transaction (fbtMockTransaction) reflects that token in tokens/outputs; then assert that the first prompt from promptHandler includes the token details (use TriggerResponseTypes.SendTransactionConfirmationResponse and check prompt data contains the non-native token UID or label) while keeping the same mocks for wallet.sendManyOutputsSendTransaction, prepareTx, signTx, and sendTransaction call to sendTransaction(rpcRequest, wallet, {}, promptHandler).
60-69: Extract a prepared-transaction factory to reduce mock drift.The prepared transaction mock shape is duplicated in several places; a small factory will make future flow updates safer.
Also applies to: 402-409, 454-461, 490-498
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/__tests__/rpcMethods/sendTransaction.test.ts` around lines 60 - 69, The test duplicates the same prepared-transaction mock in multiple places (mockTransaction in sendTransaction.test.ts) causing mock drift; extract a single factory function (e.g., createPreparedTransactionMock or buildPreparedTxMock) that returns the common shape (inputs, outputs with BigInt value, tokens, getFeeHeader mock, toHex mock) and replace each inline mockTransaction/prepareTx mock with calls to this factory (update usages in the blocks around the existing mocks noted and at the other duplicated ranges). Ensure the factory returns fresh objects per call so tests don’t share mutated state and keep the same property names used by tests (inputs, outputs, tokens, getFeeHeader, toHex).packages/hathor-rpc-handler/src/rpcMethods/sendTransaction.ts (1)
123-127: The double-cast works around an undocumented wallet-lib API; error handling exists but assertion would improve clarity.The
sendManyOutputsSendTransactionmethod is not in the wallet-lib v2.14.0 documented API (as noted in the TODO at lines 43–44). The double-cast at line 123 is intentional, and the subsequent try-catch block at lines 132–141 catches any shape mismatches and converts them toPrepareSendTransactionError. IfprepareTxis missing, the error is already handled with a meaningful message rather than propagating a cryptic "not a function" error.A runtime assertion as shown in the original suggestion would provide earlier failure detection and clearer logging during development, but isn't required given the existing error-handling layer.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/hathor-rpc-handler/src/rpcMethods/sendTransaction.ts` around lines 123 - 127, The code double-casts the return of wallet.sendManyOutputsSendTransaction to ISendTransactionObject which masks an undocumented API and can produce an unclear runtime "not a function" if the shape is wrong; add a runtime assertion right after the call to verify the returned object has the expected properties (e.g., is non-null and includes prepareTx as a function and any other required fields from ISendTransactionObject) and throw a clear, descriptive error if the check fails so failures are detected earlier and logged clearly; reference the wallet.sendManyOutputsSendTransaction call and ISendTransactionObject (and prepareTx) when adding this assertion.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/hathor-rpc-handler/__tests__/rpcMethods/sendTransaction.test.ts`:
- Around line 489-517: The test currently labeled as an FBT scenario uses the
default/native output token, so update the test to set a non-native token UID in
rpcRequest.params.outputs (e.g., a real non-native token UID) and ensure the
mocked fbt transaction (fbtMockTransaction) reflects that token in
tokens/outputs; then assert that the first prompt from promptHandler includes
the token details (use TriggerResponseTypes.SendTransactionConfirmationResponse
and check prompt data contains the non-native token UID or label) while keeping
the same mocks for wallet.sendManyOutputsSendTransaction, prepareTx, signTx, and
sendTransaction call to sendTransaction(rpcRequest, wallet, {}, promptHandler).
- Around line 60-69: The test duplicates the same prepared-transaction mock in
multiple places (mockTransaction in sendTransaction.test.ts) causing mock drift;
extract a single factory function (e.g., createPreparedTransactionMock or
buildPreparedTxMock) that returns the common shape (inputs, outputs with BigInt
value, tokens, getFeeHeader mock, toHex mock) and replace each inline
mockTransaction/prepareTx mock with calls to this factory (update usages in the
blocks around the existing mocks noted and at the other duplicated ranges).
Ensure the factory returns fresh objects per call so tests don’t share mutated
state and keep the same property names used by tests (inputs, outputs, tokens,
getFeeHeader, toHex).
In `@packages/hathor-rpc-handler/src/rpcMethods/sendTransaction.ts`:
- Around line 123-127: The code double-casts the return of
wallet.sendManyOutputsSendTransaction to ISendTransactionObject which masks an
undocumented API and can produce an unclear runtime "not a function" if the
shape is wrong; add a runtime assertion right after the call to verify the
returned object has the expected properties (e.g., is non-null and includes
prepareTx as a function and any other required fields from
ISendTransactionObject) and throw a clear, descriptive error if the check fails
so failures are detected earlier and logged clearly; reference the
wallet.sendManyOutputsSendTransaction call and ISendTransactionObject (and
prepareTx) when adding this assertion.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fbddddda-4ad3-4402-a0e1-780b605a260d
📒 Files selected for processing (2)
packages/hathor-rpc-handler/__tests__/rpcMethods/sendTransaction.test.tspackages/hathor-rpc-handler/src/rpcMethods/sendTransaction.ts
Comment on lines
+144
to
+147
| const tokenUids = params.outputs | ||
| .filter((output): output is { token: string } & typeof output => 'token' in output && typeof output.token === 'string') | ||
| .map(output => output.token) | ||
| .filter(uid => uid !== constants.NATIVE_TOKEN_UID); |
Collaborator
There was a problem hiding this comment.
Can't you also filter for output.token !== constants.NATIVE_TOKEN_UID so you don't have to loop again?
raul-oliveira
previously approved these changes
Mar 4, 2026
github-project-automation
bot
moved this from In Progress (Done)
to In Review (WIP)
in Hathor Network
andreabadesso
previously approved these changes
Mar 6, 2026
andreabadesso
approved these changes
Mar 6, 2026
raul-oliveira
approved these changes
Mar 6, 2026
github-project-automation
bot
moved this from In Review (WIP)
to Waiting to be deployed
in Hathor Network
| export type SendTransactionConfirmationPrompt = BaseConfirmationPrompt & { | ||
| type: TriggerTypes.SendTransactionConfirmationPrompt; | ||
| data: { | ||
| outputs: IDataOutput[], |
3 tasks
3 tasks
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
NOTE: Missing new wallet lib version
Checklist
master, confirm this code is production-ready and can be included in future releases as soon as it gets mergedSummary by CodeRabbit
New Features
Improvements