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

Issue#56 response listener hangs #67

Merged
merged 3 commits into from
Aug 14, 2024
Merged

Conversation

zeuslawyer
Copy link
Collaborator

Suggested Fix for Issue: #56

Makes changes to code introduced in PR #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

However:

  • rather than reducing default confirmations to 1, we could just document LocalFunctionsTestnet more expressly.
  • on the other hand devX will be improved by making default confirmations 1 as it will work out the box.

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
@zeuslawyer zeuslawyer requested a review from KuphJr as a code owner August 9, 2024 12:30
Copy link
Contributor

@KuphJr KuphJr left a comment

Choose a reason for hiding this comment

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

Looks good!
However, you will need to generate a changeset for this. All you need to do is run yarn changeset and follow the instructions. This will be a simple patch release.
Then you can commit the generated changeset file. Once a PR with a changeset is merged to main, it will trigger a new PR to be opened which will publish the release when it is merged.

@zeuslawyer zeuslawyer requested review from KuphJr August 13, 2024 08:10
@cl-sonarqube-production
Copy link

Quality Gate failed Quality Gate failed

Failed conditions
63.6% Coverage on New Code (required ≥ 75%)

See analysis details on SonarQube

@zeuslawyer zeuslawyer merged commit 641f3ec into main Aug 14, 2024
5 of 6 checks passed
@zeuslawyer zeuslawyer deleted the issue#56-response-listener-hangs branch August 14, 2024 00:26
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.

2 participants