Skip to content

Commit de3ac4b

Browse files
committed
fix(stores,stores-eth): address parallel codex review findings
Convergent findings from 4 parallel review rounds: - Thirdparty Child's imperative fetch() / waitFreshResponse / waitResponse now evaluate Alchemy coverage directly and force-register to the batch parent when the contract is missing, so non-reactive refresh paths (post-tx refresh, explicit fetch) actually exercise the fallback instead of resolving after Alchemy only. - BatchParent.getError now reads chunk ownership from a snapshot taken at rebuild time instead of recomputing index from the live refcount ordering, eliminating misattribution during the debounce window. - Child.error suppresses parent.error while batch fallback is still loading (no data, no batch error yet) so consumers stay in the loading state rather than warning on a transient Alchemy failure. - ObservableJsonRpcBatchQuery synthesizes a per-request error for any requested id missing from the batch response, preventing permanent loading on partial/dropped server responses. - Child.waitFreshResponse / waitResponse now return the effective response object, matching the existing query interface contract.
1 parent 7dc9adc commit de3ac4b

File tree

4 files changed

+63
-27
lines changed

4 files changed

+63
-27
lines changed

packages/stores-eth/src/queries/erc20-balance-batch.ts

Lines changed: 19 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,12 @@ export class ObservableQueryEthereumERC20BalancesBatchParent {
2121
@observable.ref
2222
protected lastBuiltKey = "";
2323

24+
// Snapshot of contract → batchQueries index at rebuild time, so getError
25+
// doesn't misattribute chunk ownership during debounce transitions when
26+
// the live refcount no longer matches the current batchQueries layout.
27+
@observable.ref
28+
protected chunkIndex: Map<string, number> = new Map();
29+
2430
constructor(
2531
protected readonly sharedContext: QuerySharedContext,
2632
protected readonly chainId: string,
@@ -70,14 +76,13 @@ export class ObservableQueryEthereumERC20BalancesBatchParent {
7076
}
7177

7278
// Per-contract error: surface only the error of the chunk that owns this
73-
// contract, plus any per-request error the batch returned for it. A failing
74-
// chunk no longer contaminates siblings in other chunks.
79+
// contract (plus its per-request error if any). Looks up chunk ownership
80+
// from the snapshot taken at rebuild time to avoid misattribution during
81+
// debounced refcount transitions.
7582
getError(contract: string): QueryError<unknown> | undefined {
7683
const key = contract.toLowerCase();
77-
const sortedKeys = Array.from(this.refcount.keys()).sort();
78-
const idx = sortedKeys.indexOf(key);
79-
if (idx === -1) return undefined;
80-
const chunkIdx = Math.floor(idx / BATCH_CHUNK_SIZE);
84+
const chunkIdx = this.chunkIndex.get(key);
85+
if (chunkIdx === undefined) return undefined;
8186
const q = this.batchQueries[chunkIdx];
8287
if (!q) return undefined;
8388
if (q.error) return q.error;
@@ -109,6 +114,7 @@ export class ObservableQueryEthereumERC20BalancesBatchParent {
109114
runInAction(() => {
110115
this.batchQueries = [];
111116
this.lastBuiltKey = key;
117+
this.chunkIndex = new Map();
112118
});
113119
return;
114120
}
@@ -118,6 +124,7 @@ export class ObservableQueryEthereumERC20BalancesBatchParent {
118124
runInAction(() => {
119125
this.batchQueries = [];
120126
this.lastBuiltKey = key;
127+
this.chunkIndex = new Map();
121128
});
122129
return;
123130
}
@@ -132,6 +139,11 @@ export class ObservableQueryEthereumERC20BalancesBatchParent {
132139
]),
133140
});
134141

