Implement EIP4895: Beacon Chain withdrawals #2353
Conversation
|
This would be ready for a first round of reviews (but still WIP). |
Codecov Report
Additional details and impacted files
Flags with carried forward coverage won't be shown. Click here to find out more. |
|
picking this up |
There was a problem hiding this comment.
@jochem-brouwer we can merge this, will start a separate PR to add the engine api methods and the respective handling
|
Congrats, your important contribution to this open-source project has earned you a GitPOAP! GitPOAP: 2022 EthereumJS Contributor: Head to gitpoap.io & connect your GitHub account to mint! Learn more about GitPOAPs here. |
|
That was a pretty quick merge for such a big and extensive PR here. 🤔 Let's please keep this a bit "under review" and under observation for some time post-merge, eventually we can also add some additional tests. This touches a lot of code parts of production libraries like the Block library. |
* common: add eip 4895 * block: implement EIP4895 * vm: add EIP4895 * block: eip4895 tests * vm: add eip4895 tests * block: fix trest * vm: fix tests * change withdrawal type to object format and add validator index * fix vm withdrawal spec Co-authored-by: harkamal <gajinder@g11.in>
| @@ -33,6 +42,7 @@ export class Block { | |||
| public readonly header: BlockHeader | |||
| public readonly transactions: TypedTransaction[] = [] | |||
| public readonly uncleHeaders: BlockHeader[] = [] | |||
| public readonly withdrawals?: Withdrawal[] | |||
There was a problem hiding this comment.
Thinking about this for some time now: withdrawals have this type Withdrawal here with all these non-deterministic types like BigIntLike:
export type Withdrawal = {
index: BigIntLike
validatorIndex: BigIntLike
address: AddressLike
amount: BigIntLike
}While this always comes in handy for input data I wonder if this is an optimal design choice for the internal representation of this data. In the case of withdrawals, this Withdrawal type atm actually serves for both puroposes.
With this kind of design we just can't be sure what types these internal withdrawal parameters like index or address really have. My fear is that this will lead to a myriad of follow-up problems. For test cases, client usages,... one can never really count on what the type really is here and either cast here, make code comments, add complex procedural choice conditional switches for values on processing, or minimally: one always need to convert with withdrawalToBufferArray().
I would have a relatively strong tendency that we should switch here to deterministic values, I guess that will safe us from some substantial amount of follow-up trouble. This also aligns with our standard handling of internally represented data.
So I would suggest to actually have two types here, the one from above renamed to WithdrawalData or similar and used for withdrawal inputs.
And another one Withdrawal with deterministic types. Following our tendency to represent internal values not always with Buffer any more but with more "matching" types this would be something like the following I guess:
export type Withdrawal = {
index: BigInt
validatorIndex: BigInt
address: Address
amount: BigInt
}I know that this is some extra effort to rewrite now with various code already in place but I am relatively sure that this more than pay off in the future.
There was a problem hiding this comment.
Just discovered on the follow up PR from @g11tech.
At least we think in impressively similar lines. 🤩 😂
| tape('EIP4895 tests', (t) => { | ||
| t.test('EIP4895: withdrawals execute as expected', async (st) => { | ||
| const vm = await VM.create({ common }) | ||
| vm._common.setEIPs([4895]) |
There was a problem hiding this comment.
This should be set using theeips constructor option along Common initialization and not by injecting this afterwards to the semi-private _common object.
Implements EIP 4895
TODOs: