search

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

Remove special handling for gas estimation #2274

Closed palango closed 4 months ago

palango commented 4 months ago

This is now done by the SkipDebitCredit flag.

Resolves #2267

github-actions[bot] commented 4 months ago

Coverage from tests in ./e2e_test/... for ./consensus/istanbul/... at commit 8d0f0f9e42c78e3025c445bca6b010691b42d7f0

coverage: 46.3% of statements across all listed packages 
coverage:  56.8% of statements in consensus/istanbul
coverage:  23.7% of statements in consensus/istanbul/announce
coverage:  54.3% 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:  63.3% of statements in consensus/istanbul/core
coverage:  45.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 4 months ago

5890 passed, 1 failed, 45 skipped

Test failures:
  TestWalletNotifications: keystore

Failed
    keystore_test.go:453: wallet list doesn't match required accounts: have 541, want 540
    keystore_test.go:481: can't find event with Kind=0 for 98236cbb3fc0ed453f15c72f6b4fac460ef8d5c6
This test report was produced by the test-summary action.  Made with ❤️ in Cambridge.
piersy commented 4 months ago

Hey @palango. I think this change is fine, but I think there is a problem with the previous change.

Looking in ethapi.DoCall: https://github.com/celo-org/celo-blockchain/blob/36746b2c0e45000a6d1667ab0a06ebfc60edac3a/internal/ethapi/api.go#L912

We can see that skipDebitCredit is set to true, but ethapi.DoCall is called from ethapi.Call so that previous change is also disabling debit/credit calls for eth_call which I don't think we want, right? I think it makes sense to fix that here and also include a comment for EstimateGas saying that we skip the debit/credit calls.

palango commented 4 months ago

Hey @palango. I think this change is fine, but I think there is a problem with the previous change.

Looking in ethapi.DoCall:

https://github.com/celo-org/celo-blockchain/blob/36746b2c0e45000a6d1667ab0a06ebfc60edac3a/internal/ethapi/api.go#L912

We can see that skipDebitCredit is set to true, but ethapi.DoCall is called from ethapi.Call so that previous change is also disabling debit/credit calls for eth_call which I don't think we want, right? I think it makes sense to fix that here and also include a comment for EstimateGas saying that we skip the debit/credit calls.

I fixed that in https://github.com/celo-org/celo-blockchain/pull/2273