Improve RequestManager send method

[ryanio]

Description

This PR improves RequestManager.send in a few ways:

1. Only sends { method, params } to provider.request, instead of whole jsonrpc shape, to be more conformant with spec.
2. Simplifies the code by extracting the jsonrpc result callback to its own method on RequestManager.

I also removed the bind I had in callbackify, after reading more about callbackify and bind I determined it is not needed in this case. If anyone thinks otherwise I would very much appreciate some reasoning.

Type of change

• Refactoring

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.
• I have made corresponding changes to the documentation.
• My changes generate no new warnings.
• Any dependent changes have been merged and published in downstream modules.
• I ran npm run dtslint with success and extended the tests and types if necessary.
• I ran npm run test:unit with success.
• I ran npm run test:cov and my test cases cover all the lines and branches of the added code.
• I ran npm run build and tested dist/web3.min.js in a browser.
• 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.

[coveralls]

Coverage increased (+0.02%) to 85.343% when pulling 951d0bc on improve-reqmanager-send into 545052a on 1.x.

[GregTheGreek]

LGTM! @ryanio just need to update the changelog.

But this is very interesting... im wondering what the impact might be on #3656 and related issues. I'm wondering if previously there may have been a weird collision with how jsonRPC id was being incremented and other providers being used. What im really curious to know is if the bind:

callbackify(this.provider.request.bind(this.provider))(payload, callback);

was causing the provider to miss increment because of setting _this in the websocket provider might pull the wrong instance of the provider?

anyway g2g and I'll monitor this in the future.

[frankiebee]

looks good to me 🤙

[sapjax]

@ryanio @GregTheGreek
This PR removed the bind with callbackify from:

callbackify(this.provider.request.bind(this.provider))(payload, callback);

to:

callbackRequest(requestArgs, callback);

I can confirm this change caused some problems.
Consider a wallet provider implement window.ethereum like:

//....
EthereumProvider.prototype.request = (payload) => {
    return this._eip1193Send(payload.method, payload.params)
}
window.ethereum = new EthereumProvider()

Now, web3.js call RequestManager.prototype.send, the callbackify changed the this context of provider.request method.
this in this._eip1193Send will be Window Object and can't found _eip1193Send method.

[GregTheGreek]

@frankiebee Hmmm We should add a way to test this out, maybe we can add some "buttons" to the netflify preview to allow us to test out that type of functionality?

[sebsobseb]

👋 I can confirm the issue @sapjax is explaining.

I'm currently trying to use WalletConnect as web3 provider, but when i try to call web3.eth.getAccounts() for example, it throws the following error:

When i check with the debugger, this is referring to the Window object:

This is how my code +- looks:

import WalletConnectProvider from "@walletconnect/web3-provider";
import Web3 from "web3";

const provider = new WalletConnectProvider({ infuraId, chainId });

// Enable session (triggers QR Code modal)
await provider.enable(); 

//  Create Web3
web3 = new Web3(provider);

// Get accouts
await vm.web3.eth.getAccounts(); // => throws the error!

Would appreciate the fix!