Update nim-eth types #6583
Update nim-eth types #6583
Conversation
Minimal changes neede for compatiblity with status-im/nim-eth#733 which aligns core types with execution spec.
There was a problem hiding this comment.
ethereum/execution-specs does not pass full ethereum/execution-spec-tests at this stage (constantinople doesn't work, and 7702 support only got added recently). ethereum/execution-spec-tests is generated with ethereumjs. Still agreeing with the overall change though, sharing identifiers that are closer to "specs" (where specs exist -- devp2p specs are not even reflecting dencun changes yet).
| var res {.noinit.}: array[20, byte] | ||
| res[0 ..< 20] = keccakHash(rlp.encodeList(fromAddress, tx.nonce)) | ||
| .data.toOpenArray(12, 31) | ||
| res | ||
| let hash = keccakHash(rlp.encodeList(fromAddress, tx.nonce)) | ||
| hash.to(EthAddress) |
There was a problem hiding this comment.
This one's a bit tricky, keccak returns 32 bytes and to suggests simple data type conversion. The idea here is to drop the prefix and only keep the last 20 bytes.
There was a problem hiding this comment.
It "kind of" fits with the idea of a canonical conversion from Hash to Address - in the spec, it's even defined as dropping the first 12 bytes supporting the to idea and being well-defined.
There was a problem hiding this comment.
I guess it could be named something else, if you happen to have a good name handy :)
| maxFeePerBlobGas: tx.maxFeePerBlobGas, | ||
| blobVersionedHashes: tx.versionedHashes, | ||
| blobVersionedHashes: tx.versionedHashes.mapIt(Eth2Digest(data: it.data)), |
There was a problem hiding this comment.
isn't this asEth2Digest? (if we want to align type names maybe Eth2Digest -> Root could be another one)
There was a problem hiding this comment.
most of this will go away in the next set of pr:s which will reuse the new nim-eth types in nim-web3, getting rid of most of the conversion friction - this is just a "make it work" step.
| @@ -604,7 +610,7 @@ proc blockToBlockHeader*(blck: ForkyBeaconBlock): ExecutionBlockHeader = | |||
| requestsRoot : requestsRoot) # EIP-7685 | |||
|
|
|||
| proc compute_execution_block_hash*(blck: ForkyBeaconBlock): Eth2Digest = | |||
| rlpHash blockToBlockHeader(blck) | |||
| rlpHash(blockToBlockHeader(blck)).to(Eth2Digest) | |||
There was a problem hiding this comment.
what's the difference between Eth2Digest and Root?
There was a problem hiding this comment.
Root == Hash32 == distinct array - Eth2Digest == MDigest[256] == object
it's a rather pointless alias in the spec
| Address = primitives.Address | ||
|
|
There was a problem hiding this comment.
This one is not exported, and it worked before without it, is it necessary?
There was a problem hiding this comment.
nim-eth adds an Address symbol - this local type has precedence over everything imported so it sets a preference between primitives and nim-eth for the rest of the file
| @@ -35,11 +35,13 @@ logScope: | |||
| topics = "elman" | |||
|
|
|||
| type | |||
| FixedBytes[N: static int] = web3.FixedBytes[N] | |||
There was a problem hiding this comment.
| FixedBytes[N: static int] = web3.FixedBytes[N] | |
| FixedBytes[N: static int] = web3.FixedBytes[N] |
this one is also not exported and it worked without this definition before.
| mixHash: data.mixHash.asEth2Digest, | ||
| nonce: distinctBase(data.nonce.get), | ||
| mixHash: data.mixHash.asEth2Digest.to(Hash32), | ||
| nonce: distinctBase(data.nonce.get).to(Bytes8), |
There was a problem hiding this comment.
shouldn't nonce be a number type? is it still getting serialized properly with this change? as in, as Quantity and not as a byte array, on engine?
There was a problem hiding this comment.
There was a problem hiding this comment.
is that compatible with engine API?
There was a problem hiding this comment.
(or, json-rpc, probably the better one, as nonce is not used in engine itself)
There was a problem hiding this comment.
https://ethereum.org/en/developers/docs/apis/json-rpc/#eth_getblockbyhash says it's bytes8, but the reality seems to encode it as quantity:
curl https://docs-demo.quiknode.pro/ \
-X POST \
-H "Content-Type: application/json" \
--data '{"method":"eth_getBlockByNumber","params":["0xc5043f",true],"id":1,"jsonrpc":"2.0"}' | jq '.'e.g. here nonce is "0x1d" not a bytes8
{
"blockHash": "0xa917fcc721a5465a484e9be17cda0cc5493933dd3bc70c9adbee192cb419c9d7",
"blockNumber": "0xc5043f",
"from": "0xb0cba44805f096094e8759ee9824a05590c10f27",
"gas": "0x5208",
"gasPrice": "0x7f084b500",
"hash": "0x3dc2776aa483c0eee09c2ccc654bf81dccebead40e9bb664289637bfb5e7e954",
"input": "0x",
"nonce": "0x1d",
"to": "0xb0cba44805f096094e8759ee9824a05590c10f27",
"transactionIndex": "0xcb",
"value": "0x3f3b45b9331",
"type": "0x0",
"chainId": "0x1",
"v": "0x26",
"r": "0x19610db3a46538e10c4db8597510902374ff66ef339a2251c82faebb3c00f8b9",
"s": "0x7af4c008f4bc1edbe081c8e32f49893f035a9eeacb615dd67808f6a9f5e13b52"
}the specs don't seem authoritative. https://github.com/ethereum/devp2p/blob/master/caps/eth.md here the blocks don't even have excess blob fees or parent beacn blob roots, as per spec, in reality they have.
or also, there's ethereum/execution-apis#573 which is implemented in practice but is not specced out like that.
There was a problem hiding this comment.
BytesX generally also encodes as 0x... in the new nim-eth - but that doesn't matter, we're dealing with an eth type here, not web3 - this is internal to the execution client, not a web3 impl
There was a problem hiding this comment.
ExecutionPayload of web3 is not using nonce from eth block header. And the example posted by @etan-status is not a block header, but a transaction. And a transaction's nonce is a uint64. There will be no conversion friction or serialization issue in the case of this block header nonce. And there is no problem also with the json encoding nonce of block header so far in our json-rpc.
arnetheduck
force-pushed
the
wip-types
branch
from
4d78360 to
6a6a375
Compare
Minimal changes neede for compatiblity with
status-im/nim-eth#733 which aligns core types with execution spec.