diff --git a/docs/144-instance-storage-layout-review.md b/docs/144-instance-storage-layout-review.md new file mode 100644 index 0000000..7091ff0 --- /dev/null +++ b/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. diff --git a/docs/145-ttl-bump-frequency-review.md b/docs/145-ttl-bump-frequency-review.md new file mode 100644 index 0000000..d6a7c3d --- /dev/null +++ b/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). diff --git a/docs/146-compute-intent-id-preimage-buffer-benchmark.md b/docs/146-compute-intent-id-preimage-buffer-benchmark.md new file mode 100644 index 0000000..4ac8740 --- /dev/null +++ b/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(×tamp.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`. diff --git a/docs/147-intentrecord-solverrecord-field-footprint-review.md b/docs/147-intentrecord-solverrecord-field-footprint-review.md new file mode 100644 index 0000000..0c6f9f0 --- /dev/null +++ b/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()`, 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
` | — | 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` | unix seconds | Keep. | +| `fill_amount` | `Option` | 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.