Remove support for eth_getProof - #3926
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
PR SummaryMedium Risk Overview The previous implementation (block/watermark resolution, storage key limits, KV store unwrapping for IAVL/ Reviewed by Cursor Bugbot for commit 8fd63c7. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 8158cb8. Configure here.
| startTime := time.Now() | ||
| defer func() { | ||
| recordMetricsWithError(ctx, "eth_getProof", a.connectionType, startTime, returnErr, recover()) | ||
| recordMetrics(ctx, "eth_getProof", a.connectionType, startTime) |
There was a problem hiding this comment.
GetProof metrics recorded as success
Medium Severity
GetProof always returns an error but records metrics via recordMetrics, which treats the call as success. Other unsupported stubs pass returnErr into recordMetricsWithError, so eth_getProof will show as healthy and skip error-class counters.
Reviewed by Cursor Bugbot for commit 8158cb8. Configure here.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3926 +/- ##
==========================================
- Coverage 59.51% 58.46% -1.06%
==========================================
Files 2326 2229 -97
Lines 199003 187747 -11256
==========================================
- Hits 118438 109766 -8672
+ Misses 69300 67604 -1696
+ Partials 11265 10377 -888
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR cleanly replaces the eth_getProof implementation with a documented -32000 stub and updates docs/fixtures/tests consistently. Two non-blocking issues: the stub records every (always-failing) call as a metrics success, and the returned error string does not match the message recorded in the docs table and the not-supported.iox fixture.
Findings: 0 blocking | 4 non-blocking | 2 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- Leftover dead code from the removed proof path:
sei-cosmos/store/cachekv.(*Store).GetParentandgiga/deps/store.(*Store).GetParentnow have no non-test callers (the giga one has none at all), andStateAPI.tmClientis set in the constructor but no longer read anywhere inevmrpc/state.go. Worth cleaning up in this PR or a follow-up so the removal doesn't leave orphaned accessors behind. ProofResultand its explanatory comment about IAVL-vs-trie proofs are retained even though nothing can produce one anymore. Keeping the type as the stub's return signature is reasonable, but the comment now documents behavior that no longer exists.- 2 suggestion(s)/nit(s) flagged inline on specific lines.
| startTime := time.Now() | ||
| defer func() { | ||
| recordMetricsWithError(ctx, "eth_getProof", a.connectionType, startTime, returnErr, recover()) | ||
| recordMetrics(ctx, "eth_getProof", a.connectionType, startTime) |
There was a problem hiding this comment.
[suggestion] recordMetrics delegates to recordMetricsWithError(..., nil, nil), so success is hardcoded to true — every eth_getProof call will now be counted as a success even though the method always returns an error, and the ErrEVMNotSupported error class / -32000 code bucket is never emitted. Every other intentionally-unsupported endpoint (eth_blobBaseFee, eth_syncing, eth_newPendingTransactionFilter, debug_getRaw*) uses the error-aware variant. Suggest matching them:
recordMetricsWithError(ctx, "eth_getProof", a.connectionType, startTime, returnErr, recover())(Also flagged by Codex.)
| return nil, errNoProofCapableQueryableKVStore | ||
| } | ||
| return nil, fmt.Errorf("%w: exceeded unwrap depth", errNoProofCapableQueryableKVStore) | ||
| return nil, &ErrEVMNotSupported{Msg: "eth_getProof is not supported yet; please reach out to the Sei team if you need this endpoint"} |
There was a problem hiding this comment.
[suggestion] The message here ("eth_getProof is not supported yet; please reach out...") does not match what the PR documents: both docs/evm_jsonrpc_unsupported.md and integration_test/.../eth_getProof/not-supported.iox record "eth_getProof is not supported yet on Sei EVM RPC; please reach out...". Neither catches the drift — specOnly only compares result/error presence, and TestGetProofNotSupported uses require.Contains on the shared prefix. Adding on Sei EVM RPC here keeps the code the single source of truth and matches the phrasing of every other unsupported-method message.
4 participants
Describe your changes and provide context
This PR removes the eth_getProof support and return with a proper error message.
Testing performed to validate your change