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.

Sign up for GitHub

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

Jump to bottom

Eth call with 4844 params #6661

Merged
merged 20 commits into from
Mar 5, 2024
Merged

Eth call with 4844 params #6661

merged 20 commits into from
Mar 5, 2024

Conversation

macfarla
Copy link
Contributor

@macfarla macfarla commented Mar 1, 2024 •
edited
Loading

Added support for maxFeePerBlobGas and versionedHashes to eth_call

Thanks for sending a pull request! Have you done the following?

  • Checked out our contribution guidelines?
  • Considered documentation and added the doc-change-required label to this PR if updates are required.
  • Considered the changelog and included an update if required.
  • For database changes (e.g. KeyValueSegmentIdentifier) considered compatibility and performed forwards and backwards compatibility tests

Most advanced CI tests are deferred until PR approval, but you could:

  • locally run all unit tests via: ./gradlew build
  • locally run all acceptance tests via: ./gradlew acceptanceTest
  • locally run all integration tests via: ./gradlew integrationTest
  • locally run all reference tests via: ./gradlew ethereum:referenceTests:referenceTests

PR description

Fixed Issue(s)

refs #6657

@macfarla
Copy link
Contributor Author

macfarla commented Mar 4, 2024 •
edited
Loading

Still some debate over which exact fields should be included. Geth includes maxFeePerBlobGas and blobVersionedHashes but not blobs. I included blobs as well since it's in the spec but don't do anything with it other than pass it through. took it out again

@macfarla
added changelog entry
3f96121 
Signed-off-by: Sally MacFarlane <[email protected]>
@macfarla
Revert "add blobs to callParams"
4012ac5 
Signed-off-by: Sally MacFarlane <[email protected]>

This reverts commit 0c40e45.
@macfarla
fixed test
8e5d930 
Signed-off-by: Sally MacFarlane <[email protected]>
@macfarla
add roundtrip callParam verification
477c835 
Signed-off-by: Sally MacFarlane <[email protected]>
@macfarla
added a failure case test
25f346b 
Signed-off-by: Sally MacFarlane <[email protected]>
@macfarla
integration tests
4e1d0ce 
Signed-off-by: Sally MacFarlane <[email protected]>
@macfarla macfarla added the doc-change-required Indicates an issue or PR that requires doc to be updated label Mar 4, 2024
@macfarla macfarla marked this pull request as ready for review March 4, 2024 04:51
@macfarla
spotless for integration tests
a298fcd 
Signed-off-by: Sally MacFarlane <[email protected]>
@macfarla
removed toDo
2c234c3 
Signed-off-by: Sally MacFarlane <[email protected]>
shemnon
shemnon approved these changes Mar 4, 2024
View reviewed changes
Copy link
Contributor

@shemnon shemnon left a comment

Choose a reason for hiding this comment

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

LGTM, but I have some questions about the Integration Tests.

All the integration tests just null out the parameters (the transaction simulator does pass in real values, so the code is exercised by tests). Should there be at least one integration test that passes in a non-null pair to validate via integration? If not what purpose do the integration tests provide? Should they be removed if they are not keeping pace with the evolution of the APIs they are testing?

siladu
siladu approved these changes Mar 4, 2024
View reviewed changes
Copy link
Contributor

@siladu siladu left a comment

Choose a reason for hiding this comment

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

Maybe not in this PR but do we need to update GraphQL with this as well? Not sure if/what the equivalent of eth_call is for GQL.

@macfarla
Copy link
Contributor Author

macfarla commented Mar 5, 2024

LGTM, but I have some questions about the Integration Tests.

All the integration tests just null out the parameters (the transaction simulator does pass in real values, so the code is exercised by tests). Should there be at least one integration test that passes in a non-null pair to validate via integration? If not what purpose do the integration tests provide? Should they be removed if they are not keeping pace with the evolution of the APIs they are testing?

had a quick look at this - current eth call integration tests are only testing frontier and london forks. think this has kind of been superseded by the spec tests

