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

Not getting success callback/ receipt with walletconnect with web3.js 1.3.3 - Closes #3891 #4304

Merged
merged 12 commits into from
Sep 22, 2021

Conversation

nazarhussain
Copy link
Contributor

@nazarhussain nazarhussain commented Sep 9, 2021

Description

We were using an assumption that if any provider contains on handler, it must also support newBlockHeaders event. Which was not the case for all providers and was not enforced by EIP-1193. So for the flow of waiting for the receipt we reverted back only to polling technique.

Fixes: #3891

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist:

  • I have selected the correct base branch.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • My changes generate no new warnings.
  • I have tested my code on the live network.
  • I have checked the Deploy Preview and it looks correct.
  • I have updated the CHANGELOG.md file in the root folder.

@render
Copy link

render bot commented Sep 9, 2021

@nazarhussain
Copy link
Contributor Author

To come up with a solution which impact least on bandwidth usage and to providers which support newBlockHeaders I came up with this solution.

  1. If provider does not support on handler use polling anyway.
  2. If provider support on handler subscribe to newBlockHeaders event
  3. Check after blockHeaderTimeout default to 10s, if block header event received or not
  4. If not, that's probably implies provider does not support that event and we fallback to polling.

This approach will introduce no behaviour changes for providers which support newBlockHeaders events, and would introduce some wait time for others.

@jdevcs @spacesailor24 Let me know if that solutions seems feasible to you in reference to the comment posted on issue earlier #3891 (comment). If its ok to you guys then I will add few more tests as well. Also let me know if that change required to update some other places as well.

@coveralls
Copy link

coveralls commented Sep 14, 2021

Pull Request Test Coverage Report for Build 1262839551

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 40 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.05%) to 74.909%

Files with Coverage Reduction New Missed Lines %
packages/web3-core-method/lib/index.js 13 93.81%
packages/web3-eth/lib/index.js 13 90.25%
packages/web3-eth-contract/lib/index.js 14 92.05%
Totals Coverage Status
Change from base Build 1262436089: -0.05%
Covered Lines: 3216
Relevant Lines: 4057

💛 - Coveralls

Copy link
Contributor

@spacesailor24 spacesailor24 left a comment

Choose a reason for hiding this comment

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

Couple questions, but otherwise changes look good

Could use at least one other test if my test related question is correct, also needs CHANGELOG.md update

@nazarhussain
Copy link
Contributor Author

nazarhussain commented Sep 15, 2021

I had tested it with @aksdevac provided code repo by linking web3-eth and web3-core-method packages and it worked. For the wallet connect provider is that it wait for 5 seconds for newBlockHeaders event and since it does not return then it fallback to polling and fetch the tx receipt and return full-filled promise.

I'm just going to commit this since it's a comment change, and so I can provide my approval without it going stale
spacesailor24
spacesailor24 previously approved these changes Sep 15, 2021
Copy link
Contributor

@spacesailor24 spacesailor24 left a comment

Choose a reason for hiding this comment

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

Thank you for updating the docs!

@imaksp
Copy link

imaksp commented Sep 17, 2021

it seems many files are not using es6 & es7 features, is there any plan to update it.?

@nazarhussain
Copy link
Contributor Author

@aksdevac Yes for 1.x we are not refactoring the code a lot. As our focus is on the 4.x rewrite, which will cover best coding practices and Typescript language features.

@xale76
Copy link

xale76 commented Sep 19, 2021

please help. I don't understand how to let it work. No events received after send calls

spacesailor24
spacesailor24 previously approved these changes Sep 21, 2021
@nazarhussain
Copy link
Contributor Author

@xale76 Hey, Can you please explain your comment. Are you referring to this PR? or to the issue itself? And how did you subscribed to those events which you mentioned not receiving?

@xale76
Copy link

xale76 commented Sep 21, 2021

@nazarhussain hello, and thanks. I'm using web3modal together with walletconnectprovider. My react app works perfectly from browser and metamask extension (connected by infura node on rinkeby). If I use walletconnectprovider, so I use QR code and I confirm actions on metamask app on my android mobile, after the confirmation (in example an "approve" action), my code remains stuck in "await". In example

await mytoken.methods.approve(spender,amount).send({from:address});

I don't know what you mean with "subscribed" to events. I don't know, what events you're talking about, and I think it's not easy to list every events and register for them. Is there any other solution?

@nazarhussain
Copy link
Contributor Author

@xale76 This PR is about to fix that problem. Did you get the error even after compiling and linking this branch locally?

@xale76
Copy link

xale76 commented Sep 21, 2021

@nazarhussain how to install this branch from npm?

@nazarhussain nazarhussain merged commit 40974e9 into 1.x Sep 22, 2021
@nazarhussain nazarhussain deleted the nh/3891-receipt-callback branch September 22, 2021 17:51
@nazarhussain
Copy link
Contributor Author

@xale76 You can not install the branch from npm. But this issue is scheduled for next release soon it will be available for you to test.

@hoductrong
Copy link

Any update for this issues, or which release contains this fix?, plz

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.

Not getting success callback/ receipt with walletconnect with web3.js 1.3.3
7 participants