Skip to content
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

responseListener.listenForResponseFromTransaction #190

Merged
merged 2 commits into from
Nov 22, 2023

Conversation

KuphJr
Copy link
Contributor

@KuphJr KuphJr commented Oct 27, 2023

Use new responseListener.listenForResponseFromTransaction method from the latest Functions Toolkit release

spinner.start(
`Request ${requestId} has been initiated. Waiting for fulfillment from the Decentralized Oracle Network...\n`
`Request ${requestTxReceipt.events[2].args.id} has been initiated. Waiting for fulfillment from the Decentralized Oracle Network...\n`
Copy link
Collaborator

Choose a reason for hiding this comment

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

I changed other examples to print a transaction hash here and the request ID only after fulfillment. Maybe do the same here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there informational value in having both? something along the lines of:

RequestId ${reqId} is waiting for fulfillment from the Decentralized Oracle Network. Track this fulfillment by its Tx hash ${txHash}.

networks.js Outdated
@@ -101,7 +101,7 @@ const networks = {
accounts,
verifyApiKey: process.env.POLYGONSCAN_API_KEY || "UNSET",
chainId: 80001,
confirmations: DEFAULT_VERIFICATION_BLOCK_CONFIRMATIONS,
confirmations: DEFAULT_VERIFICATION_BLOCK_CONFIRMATIONS * 4,
Copy link
Collaborator

Choose a reason for hiding this comment

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

That looks a little weird. Does it actually hurt to keep it at DEFAULT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

do we want to have network specific defaults?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll just set this to 10 blocks. I've noticed that it can be a problem if we don't wait long enough before verifying contracts.

Copy link
Collaborator

Choose a reason for hiding this comment

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

10 blocks on Sepolia is 2 minutes, way too slow IMO

Copy link
Collaborator

@zeuslawyer zeuslawyer left a comment

Choose a reason for hiding this comment

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

LGTM

networks.js Outdated
@@ -101,7 +101,7 @@ const networks = {
accounts,
verifyApiKey: process.env.POLYGONSCAN_API_KEY || "UNSET",
chainId: 80001,
confirmations: DEFAULT_VERIFICATION_BLOCK_CONFIRMATIONS,
confirmations: DEFAULT_VERIFICATION_BLOCK_CONFIRMATIONS * 4,
Copy link
Collaborator

Choose a reason for hiding this comment

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

do we want to have network specific defaults?

spinner.start(
`Request ${requestId} has been initiated. Waiting for fulfillment from the Decentralized Oracle Network...\n`
`Request ${requestTxReceipt.events[2].args.id} has been initiated. Waiting for fulfillment from the Decentralized Oracle Network...\n`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there informational value in having both? something along the lines of:

RequestId ${reqId} is waiting for fulfillment from the Decentralized Oracle Network. Track this fulfillment by its Tx hash ${txHash}.

@KuphJr KuphJr requested a review from bolekk November 17, 2023 17:45
@KuphJr KuphJr merged commit dc3fba4 into main Nov 22, 2023
2 checks passed
@KuphJr KuphJr deleted the listenForResponseFromTx branch November 22, 2023 22:08
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.

4 participants