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.

Sign up for GitHub

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

Kick ethers off the block #2633

Merged
merged 11 commits into from
Apr 14, 2023
Merged

Kick ethers off the block #2633

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
9c32c76
Make `block.fromEthersProvider` provider agnostic
acolytec3 Apr 12, 2023
650fb3f
Finish block
acolytec3 Apr 12, 2023
d177332
Move provider utils to util
acolytec3 Apr 13, 2023
a21a116
Remove ethers from tx
acolytec3 Apr 13, 2023
d99e6b1
skip fetch test in browser context
acolytec3 Apr 13, 2023
0b45f5d
Fix tests
acolytec3 Apr 13, 2023
5032c1d
Fix td typing
acolytec3 Apr 13, 2023
65d63d9
more td typing
acolytec3 Apr 13, 2023
1dc1fcf
Switch fetch dep to micro-ftch
acolytec3 Apr 13, 2023
8bd0129
Add error handling
acolytec3 Apr 13, 2023
4dce10c
Merge remote-tracking branch 'origin/master' into kickethersofftheblock
acolytec3 Apr 13, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
85 changes: 60 additions & 25 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

6 changes: 3 additions & 3 deletions packages/block/package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,12 +43,12 @@
"@ethereumjs/trie": "^5.0.4",
"@ethereumjs/tx": "^4.1.1",
"@ethereumjs/util": "^8.0.5",
"ethereum-cryptography": "^1.1.2",
"ethers": "^5.7.1"
"ethereum-cryptography": "^1.1.2"
},
"devDependencies": {
"@types/lru-cache": "^5.1.0",
"c-kzg": "^1.0.8"
"c-kzg": "^1.0.8",
"testdouble": "^3.17.2"
},
"engines": {
"node": ">=14"
Expand Down
36 changes: 25 additions & 11 deletions packages/block/src/block.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,13 @@ import {
bigIntToHex,
bufArrToArr,
bufferToHex,
fetchFromProvider,
getProvider,
intToHex,
isHexPrefixed,
ssz,
} from '@ethereumjs/util'
import { keccak256 } from 'ethereum-cryptography/keccak'
import { ethers } from 'ethers'

import { blockFromRpc } from './from-rpc'
import { BlockHeader } from './header'
Expand Down Expand Up @@ -230,37 +231,50 @@ export class Block {
* @returns the block specified by `blockTag`
*/
public static fromEthersProvider = async (
provider: ethers.providers.JsonRpcProvider | string,
provider: any,
blockTag: string | bigint,
opts: BlockOptions
) => {
let blockData
const prov =
typeof provider === 'string' ? new ethers.providers.JsonRpcProvider(provider) : provider
const providerUrl = getProvider(provider)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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.


if (typeof blockTag === 'string' && blockTag.length === 66) {
blockData = await prov.send('eth_getBlockByHash', [blockTag, true])
blockData = await fetchFromProvider(providerUrl, {
method: 'eth_getBlockByHash',
params: [blockTag, true],
})
} else if (typeof blockTag === 'bigint') {
blockData = await prov.send('eth_getBlockByNumber', [bigIntToHex(blockTag), true])
blockData = await fetchFromProvider(providerUrl, {
method: 'eth_getBlockByNumber',
params: [bigIntToHex(blockTag), true],
})
} else if (
isHexPrefixed(blockTag) ||
blockTag === 'latest' ||
blockTag === 'earliest' ||
blockTag === 'pending'
) {
blockData = await prov.send('eth_getBlockByNumber', [blockTag, true])
blockData = await fetchFromProvider(providerUrl, {
method: 'eth_getBlockByNumber',
params: [blockTag, true],
})
} else {
throw new Error(
`expected blockTag to be block hash, bigint, hex prefixed string, or earliest/latest/pending; got ${blockTag}`
)
}

if (blockData === null) {
throw new Error('No block data returned from provider')
}

const uncleHeaders = []
if (blockData.uncles.length > 0) {
for (let x = 0; x < blockData.uncles.length; x++) {
const headerData = await prov.send('eth_getUncleByBlockHashAndIndex', [
blockData.hash,
intToHex(x),
])
const headerData = await fetchFromProvider(providerUrl, {
method: 'eth_getUncleByBlockHashAndIndex',
params: [blockData.hash, intToHex(x)],
})
uncleHeaders.push(headerData)
}
}
Expand Down
30 changes: 29 additions & 1 deletion packages/block/test/from-rpc.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
import { Chain, Common, Hardfork } from '@ethereumjs/common'
import { randomBytes } from 'crypto'
import * as tape from 'tape'
import * as td from 'testdouble'

import { blockFromRpc } from '../src/from-rpc'
import { blockHeaderFromRpc } from '../src/header-from-rpc'
Expand Down Expand Up @@ -185,13 +187,39 @@ tape('[fromRPC] - Alchemy/Infura API block responses', (t) => {

tape('[fromEthersProvider]', async (t) => {
const common = new Common({ chain: Chain.Mainnet, hardfork: Hardfork.London })
const provider = new MockProvider()
const provider = new MockProvider() // mimics some properties of an Ethers JsonRPCProvider

const fakeFetch = async (_url: string, req: any) => {
if (
req.method === 'eth_getBlockByHash' &&
req.params[0] === '0x1850b014065b23d804ecf71a8a4691d076ca87c2e6fb8fe81ee20a4d8e884c24'
) {
const block = await import(`./testdata/infura15571241wtxns.json`)
return block
} else {
return null // Infura returns null if no block is found
}
}

const providerUtils = require('@ethereumjs/util/dist/provider')
Copy link
Member

Choose a reason for hiding this comment

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

Does this needs to be required in such a strange way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

td.replace<any>(providerUtils, 'fetchFromProvider', fakeFetch)

const blockHash = '0x1850b014065b23d804ecf71a8a4691d076ca87c2e6fb8fe81ee20a4d8e884c24'
const block = await Block.fromEthersProvider(provider, blockHash, { common })
t.equal(
'0x' + block.hash().toString('hex'),
blockHash,
'assembled a block from blockdata from a provider'
)
try {
await Block.fromEthersProvider(provider, '0x' + randomBytes(32).toString('hex'), {})
t.fail('should throw')
} catch (err: any) {
t.ok(
err.message.includes('No block data returned from provider'),
'returned correct error message'
)
}
td.reset()
t.end()
})
20 changes: 3 additions & 17 deletions packages/block/test/mockProvider.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,19 +1,5 @@
import { ethers } from 'ethers'

export class MockProvider extends ethers.providers.JsonRpcProvider {
send = async (method: string, params: Array<any>) => {
switch (method) {
case 'eth_getBlockByHash':
return this.getBlockValues(params as any)
case 'eth_chainId': // Always pretends to be mainnet
return 1
default:
throw new Error(`method ${method} not implemented`)
}
}

private getBlockValues = async (_params: any) => {
const block = await import(`./testdata/infura15571241wtxns.json`)
return block
export class MockProvider {
connection = {
url: 'http://localhost',
}
}
4 changes: 2 additions & 2 deletions packages/client/test/client.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ tape('[EthereumClient]', async (t) => {
FullEthereumService.prototype.open = td.func<any>()
FullEthereumService.prototype.start = td.func<any>()
FullEthereumService.prototype.stop = td.func<any>()
td.replace('../lib/service', { FullEthereumService })
td.replace<any>('../lib/service', { FullEthereumService })
td.when(FullEthereumService.prototype.open()).thenResolve()
td.when(FullEthereumService.prototype.start()).thenResolve()
td.when(FullEthereumService.prototype.stop()).thenResolve()
Expand All @@ -31,7 +31,7 @@ tape('[EthereumClient]', async (t) => {
Server.prototype.start = td.func<any>()
Server.prototype.stop = td.func<any>()
Server.prototype.bootstrap = td.func<any>()
td.replace('../lib/net/server/server', { Server })
td.replace<any>('../lib/net/server/server', { Server })
td.when(Server.prototype.start()).thenResolve()
td.when(Server.prototype.stop()).thenResolve()
td.when(Server.prototype.bootstrap()).thenResolve()
Expand Down
Loading