celo-org / celo-blockchain

Official repository for the golang Celo Blockchain
https://celo.org
GNU Lesser General Public License v3.0
552 stars 196 forks source link

feat (internal/ethapi): implement eth_getBlockReceipts #2284

Closed kamikazechaser closed 3 months ago

kamikazechaser commented 4 months ago

Description

Addition of a new ETH RPC method eth_getBlockReceipts.

https://github.com/ethereum/execution-apis/pull/438

Standardized and implemented in several eth clients e.g. geth, nethermind, nimbus, besu e.t.c.

Other changes

None.

Tested

internal/ethapi/api.go is not covered by any tests.

To be tested on a devnet/testnet with existing chaindata.

Related issues

Resolves #2187

Backwards compatibility

Backwards compatible.

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 0% with 21 lines in your changes are missing coverage. Please review.

Project coverage is 55.76%. Comparing base (f0adb14) to head (d77ad8e). Report is 26 commits behind head on master.

:exclamation: Current head d77ad8e differs from pull request most recent head 33d4baa. Consider uploading reports for the commit 33d4baa to get more accurate results

Files Patch % Lines
internal/ethapi/api.go 0.00% 15 Missing :warning:
ethclient/ethclient.go 0.00% 6 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2284 +/- ## ========================================== + Coverage 55.06% 55.76% +0.70% ========================================== Files 684 684 Lines 114596 92000 -22596 ========================================== - Hits 63097 51308 -11789 + Misses 47617 36795 -10822 - Partials 3882 3897 +15 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

palango commented 4 months ago

For future reference, upstream PR: https://github.com/ethereum/go-ethereum/pull/27702

palango commented 3 months ago

@kamikazechaser I had another look and I think we need to add the effective gas price to the receipts as it is done in GetTransactionReceipt, otherwise the receipts will differ.

https://github.com/celo-org/celo-blockchain/blob/d77ad8e6f617d33f16ab521210f2ed8d703ce044/internal/ethapi/api.go#L1693-L1710

kamikazechaser commented 3 months ago

@palango I have refactored the generateReceiptResponse helper to also accept a context and a backend interface. Tested on devnet.

I have also noticed there is an GetBlockReceipt method that is not a standardized API (added in #1628) that calls into generateReceiptResponse. because the tx passed by this method is nil, no effectiveGasPrice is assigned to that response. I would personally propose to deprecate that method and strictly follow the standardized spec. Anyways let me know what should be done for this case.

palango commented 3 months ago

@palango I have refactored the generateReceiptResponse helper to also accept a context and a backend interface. Tested on devnet.

Thanks, the changes look good. Needs a merge from master, where a new transaction type was added in the meantime, but the conflict is easy to resolve.

I have also noticed there is an GetBlockReceipt method that is not a standardized API (added in #1628) that calls into generateReceiptResponse. because the tx passed by this method is nil, no effectiveGasPrice is assigned to that response. I would personally propose to deprecate that method and strictly follow the standardized spec. Anyways let me know what should be done for this case.

Yes, this is the RPC API to get receipts for celo system transactions. Not having the field added for them is fine.

codecov-commenter commented 3 months ago

Codecov Report

Attention: Patch coverage is 0% with 21 lines in your changes are missing coverage. Please review.

Project coverage is 55.76%. Comparing base (f0adb14) to head (d77ad8e). Report is 26 commits behind head on master.

:exclamation: Current head d77ad8e differs from pull request most recent head 9eeeb3d. Consider uploading reports for the commit 9eeeb3d to get more accurate results

Files Patch % Lines
internal/ethapi/api.go 0.00% 15 Missing :warning:
ethclient/ethclient.go 0.00% 6 Missing :warning:
Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #2284 +/- ## ========================================== + Coverage 55.06% 55.76% +0.70% ========================================== Files 684 684 Lines 114596 92000 -22596 ========================================== - Hits 63097 51308 -11789 + Misses 47617 36795 -10822 - Partials 3882 3897 +15 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

kamikazechaser commented 3 months ago

Not having the field added for them is fine.

Sounds good. I have also added the DenominatedFeeCurrency changes.

github-actions[bot] commented 3 months ago

Coverage from tests in ./e2e_test/... for ./consensus/istanbul/... at commit 6351612d67d4cf5e18708a0536f0cb43afa19e43

coverage: 51.1% of statements across all listed packages
coverage:  63.4% of statements in consensus/istanbul
coverage:  43.3% of statements in consensus/istanbul/announce
coverage:  56.0% of statements in consensus/istanbul/backend
coverage:   0.0% of statements in consensus/istanbul/backend/backendtest
coverage:  24.3% of statements in consensus/istanbul/backend/internal/replica
coverage:  65.6% of statements in consensus/istanbul/core
coverage:  50.0% of statements in consensus/istanbul/db
coverage:   0.0% of statements in consensus/istanbul/proxy
coverage:  64.2% of statements in consensus/istanbul/uptime
coverage:  51.8% of statements in consensus/istanbul/validator
coverage:  79.2% of statements in consensus/istanbul/validator/random
github-actions[bot] commented 3 months ago
5882 passed, 45 skipped