Skip to content

Conversation

@lissavxo
Copy link
Collaborator

@lissavxo lissavxo commented Jul 23, 2025

Related to #519

Depends on

Description

Removed connection with paybutton-server websocket and added connection to chronik websocket directly.

Test plan

  • Make sure payment is received in both paybutton and widget

@Klakurka Klakurka requested review from Klakurka and chedieck July 23, 2025 19:23
@lissavxo lissavxo force-pushed the feat/connect-chronik-directly branch from c1f0983 to 049cf5a Compare July 23, 2025 19:23
@Klakurka Klakurka requested a review from Copilot July 23, 2025 19:23
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR removes the dependency on paybutton-server websocket and integrates direct connection to the Chronik websocket for receiving payment notifications. This change eliminates the need for an intermediary server and allows the application to receive transaction data directly from the blockchain indexer.

  • Adds Chronik client integration with websocket support for real-time transaction monitoring
  • Implements OP_RETURN data parsing functionality for payment metadata
  • Updates Widget and PayButton components to use the new Chronik websocket instead of the legacy socket.io connection

Reviewed Changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 7 comments.

Show a summary per file
File Description
react/package.json Adds chronik-client-cashtokens, decimal.js, and ecashaddrjs dependencies
react/lib/util/types.ts Extends Transaction interface with opReturn field
react/lib/util/socket.ts Adds Chronik websocket setup and message handling functions
react/lib/util/opReturn.ts Implements OP_RETURN data parsing utilities
react/lib/util/chronik.ts New file with complete Chronik client integration and transaction processing
react/lib/components/Widget/WidgetContainer.tsx Updates to handle internal transaction state management
react/lib/components/Widget/Widget.tsx Replaces socket.io setup with Chronik websocket
react/lib/components/PayButton/PayButton.tsx Replaces socket.io setup with Chronik websocket

address,
timestamp: transaction.block !== undefined ? transaction.block.timestamp : transaction.timeFirstSeen,
confirmed: transaction.block !== undefined,
opReturn,
Copy link

Copilot AI Jul 23, 2025

Choose a reason for hiding this comment

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

The rawMessage field is being set from parsedOpReturn but this conflicts with line 155 where opReturn already contains the stringified JSON with rawMessage. This creates inconsistent data where the top-level opReturn contains serialized data but individual fields contain parsed values.

Suggested change
opReturn,
opReturn: JSON.stringify(parsedOpReturn), // Ensure opReturn matches parsedOpReturn

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

@chedieck chedieck Jul 23, 2025

Choose a reason for hiding this comment

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

very confusing message, referencing the wrong line, and bad suggestion overall, since the main purpose of sending the opReturn is to have access to it without any processing.

params.setTxsSocket(undefined);
}

const newChronikSocket = await initializeChronikWebsocket(params.address, (transactions: Transaction[]) => { onMessage(transactions, params.setNewTxs, params.setDialogOpen, params.checkSuccessInfo) });
Copy link

Copilot AI Jul 23, 2025

Choose a reason for hiding this comment

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

[nitpick] This line exceeds reasonable length and has complex nested function calls. Consider extracting the callback function to improve readability: const handleTransactions = (transactions: Transaction[]) => onMessage(transactions, params.setNewTxs, params.setDialogOpen, params.checkSuccessInfo);

Suggested change
const newChronikSocket = await initializeChronikWebsocket(params.address, (transactions: Transaction[]) => { onMessage(transactions, params.setNewTxs, params.setDialogOpen, params.checkSuccessInfo) });
const newChronikSocket = await initializeChronikWebsocket(params.address, handleTransactions(params.setNewTxs, params.setDialogOpen, params.checkSuccessInfo));

Copilot uses AI. Check for mistakes.
Copy link
Collaborator

Choose a reason for hiding this comment

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

just break some lines

Comment on lines 492 to 495
// if (thisTxsSocket !== undefined) {
//thisTxsSocket.disconnect();
//setThisTxsSocket(undefined);
// }
Copy link

Copilot AI Jul 23, 2025

Choose a reason for hiding this comment

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

Commented-out cleanup code should be removed or properly implemented. The cleanup logic for Chronik websockets may be needed to prevent memory leaks.

Suggested change
// if (thisTxsSocket !== undefined) {
//thisTxsSocket.disconnect();
//setThisTxsSocket(undefined);
// }
if (thisTxsSocket !== undefined) {
thisTxsSocket.disconnect();
setThisTxsSocket(undefined);
}

Copilot uses AI. Check for mistakes.
Comment on lines 224 to 227
// if (txsSocket !== undefined) {
// txsSocket.disconnect();
//setTxsSocket(undefined);
// }
Copy link

Copilot AI Jul 23, 2025

Choose a reason for hiding this comment

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

Commented-out cleanup code should be removed or properly implemented. The cleanup logic for Chronik websockets may be needed to prevent memory leaks.

Suggested change
// if (txsSocket !== undefined) {
// txsSocket.disconnect();
//setTxsSocket(undefined);
// }
if (txsSocket !== undefined) {
txsSocket.disconnect();
setTxsSocket(undefined);
}

Copilot uses AI. Check for mistakes.
txsSocket.disconnect();
setTxsSocket(undefined);
}
// if (txsSocket !== undefined) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you feel these might be necessary and don’t want to delete it completely, it would be good to leave a comment explaining your hesitation.

address: string,
onTransaction: Function
): Promise<WsEndpoint> => {
const chronik = new ChronikClient(['https://chronik.e.cash']);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should be in paybutton-config.json, not hardcoded.

params.setTxsSocket(undefined);
}

const newChronikSocket = await initializeChronikWebsocket(params.address, (transactions: Transaction[]) => { onMessage(transactions, params.setNewTxs, params.setDialogOpen, params.checkSuccessInfo) });
Copy link
Collaborator

Choose a reason for hiding this comment

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

just break some lines

@chedieck
Copy link
Collaborator

Also, failed to build:
image

Copy link
Member

@Klakurka Klakurka left a comment

Choose a reason for hiding this comment

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

Getting a build error:

image

@lissavxo lissavxo force-pushed the feat/connect-chronik-directly branch from ed59d82 to 4760cc2 Compare July 29, 2025 15:15
@lissavxo lissavxo requested review from Klakurka and chedieck July 29, 2025 20:39
Copy link
Member

@Klakurka Klakurka left a comment

Choose a reason for hiding this comment

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

Unable to build:

image

@lissavxo
Copy link
Collaborator Author

Unable to build:

image

need to add this param to your paybutton-config.json file

  "networkBlockchainURLs": {
    "ecash": [
      "https://xec.paybutton.io",
      "https://chronik1.alitayin.com",
      "https://chronik2.alitayin.com",
      "https://chronik.e.cash",
      "https://chronik-native1.fabien.cash",
      "https://chronik-native2.fabien.cash",
      "https://chronik-native3.fabien.cash",
      "https://chronik.pay2stay.com/xec",
      "https://chronik.pay2stay.com/xec2"
    ],
    "bitcoincash": ["https://bch.paybutton.io", "https://chronik.pay2stay.com/bch"]
  }

Copy link
Member

@Klakurka Klakurka left a comment

Choose a reason for hiding this comment

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

  • Try opening localhost:10001, opening the network tab, clearing the logs, clicking the 'Basic' paybutton to open the dialog. On the first opening, you'll see 3 web sockets open. Close the dialog and clear the network logs again. Open it again and you'll see no new web socket. Why 3 on the first 1? Why 0 on the 2nd one?
  • Build warnings:
image image

Comment on lines 7 to 17
"https://xec.paybutton.io",
"https://chronik1.alitayin.com",
"https://chronik2.alitayin.com",
"https://chronik.e.cash",
"https://chronik-native1.fabien.cash",
"https://chronik-native2.fabien.cash",
"https://chronik-native3.fabien.cash",
"https://chronik.pay2stay.com/xec",
"https://chronik.pay2stay.com/xec2"
],
"bitcoincash": ["https://bch.paybutton.io", "https://chronik.pay2stay.com/bch"]
Copy link
Member

Choose a reason for hiding this comment

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

let's use the .org URLs instead of the .io ones as access will eventually be restricted at some point in the future to only our server(s).

@lissavxo
Copy link
Collaborator Author

  • Try opening localhost:10001, opening the network tab, clearing the logs, clicking the 'Basic' paybutton to open the dialog. On the first opening, you'll see 3 web sockets open. Close the dialog and clear the network logs again. Open it again and you'll see no new web socket. Why 3 on the first 1? Why 0 on the 2nd one?
  • Build warnings:

image image

are you testing on the demo page? we open a new websocket instance for each widget might be the reason it is opening more than one. And we stick with the first connection open for a dialog, that is why it does not open again

@Klakurka
Copy link
Member

Yes - demo page.

AFAICT it is sending non-ws messages that trigger payment detection / onSuccess.

Can you check that?

@Klakurka Klakurka force-pushed the feat/connect-chronik-directly branch from 8fa8252 to ae27962 Compare July 29, 2025 22:31
@Klakurka
Copy link
Member

My main concern with this is that we're introducing the possibility of a user seeing onSuccess run but there still potentially be an issue if there is some kind of problem on the server preventing payment triggers from firing (eg. wp plugin.

@lissavxo lissavxo force-pushed the feat/connect-chronik-directly branch from 0b5b11b to 0859baa Compare August 18, 2025 18:13
@Klakurka
Copy link
Member

Klakurka commented Aug 22, 2025

Couple of things:

  • We need to get the .org websocket fixed before we can use that. @ScottMcDermid can have a crack at that and if it's too difficult then we can go with `.io for now and migrate over in a later release.
  • Let's explicitly set ConnectionStrategy to AsOrdered, with us at the top. As long as our Chronik instance is online, connecting to that should be reliably the fastest since Cashtab (by the time this has been merged) will broadcast to us right away upon detecting a pb tx.
  • I think op_return_raw isn't being sent to Cashtab (extension) when using the "Send with Cashtab" button.

@lissavxo lissavxo force-pushed the feat/connect-chronik-directly branch from 0859baa to 68a86bd Compare August 22, 2025 20:20
@chedieck chedieck self-requested a review August 27, 2025 13:16
Copy link
Collaborator

@chedieck chedieck left a comment

Choose a reason for hiding this comment

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

Just requesting the fix of a typo in a variable name.

Most of the code here is copy + pasted from paybutton-server, which is fine for now, but if we want to edit this code in the future we probably should consider actually separating it in a 3rd repo that we could import on both paybutton-server and paybutton, to avoid having to edit in both repos and risking have conflicting behavior between them.

address: string,
setNewTx: Function
): Promise<WsEndpoint> => {
const networSlug = getAddressPrefix(address)
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo here, should be networkSlug

@lissavxo lissavxo requested review from Klakurka and chedieck August 27, 2025 16:22
@Klakurka Klakurka merged commit 4ee67d9 into master Aug 28, 2025
2 checks passed
@Klakurka Klakurka linked an issue Aug 28, 2025 that may be closed by this pull request
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.

Connect directly to Chronik for payment detection

4 participants