adapt to eip-1193 provider changes

[kanthesha]

After SafeEventEmitterProvider is updated to support EIP-1193 and a new version of @metamask/eth-json-rpc-provider is released, we should adapt to the changes:

We should bump @metamask/eth-json-rpc-provider to rely on the new changes.
At that point, calling sendAsync will be deprecated; we should use request instead.

Fixes #231

[mcmire]

Do we want to keep the log statements and error handling? We could use wrapError from @metamask/utils, which automatically preserves the entirety of original error (something that the previous code was not doing — it was just preserving the message but not the stack):

Suggested change

	return this._provider.request(req);
	try {
	const result = this._provider.request(req);
	log('Got result', result);
	return result;
	} catch (error) {
	throw wrapError(error, 'PollingBlockTracker - encountered error fetching block');
	}

[kanthesha]

Sure, we can retain the log statement.
And I think you mean with await?

const result = await this._provider.request(req);

And regarding the error handling, we didn't had a try catch error handling, but we were handling in case of an error object in the response object. With the suggested change, almost 50 plus unit tests are going to break! As earlier we didn't had this and managed in calling function.

[mcmire]

Yes, I see what you mean. I guess since request now automatically throws if the response has an error, we do not need to throw an error ourselves.

I think the error we were throwing was useful from a debugging perspective, though, so what are your thoughts on replacing it with a log statement, at least, then? Perhaps something like this (also, you can supply type parameters to request instead of having to typecast; and getErrorMessage again comes from @metamask/utils):

Suggested change

	return this._provider.request(req);
	try {
	const result = await this._provider.request<[], string>(req);
	log('Got result', result);
	return result;
	} catch (error) {
	log('Encountered error fetching block', getErrorMessage(error));
	throw error;
	}

[kanthesha]

Sure, make sense.
Pushed the changes.

[mcmire]

The way I used the word "response" was to refer to a response object. We don't have that anymore, because request more or less hides the response object. We have either a value that we want request to return (which comes from the result of the response object), or we have an error that we want it to throw.

Maybe we should name this result instead of response so that if someone doesn't know exactly what this is doing they at least won't be misled into thinking they're overriding the response object?

[socket-security]

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package	New capabilities	Transitives	Size	Publisher
npm/@metamask/eth-json-rpc-provider@4.1.0	None	+3	376 kB	metamaskbot

🚮 Removed packages: npm/@metamask/eth-json-rpc-provider@4.0.0

View full report↗︎

[cryptodev-2s]

LGTM!

[legobeat]

This change is causing failure downstream (releases as non-breaking in 10.1.0: MetaMask/eth-json-rpc-filters#176 (comment)

[legobeat]

• Regression in v10.1.0 #258