chore: review storage footprint and compute-cost of hot paths (#144-#147)

docs/144-instance-storage-layout-review.md

@@ -0,0 +1,72 @@
+# Instance-Storage Layout — Single Entry vs. Split Entries
+
+**Issue:** [#144](https://github.com/stellar-vortex-protocol/vortex-contracts/issues/144)
+**Status:** Reviewed — no split recommended; before/after benchmark blocked
+on the resource-benchmarking harness (see
+[docs/149](149-resource-cost-per-entrypoint.md))
+
+---
+
+## 1. Soroban's instance-storage model (confirmed)
+
+All of a contract's **instance** storage is a single `ScMap` held in **one
+ledger entry** — the `ContractData` entry with `durability = instance`,
+which also carries the reference to the contract's executable. This is
+already stated in
+[docs/ttl-constants-rationale.md](ttl-constants-rationale.md): *"a single
+ledger entry that holds the contract's global state (`Admin`,
+`FeeRecipient`, `BondToken`, protocol stats) and the contract's own
+executable code."*
+
+Therefore the issue's premise is **correct**: `Admin`, `FeeRecipient`,
+`BondToken`, `TotalIntents`, `OpenIntents`, `TotalVolume`, `TotalSolvers`,
+`Paused`, `DstAllowlistEnabled`, `ProtocolConfig`, … all live in that one
+map, and any `env.storage().instance().set(...)` re-serializes and
+re-writes the **entire** map. `submit_intent` bumping `TotalIntents`
+rewrites every other instance key alongside it.
+
+## 2. Why splitting the hot counters is still not worth it
+
+Splitting `TotalIntents` / `TotalVolume` into their own entries means
+**persistent** storage (instance storage cannot have per-key entries). For
+each such counter, per call that touches it:
+
+| | Current (in instance map) | Split into a persistent entry |
+|---|---|---|
+| Read | free — instance entry already in footprint | **extra** persistent read (own footprint slot) |
+| Write | re-serialize the instance map (~a few hundred bytes of small scalars) | write a tiny dedicated entry … |
+| TTL | covered by the existing `bump_instance_ttl` | … **plus** its own `extend_ttl` + rent |
+
+The instance map here is ~a dozen small scalars and addresses. Re-writing
+it is a small "write bytes" cost. Moving a counter out trades that for a
+whole extra ledger entry with its own read, write, and TTL lifecycle —
+**strictly more ledger I/O per call**, not less.
+
+Splitting a combined entry only pays off when the entry is large enough
+that re-serializing the *unrelated* fields dominates. That is not the case
+here.
+
+## 3. Context: the counter update is already in the noise
+
+Every `submit_intent` also performs a **persistent `IntentRecord` write**
+plus a `UserNonce` read/write. Those persistent operations dwarf the cost
+of re-writing the small instance map. Optimising the instance-entry write
+would not move the needle on the entrypoint's total cost.
+
+## 4. Benchmark
+
+A before/after measurement (the issue asks for one via the
+resource-benchmarking harness) cannot be run: the harness does not exist in
+this repo — same blocker as [docs/149](149-resource-cost-per-entrypoint.md).
+
+## 5. Recommendation
+
+**Do not split.** Keep the single instance entry. This doc records the
+storage model (one entry, full rewrite on every instance write) so the
+trade-off is on file. Revisit only if either becomes true:
+
+- the instance map grows large (many big values), so unrelated-field
+  re-serialization becomes the dominant write cost; or
+- a counter becomes extremely hot on a path that does **not** already do a
+  persistent write, so a dedicated entry would not be adding an otherwise
+  absent persistent operation.

docs/145-ttl-bump-frequency-review.md

@@ -0,0 +1,70 @@
+# TTL Bump Frequency — Unconditional `extend_ttl` Review
+
+**Issue:** [#145](https://github.com/stellar-vortex-protocol/vortex-contracts/issues/145)
+**Status:** Reviewed — no change recommended
+**Scope:** `bump_instance_ttl` / `bump_intent_ttl` / `bump_solver_ttl`
+(`intent_settlement/src/lib.rs`), called on ~25 write paths.
+
+---
+
+## 1. The concern
+
+`bump_instance_ttl`, `bump_intent_ttl`, and `bump_solver_ttl` each call
+`extend_ttl` unconditionally on every write, e.g.:
+
+```rust
+env.storage().persistent().extend_ttl(
+    &DataKey::Intent(intent_id.clone()),
+    PERSISTENT_TTL_THRESHOLD,   // DAY_IN_LEDGERS * 14
+    PERSISTENT_TTL_EXTEND_TO,   // DAY_IN_LEDGERS * 30
+);
+```
+
+The worry: paying to bump TTL even when the entry has ~30 days of TTL left
+and is nowhere near the 14-day threshold.
+
+## 2. What `extend_ttl(threshold, extend_to)` actually does (soroban-sdk 21)
+
+The two-argument form is already conditional **inside the host**:
+
+- The host reads the entry's current `live_until_ledger`.
+- If `live_until_ledger - current_ledger > threshold` (TTL still healthy),
+  it **returns without doing anything else** — no ledger write is emitted
+  for the entry, and no rent is charged.
+- Only when the remaining TTL has decayed **below `threshold`** does it
+  extend `live_until_ledger` to `current_ledger + extend_to` and charge
+  rent for the added ledger range.
+
+So in the common case (entry written again well before 14 days elapse) the
+cost of each `bump_*` call is: **one host-function invocation performing a
+subtract-and-compare.** No write, no rent.
+
+## 3. Would a guest-side pre-check help?
+
+A pre-check would be:
+
+```rust
+let ttl = env.storage().persistent().get_ttl(&key); // host call
+if ttl < PERSISTENT_TTL_THRESHOLD {
+    env.storage().persistent().extend_ttl(&key, ..., ...); // host call
+}
+```
+
+`get_ttl` is itself a host-function call reading the same
+`live_until_ledger` field the host already inspects inside `extend_ttl`. In
+the common path this **replaces one cheap host call with one cheap host
+call** and adds branch logic; in the uncommon path it makes **two** host
+calls instead of one. Net negative.
+
+For `bump_instance_ttl` the instance entry is already loaded into the
+transaction footprint on every call, so the `extend_ttl` comparison is
+running against data the host holds anyway.
+
+## 4. Recommendation
+
+**No change.** The unconditional `extend_ttl(threshold, extend_to)` pattern
+is the idiomatic Soroban usage precisely because the host already
+short-circuits below-threshold. A guest-side threshold gate would add code
+and, in the common case, no saving — in the rare case, an extra host call.
+The current constants and their rationale are documented in
+[docs/ttl-constants-rationale.md](ttl-constants-rationale.md).

docs/146-compute-intent-id-preimage-buffer-benchmark.md

@@ -0,0 +1,68 @@
+# `compute_intent_id` — Preimage Construction vs. Fixed-Size Buffer
+
+**Issue:** [#146](https://github.com/stellar-vortex-protocol/vortex-contracts/issues/146)
+**Status:** Reviewed — no change recommended; hard baseline blocked on the
+resource-benchmarking harness (see [docs/149](149-resource-cost-per-entrypoint.md))
+
+---
+
+## 1. Current implementation
+
+`compute_intent_id` (`intent_settlement/src/lib.rs`) builds its SHA-256
+preimage as:
+
+```rust
+let mut preimage = Bytes::new(env);
+preimage.append(&user.clone().to_xdr(env));       // host call + to_xdr
+preimage.append(&src_chain.clone().to_xdr(env));  // host call + to_xdr
+preimage.extend_from_array(&amount.to_be_bytes());     // host call, 16 B
+preimage.extend_from_array(&timestamp.to_be_bytes());  // host call, 8 B
+preimage.extend_from_array(&nonce.to_be_bytes());      // host call, 8 B
+env.crypto().sha256(&preimage).into()                  // host call
+```
+
+Preimage size ≈ 40 (`Address` XDR) + ~12–20 (`String` XDR) + 32 (the three
+integers) ≈ **~85–95 bytes**, and it is **variable-length** because the
+`Address` and `String` XDR encodings vary.
+
+## 2. Why a fixed-size stack buffer does not cleanly apply
+
+`Bytes` is a host object in Soroban. The two costly-looking pieces —
+serializing `user` and `src_chain` — go through `to_xdr`, which **must**
+call the host; there is no guest-side XDR encoder for `Address`/`String`.
+So a `[u8; N]` buffer:
+
+- still needs `user.to_xdr(env)` and `src_chain.to_xdr(env)` (2 host calls,
+  unavoidable), then a guest-side `copy_from_slice` of their bytes;
+- cannot be truly fixed-size because those two lengths vary — it would need
+  to be over-provisioned to a worst-case `N` plus a length cursor.
+
+The only part that genuinely collapses is the **three integer appends**:
+`amount` (16) + `timestamp` (8) + `nonce` (8) can be packed into one
+`[u8; 32]` guest-side and appended with a single `extend_from_array`,
+removing **2 host calls** out of roughly 8.
+
+## 3. Expected effect
+
+The dominant cost in this function is `sha256` over ~90 bytes plus the two
+`to_xdr` host calls, all of which remain. Removing 2 of ~8 host boundary
+crossings for the integer packing is a sub-1% change to the function, and
+`compute_intent_id` is itself a small fraction of `submit_intent` (which
+also does a persistent `IntentRecord` write, nonce read/write, and instance
+bookkeeping).
+
+## 4. Baseline measurement
+
+This issue asks for a measured baseline first. The repo has **no
+resource-benchmarking harness** (no `benches/`, no `Budget`/instruction-count
+usage in `intent_settlement`) — the same blocker documented in
+[docs/149](149-resource-cost-per-entrypoint.md). A CPU-instruction baseline
+and A/B comparison cannot be produced on this branch.
+
+## 5. Recommendation
+
+**No change.** The candidate optimisation (packing the three trailing
+integers into one append) is small, its benefit is unmeasurable without the
+harness, and it trades the current straightforwardly-readable preimage build
+for a length-cursor buffer. Revisit only if the harness lands **and** a
+measurement shows a real instruction-count win on `submit_intent`.

docs/147-intentrecord-solverrecord-field-footprint-review.md

@@ -0,0 +1,88 @@
+# IntentRecord / SolverRecord — Field Type & Ordering Review
+
+**Issue:** [#147](https://github.com/stellar-vortex-protocol/vortex-contracts/issues/147)
+**Status:** Reviewed — no type/ordering change recommended
+**Scope:** `intent_settlement/src/lib.rs` — `IntentRecord` (structs section) and `SolverRecord`
+
+---
+
+## 1. How Soroban actually stores these structs
+
+A `#[contracttype]` struct is serialized to an `ScVal::Map` (`ScMap`): one
+entry per field, the key is `ScVal::Symbol(<field name>)`, and **the host
+sorts the entries by key**. Declaration order in the Rust source is *not*
+preserved on the wire.
+
+Consequence for this issue: **reordering the fields in the struct cannot
+change the stored footprint.** Any "pack the small fields together" style
+optimisation that would help a C struct has no effect here — the layout is
+a name-keyed, name-sorted map, not a packed record.
+
+Integer payload widths in the `ScVal` encoding:
+
+| Rust type | `ScVal` | Payload bytes |
+|---|---|---|
+| `bool` | `Bool` | 1 |
+| `u32` | `U32` | 4 |
+| `u64` | `U64` | 8 |
+| `i128` / `u128` | `I128` / `U128` | 16 |
+| `Address` | `Address` | ~32 + tag |
+| `String` / `Bytes` | length-prefixed | variable |
+| `Option::None` | `Void` | 0 (entry still present) |
+
+Each entry also carries the symbol key and per-`ScVal` tags, so the fixed
+overhead per field is larger than the payload for the small scalars.
+
+## 2. IntentRecord — field by field
+
+| Field | Type | Realistic range | Verdict |
+|---|---|---|---|
+| `intent_id` | `BytesN<32>` | SHA-256 output | Exact fit. |
+| `user` | `Address` | — | Required. |
+| `src_chain` | `String` | chain name, e.g. `"ethereum"` | Kept as `String`. Could become a `u8`/enum chain code, but `AllowedSrcChain(String)` and the public API are keyed on the string; that is an API change, not type-narrowing. Out of scope. |
+| `src_token` | `String` | source-chain token address (EVM `0x…` = 42 chars; other chains differ) | Must stay variable-width `String`. |
+| `src_amount` | `i128` | bounded by `MAX_AMOUNT = 1e30` | `1e30 > u64::MAX (~1.8e19)`, so 128 bits are genuinely needed. `u128` would save 0 bytes vs `i128`. Keep. |
+| `min_dst_amount` | `i128` | as above | Keep (128 bits needed). |
+| `solver` | `Option<Address>` | — | Required. |
+| `state` | `IntentState` | 8 unit variants | Already minimal. |
+| `created_at` | `u64` | unix seconds | `env.ledger().timestamp()` is `u64`; `u32` seconds overflow in 2106. Keep. |
+| `deadline` | `u64` | unix seconds | Keep (SDK-native). |
+| `filled_at` | `Option<u64>` | unix seconds | Keep. |
+| `fill_amount` | `Option<i128>` | cumulative dst tokens | 128 bits needed. |
+| `total_filled` | `i128` | cumulative dst tokens | 128 bits needed. |
+
+## 3. SolverRecord — field by field
+
+| Field | Type | Realistic range | Verdict |
+|---|---|---|---|
+| `address` | `Address` | — | **Duplicates the storage key** `DataKey::Solver(Address)`. The record is only ever loaded by address. Removing it saves a whole map entry (~36 B) but touches every construction/read site. See §4. |
+| `bond_amount` | `i128` | USDC smallest unit; realistically `< 1e15` | Fits `u64`, but kept `i128` for arithmetic consistency with the token-transfer paths and headroom. Marginal 8 B. |
+| `fills_completed` | `u32` | lifetime fill count | 4.2e9 ceiling — ample. Minimal. |
+| `fills_failed` | `u32` | as above | Minimal. |
+| `total_volume` | `i128` | cumulative dst volume over solver lifetime | Unbounded growth; 128 bits justified. |
+| `is_active` | `bool` | — | Minimal. |
+| `registered_at` | `u64` | unix seconds | Keep (SDK-native). |
+| `active_intents` | `u32` | concurrent accepted intents | Minimal. |
+| `last_slash_time` | `u64` | unix seconds | Keep. |
+
+## 4. Findings
+
+1. **Field ordering is a non-issue.** `#[contracttype]` structs serialize as
+   a key-sorted `ScMap`; no reordering can reduce the footprint.
+2. **Every integer field is already at the right width.** `u64` timestamps
+   are the SDK-native type and cannot safely drop to `u32`. The `i128`
+   amount fields are bounded by `MAX_AMOUNT` (`1e30`), which exceeds
+   `u64::MAX`, so they need 128 bits; switching them to `u128` saves zero
+   bytes. The `u32` counters are already minimal.
+3. **The only real footprint wins are structural, not type-level, and are
+   deliberately left out of this issue:**
+   - `SolverRecord.address` duplicates the `DataKey::Solver(addr)` key.
+   - `IntentRecord.fill_amount` and `IntentRecord.total_filled` appear to
+     track the same quantity (cumulative dst tokens delivered).
+   Each is roughly one whole map entry. Both change call-site logic and,
+   for `fill_amount`, event payloads, so they belong in their own issues
+   rather than a "type/ordering" pass.
+4. **Net recommendation: no type or ordering change.** Nothing in the
+   current definitions loses range headroom or wastes an integer width that
+   a safe narrowing could reclaim. No serialization tests change because no
+   field type changes.