@macfarla macfarla enabled auto-merge (squash) March 5, 2024 05:34
@macfarla
merge
2200855 
Signed-off-by: Sally MacFarlane <[email protected]>
@macfarla macfarla merged commit cea2099 into hyperledger:main Mar 5, 2024
51 of 55 checks passed
@macfarla macfarla mentioned this pull request Mar 5, 2024
Merged
8 tasks
jflo pushed a commit to jflo/besu that referenced this pull request Mar 5, 2024
@macfarla @jflo
Eth call with 4844 params (hyperledger#6661)
1695e55 
* use accessList in hash and equals

* add blob fields to callParams

* blob fields from callParam used in transaction simulator

* don't zero maxFeePerBlobs if not present

* add roundtrip callParam verification

* added a failure case test

* error conversion

* added some spec tests

Signed-off-by: Sally MacFarlane <[email protected]>

---------

Signed-off-by: Sally MacFarlane <[email protected]>
Signed-off-by: Justin Florentine <[email protected]>
@joaniefromtheblock joaniefromtheblock mentioned this pull request Mar 7, 2024
Closed
@joaniefromtheblock joaniefromtheblock removed the doc-change-required Indicates an issue or PR that requires doc to be updated label Mar 7, 2024
jflo pushed a commit to jflo/besu that referenced this pull request Mar 8, 2024
@macfarla @jflo
Eth call with 4844 params (hyperledger#6661)
7f9c477 
* use accessList in hash and equals

* add blob fields to callParams

* blob fields from callParam used in transaction simulator

* don't zero maxFeePerBlobs if not present

* add roundtrip callParam verification

* added a failure case test

* error conversion

* added some spec tests

Signed-off-by: Sally MacFarlane <[email protected]>

---------

Signed-off-by: Sally MacFarlane <[email protected]>
Signed-off-by: Justin Florentine <[email protected]>
@macfarla macfarla mentioned this pull request Mar 15, 2024
Open
amsmota pushed a commit to Citi/besu that referenced this pull request Apr 16, 2024
@macfarla @amsmota
Eth call with 4844 params (hyperledger#6661)
33f06b3 
* use accessList in hash and equals

* add blob fields to callParams

* blob fields from callParam used in transaction simulator

* don't zero maxFeePerBlobs if not present

* add roundtrip callParam verification

* added a failure case test

* error conversion

* added some spec tests

Signed-off-by: Sally MacFarlane <[email protected]>

---------

Signed-off-by: Sally MacFarlane <[email protected]>
Signed-off-by: amsmota <[email protected]>
amsmota pushed a commit to Citi/besu that referenced this pull request Apr 16, 2024
@macfarla @amsmota
Eth call with 4844 params (hyperledger#6661)
35682e4 
* use accessList in hash and equals

* add blob fields to callParams

* blob fields from callParam used in transaction simulator

* don't zero maxFeePerBlobs if not present

* add roundtrip callParam verification

* added a failure case test

* error conversion

* added some spec tests

Signed-off-by: Sally MacFarlane <[email protected]>

---------

Signed-off-by: Sally MacFarlane <[email protected]>
Signed-off-by: amsmota <[email protected]>
matthew1001 pushed a commit to kaleido-io/besu that referenced this pull request Jun 7, 2024
@macfarla @matthew1001
Eth call with 4844 params (hyperledger#6661)
3cf8c55 
* use accessList in hash and equals

* add blob fields to callParams

* blob fields from callParam used in transaction simulator

* don't zero maxFeePerBlobs if not present

* add roundtrip callParam verification

* added a failure case test

* error conversion

* added some spec tests

Signed-off-by: Sally MacFarlane <[email protected]>

---------

Signed-off-by: Sally MacFarlane <[email protected]>
@macfarla macfarla deleted the eth-call-4844 branch July 16, 2024 02:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Gabriel-Trintinalia Gabriel-Trintinalia Gabriel-Trintinalia approved these changes

@siladu siladu siladu approved these changes

@shemnon shemnon shemnon approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@macfarla @shemnon @siladu @Gabriel-Trintinalia @joaniefromtheblock