-
Notifications
You must be signed in to change notification settings - Fork 3
feat: broadcast input addresses #917
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.
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
Conversation
services/chronikService.ts
Outdated
| console.log(`${this.CHRONIK_MSG_PREFIX}: [${msg.msgType}] ${msg.txid}`) | ||
| const transaction = await this.chronik.tx(msg.txid) | ||
| const addressesWithTransactions = await this.getAddressesForTransaction(transaction) | ||
| const inputAddresses = transaction.inputs.map(inp => outputScriptToAddress(this.networkSlug, inp.outputScript)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we sort this by amount contributed by each address?
Also, if 2 or more inputs use the same address, does this already consolidate them to only show them in the addressInput list once?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This deserves some tests.
ws-service/types.ts
Outdated
| timestamp: number | ||
| address: string | ||
| rawMessage: string | ||
| inputAddresses?: Array<string | undefined> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't get this typing (here and in the other parts of this PR where this same type appears): Array<string | undefined> . The function you created, getInputAddresses, returns string[], so why this undefined?
services/chronikService.ts
Outdated
| } | ||
| } | ||
|
|
||
| private getInputAddresses (transaction: Tx_InNode): string[] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe call this getSortedInputAddresses since is does that
services/chronikService.ts
Outdated
| } | ||
| for (const transaction of blockTxsToSync) { | ||
| const addressesWithTransactions = await this.getAddressesForTransaction(transaction) | ||
| const inputAddresses = transaction.inputs.map(inp => outputScriptToAddress(this.networkSlug, inp.outputScript)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why not use the function you created here? Why sorted input addresses elsewhere but not here?
|
Tested and it works as expected (checked a few edge cases just to be sure) but I'll wait to approve until @chedieck has done so. |
|
Conflicts. |
db5c50c to
9c0c7df
Compare
|
resolved |
Related to #916
Description
Send input addresses in broadcast incoming tx data, this will be used in paybutton client, this will make input addresses available on client callbacks
Test plan