Add ResponseListener.listenForResponseFromTransaction() #36

bolekk · 2023-10-19T02:03:50Z

No description provided.

chudilka1

@bolekk, would you be so kind as to address Sonar's issues? They fall under critical "code smells." If you believe they are irrelevant, please let me know, and we will provide a corresponding resolution for those...

bolekk · 2023-10-19T16:26:25Z

The PR is still in draft, I'll address everything once it's ready.

bolekk · 2023-10-19T22:22:52Z

@chudilka1 I fixed most issues except two:

Sonar tells me that "checkTimeout" should be const but it can't be const.
Prettier wants to add one semicolon and eslint wants to remove it so I can't satisfy lint.

What should I do?

justinkaseman · 2023-10-23T17:50:56Z

2. Prettier wants to add one semicolon and eslint wants to remove it so I can't satisfy lint.

What should I do?

It looks like this repo is missing a prettier <> eslint integration, which is how you make them play nicely together.
Docs here: https://prettier.io/docs/en/integrating-with-linters.html
Let me know if you want me to iron this out.

KuphJr

We should add a README entry for this method. Also, maybe we should probably update our README, docs & tutorials to make this the preferred method for listening for Functions responses given that listening based on requestId can break during reorgs.

KuphJr · 2023-10-23T17:33:25Z

src/ResponseListener.ts

+  ): Promise<FunctionsResponse> {
+    return new Promise<FunctionsResponse>((resolve, reject) => {
+      (async () => {
+        let requestID: string = ''


nit: you shouldn't need to specify a type here. Typescript can infer the type based on the initial empty string value.

KuphJr · 2023-10-23T17:34:30Z

src/ResponseListener.ts

+    return new Promise<FunctionsResponse>((resolve, reject) => {
+      (async () => {
+        let requestID: string = ''
+        // eslint-disable-next-line


What is being disabled here? Usually eslint-disble statementments disable a single warning:
example: // eslint-disable-next-line no-implicit-any (usually Copilot helps me do this automatically)

Lint claims this can be const but it can't, right? (I'm using it before setting)

Adding "@typescript-eslint/prefer-as-const" doesn't work

KuphJr · 2023-10-23T17:54:28Z

src/ResponseListener.ts

+          reject('Response not received within timeout period')
+        }, timeout)
+
+        const check = async () => {


Might this method lead to a potential race condition if the check interval is much higher than the block time?

User sends Functions request & calls listenForReponseFromTransaction which extracts the current requestId and listens for the response.

A reorg occurs and the reorged request is fulfilled before a check is called again.

Now we are listening for a response after the response has already occurred, meaning we won't ever actually hear it and this function hangs forever.

I think the logic is fine here as this only occurs with high checkInvervals, but maybe something to note in the README entry.

Yes. The old method has a similar problem - when we start listening after the response is emitted, we will miss it. It seems to be inherent to a functionsRouter.on() call. I'll add a note to README.

KuphJr · 2023-10-23T18:37:25Z

prettier.io/docs/en/integrating-with-linters.html

Thanks @justinkaseman! I'll take care of this now since I am the one who set up esLint to begin with.

chudilka1 · 2023-10-24T11:43:38Z

@chudilka1 I fixed most issues except two:

Sonar tells me that "checkTimeout" should be const but it can't be const.

Prettier wants to add one semicolon and eslint wants to remove it so I can't satisfy lint.

@bolekk

Is addressed from within Sonar (as won't fix with your comment)
Rebase your branch from main, prettier <> lint setup has been addressed by @KuphJr in this commit

cl-sonarqube-production · 2023-10-24T21:45:18Z

SonarQube Quality Gate

0 Bugs
0 Vulnerabilities
0 Security Hotspots
1 Code Smell

80.8% Coverage
0.0% Duplication

Makes changes to code introduced in #36. Things to note: 1. 2 confirmations is a sensible default but have made 1 to reduce friction for devs so it works out of the box with local functions testnet. 2. Alternative could be to check inside `listenForResponseFromTransaction()` for ChainID 1337 (local funcs testnet) and then change the default confirmations to 1. 3. DevX Improvement: . README updates and added inline docs to the function so that it shows on however in the IDE

bolekk requested review from pinebit, justinkaseman and KuphJr October 19, 2023 02:03

chudilka1 self-requested a review October 19, 2023 10:28

chudilka1 requested changes Oct 19, 2023

View reviewed changes

bolekk force-pushed the txwait branch 5 times, most recently from 7dbfc33 to 2880bdf Compare October 19, 2023 22:15

bolekk marked this pull request as ready for review October 19, 2023 22:21

KuphJr reviewed Oct 23, 2023

View reviewed changes

bolekk force-pushed the txwait branch from 2880bdf to adf06ae Compare October 24, 2023 19:59

Add ResponseListener.listenForResponseFromTransaction()

418d3d0

bolekk force-pushed the txwait branch from adf06ae to 418d3d0 Compare October 24, 2023 20:08

KuphJr and others added 3 commits October 24, 2023 16:21

Lint fix

f3052e4

changeset

6026203

Merge branch 'main' into txwait

74ce9c3

KuphJr merged commit 97e8dde into main Oct 24, 2023
6 checks passed

KuphJr deleted the txwait branch October 24, 2023 22:08

app-token-issuer-functions bot mentioned this pull request Oct 24, 2023

Version Packages #39

Merged

YoutacRandS-VA mentioned this pull request Jun 20, 2024

[Snyk] Upgrade @chainlink/functions-toolkit from 0.2.3 to 0.2.8 YoutacRandS-VA/functions-lens-eventbrite#3

Open

zeuslawyer mentioned this pull request Aug 9, 2024

Issue#56 response listener hangs #67

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ResponseListener.listenForResponseFromTransaction() #36

Add ResponseListener.listenForResponseFromTransaction() #36

bolekk commented Oct 19, 2023

chudilka1 left a comment

bolekk commented Oct 19, 2023

bolekk commented Oct 19, 2023

justinkaseman commented Oct 23, 2023

KuphJr left a comment

KuphJr Oct 23, 2023

KuphJr Oct 23, 2023

bolekk Oct 24, 2023

bolekk Oct 24, 2023

KuphJr Oct 23, 2023

bolekk Oct 24, 2023

KuphJr commented Oct 23, 2023

chudilka1 commented Oct 24, 2023 •

edited

Loading

cl-sonarqube-production bot commented Oct 24, 2023

Add ResponseListener.listenForResponseFromTransaction() #36

Add ResponseListener.listenForResponseFromTransaction() #36

Conversation

bolekk commented Oct 19, 2023

chudilka1 left a comment

Choose a reason for hiding this comment

bolekk commented Oct 19, 2023

bolekk commented Oct 19, 2023

justinkaseman commented Oct 23, 2023

KuphJr left a comment

Choose a reason for hiding this comment

KuphJr Oct 23, 2023

Choose a reason for hiding this comment

KuphJr Oct 23, 2023

Choose a reason for hiding this comment

bolekk Oct 24, 2023

Choose a reason for hiding this comment

bolekk Oct 24, 2023

Choose a reason for hiding this comment

KuphJr Oct 23, 2023

Choose a reason for hiding this comment

bolekk Oct 24, 2023

Choose a reason for hiding this comment

KuphJr commented Oct 23, 2023

chudilka1 commented Oct 24, 2023 • edited Loading

cl-sonarqube-production bot commented Oct 24, 2023

chudilka1 commented Oct 24, 2023 •

edited

Loading