docs: add the 2026-09 first-principles review of the SDK

[kalepail]

Summary

Adds docs/research/2026-09-06-first-principles-review/ — a twelve-document review of the SDK, relayer proxy, indexer contract, bindings package and demo, written as if rewriting from first principles (no legacy or backwards-compatibility constraints). Docs only; no code changes. Also adds one link under Documentation in the README.

Reviewed: main@1a0c0eb against OpenZeppelin stellar-contracts@1e513890 (the deployed P27 artefacts) and OZ main@6ea3075; host behaviour checked against rs-soroban-env; SDK behaviour against @stellar/stellar-sdk@16.0.1. Baseline tsc clean, 414/414 tests green.

What's in it

Doc	Contents
01-executive-summary	verdict, numbers, the ten findings that matter most, cross-cutting themes, what to keep, quick fixes for main
02–07	security · API/DX · architecture · contract parity · tests/tooling · services — 113 findings (21 high, 61 medium, 26 low), every one cited to file:line, each doc opening with a coverage statement
08-rewrite-blueprint	reconciled design: public API, module layout with LOC budget (~3.1k vs 8.6k today), trust model, parity-as-build-step, service topology, test strategy, build order
09-decisions	twenty decisions with options and recommendations; D1–D5 gate the rest
10-findings-register	all findings in one table + 17 independently-confirmed clusters + "checked and OK" list
11-verification-report	post-synthesis spot-check of 22 citations with the empirical probes re-run

Headline findings (details and evidence in the docs): the RPC is inside the signing trust base (SEC-F1); the derived address does not commit to the constructor (SEC-F2); rules.add/constructor pass an unsorted policies ScMap (PAR-F1); CreateContract rules are unreachable due to wrong XDR accessor names (ARCH-F13); the indexer is required but rules.list() can be exact from ledger entries alone (SVC-F12); connection is passkey-only (API-F1); userVerification: "preferred" against a verifier that requires UV (SEC-F4); the relayer proxy's CORS preflight rejects the SDK's own headers (SVC-F1).

Suggested next steps

• Answer D1–D5 in 09-decisions.md.
• Land the small verified fixes listed in 01-executive-summary.md §8 on main independently of any rewrite.

🤖 Generated with Claude Code

https://claude.ai/code/session_019Rs5rWTVy1YBAwU5xkeEyM

[Copilot]

Pull request overview

Adds a documentation-only first-principles review of the SDK and related services, linked from the main README.

Changes:

• Documents security, API, architecture, parity, testing, and service findings.
• Proposes a rewrite blueprint and records open decisions.
• Consolidates findings and verification results.

Reviewed changes

Copilot reviewed 13 out of 13 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
README.md	Links to the review.
docs/research/2026-09-06-first-principles-review/11-verification-report.md	Reports citation and empirical verification.
docs/research/2026-09-06-first-principles-review/10-findings-register.md	Consolidates all findings.
docs/research/2026-09-06-first-principles-review/09-decisions.md	Records product and architecture decisions.
docs/research/2026-09-06-first-principles-review/08-rewrite-blueprint.md	Proposes a replacement architecture and API.
docs/research/2026-09-06-first-principles-review/07-services.md	Reviews relayer, indexer, bindings, and demos.
docs/research/2026-09-06-first-principles-review/04-architecture.md	Reviews internal architecture.
docs/research/2026-09-06-first-principles-review/03-api-and-dx.md	Reviews API and developer experience.
docs/research/2026-09-06-first-principles-review/02-security.md	Reviews security and trust boundaries.
docs/research/2026-09-06-first-principles-review/01-executive-summary.md	Summarizes findings and recommendations.
docs/research/2026-09-06-first-principles-review/00-README.md	Introduces scope, methodology, and reading order.

Suppressed comments (21)

docs/research/2026-09-06-first-principles-review/01-executive-summary.md:19

• This row combines the source-test line count with the all-suite file/test totals. The 34 src/**/*.test.ts files total 8,191 lines, while all 37 test files total 9,159 lines and produce the stated 414 tests. Use 9,159 here or explicitly scope every metric to src/.

| Tests | 8,191 lines, 414 tests, 37 files, ~15 s | 130 `as never` / `as unknown as` casts; tests excluded from `tsc` (128 type errors when included) |

docs/research/2026-09-06-first-principles-review/01-executive-summary.md:74

• The reconciled blueprint defines the root plus six subpaths (passkey, node, wallets, relayer-protocol, testing, testnet), i.e. seven entry points, not three. This headline description omits four public entry points that the same paragraph/design relies on.

