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

Add both unused gas penalty and a no-op transaction subtype to the Native AA EIP #5

Merged
merged 2 commits into from
Nov 14, 2023

Conversation

forshtat
Copy link

@forshtat forshtat commented Nov 1, 2023

When opening a pull request to submit a new EIP, please use the suggested template: https://github.com/ethereum/EIPs/blob/master/eip-template.md

We have a GitHub bot that automatically merges some PRs. It will merge yours immediately if certain criteria are met:

  • The PR edits only existing draft PRs.
  • The build passes.
  • Your GitHub username or email address is listed in the 'author' header of all affected PRs, inside .
  • If matching on email address, the email address is the one publicly listed on your GitHub profile.

EIPS/eip-00.md Outdated
@@ -96,6 +97,28 @@ If `paymasterData` is specified, its first 20 bytes contain the address of a `pa

If `deployerData` is specified, its first 20 bytes contain the address of a `deployer` contract.

### No-op transaction subtype

Choose a reason for hiding this comment

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

why is it better than prefix+counter ?
a prefix+counter can be extended to have an aggregator
So a prefix-counter (with empty aggregator) is what we need now, and when we fully define aggregation, we add prefix-counter-aggregator.

The important thing is use the same mechanism now, instead of changing mechanism when we introduce aggregator.

Copy link
Author

Choose a reason for hiding this comment

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

In aggregation there is some execution and state change in the header transaction, but here the header would be just as much of a no-op transaction, just a required one and before the batch instead of after it.

Copy link

Choose a reason for hiding this comment

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

When adding support for aggregation, we'd need a lot more than just prefix+counter+aggregator. We also have state changes in this transaction. So I'm not sure how close these mechanisms are. I think that a no-op and a prefix+counter is very similar right now.

Choose a reason for hiding this comment

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

currently, we can implement either prefix+counter, or suffix. There is no real reason to implement one or the other.
Looking forward, if we ever add aggregation, we will have to have prefix+counter (+aggregator).
So if we now add prefix+counter, then we have it "for free" when we do aggregation.
if we add now suffix, then it will have to be changed (or worse, add both mechanisms) when we add aggregation.

So I'm saying we need some good argument to add a new "suffix" transaction type now.

Copy link

Choose a reason for hiding this comment

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

I think it makes sense to use prefix+counter in a way that can be later extended for aggregation, since we already know we'll need that counter then. Although the NOP below is compelling since it's really minimal.

Another question: if we add a prefix+counter should it be mandatory, or can we make it optional and revert to the current algorithm if we encounter type4 transactions without the prefix? This prefix will only be needed when concatenating separate batches, or in aggregation.

Copy link
Author

Choose a reason for hiding this comment

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

Another question: if we add a prefix+counter should it be mandatory, or can we make it optional and revert to the current algorithm if we encounter type4 transactions without the prefix? This prefix will only be needed when concatenating separate batches, or in aggregation.

We can make it optional so in most cases it won't even be used, I think. A small bit of "over-engineering" but also makes it cleaner for the most common use-case.

Copy link
Author

Choose a reason for hiding this comment

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

@yoavw @drortirosh @shahafn
I switched from no-op to header transaction for this PR, please re-review!

@forshtat forshtat marked this pull request as ready for review November 14, 2023 15:44
@forshtat forshtat merged commit 2a28809 into native_account_abstraction Nov 14, 2023
8 of 14 checks passed
@forshtat forshtat deleted the unused_gas_penalty_separator_tx branch November 14, 2023 15:44
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.

4 participants