Skip to content

docs: Fullnode Typings [12] - SendTransaction#1014

Closed
tuliomir wants to merge 4 commits intomasterfrom
docs/ts-sendTransaction
Closed

docs: Fullnode Typings [12] - SendTransaction#1014
tuliomir wants to merge 4 commits intomasterfrom
docs/ts-sendTransaction

Conversation

@tuliomir
Copy link
Copy Markdown
Contributor

@tuliomir tuliomir commented Feb 9, 2026

Depends on #1013

Acceptance Criteria

  • Fixes a minor typing issue on SendTransaction, with an ignored mandatory parameter

Security Checklist

  • Make sure you do not include new dependencies in the project unless strictly necessary and do not include dev-dependencies as production ones. More dependencies increase the possibility of one of them being hijacked and affecting us.

@tuliomir tuliomir self-assigned this Feb 9, 2026
@tuliomir tuliomir moved this from Todo to In Progress (WIP) in Hathor Network Feb 9, 2026
@codecov
Copy link
Copy Markdown

codecov bot commented Feb 9, 2026

Codecov Report

❌ Patch coverage is 0% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 67.39%. Comparing base (63c4928) to head (b266952).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/new/sendTransaction.ts 0.00% 1 Missing ⚠️

❌ Your patch status has failed because the patch coverage (0.00%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage.
❌ Your project status has failed because the head coverage (67.39%) is below the target coverage (75.00%). You can increase the head coverage or adjust the target coverage.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1014   +/-   ##
=======================================
  Coverage   67.39%   67.39%           
=======================================
  Files         114      114           
  Lines        8644     8644           
  Branches     1933     1933           
=======================================
  Hits         5826     5826           
  Misses       2790     2790           
  Partials       28       28           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@tuliomir tuliomir moved this from In Progress (WIP) to In Progress (Done) in Hathor Network Feb 9, 2026
await addCreatedTokenFromTx(transaction as CreateTokenTransaction, storage);
// Add new transaction to the wallet's storage.
wallet.enqueueOnNewTx({ history: historyTx });
wallet.enqueueOnNewTx({ type: '', history: historyTx }); // FIXME: Add a type here
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why? This function does not require a type
The type it expexts is { history: IHistoryTx[]} (IHistoryTx).

The handleWebsocketMsg requires a type, but not the enqueueOnNewTx

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This may be our desired end result, but as of now handleWebsocketMsg calls enqueueOnNewTx passing the entire websocket object ( see #1013 , which adds explicit typing for this part of the code ).

Changing the type here would require a refactor, which is outside my current scope. The "Fixme" and empty string are meant to reinforce that this needs to be improved in a future refactor.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should set a type here just like what I commented here which begs the question, this change is being made on both PRs, why not let it be part of #1013 since it becomes required only there?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Accepted this suggestion and closed this PR.


export interface ISendTokenOutput {
type: OutputType.P2PKH | OutputType.P2SH;
type: OutputType.P2PKH | OutputType.P2SH; // XXX: This type is ignored in the only place it is used
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The idea is that the ISendOutput is a combination of the ISendDataOutput and ISendTokenOutput which can be checked through the type field.

Originally (before moving to typescript) the type field was only present on the data output which is why we don't use the others.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's your point here? I didn't follow

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, this was an "information only" comment, not trying to request any changes

@tuliomir tuliomir moved this from In Progress (Done) to In Review (WIP) in Hathor Network Feb 14, 2026
@tuliomir tuliomir moved this from In Review (WIP) to Todo in Hathor Network Feb 23, 2026
@tuliomir tuliomir moved this from Todo to In Progress (Done) in Hathor Network Feb 23, 2026
@tuliomir tuliomir moved this from In Progress (Done) to In Review (WIP) in Hathor Network Feb 23, 2026
@tuliomir
Copy link
Copy Markdown
Contributor Author

Accepted @r4mmer suggestion and incorporated this change into #1013

@tuliomir tuliomir closed this Feb 25, 2026
@github-project-automation github-project-automation bot moved this from In Review (WIP) to Waiting to be deployed in Hathor Network Feb 25, 2026
@tuliomir tuliomir deleted the docs/ts-sendTransaction branch February 25, 2026 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants