-
Notifications
You must be signed in to change notification settings - Fork 363
Conversation
CLA Assistant Lite All Contributors have signed the CLA. |
ESLint Summary View Full Report
Report generated by eslint-plus-action |
E2E Tests Failed Failed tests:
|
@iamacook could you expand a bit on the patch? Is it from a forum or you debugged it yourself? |
No patch is now used. |
Wouldn't make sense to upgrade bnc-onboard to latest version first? v1.37.3 https://github.com/blocknative/onboard/releases There were quite lot of changes, even WalletConnect upgrade from v1.6.2 to 1.7.1 including several fixes |
I'll look into this. |
They haven't updated their WalletConnect module since October. Seems like there's the same issue, even on the latest version of Onboard:
I've only had success by patching out the @dasanra, what are your thoughts? If you have some spare time, I'd love to look at it together. |
I've upgraded |
-Web3ProviderEngine.prototype.sendAsync = function(payload, cb){ | ||
+Web3ProviderEngine.prototype.sendAsync = function (payload, cb) { | ||
+ if (payload.method === 'eth_getBalance') { | ||
+ return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did you test it when transacting? IIRC in some methods, the web3.js calls getBalance
under the hood, and it can break something
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing it out now across different wallets
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice find. It causes issues with the estimation. I've decided to write a modified wallet module instead for WalletConnect that has no balance subscription.
After researching this and trying different approaches, it seems to come from the balance In an effort to avoid patching code that may break, I've decided to opt for a custom module that doesn't include a balance |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👏 👏 👏
What it solves
Unnecessary Infura requests
How this PR fixes it
Main investigation of our high-polled Infura requests showed that they are coming from our provider watcher (
eth_chainId
), Onboard (eth_getBalance
) and WalletConnect (eth_blockNumber
/eth_getBlockByNumber
).eth_blockNumber
eth_getBalance
eth_getBlockByNumber
eth_chainId
The base branch of this (the provider watcher cleanup) removed the 2 second interval polling for
ethChainId
andethAccounts
.This removes the
eth_blockNumber
/eth_getBlockByNumber
over-polling by reducing theblockPollingInterval
of WalletConnect (via a custom WC module). This does not seem to have an adverse affect on WC, as it is apparently a remnant from legacyweb3
configuration.The overpolling of
eth_getBalance
stems from an issue somewhere in the depency tree of the balancestateSyncer
in the Onboard's WC module. I'The modified WC module also has no balancestateSyncer
. We can't retrieve the balance via Onboard, but this is never used in the Safe.Links
How to test it
Connect to MM via Onboard:
eth_blockNumber
/eth_getBlockByNumber
- there should be no continuous polling per 4s for theseeth_chainId
- there should be no continuous polling per 2s for thisConnect to WC via Onboard:
eth_getBalance
- there should be no continuous polling per second for this