One package, three entry points. `smart-account-kit` is an isomorphic core over `Uint8Array` with zero runtime dependencies besides a `^16` peer on `@stellar/stellar-sdk`: a tiny pure `auth/` module (digest, payload codec, host-order compare, DER, contexts-of-entry, intent matching, expiration policy, signer validation) tested against vectors generated from the Rust suite; one `Signer` interface with Ed25519 and Delegated implementations; an immutable `SmartAccount` value whose reads come from ledger entries and whose writes are thin builders over generated clients for all six WASMs; `PolicyInstall` values (`policy.threshold(2)`) with typed clients and a plugin interface mirroring the Rust `Policy` trait; one `authorize(tx, { signers, intent, ruleIds? })` pipeline and one `submit(tx, sponsor)` + `confirm(hash, intent)`; a `Sponsor` interface (HTTP relayer speaking a typed, versioned protocol exported from a `/relayer-protocol` subpath; keypair; custom) and a `Discovery` interface (events, HTTP, Mercury, local). `smart-account-kit/passkey` is the browser profile: `PasskeySigner.register/authenticate` over `navigator.credentials` directly, `discover()` and a fail-closed `connect()` built as a pipeline of pure checks over fetched facts, session storage. `smart-account-kit/node` re-exports keypair signers and sponsors. Config is `{ network: "testnet" | "mainnet" | NetworkConfig, sponsor?, discovery?, storage? }` with `networks.*` manifests generated from one JSON file. Errors are thrown, typed, with string-union codes; `send()` returns a `Receipt { hash, ledger, returnValue, events }`. The full design — public API, module layout with LOC budget, trust model, service topology, test strategy, build order — is in [08-rewrite-blueprint.md](08-rewrite-blueprint.md).

docs/research/2026-09-06-first-principles-review/01-executive-summary.md:12

• The underlying registry has 43 entries, not 45 (16 SmartAccount, 10 WebAuthn, 4 simple-threshold, 5 weighted-threshold, and 8 spending-limit). This headline should be corrected together with the parity report and findings register unless two additional codes can be cited.

The hard, security-critical core is right: the auth digest the SDK signs is byte-identical to what `__check_auth` computes, signers only ever sign that digest, the host-order `ScMap` sort is correct (verified against `rs-soroban-env`), DER parsing is bounds-checked, nothing secret is persisted or logged, all 45 contract error codes match, and the v0.7.0 connect path really is fail-closed against a hostile indexer. What is *not* right is everything around that core: the trust model is implicit (the RPC is inside the signing trust base — one biometric tap signs whatever `simulateTransaction` returns), the wallet address does not commit to the constructor (so ~400 lines of provenance code compensate for a property the salt could simply guarantee), the passkey-wallet product is baked into a client for a contract that is far more general (backend agents and multi-signer births are unreachable), the public surface is the accretion history of the demo (151 exports, 24 config options, seven "signer" concepts, four error models, five signing pipelines), hand-maintained mirrors of the contract have drifted (unsorted policy maps, wrong XDR accessor names, `count()` documented backwards), the satellites carry more complexity than the SDK needs from them (a required indexer that isn't required, a relayer proxy whose browser path is broken as committed, a bindings package that re-exports all of stellar-sdk), and the test suite — fast and honest as it is — covers the v0.7.0 security gates by absence and never binds SDK output to the contract. A from-scratch version with full parity is realistically 3,000–3,500 lines of code against 8,580 today.

docs/research/2026-09-06-first-principles-review/01-executive-summary.md:42

• The fixed “three calls” claim ignores the 200-key limit documented in SVC-F12 and the review's own statement that rule IDs are unbounded. Large or sparse accounts require chunked getLedgerEntries requests, so summarize this as batched ledger reads rather than exactly three calls.

**4. The indexer is required, and it doesn't need to be (SVC-F10/F11/F12, PAR-F8, high).** Fresh-device connect throws without a complete schema-2 response even when the caller supplies the contract ID; mainnet Mercury serves the legacy shape per the repo's own README; `rules.list()` silently stops at rule id 8; the freshness gate demands a self-reported ledger that an honest one-ledger-behind indexer fails and a dishonest one trivially passes. Meanwhile the contract keeps `NextId`/`Count` in instance storage and each rule in `ContextRuleData(id)` persistent storage, so **three `getLedgerEntries` calls enumerate the exact live rule set with no indexer, no probe, no simulation** — and expose that `get_context_rules_count` is the *active* count, not the "monotonic counter" the SDK documents. *Rewrite:* RPC-only `rules.list()`; the indexer becomes an optional `Discovery` plug-in for credential → contract reverse lookup only.

docs/research/2026-09-06-first-principles-review/03-api-and-dx.md:242

• This proposed class also declares a signers management namespace at line 262, producing a duplicate class member. Rename the signer collection or namespace so the API sketch is valid TypeScript.

  readonly signers: readonly Signer[];

docs/research/2026-09-06-first-principles-review/04-architecture.md:74

• This points to F15, which covers ExternalSignerManager; the legacy-shape shim is ARCH-F12. Correcting the reference is important because finding IDs are advertised as stable navigation keys.

- **What:** Three copies of "build an invokeHostFunction tx from a fixed account, simulate, decode `sim.result.retval`, map `sim.error` through `decodeContractError`". Two different deterministic seeds (`"smart-account-kit-context-rule-read"`, `"smart-account-kit-policy-read"`) and one literal `GAAAA…AWHF`. `contract.Client` / `AssembledTransaction.build` already do this (`isReadCall`, `.result`), and `context-rules.ts:322` already *has* the client call (`wallet.get_context_rule`) — the RPC path exists only to feed the legacy-shape shim (F15).

docs/research/2026-09-06-first-principles-review/05-contract-parity.md:50

• This row says CreateContract contexts are resolvable, contradicting TEST-F1/ARCH-F13 and this review's own headline conclusion that both V1 and V2 extraction throw. The parity matrix should mark automatic CreateContract resolution as broken rather than merely lacking a builder.

| `__check_auth(payload, AuthPayload, contexts)` | `kit.signAuthEntry` (`webauthn-ops.ts:120-209`), `kit.multiSigners.*` (`multi-signer-manager.ts`) | partial | Passkey ✓, Ed25519 ✓, `Delegated(G…)` ✓ (nested `__check_auth(auth_digest)` entry, `multi-signer-manager.ts:414-461`). `Delegated(C…)` ✗, custom `External` verifiers ✗ (`SelectedSigner.type` is `"passkey"\|"wallet"\|"ed25519"`, `types.ts:654`), `CreateContract` contexts resolvable (`context-rules.ts:102-112`) but no builder produces one. See F5, F6. |

docs/research/2026-09-06-first-principles-review/05-contract-parity.md:201

• “Resolvable” is factually inconsistent with TEST-F1/ARCH-F13: buildInvocationContextTypes throws for both CreateContract arms. State that the rule is buildable but automatic resolution is broken; otherwise this gap list understates a verified correctness bug.

5. **`CreateContract` rules** — buildable (`createCreateContractContext`) and resolvable, but nothing constructs an account-authorised deploy (`create_contract` host fn with `from_address = account`), so a "deploy-scoped session key" is unusable end-to-end.

docs/research/2026-09-06-first-principles-review/05-contract-parity.md:24

• The registry shown in src/contract-errors.ts:67-117 contains 43 entries (16 SmartAccount + 10 WebAuthn + 4 simple-threshold + 5 weighted-threshold + 8 spending-limit), not 45. Either identify the two omitted contract codes or correct this headline and its copies; otherwise the claimed completeness count is unsupported.

The SDK's model of the *core* contract is mostly right where it counts: the `AuthPayload` wire shape, the `Signer` enum encoding, the auth-digest formula, the host-order `ScMap` sort in `compareScVal` (I verified it against the host's `Compare` impls), the pre-order `auth_contexts` alignment, `valid_until` inclusivity, and the four numeric limits all match Rust. The numeric error registry is complete and correct for all 45 codes. The "Full contract parity" claim in the README is true only at the level of "there is a wrapper per entry point": every account function has *a* method, and the three example policies have typed getters/setters.

docs/research/2026-09-06-first-principles-review/07-services.md:97

• The three-round-trip claim is not valid for the unbounded u32 rule IDs documented in PAR §4.4. Since getLedgerEntries accepts only 200 keys, ContextRuleData(0..nextId) must be chunked once nextId > 199; the signer/policy phase can also exceed 200 keys. Describe this as chunked calls and update the repeated three-call claims in the executive summary and findings register.

