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

support estimate gas for blob tx #299

Conversation

zhiqiangxu
Copy link
Contributor

@zhiqiangxu zhiqiangxu commented Apr 15, 2024

Changes for TransactionArgs are pulled from this pr.

Currently when batcher estimates gas, it doesn't pass parameters for blobs.

I'm going to create a pr for that after this one is merged since it refers to the CallMsg type from op-geth.

@zhiqiangxu zhiqiangxu requested a review from a team as a code owner April 15, 2024 15:40
@zhiqiangxu zhiqiangxu requested review from ajsutton and removed request for a team April 15, 2024 15:40
@ajsutton
Copy link
Contributor

Is there a reason we need to port this individually instead of just waiting for the next upstream release to be merged in (there's work to get us up to date happening now)?

@zhiqiangxu
Copy link
Contributor Author

zhiqiangxu commented Apr 15, 2024

Is there a reason we need to port this individually instead of just waiting for the next upstream release to be merged in (there's work to get us up to date happening now)?

Currently the batcher is already sending blob tx to L1, but when it estimates the gas, it's not estimating the exact CallMsg, it's not a problem when the batch inbox is an EOA, but it may become a problem when the batch inbox is a contract(i.e, for customized deployment of OP Stack).

The exact CallMsg is like this.

@ajsutton
Copy link
Contributor

The batch inbox must not be a contract (it should be an arbitrary address with no code and no key so its completely unowned). So it sounds like this isn't an issue and we can just pull the fix in with the other upstream changes.

@zhiqiangxu
Copy link
Contributor Author

The batch inbox must not be a contract (it should be an arbitrary address with no code and no key so its completely unowned). So it sounds like this isn't an issue and we can just pull the fix in with the other upstream changes.

When integrating OP Stack with EthStorage(a long term DA solution), the batch inbox is a contract, here are more details.

@tynes
Copy link
Contributor

tynes commented Apr 16, 2024

@zhiqiangxu Appreciate the changes but there are no tests around them. This is adding extra work for us, based on what @ajsutton is saying these exact changes are in the upstream geth release and we will get to them as part of pulling in the latest geth changes. We do this on a regular basis

@zhiqiangxu
Copy link
Contributor Author

Never mind, we'll host the changes downstream then:)

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