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

ethclient: add AuthList field in toCallArg #31198

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

colinlyguo
Copy link
Contributor

This PR adds the AuthList field in toCallArg to support eth_call and eth_estimateGas of SetCodeTx.

Some unit tests are added in this PR (containing uncovered cases e.g., ErrSetCodeTxCreate).

A typo is also fixed.

@fjl
Copy link
Contributor

fjl commented Feb 17, 2025

We have been discussing this. At the moment, it doesn't feel very useful since you need a signed authorization in order to invoke the call. It's not super convenient. And it's not really necessary to override an account with code via EIP-7702 in eth_call because it already supports arbitrary state overrides in Geth. And the new method eth_simulateV1 supports overrides in all clients.

@colinlyguo
Copy link
Contributor Author

colinlyguo commented Feb 17, 2025

We have been discussing this. At the moment, it doesn't feel very useful since you need a signed authorization in order to invoke the call. It's not super convenient. And it's not really necessary to override an account with code via EIP-7702 in eth_call because it already supports arbitrary state overrides in Geth. And the new method eth_simulateV1 supports overrides in all clients.

aha. I see and thanks. IMHO the main intention for this PR is not to overwrite accounts but to return an accurate gas estimation. e.g. bundlers may need to estimate gas (if the bundle transactions have some delegation submissions. I learned this from here. Though they might be written in other languages, too). In the meantime, it doesn't make it worse to support AuthList here, haven't thought of any side effects.

btw, do you think the added unit tests are useful here? might tweak some if duplicated.

@s1na s1na changed the title client: add AuthList field in toCallArg ethclient: add AuthList field in toCallArg Feb 18, 2025
// Should retrieve the code of 0xef0001 || accounts[0].addr and return an invalid opcode error.
{
blockNumber: rpc.LatestBlockNumber,
call: TransactionArgs{
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 mind-twisting test but I think I get it now :)

Authorization: account0 to itself
Execute: recipient is account0, it will fetch the delegate which is again itself (recursive). So it will return the raw delegation code (prefix+account0) and try to execute it which will fail.

Is that the intention?

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

Successfully merging this pull request may close these issues.

3 participants