Skip to content
This repository has been archived by the owner on Dec 4, 2024. It is now read-only.

Eip 2929 #1748

Merged
merged 27 commits into from
Aug 18, 2023
Merged

Eip 2929 #1748

merged 27 commits into from
Aug 18, 2023

Conversation

rachit77
Copy link
Contributor

Description

EIP-2929 implementation for Edge
Reference: https://eips.ethereum.org/EIPS/eip-2929

Changes include

  • Bugfix (non-breaking change that solves an issue)
  • Hotfix (change that solves an urgent issue, and requires immediate attention)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)

Checklist

  • I have assigned this PR to myself
  • I have added at least 1 reviewer
  • I have added the relevant labels
  • I have updated the official documentation
  • I have added sufficient documentation in code

Testing

  • I have tested this code with the official test suite
  • I have tested this code manually

Manual tests

Manually verified the call stack for tests included

@rachit77 rachit77 added don't merge Please don't merge this functionality temporarily feature New update to Polygon Edge labels Jul 25, 2023
@rachit77 rachit77 self-assigned this Jul 25, 2023
chain/params.go Show resolved Hide resolved
state/runtime/precompiled/precompiled.go Outdated Show resolved Hide resolved
state/runtime/evm/instructions.go Outdated Show resolved Hide resolved
state/runtime/access_list.go Outdated Show resolved Hide resolved
state/runtime/evm/instructions.go Outdated Show resolved Hide resolved
@@ -437,6 +437,7 @@ func TestCreate(t *testing.T) {
}
}

// EIP2929: check some test return error when triggered from base
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could you add some unit tests that would test different gas consumptions per opcodes? I see that you have opted to test it out on a higher level of abstraction (namely on the EVM level), but I think it would be beneficial for us to have some "atomic" unit tests (namely the ones that would test gas consumption per specific op code, depending on which hard fork is enabled).
WDYT?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah that makes sense to me, I will add those unit test.

For these EVM level transactions I have also verified the cost of individual opcodes by printing call stack.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added uint test cases for some opcodes and will add more with eip-2930 implementation

state/runtime/evm/instructions.go Outdated Show resolved Hide resolved
state/runtime/precompiled/precompiled.go Outdated Show resolved Hide resolved
@rachit77 rachit77 changed the base branch from develop to feature/berlin August 15, 2023 11:02
@rachit77 rachit77 requested review from Stefan-Ethernal and goran-ethernal and removed request for vcastellm August 15, 2023 14:15
@rachit77 rachit77 merged commit 31ce331 into 0xPolygon:feature/berlin Aug 18, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Aug 18, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
don't merge Please don't merge this functionality temporarily feature New update to Polygon Edge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants