0xMiden / 0xMiden/web-sdk

Refactor _withInnerWebClient re-entrancy: replace global depth counter with a trusted handle

オープン
#164 コメント 0 件 リアクション 0 件 担当者 1 名 @WiktorStarczewski が担当を希望しています GitHub で見る
enhancement
主要言語
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

新しい issue をメールで受け取る

初心者向けの GitHub issue を短くまとめたダイジェスト。