Refactor _withInnerWebClient re-entrancy: replace global depth counter with a trusted handle
- 主要言語
- TypeScript
- スター
- 1
- フォーク
- 21
- 平均マージ
- 12時間 14分
- マージ済み PR(30日)
- 41
説明
Follow-up to #152 (merged). That PR fixed the `_withInnerWebClient` re-entrancy deadlock with a **global `_withInnerLockDepth` counter** on the inner client: while `fn` is in flight (depth > 0), `_serializeWasmCall` runs its callback inline instead of enqueuing on the chain. It's a minimal, correct fix for the deadlock, but it leaves a footgun and a safety-contract hole worth removing.
## Why revisit it
- **Global state.** The depth counter is shared mutable state on the inner client. It's bookkeeping, not re-entrancy *detection* — someone reading `_withInnerLockDepth > 0` could mistake it for "we're inside a re-entrant call" when it only tracks nesting depth.
- **Safety-contract hole.** Because the counter is global (not scoped to `fn`'s async context), *any* `_serializeWasmCall` that fires while `fn` is mid-flight runs inline — including one issued by code **outside** `fn` that races in during one of `fn`'s awaits. That caller does not queue behind the outer slot; it interleaves and can race wasm-bindgen's borrow check. The current mitigation is a documented SAFETY CONTRACT requiring callers to hold an external mutex (the Miden Wallet's `withWasmClientLock` satisfies it). The characterization test added in #152 (`an external SDK call made during fn runs inline`) pins this behavior.
## Proposed approach (Santiago, in #152)
Replace "flip a global flag/counter while `fn` runs" with "hand `fn` a different object whose methods don't go through `_serializeWasmCall` at all":
```js
// In MidenClient
_withInnerWebClient(fn) {
return this.#inner._serializeWasmCall(() => {
const trusted = createTrustedHandle(this.#inner); // bypasses the chain
return fn(trusted);
});
}
```
`createTrustedHandle(inner)` returns an object exposing the same method surface as the proxy-wrapped inner, but each method calls the underlying `wasm-bindgen` client **directly**, skipping `_serializeWasmCall`. Serialization is provided once, by the outer `_serializeWasmCall` slot that wraps the whole `fn`. This removes the shared counter and global flag entirely, and external callers don't need to know about any of it.
## Considerations / cost (Wiktor, in #152)
- `createTrustedHandle(inner)` must mirror the **full proxy method surface** — every `wasm-bindgen` method the proxy currently wraps — as direct passthroughs.
- It has to account for the **two wrapper layers**: `MidenClient` wraps an inner that already proxy-wraps the wasm client, so the trusted handle must either reach through both layers or duplicate them.
- This is a larger refactor than the 1-line counter fix, hence deferred out of #152.
## Acceptance
- `_withInnerWebClient` no longer relies on `_withInnerLockDepth` (counter/global flag removed).
- Re-entrant calls from `fn` still don't deadlock; the existing reentrancy regression tests pass.
- The trusted handle covers the same method surface `fn` consumers rely on (executeTransaction, prove*, submit, apply, getInputNote, syncState, etc.).
- Update / replace the `documents the safety-contract hole` characterization test to reflect the new model.
Refs: #152, and the review thread there (Santiago's "trusted handle" suggestion + agreement to track this: _"Yeah, please. I think that it is the better approach to implement eventually."_).
コントリビューションガイド
調査の方向性
Look at the MidenClient's _withInnerWebClient method and the inner client's proxy-wrapped wasm-bindgen methods. The task is to create a trusted handle that bypasses _serializeWasmCall. Start by examining the proxy layer to understand the method surface (executeTransaction, prove*, submit, etc.). The existing reentrancy tests from PR #152 must pass after the refactor. Done means the global counter is removed, the trusted handle works, and the safety-contract test is updated.
索引モデルが issue の本文から書いたものです。
評価
- 技術スタック
- javascript, typescript, wasm
- 領域
- api, backend, devtools
- issue の種類
- リファクタリング
- 難易度
- 4/5
- 見積もり時間
- 3〜5日
- 活発さ
- 静か
- 明瞭さ
- 明確に書かれている
- 初心者へのやさしさ
- 45/100