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/gethclient: testcase for createAccessList, make tabledriven #30194

Merged
merged 4 commits into from
Nov 8, 2024

Conversation

SangIlMo
Copy link
Contributor

Add Testcase for createAccessList when user requested gasPrice is less than baseFee

In test, the baseFee for first block is 875000000. So, I just input the gasPrice '1'.

This PR is derived from this issue( #30145 ), so more specific information plz check this issue.

Value: big.NewInt(1),
}
al, gas, vmErr, err = ec.CreateAccessList(context.Background(), msg)
if errors.Is(err, core.ErrBlobFeeCapTooLow) {
Copy link
Member

Choose a reason for hiding this comment

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

I don't get it, you are waiting for a specific error here, right?
Why are you checking for BlobFeeCap too low, which has nothing to do with the basefeecap?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right. I just fix this code. (ErrFeeCapTooLow)

One thing is the err at this code, cannot be unwrapped because it was from RPC API response format. So I choose strings.Contains to get the error contains the ErrFeeCapTooLow.

Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

This method (testAccessList) is already borderline too bloated. Instead of copy-pasting another round of tests, perhaps it can be reformulated into a table-driven test, so that we have a for-loop iterating over the tests, and in the loop body, does the invocation and checks the post-conditions.

@holiman holiman force-pushed the add-testcase-for-createAccessList branch from ed9e06b to 3262033 Compare November 8, 2024 08:24
Copy link
Contributor

@holiman holiman left a comment

Choose a reason for hiding this comment

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

Fixed, LGTM now

@holiman holiman changed the title ethclient/gethclient/gethclient_test.go Add testcase for createAccessList (if gasPrice < baseFee) ethclient/gethclient: testcase for createAccessList, make tabledriven Nov 8, 2024
@holiman holiman added this to the 1.14.12 milestone Nov 8, 2024
@holiman holiman merged commit 8e00f95 into ethereum:master Nov 8, 2024
3 checks passed
holiman added a commit that referenced this pull request Nov 19, 2024
…#30194)

Adds testcase for createAccessList when user requested gasPrice is less than baseFee, also makes the tests tabledriven
---------

Co-authored-by: Martin Holst Swende <[email protected]>
zfy0701 pushed a commit to sentioxyz/go-ethereum that referenced this pull request Dec 3, 2024
…ethereum#30194)

Adds testcase for createAccessList when user requested gasPrice is less than baseFee, also makes the tests tabledriven
---------

Co-authored-by: Martin Holst Swende <[email protected]>
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