Skip to content

docs: Fullnode Typings [12] - SendTransaction #1014

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
tuliomir wants to merge 4 commits into from
Closed

docs: Fullnode Typings [12] - SendTransaction #1014

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
09ff085
docs: types to sendTransaction
tuliomir Feb 9, 2026
be6d512
Merge branch 'master' into docs/ts-sendTransaction
tuliomir Feb 13, 2026
642fc64
Merge branch 'master' into docs/ts-sendTransaction
tuliomir Feb 19, 2026
b266952
Merge branch 'master' into docs/ts-sendTransaction
tuliomir Feb 23, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/new/sendTransaction.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
}

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

@r4mmer r4mmer Feb 12, 2026

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

@raul-oliveira raul-oliveira Feb 19, 2026

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

@r4mmer r4mmer Feb 23, 2026

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

address: string;
value: OutputValueType;
token: string;
Expand Down Expand Up @@ -506,7 +506,7 @@
// This just returns if the transaction is not a CREATE_TOKEN_TX
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

Check warning on line 509 in src/new/sendTransaction.ts

View check run for this annotation

Codecov / codecov/patch

src/new/sendTransaction.ts#L509 

Added line #L509 was not covered by tests
Copy link
Copy Markdown
Member

@r4mmer r4mmer Feb 12, 2026

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

@tuliomir tuliomir Feb 19, 2026

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

@r4mmer r4mmer Feb 23, 2026

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

@tuliomir tuliomir Feb 25, 2026

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.

})(this.wallet, this.storage, this.transaction);
}
this.emit('send-tx-success', this.transaction);
Expand Down
Loading