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.

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

Implement EIP 7002 (withdrawal requests) #3385

Merged
merged 12 commits into from
May 1, 2024
Merged

Conversation

Base automatically changed from eip-7685 to master April 30, 2024 18:12

// Note: generatedBlock is now overridden with the new generated block (this is thus block number 3)
// Ensure there are 2 requests
assert.ok(generatedBlock!.requests!.length === 2)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a fantastic test. I didn't even think about being able catch the generated block from teh afterBlock event.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, actually I want to raise a point about this, isn't the entire point of generate to actually retrieve the generated block (i.e. build the block?). I was somewhat surprised that this block was not returned from runBlock so had to figure out if I would be able to extract it (if that was even possible). So this is something we could take a look at in the future (not part of this PR)

acolytec3
acolytec3 previously approved these changes May 1, 2024
Copy link
Contributor

@acolytec3 acolytec3 left a comment

Choose a reason for hiding this comment

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

Going to approve because this looks fantastic. Nits can be disregarded if not applicable.

@jochem-brouwer
Copy link
Member Author

jochem-brouwer commented May 1, 2024

Just pushed a commit for the final cleanups, thanks for the review @acolytec3. If you could re-approve (after CI passes) then from my side this is OK to merge 😄

@jochem-brouwer jochem-brouwer merged commit 36cd606 into master May 1, 2024
33 of 34 checks passed
@holgerd77 holgerd77 deleted the eip-7002-new branch May 2, 2024 08:24
Copy link
Member

@holgerd77 holgerd77 left a comment

Choose a reason for hiding this comment

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

Totally agree with Andrew, this looks fantastic! 🤩

Some comments on docs, but totally and solely on the "would make this even better" level, since I think think this is already so close on the self-readability level that we could even jump over "the last hurdle to perfect" (lol). 😝

One side bonus question:

Would it make sense to also add one test to trigger this behavioral part of the withdrawal request contract? Then we would have all the functionality from the contract "covered"?

grafik

Or is this over the top since this is normally only called on the user side?

(would nevertheless be a somewhat nice bonus feature - especially in these early stages - that we would then test the withdrawal request contract itself (to some extend).)

bigIntToBytes(vm.common.param('vm', 'withdrawalRequestPredeployAddress')),
20
)
const withdrawalsAddress = Address.fromString(bytesToHex(addressBytes))
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 name this a bit more verbose, I would suggest withdrawalRequestContractAddress?

For withdrawalsAddress imagination can go in various - and sometimes wrong - directions what this could mean (address for sending the withdrawals towards e.g.).

caller: systemAddress,
gasLimit: BigInt(1_000_000),
to: withdrawalsAddress,
})
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 document the single steps a bit more, so that the whole code gets a bit easier reproduceable/understandable if one is not so deep into the EIP (any more)? 🙂

So this call basically triggers this part from the EIP, right? (respectively, in more practical terms: trigger withdrawal request contract code execution doing these things as described)

grafik

So it would be nice if we have some summary docs here like (to be freely adopted):

// Perform a system call to the withdrawal request contract. This triggers the contract to dequeue withdrawal requests from the queue, update excess withdrawal requests, reset the counter and then return the respective requests from the queue.
``

(Is this an accurate high level description?)

bigIntToBytes(common.param('vm', 'withdrawalRequestPredeployAddress')),
20
)
const withdrawalsAddress = Address.fromString(bytesToHex(addressBytes))
Copy link
Member

Choose a reason for hiding this comment

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

Same here: rename would be nice.

Copy link
Contributor

@g11tech g11tech left a comment

Choose a reason for hiding this comment

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

lgtm (post merge review)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants