Repository navigation
rpc: remember deployed accounts instead of re-reading getClassHashAt - #741
Conversation
Every deployment-status read is one starknet_getClassHashAt, and the background repeats it for the same address on each Home refresh, fee review, approval, deploy screen and recovery rescan. A Starknet contract cannot be removed from its address, so once a node reports "deployed" the production RuntimeRpc now answers that address from worker memory. Only "deployed" is kept: "not deployed" and failures always reach the chain, so a fresh deployment is seen at once and a Retry still re-reads (Sherlock 218). Only existence is kept, never the class hash: upgrades change it, and the signing-path compatibility checks keep reading it live at a pinned block outside this cache. Entries are keyed by canonical address plus the exact endpoint list, bounded at 512, and never persisted. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fd1194158
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ocal node (#741 review) "Deployed" can stop being true: a devnet reset under the same URL, or a reorg of the latest block, leaves a remembered address absent while the cache keeps answering true, and the deploy screen then refuses with "account already deployed" until the worker restarts. An endpoint list that includes a loopback node (127.0.0.1 or localhost, which covers the built-in SN_DEVNET) is now never remembered, and every other "deployed" answer is read again once it is five minutes old, longer than the side panel's 60-second refresh. A read that then answers "not deployed" drops the entry. isLoopbackServiceUrl is exported from the service-url policy so its loopback host set stays single-sourced. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
AI-assisted review (Claude, with an independent Codex gpt-6-astra pass), posted from @derek-flossman's account. Findings were checked against the code; treat them as review input, not approval. Low — out-of-order reads. Two concurrent misses for the same key: if the earlier read ("deployed") resolves after a newer one ("not deployed"), |
…741 review) Two reads of one address can overlap. If the older one answers "deployed" and settles after a newer "not deployed" (a reorg one remote node has already seen), it re-stored the stale answer for another five minutes. Each read now takes a ticket for its key, and only the newest ticket may store or clear the remembered answer; superseded reads still return their own result to their caller. The ticket map only holds reads in flight, since each entry is removed when its read settles. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
adrienlacombe
left a comment
There was a problem hiding this comment.
Thanks, this is a careful, small change. I looked mainly at how stale answers could hurt, and I didn't find a correctness problem.
What I checked
- Network switch / endpoint change: the key is the canonical address plus the exact
rpcUrlslist. Switching network or editing an endpoint reads the chain again. ✅ - Undeployed accounts: "not deployed" and failed reads are never stored, so a deploy shows up on the next read and Retry still reaches the chain (Sherlock 218). ✅
- Class upgrade: only existence is cached.
isCompatibleAccountDeployedandrequireLiveAuthorizableClassstill read live. - Home read-only flag: in
account-signability.ts, a cached "deployed" still goes through the livegetBraavosSigningRestrictionprobe. An undeployed Braavos is never cached, so the "Not deployed" flag clears on the first refresh after deployment. ✅ - Overlapping reads: the ticket logic means only the newest read may store or clear an answer, and the tests pin both orders. ✅
- Persistence: the cache is in worker memory only, nothing goes to storage, and it is capped at 512 entries. When the MV3 worker sleeps, the saving resets. That only costs efficiency, not correctness.
Small, non-blocking notes
- Only
127.0.0.1andlocalhostcount as local. A devnet reset behind a non-loopback HTTPS host would keep "deployed" for up to 5 minutes. That's acceptable, and the module comment already says so. - Heads-up for merge order: #755 also edits
createDefaultRuntimeRpcon the line right aftergetDeploymentStatus,. Whichever PR lands second gets a small textual conflict inruntime-rpc.ts.
CI: fast-checks and pr-guards pass on f485677. The branch is behind release/v0.10.8, so it needs a rebase or update before merge.
|
Review follow-up (AI-assisted, by Claude), posted from @derek-flossman's account. Independent check before merge. No findings. Earlier findings are resolved in the code:
Confirmed in code:
Run on this PR merged onto current
Merge order: a merge-tree check against the open PRs finds one textual conflict, with #755 (draft) in |
|
Merged by the repo owner via the release-review bypass; no second review. Head 758bda5 was up to date with release/v0.10.8, with fast-checks and pr-guards green. |
Closes #742
Targets
release/v0.10.8.Summary
Every deployment-status read the background makes is one
starknet_getClassHashAt, and it repeats that read for the same address on every Home refresh (the read-only flag), fee review, approval, deploy screen, STRK20 staging step and recovery rescan. This PR remembers the answer when it is "deployed".rememberDeployedAccountswrapsgetDeploymentStatusin the productionRuntimeRpcwiring. Once a node reports an address deployed, later reads for that address and endpoint list are answered from worker memory for 5 minutes. That's longer than the side panel's 60-second refresh.127.0.0.1orlocalhost, which covers the built-inSN_DEVNET) is never remembered.isLoopbackServiceUrlis exported from@bramble/core/service-urland reuses its existing loopback host set, rather than adding another copy.isCompatibleAccountDeployed,requireLiveAuthorizableClass, the recovery signing-restriction probes) call the provider directly at a pinned block and do not go through this cache.getDeploymentStatusitself accepts get a key, so a spelling it refuses (decimal, uppercase, zero) still reaches it and is refused. The map holds at most 512 addresses, drops the oldest first, and is never persisted.Expected effect: smaller than the "~660 calls per scan" estimate
The recovery scan checks 7 candidates per index. For an undeployed candidate it makes 1
getClassHashAtplus one balance read per probe token: 2 tokens on Sepolia, 3 on mainnet. That is where "about a third of every scan" comes from.Nearly every candidate is undeployed, and those answers must still be read live. So:
The main saving is on repeated reads of the user's own deployed accounts during normal use.
Test plan
packages/platform/starknet/tests/transactions/deployment-status-cache.test.ts:getDeploymentStatus, a deployed account costs onegetClassHashAtisLoopbackServiceUrlcases inpackages/foundation/core/tests/service-url.test.tsbun run check🤖 Generated with Claude Code