Kick ethers off the block
#2633
Conversation
Codecov Report
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
acolytec3
added
package: devp2p
package: client
package: util
package: statemanager
and removed
PR state: WIP
labels
| } | ||
| } | ||
|
|
||
| const providerUtils = require('@ethereumjs/util/dist/provider') |
There was a problem hiding this comment.
Does this needs to be required in such a strange way?
There was a problem hiding this comment.
I have to get a concrete reference to the function I want to stub using testdouble and this is the only way I could come up with to do it. I spent several hours trying to stub the fetch calls directly without success when I was using cross-fetch as the backend. I might be able to do more direct stubbing now that we're using micro-ftch so will see if I can make it work correctly.
| blockTag: string | bigint, | ||
| opts: BlockOptions | ||
| ) => { | ||
| let blockData | ||
| const prov = | ||
| typeof provider === 'string' ? new ethers.providers.JsonRpcProvider(provider) : provider | ||
| const providerUrl = getProvider(provider) |
There was a problem hiding this comment.
so the provider is essentially being stripped out. not sure if this is the right approach as the provider might be providing some wrapper functionality (like proof seeking and verification)
May be we can later revisit this on how to best interface a provider
There was a problem hiding this comment.
Where would a provider do proof verification? We could put cleanly define the provider interface (at least for the minimal functions we'd need to call) I supposed but that feels like more work than it's worth and very brittle if ethers or web3js (if we support that provider type too in the future) ever change their API. At the end of the day, we're just making an HTTP call to some remote RPC provider to get the json blob back for the blob or the tx. But, that said, happy to discuss further. We need to add a todo on the breaking release to rename the fromEthersProvider functions anyway.
There was a problem hiding this comment.
Can we eventually test for the Ethers import to work (in a try/catch block) and only do this stripping out as a fallback solution? Then we might somewhat have best of both worlds.
There was a problem hiding this comment.
I guess we could but all we were doing before was calling the provider.send method which is just abstracting a raw JSON RPC call. Here again, we weren't using any provider-specific functionality, just a simple wrapper around HTTP requests. As such, I'm not sure what we gain since we'd have to blindly call the provider and hope nothing in the API has changed. I don't think what I've done in this PR is any worse than that.
There was a problem hiding this comment.
Ok, good reasoning, then let's merge here.
I would now plan a round of "normal" releases for sometime mid next week, will prepare release notes next Monday or so.
| const LesProtocol = td.constructor([] as any) | ||
| td.replace('../../lib/net/protocol/lesprotocol', { LesProtocol }) | ||
| td.replace<any>('../../lib/net/protocol/lesprotocol', { LesProtocol }) |
There was a problem hiding this comment.
just wondering why do we have to now explicitly type it to any?
There was a problem hiding this comment.
Testdouble did a minor version upgrade that got picked up when I added it to tx and block as a devDependency and their td.replace function now explicitly returns an object of type unknown instead any if you don't specify the type to return. I can back this version bump out and pin to the previous version if people are concerned about it.
Removes
ethersfromblockandtxlibraries as a dependency and replacefromEthersProviderinternals withfetchcalls to retrieve data from providerfetchFromProviderutility inutilfor retrieving data from a generic JSON RPC providerethersfrom bothtxandblocktd.replacetyping inclient/devp2p