- **What:** The instance ledger entry the SDK already fetches contains `NextId` and `Count`. `getLedgerEntries` accepts up to 200 keys per call. Algorithm: (1) read instance → `nextId`, `count`, wasm hash; (2) `getLedgerEntries(ContextRuleData(0..nextId))` → the exact live rule set (removed rules have no entry; if fewer than `count` come back, some are TTL-archived — a condition the current design cannot even detect); (3) `getLedgerEntries(SignerData(ids) ∪ PolicyData(ids))` → signers and policy addresses. Three RPC round-trips, no simulation, no indexer, no probe heuristics, no `hydrateContextRuleIds` shim (`context-rules.ts:130-179`), and `signer_ids`/`policy_ids` come for free. The entry types are `#[contracttype]` structs (symbol-keyed `ScMap`), ~40 lines to decode; gate the decoder on the accepted WASM hash list the SDK already maintains.

docs/research/2026-09-06-first-principles-review/07-services.md:39

• The protocol subpath is proposed by SVC-F9, not F10; F10 discusses the required indexer. This stale reference sends the dependency recommendation to the wrong finding.

- **Rewrite recommendation:** Declare `@stellar/stellar-sdk` (pinned to the SDK's version) in the worker's `package.json`; delete the standalone lockfile and the `--ignore-workspace` install; make the worker depend on `smart-account-kit` (subpath exports, F10) so there is exactly one XDR vocabulary. Prefer a plugin version that peer-depends on stellar-sdk ≥16, or bypass the plugin: `ChannelsClient` is two HTTP calls.

docs/research/2026-09-06-first-principles-review/07-services.md:74

• The missing protocol package is SVC-F9, not F10. Correct the reference so this maintainability conclusion links to the actual shared-wire-format finding.

- **Why it matters:** Maintainability and self-hostability; the duplicated constants are the visible symptom of the missing protocol package (F10).

docs/research/2026-09-06-first-principles-review/07-services.md:33

• Both “see F10” references are wrong: the shared SDK↔proxy protocol and cross-boundary test are SVC-F9. F10 is the unrelated indexer requirement finding.

- **Why it matters:** Every browser relayer submission from an allowed origin fails at preflight with the code as written. The demo (`demo/.env.example` → `VITE_RELAYER_URL`) is the only consumer and it *is* a browser. Either the deployed worker predates this commit or the browser path has not been exercised since. There is no SDK↔proxy integration test to catch it (see F10).
- **Rewrite recommendation:** One shared protocol module defines the header set and the CORS policy (F10). In the worker, use `cors({ origin: allowlist, allowHeaders: PROTOCOL_HEADERS })` or drop the custom headers entirely (they are not consumed anywhere on the proxy). Add a contract test: SDK client → in-process worker `fetch`, including preflight.

docs/research/2026-09-06-first-principles-review/08-rewrite-blueprint.md:47

• This public type contradicts the stated DOM-free root and lib: ["ES2022"]: both typeof fetch and Crypto come from DOM/Node ambient declarations, while the blueprint removes Node types. Define SDK-owned minimal FetchLike/CryptoLike interfaces or import runtime-neutral types so the root declarations compile under the promised library set.

  fetch?: typeof fetch; crypto?: Crypto;     // injection for tests / exotic runtimes

docs/research/2026-09-06-first-principles-review/08-rewrite-blueprint.md:46

• allowHttp cannot be applied to sponsor and discovery URLs because Config only receives opaque Sponsor/Discovery objects, and the HTTP factory signatures do not accept this flag. Move this option into httpSponsor/httpDiscovery (or make Config own URL descriptors) so the documented HTTPS policy is enforceable.

  allowHttp?: boolean;                       // applies to every URL (rpc, horizon, sponsor, discovery)

docs/research/2026-09-06-first-principles-review/08-rewrite-blueprint.md:218

• A WebAuthn authentication assertion does not contain the credential public key, so a fresh device cannot construct the promised PasskeySigner (whose publicKey is required). Requiring discover(cfg, signer) then creates a circular flow. Have authentication return the credential ID and assertion, discover/verify the on-chain or birth key, and only then construct the signer.

  static authenticate(o?: { allow?: CredentialId[] }): Promise<{ signer: PasskeySigner; assertion: Assertion }>;                  // discoverable credential

docs/research/2026-09-06-first-principles-review/08-rewrite-blueprint.md:95

• The text says built-in policy addresses default from the account's network, but these global builders receive no network and PolicyInstall.address is mandatory. As written, policy.threshold(2) in the quick start cannot produce a valid PolicyInstall; either pass network/config or defer address resolution in the value type.

  threshold(n: number, address?: ContractAddress): PolicyInstall;

docs/research/2026-09-06-first-principles-review/08-rewrite-blueprint.md:332

• The stated dependency graph is cyclic with the proposed APIs: auth/plan.ts consumes Signer from signers/ and Rule from account/, while signers/ and account/ are declared to depend on auth/. Move planning above those modules or move its pure input types into core/ before treating these arrows as implementation constraints.

Dependency arrows (all one-way; `core/` imports nothing internal; `auth/` imports only `core/`):

passkey/ node/ wallets/ ──▶ signers/ ──▶ auth/ ──▶ core/
verify/ discovery/ ────────▶ account/ ──▶ auth/, core/

**docs/research/2026-09-06-first-principles-review/08-rewrite-blueprint.md:256**
* This “≈25” target is inconsistent with the list itself: it names about 25 runtime values plus roughly 18 exported types, over 40 named root exports. Since the review's current 151-export count includes type exports, clarify that the target is runtime-only or reduce the listed type surface.

2.11 Root export list (target ≈ 25)

SmartAccount, networks, Ed25519Signer, DelegatedSigner, customSigner, scope, policy, authorize, submit, confirm, planAuth, httpSponsor, keypairSponsor, customSponsor, httpDiscovery, eventsDiscovery, localDiscovery, SmartAccountError, ContractError, parseUnits/formatUnits, LEDGERS_PER_HOUR/DAY/WEEK; types Config, NetworkConfig, Signer, SignerKey, Rule, RuleSpec, Scope, PolicyInstall, Receipt, AuthPlan, Intent, Sponsor, Discovery, KeyValueStorage, ErrorCode, DecodedEvent; re-export AssembledTransaction.

**docs/research/2026-09-06-first-principles-review/10-findings-register.md:151**
* `src/contract-errors.ts:67-117` contains 43 registered contract errors, not 45. Correct this copied total or identify the two codes that are claimed but absent from the supposedly complete registry.

Not everything is a finding. The security review explicitly verified and cleared the following (full list in 02-security.md § Checked and OK): the auth digest is byte-identical to do_check_auth; every signer type signs only the digest; delegated nesting matches require_auth_for_args; the WebAuthn challenge matches validate_challenge; DER→compact is bounds-checked with low-S and [1, n) range checks; compareScVal matches the host's Compare impls (independently verified against rs-soroban-env by the parity review); AuthPayload/Signer decoders check every discriminant; nonces and challenges are CSPRNG; re-simulation cannot swap signed entries; the deploy auth entry is validated against the operation; the connect path is fail-closed against a hostile indexer; stored sessions and credentials contain only public data; no secrets are logged; no prototype-pollution, ReDoS or unbounded-loop patterns; the relayer proxy validates before key use and requires byte-equal auth roots. All 45 contract error codes, names and families match the Rust enums; OZ main (2026-09-04) adds no interface, error, event or limit changes over the pinned 1e513890.

**docs/research/2026-09-06-first-principles-review/10-findings-register.md:18**
* “Three `getLedgerEntries` calls” is only true while each key set stays within the documented 200-key RPC limit. Since `NextId` is unbounded, the register should describe batched reads instead of claiming a fixed call count.

| The indexer is required, and needn't be | SVC-F10, SVC-F11, SVC-F12, PAR-F8, API-F10, ARCH-F21, SEC-F12 | Fresh-device connect needs a complete schema-2 response even with a known contract ID; rules.list() silently under-reports past id 8; the freshness gate is self-reported. Yet NextId/Count live in instance storage and each rule in ContextRuleData(id) persistent storage, so three getLedgerEntries calls enumerate the exact live set. Also: get_context_rules_count returns the active count (decremented on remove) — the SDK, README and demo call it "monotonic". | Storage layout and the Count decrement confirmed in storage.rs during synthesis. |

</details>



---

💡 <a href="/stellar/smart-account-kit/new/main?filename=.github/skills/code-review/SKILL.md" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Add a `code-review` agent skill</a> or configure MCP servers for context-aware, tailored reviews. <a href="https://docs.github.com/copilot/how-tos/use-copilot-agents/request-a-code-review/use-code-review?tool=webui#mcp-servers-and-agent-skills" class="Link--inTextBlock" target="_blank" rel="noopener noreferrer">Learn more in the docs.</a>

[kalepail]

Pull request #10 implements and verifies the remaining review findings. This pull request keeps the research discussion available as evidence. Closing it removes the stale implementation lane.