142+
const nextChunkIndex = new Map<string, number>();
143+
chunks.forEach((chunk, idx) => {
144+
for (const c of chunk) nextChunkIndex.set(c, idx);
145+
});
146+
135147
runInAction(() => {
136148
this.batchQueries = chunks.map((chunk) => {
137149
const requests: JsonRpcBatchRequest[] = chunk.map((c) => ({
@@ -147,6 +159,7 @@ export class ObservableQueryEthereumERC20BalancesBatchParent {
147159
);
148160
});
149161
this.lastBuiltKey = key;
162+
this.chunkIndex = nextChunkIndex;
150163
});
151164
}
152165

packages/stores-eth/src/queries/erc20-balance.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ export class ObservableQueryEthereumERC20BalanceImpl
129129
Readonly<QueryResponse<unknown>> | undefined
130130
> {
131131
await this.ensureFetched();
132-
return undefined;
132+
return this.response;
133133
}
134134

135135
async waitResponse(): Promise<Readonly<QueryResponse<unknown>> | undefined> {

packages/stores-eth/src/queries/erc20-balances.ts

Lines changed: 26 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -228,18 +228,20 @@ export class ObservableQueryThirdpartyERC20BalancesImpl
228228
.forceFindCurrency(denom);
229229
}
230230

231+
@computed
231232
@computed
232233
get error(): Readonly<QueryError<unknown>> | undefined {
233234
if (this.isInBatch) {
234235
const contract = this.denomHelper.contractAddress;
235236
const batchErr = this.parent.batchParent.getError(contract);
236237
if (batchErr) return batchErr;
237-
// Once batch has valid data, an Alchemy error is no longer meaningful —
238-
// the fallback successfully provided the balance.
238+
// Batch has valid data — Alchemy's earlier error no longer matters.
239239
if (this.parent.batchParent.getBalance(contract) !== undefined) {
240240
return undefined;
241241
}
242-
return this.parent.error;
242+
// Batch still loading — stay in loading state, suppress parent.error so
243+
// consumers don't prematurely warn before the fallback resolves.
244+
return undefined;
243245
}
244246
return this.parent.error;
245247
}
@@ -307,31 +309,37 @@ export class ObservableQueryThirdpartyERC20BalancesImpl
307309
);
308310
}
309311
const alchemyFetch = this.parent.duplicatedFetchResolver;
310-
return alchemyFetch.then(async () => {
311-
// `isInBatch` flips only after Alchemy responds and the reaction runs,
312-
// so check after awaiting Alchemy to include fallback refreshes.
313-
if (this.isInBatch) {
314-
await this.parent.batchParent.waitFreshResponse();
315-
}
316-
});
312+
return alchemyFetch.then(() => this.awaitFallbackIfMissing());
313+
}
314+
315+
// After Alchemy resolves, check Alchemy coverage directly (not via the
316+
// observation-driven reaction) so imperative callers also exercise the
317+
// batch fallback for tokens missing from Alchemy.
318+
protected async awaitFallbackIfMissing(): Promise<void> {
319+
const contract = this.denomHelper.contractAddress;
320+
const coveredByAlchemy =
321+
!!this.parent.response && this.parent.hasAlchemyBalance(contract);
322+
if (coveredByAlchemy) return;
323+
this.parent.batchParent.addContract(contract);
324+
try {
325+
await this.parent.batchParent.waitFreshResponse();
326+
} finally {
327+
this.parent.batchParent.removeContract(contract);
328+
}
317329
}
318330

319331
async waitFreshResponse(): Promise<
320332
Readonly<QueryResponse<unknown>> | undefined
321333
> {
322334
await this.parent.waitFreshResponse();
323-
if (this.isInBatch) {
324-
await this.parent.batchParent.waitFreshResponse();
325-
}
326-
return undefined;
335+
await this.awaitFallbackIfMissing();
336+
return this.response;
327337
}
328338

329339
async waitResponse(): Promise<Readonly<QueryResponse<unknown>> | undefined> {
330340
await this.parent.waitResponse();
331-
if (this.isInBatch) {
332-
await this.parent.batchParent.waitFreshResponse();
333-
}
334-
return undefined;
341+
await this.awaitFallbackIfMissing();
342+
return this.response;
335343
}
336344
}
337345

packages/stores/src/common/query/json-rpc-batch.ts

Lines changed: 17 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -96,14 +96,29 @@ export class ObservableJsonRpcBatchQuery<T = unknown> extends ObservableQuery<
9696

9797
const data: Record<string, T> = {};
9898
const perRequestErrors: Record<string, JsonRpcBatchRequestError> = {};
99+
const responded = new Set<string>();
99100

100101
for (const res of response.data) {
102+
const id = String(res.id);
103+
responded.add(id);
101104
if (res.error) {
102-
perRequestErrors[String(res.id)] = res.error;
105+
perRequestErrors[id] = res.error;
103106
continue;
104107
}
105108

106-
data[String(res.id)] = res.result as T;
109+
data[id] = res.result as T;
110+
}
111+
112+
// Servers may silently omit IDs on partial failures — synthesize an error
113+
// so callers can't sit in permanent loading on a missing response.
114+
for (const req of this.requests) {
115+
const id = String(req.id);
116+
if (!responded.has(id)) {
117+
perRequestErrors[id] = {
118+
code: -32603,
119+
message: `No response for request id ${id}`,
120+
};
121+
}
107122
}
108123

109124
runInAction(() => {

0 commit comments

Comments
 (0)