refactor(v10): check storage existence only for zeros

[brbrr]

Bench results:
(Generated using sepolia snapshot, 1 block temp db wasn't representative, as the data was hot, and results were marginal)

Note that only for the missing contract on the latest block we would face degradation.

Reader	Scenario	main sec/op	branch sec/op	delta time	main allocs	branch allocs	Δ allocs
latest	non_zero	38.45µs	24.74µs	-35.7%	81	70	-13.6%
latest	zero_exists	58.16µs	57.08µs	~	86	86	~
latest	missing_contract	21.71µs	49.65µs	+128.7%	58	88	+51.7%
historical	non_zero	112.0µs	59.85µs	-46.6%	120	88	-26.7%
historical	zero_exists	129.5µs	81.69µs	-36.9%	124	93	-25.0%
historical	missing_contract	20.78µs	21.96µs	~	55	55	~

[github-actions]

Claude finished @brbrr's task in 2m 21s —— View job

---

Code Review — refactor(v10): check storage existence only for zeros

• Read the diff and surrounding StorageAt handler
• Trace ContractStorage semantics across all reader implementations
• Verify correctness of the "probe only on zero" optimization
• Review test coverage
• Post feedback

---

This is a clean, correct refactor. The optimization skips the ContractClassHash existence probe when the storage value is non-zero, since a non-zero value already proves the contract exists. I traced the semantics across all reader implementations to confirm the invariant the change relies on holds:

ContractStorage returns db.ErrKeyNotFound only when the contract is absent — never for an unset slot on an existing contract:

• Head readers (core/state/state_reader.go, core/deprecatedstate): a missing contract or unset slot both read as felt.Zero, nil — no error. This is exactly why the zero-value probe is still required, and the code does it. ✅
• Historical readers (core/state/history.go, core/deprecatedstate/history.go): checkDeployed runs first and returns ErrKeyNotFound for a non-deployed contract; an unset slot on a deployed contract resolves to felt.Zero, nil via ErrNoHistoryValue / ErrCheckHeadState fallback. So ErrKeyNotFound unambiguously means "contract not found." ✅

So mapping ContractStorage → ErrKeyNotFound → ErrContractNotFound is sound, and non-zero ⟹ contract exists holds for every reader. The change is also a net read reduction (non-zero reads: 1 instead of 2; zero reads: unchanged).

Test coverage is good — the new "non-zero value skips the existence probe" case asserts ContractClassHash is not called, and the updated "non-existent contract" / "contract not found" cases exercise both the head-reader probe path and the historical ErrKeyNotFound path.

Summary

• Blocker: none
• Important: none
• Nit: the ContractStorage internal-error path no longer logs, while the class-hash probe path still does — see the inline comment. Non-blocking.

Note: I was unable to run go test ./rpc/v10/... in this environment (command not permitted), so the pass/fail is inferred from the mock expectations, not executed.
· refactor/v10-storage-existence-check

[Copilot]

Pull request overview

Refactors the v10 StorageAt RPC handler to avoid always probing contract existence, by only doing the existence check when the storage slot value is zero (where “unset slot” and “missing contract” can both appear as zero on the head reader).

Changes:

• Fetch storage first and only perform a ContractClassHash existence probe when the returned storage value is zero.
• Update StorageAt unit tests to reflect the new call sequence/behavior and add a test asserting non-zero values skip the probe.
• Minor gofmt-driven test formatting adjustments.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
rpc/v10/storage.go	Changes StorageAt to probe contract existence only when storage returns zero.
rpc/v10/storage_test.go	Updates mocks/expectations and adds a test to verify probe is skipped on non-zero values.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[codecov]

Codecov Report

❌ Patch coverage is 77.77778% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 75.11%. Comparing base (7dbcfd9) to head (a3da77a).
⚠️ Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
rpc/v10/storage.go	77.77%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3790      +/-   ##
==========================================
- Coverage   75.34%   75.11%   -0.23%     
==========================================
  Files         438      438              
  Lines       39528    39602      +74     
==========================================
- Hits        29781    29748      -33     
- Misses       7676     7752      +76     
- Partials     2071     2102      +31     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[rodrodros]

benchmark numbers please

[rodrodros]

Looks good 👍 , you've good findings from Copilot and Claude

[brbrr]

@rodrodros i've addressed the bot's feedback and added bench results to the description. please take a look.

[github-actions]

Claude finished @brbrr's task in 1m 35s —— View job

---

Re-review after addressing feedback

• Read updated StorageAt handler (rpc/v10/storage.go)
• Verify the two feedback items were addressed
• Re-trace correctness of the historical-reader guard
• Review test coverage

---

Both pieces of bot feedback are addressed correctly, and the change is a strict improvement over the prior revision. 👍

1. Copilot's point — skip the probe for historical readers (storage.go:119)

The existence probe now only runs on head-type readers:

if value.IsZero() && (id.IsLatest() || id.IsPreConfirmed()) {

I traced stateByBlockID (rpc/v10/helpers.go:167) against every reader's ContractStorage to confirm the guard is exactly right:

BlockID	Reader	Missing contract →
latest	HeadState	felt.Zero, nil → probe needed ✅
pre_confirmed	pending.State → falls through to head.ContractStorage (core/pending/state.go:69)	felt.Zero, nil → probe needed ✅
hash / number / l1_accepted	stateHistory	checkDeployed returns db.ErrKeyNotFound first (core/state/history.go:52, core/deprecatedstate/history.go:63) → no probe needed ✅

So the IsLatest() || IsPreConfirmed() condition matches precisely the set of readers that can return a zero value for a non-existent contract. Historical readers already surface ErrKeyNotFound, which is mapped to ErrContractNotFound at line 110-112. Correct, and it removes an extra DB read on every zero-slot historical response.

2. Logging nit (storage.go:113)

The internal-error path on the primary ContractStorage read now logs (h.logger.Error("Failed to get contract storage", ...)), consistent with the class-hash probe path. ✅

Test coverage — good. The new "zero value on historical reader skips the existence probe" case (storage_test.go:183) locks in the new guard by asserting no ContractClassHash call happens on a historical reader, and "non-zero value skips the existence probe" covers the non-zero fast path. The head-reader probe path ("non-existent contract") and internal-error path remain covered.

Summary

• Blocker: none
• Important: none
• Nit: none — the earlier logging nit is resolved. The 2 lines Codecov flags as uncovered are the internal-DB-error logger.Error + ErrInternal returns (lines ~113/124), which are hard-to-exercise failure branches; not worth a mock for.

Note: I could not execute go test ./rpc/v10/... in this environment (command not permitted), so pass/fail is inferred from the mock expectations rather than an actual run.
· refactor/v10-storage-existence-check