From a22443f4e27067c61509a6b69753b80a59bec85c Mon Sep 17 00:00:00 2001 From: Artem Korotkikh <105195369+artkorotkikh-dfinity@users.noreply.github.com> Date: Mon, 24 Aug 2026 14:01:18 +0200 Subject: [PATCH 01/15] docs(ai): add the local-stack workflow for multi-PR specs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A spec that plans several atomic PRs has no page describing how to build it. The gap shows up as soon as you try: the feature only works once the last PR exists, but each PR has to land on its own, and nothing said where the whole thing gets assembled or tested. Records the linear stack — one branch per planned PR, each cut from its parent, so the tip is the complete feature and needs no integration branch. Adds the two rules that keep it honest: a fix goes to the branch that owns the code and then cascades, and syncing is a merge, never a rebase. Also adds the tips spec's build plan in its asset folder, where Step 7 already removes it once the feature ships, so the tracker cannot go stale in main. It carries two findings from the repo that shrink the spike: spender_subaccount is already present on TransferFromArgs in the vendored cycles-ledger types and in the frontend SDK, so open question 1 is behavioural only. The docs/ai/README.md map was missing the whole spec-driven-development subtree, which would have left the new page unreachable from AGENTS.md. Co-Authored-By: Claude Opus 5 --- docs/ai/README.md | 5 + .../build-plan.md | 96 ++++++++++++++ .../spec-driven-development/stacked-builds.md | 125 ++++++++++++++++++ docs/ai/spec-driven-development/workflow.md | 3 + 4 files changed, 229 insertions(+) create mode 100644 docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md create mode 100644 docs/ai/spec-driven-development/stacked-builds.md diff --git a/docs/ai/README.md b/docs/ai/README.md index c03bee685de..aa76106cbb0 100644 --- a/docs/ai/README.md +++ b/docs/ai/README.md @@ -15,6 +15,11 @@ docs/ai/ ├── README.md ← you are here ├── governance.md Truth hierarchy, boundaries, capabilities, meta-update rule ├── pr-and-ci.md PR title regex, body template, CI cheatsheet, local gates +├── PRODUCT.md Living description of all current product behaviours +├── spec-driven-development/ Spec-first workflow (Cowork → Claude Code) +│ ├── workflow.md The workflow itself, step by step +│ ├── stacked-builds.md Building a multi-PR spec as a local stack +│ └── specs/ One spec per feature / improvement / bugfix ├── integrations/ Third-party APIs/services: what data we fetch & where │ ├── README.md Provider index (Alchemy, Infura, Etherscan, OnRamper, …) │ └── .md One reference doc per provider diff --git a/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md b/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md new file mode 100644 index 00000000000..9dfb2adae68 --- /dev/null +++ b/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md @@ -0,0 +1,96 @@ +# Build plan — tips via link or QR code + +Working state for [`2026-08-05-feat-tips-via-link.md`](../2026-08-05-feat-tips-via-link.md), +built as a local stack per [`stacked-builds.md`](../../stacked-builds.md). + +Disposable by design: this file leaves with the rest of the folder at +[Step 7 — Post-merge cleanup](../../workflow.md#step-7--post-merge-cleanup-claude-code). +The spec stays the source of truth for **what** to build; this only records +**where** each piece lives. + +## The stack + +| # | Branch | Spec PR | Contains | Status | +| --- | ------------------------------ | ------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | ----------- | +| 1 | `feat/tips-1-backend` | PR-1 | tip store, `create_tip` / `get_tip` / `claim_tip` / `cancel_tip` / `get_my_tips`, claim-code hashing, atomic claim, expiry, rate limiter, pruning, candid + `npm run generate` | not started | +| 2 | `feat/tips-2-service` | PR-2 | `tip.api.ts` + `tip.services.ts`: approve → record with idempotent retry, link + QR construction, claim, cancel | not started | +| 3 | `feat/tips-3-sender-ui` | PR-3 | `Issue Tip` menu entry, intro modal, token picker + empty state, Issue Tip step, share screen, `tip.*` i18n | not started | +| 4 | `feat/tips-4-recipient-ui` | PR-4 | `/tip/` landing, Tip Status modal, II hand-off across the fragment, claim review, success, **unavailable** | not started | +| 5 | `feat/tips-5-history` | PR-5 | History with all five statuses, claimer principal on claimed rows, cancel action | not started | +| 6 | `feat/tips-6-reserved-balance` | PR-2b | subtract reserved amounts once, in the derived store the token list, send, swap and both MAX controls read | not started | + +**Why reserved-balance sits at the top, not at position 3 as the spec lists it.** +It touches the most load-bearing derived state in the app, so it is the slowest +branch to review — and nothing else in the stack depends on it. At the top it +blocks nothing; at position 3 it would hold up four UI PRs behind its review. + +Land bottom-up. Each branch is cut from its parent, and a fix goes to the branch +that owns the code, then cascades down. + +## PR-0 — the spike + +Throwaway, no branch, no PR. Everything above waits on it. It runs entirely +against a local replica: the local ck-ledgers are the real +`ic-icrc1-ledger.wasm` from `dfinity/ic` at the commit pinned in +[`scripts/download.ckbtc.sh`](../../../../../scripts/download.ckbtc.sh), so the +ICRC-2 mechanics proved locally are the mainnet ones. + +What it must answer: + +- [ ] **Open question 1** — `approve` to `{owner: backend, subaccount: H(tip_id)}` + followed by `icrc2_transfer_from` with the matching `spender_subaccount`, + against a live ledger. +- [ ] **Open question 3** — `expected_allowance` on replace, so a retried + creation cannot destroy a live reservation. +- [ ] **Open question 2** — which ledgers advertise ICRC-2 via + `icrc1_supported_standards`. The answer **is** the v1 token list, and it + settles the ckETH pending decision. This is the one part the local rig + cannot answer: local proves the wasm supports it, not what mainnet has + deployed. Query the mainnet ledgers read-only. + +Already established from the repo, so nobody re-derives it: + +- `spender_subaccount` exists on `TransferFromArgs` in + [`src/cycles_ledger/types/src/lib.rs`](../../../../../src/cycles_ledger/types/src/lib.rs) + (`Option`) — generated from a ledger candid, in a + canister this backend already calls ICRC-2 against. +- The frontend SDK exposes it too: `TransferFromParams.spender_subaccount` in + `@icp-sdk/canisters/ledger/icrc`, the package + [`icrc-ledger.api.ts`](../../../../../src/frontend/src/icp/api/icrc-ledger.api.ts) + already wraps. + +So the spec's premise for open question 1 — that the field is absent from every +vendored candid in this repo — is wrong. What remains is behavioural: does a +real ledger honour it. + +## The local rig + +Everything needed is already in the repo. `npm run deploy` brings up the backend, +a local `internet_identity`, and the real `icp_ledger`, `ckbtc_ledger`, +`cketh_ledger` and `ckusdc_ledger` — which is the entire v1 token set. + +| Step | Command | +| ----------------------------------------------------------- | ------------------------------------------- | +| Toolchain (pinned in [`dfx.json`](../../../../../dfx.json)) | `dfxvm install 0.26.1` | +| Replica | `dfx start --clean --background` | +| Canisters | `npm run deploy` | +| Fund the sender | `scripts/send.tokens.sh ` | +| App | `npm run dev` | + +Use **two local Internet Identity anchors in two browser profiles**: one sender, +one recipient that has never existed before. That is not a convenience — it is +the only way to test +[open questions 5 and 6](../2026-08-05-feat-tips-via-link.md#open-questions-facts-to-confirm) +(a brand-new principal claiming, and a fresh identity seeing the received token +without manual setup) for real. No unit test can fake a principal with no +profile. + +## Blocked on people, not on code + +These do not block building, but they block landing. Longest lead time first. + +| What | Gates | Owner | +| -------------------------------------------------------------------------------------------------------------------------- | ---------------- | -------------------------------- | +| Compliance sign-off on OISY holding a bounded, revocable authorisation over user funds for up to a week (open question 11) | branch 1 | — | +| The `Uncovered` wording — it is information about the sender | branch 4 | whoever owns the privacy promise | +| The five undrawn states, plus a light theme for every screen (the Figma page is dark-only) | branches 3, 4, 5 | design | diff --git a/docs/ai/spec-driven-development/stacked-builds.md b/docs/ai/spec-driven-development/stacked-builds.md new file mode 100644 index 00000000000..b81d1b24f5a --- /dev/null +++ b/docs/ai/spec-driven-development/stacked-builds.md @@ -0,0 +1,125 @@ +# Building a multi-PR spec as a local stack + +A spec from [`workflow.md`](./workflow.md) normally plans several atomic PRs — +commandments [2 and 3](../../../AGENTS.md#2-the-10-commandments-read-before-every-change) +require it. That creates a tension: each PR has to land on its own and stay small +enough to review, but the feature only works end to end once the last one exists. + +This page is how to resolve it locally — one branch per planned PR, stacked +linearly, so the tip of the stack is the whole feature. + +## The shape + +``` +main +└── feat/-1- PR 1 + └── feat/-2- PR 2 + └── feat/-3- PR 3 +``` + +Every branch is created from its **parent**, never from `main`. Three +consequences, and they are the whole point: + +- The **tip is the complete feature.** Check it out and the flow runs locally. + There is no integration branch to build and no merge to redo. +- **Each diff against its parent is exactly that PR's change**, so review stays + as small as the spec intended. +- **Stack order is dependency order.** If PR 2 needs the declarations PR 1 + generates with `npm run generate`, it simply has them. + +### Why linear, and not a tree + +Sibling branches off a shared parent look tidier, but then nothing contains the +whole feature: you need a separate integration branch that merges the siblings, +and it has to be rebuilt after every fix. A linear stack gets that integration +for free. The cost is that review serialises. + +Prefer linear. Branch sideways only when two PRs are genuinely independent +**and** both need review in parallel. + +## Naming + +`/--`, using the spec's own type prefix and +short slug: + +| Spec | Branch | +| --------------------------------------------- | -------------------------- | +| `2026-08-05-feat-tips-via-link.md`, PR 1 of 6 | `feat/tips-1-backend` | +| same spec, PR 4 of 6 | `feat/tips-4-recipient-ui` | + +The position number is what makes `git branch --list 'feat/tips-*'` print the +stack in order — which is the cheapest possible status view. + +## The three operations + +**Add the next branch** — from the current tip, not from `main`: + +```bash +git checkout -b feat/tips-3-sender-ui feat/tips-2-service +``` + +**Propagate a fix — downward, never sideways.** Fix on the branch that _owns_ +the code, then merge that branch into each descendant in order: + +```bash +git checkout feat/tips-3-sender-ui && git merge --no-edit feat/tips-2-service +``` + +Do not fix a bug at the tip just because that is where you noticed it. The fix +would land in the wrong PR and the earlier one would ship broken. This is the +single rule that keeps a stack honest, and the only one worth being strict about. + +**Sync with `main`** — merge into the bottom branch, then cascade: + +```bash +git checkout feat/tips-1-backend && git merge --no-edit origin/main +``` + +Merge, never rebase: rewriting a pushed branch is forbidden by +[pr-and-ci.md §7](../pr-and-ci.md#7-updating-an-existing-pr). Only sync when you +actually need something from `main` — being behind is fine, the merge queue +handles it at merge time. + +## On the remote + +- Open each PR with **its parent branch as base**, not `main`. GitHub then shows + only that PR's own diff, and retargets the child to `main` by itself when the + parent merges. +- Stacked PRs are the sanctioned alternative to force-pushing — see + [pr-and-ci.md §7](../pr-and-ci.md#7-updating-an-existing-pr). Never rebase a + stack to tidy it. +- **Land bottom-up.** A PR whose parent has not merged is not ready for the + merge queue. +- Name the parent PR in each body, so a reviewer knows what they are standing on. + +## Where the gates run + +| Where | What must pass | +| --------------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------ | +| Every branch, before it is pushed | the full [local gates](../pr-and-ci.md#4-local-quality-gates) — plus the backend scripts if Rust changed | +| The tip | the manual end-to-end run of the feature against a local replica ([HACKING.md → Local development](../../../HACKING.md#local-development)) | + +A branch that is green on its own but only _works_ with its descendants present +is a sign the split is wrong — either the dependency runs the wrong way, or the +two branches are one PR. + +## Folding + +If a branch turns out to be a handful of lines, fold it into its parent before +pushing instead of opening a PR nobody benefits from reviewing separately. The +spec's PR list is a plan, not a contract: commandment 3 asks for small PRs, not +for a specific number of them. + +## Tracking + +Keep the stack table in the spec's **asset folder** — `specs//build-plan.md` +— with one row per PR: branch, what it contains, status. + +It goes there rather than in the spec itself because it is disposable working +state, and the asset folder is already removed at +[Step 7 — Post-merge cleanup](./workflow.md#step-7--post-merge-cleanup-claude-code). +The tracker dies with the thing it tracks, which is the only way it never goes +stale in `main`. + +The spec stays the source of truth for **what** to build. The build plan only +records **where** each piece currently lives. diff --git a/docs/ai/spec-driven-development/workflow.md b/docs/ai/spec-driven-development/workflow.md index 10c345cba2c..3c59de5208c 100644 --- a/docs/ai/spec-driven-development/workflow.md +++ b/docs/ai/spec-driven-development/workflow.md @@ -16,6 +16,7 @@ dfinity/oisy-wallet/ ├── PRODUCT.md # Living description of all current product behaviors └── spec-driven-development/ ├── workflow.md # This document + ├── stacked-builds.md # Building a multi-PR spec as a local stack └── specs/ ├── 2026-05-10-feat-add-token-swapping.md ├── 2026-05-24-fix-wallet-sync-race.md @@ -87,6 +88,8 @@ Open Claude Code in the oisy-wallet repo and say: Claude Code reads the spec, reads `docs/ai/PRODUCT.md` for system context, and begins building. It has the GitHub MCP configured and can open PRs, create branches, and interact with issues directly. +**Specs that plan more than one PR** — most of them do — are built as a local stack, one branch per planned PR, so the tip of the stack is the whole feature and can be tested end to end before anything is pushed. See [`stacked-builds.md`](./stacked-builds.md). + **Update `docs/ai/PRODUCT.md` in the same PR** as the behaviour change, not afterwards. Claude Code is best placed to write the description because by the time the PR is ready, it has the implementation context (what _actually_ shipped, including any [Step 5 — Adjust](#step-5--adjust-claude-code--spec) deviations from the spec). Landing PRODUCT.md alongside the code also keeps `main` from briefly disagreeing with itself between merge and the cleanup PR. Cowork can still review the draft if a product re-think emerges. ### Step 5 — Adjust (Claude Code ↔ Spec) From 0719304a87e060caceee91c1024ae338aab42c58 Mon Sep 17 00:00:00 2001 From: Artem Korotkikh <105195369+artkorotkikh-dfinity@users.noreply.github.com> Date: Mon, 24 Aug 2026 14:06:37 +0200 Subject: [PATCH 02/15] docs(ai): record the spike findings the local rig already settled MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Open question 1 is answered at the interface level: the ledger candid the local replica installs declares TransferFromArgs.spender_subaccount and all three icrc2_* methods, in both implementations that matter — the ICRC-1 ledger shared by ckBTC, ckETH and ckUSDC, and the ICP ledger. ApproveArgs also carries expected_allowance and expires_at, which is what lets the allowance hold the tip's own deadline. That resolves the ckETH pending decision: all four v1 candidates expose ICRC-2, so ckETH can join v1. What is left is behaviour rather than interface — a declared field is not a working one — so the spike shrinks to the live approve/transfer_from round-trip, expected_allowance on replace, and a read-only check that mainnet runs a version matching these candids. Co-Authored-By: Claude Opus 5 --- .../build-plan.md | 64 ++++++++++++------- 1 file changed, 41 insertions(+), 23 deletions(-) diff --git a/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md b/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md index 9dfb2adae68..623539339c5 100644 --- a/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md +++ b/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md @@ -35,33 +35,51 @@ against a local replica: the local ck-ledgers are the real [`scripts/download.ckbtc.sh`](../../../../../scripts/download.ckbtc.sh), so the ICRC-2 mechanics proved locally are the mainnet ones. -What it must answer: - -- [ ] **Open question 1** — `approve` to `{owner: backend, subaccount: H(tip_id)}` - followed by `icrc2_transfer_from` with the matching `spender_subaccount`, - against a live ledger. -- [ ] **Open question 3** — `expected_allowance` on replace, so a retried - creation cannot destroy a live reservation. -- [ ] **Open question 2** — which ledgers advertise ICRC-2 via - `icrc1_supported_standards`. The answer **is** the v1 token list, and it - settles the ckETH pending decision. This is the one part the local rig - cannot answer: local proves the wasm supports it, not what mainnet has - deployed. Query the mainnet ledgers read-only. - -Already established from the repo, so nobody re-derives it: - -- `spender_subaccount` exists on `TransferFromArgs` in +**Settled at the interface level** by reading the ledger candid that the local +replica actually installs (`target/ic/*.did`, downloaded from the pinned IC +commit — the same wasm the mainnet ck-ledgers run): + +- [x] **Open question 1, candid half.** `TransferFromArgs.spender_subaccount : opt Subaccount` + is declared, and `icrc2_approve` / `icrc2_allowance` / `icrc2_transfer_from` + are all in the service — in **both** ledger implementations that matter: + `ckbtc_ledger.did` (the ICRC-1 ledger, shared by ckBTC, ckETH and ckUSDC) + and `icp_ledger.did`. +- [x] **Open question 2, interface half.** All four v1 candidates — ICP, ckBTC, + ckETH, ckUSDC — expose ICRC-2. So the third + [pending decision](../2026-08-05-feat-tips-via-link.md#pending-decisions-facts-clear--owner-must-decide) + resolves: **ckETH can join v1.** +- [x] `ApproveArgs` carries both `expected_allowance : opt nat` and + `expires_at : opt Timestamp`, which is what lets the allowance carry the + tip's own deadline instead of relying on the backend record alone. +- [x] Confirmed against the **installed** wasm, not only the candid: + `dfx canister call icp_ledger icrc1_supported_standards '()'` on the local + replica returns ICRC-1, **ICRC-2** and ICRC-21. + +Also already established from the repo, so nobody re-derives it: + +- `spender_subaccount` on `TransferFromArgs` in [`src/cycles_ledger/types/src/lib.rs`](../../../../../src/cycles_ledger/types/src/lib.rs) - (`Option`) — generated from a ledger candid, in a - canister this backend already calls ICRC-2 against. -- The frontend SDK exposes it too: `TransferFromParams.spender_subaccount` in - `@icp-sdk/canisters/ledger/icrc`, the package - [`icrc-ledger.api.ts`](../../../../../src/frontend/src/icp/api/icrc-ledger.api.ts) + — the Rust shape the backend needs, in a canister this backend already calls + ICRC-2 against. +- `TransferFromParams.spender_subaccount` in `@icp-sdk/canisters/ledger/icrc`, + the package [`icrc-ledger.api.ts`](../../../../../src/frontend/src/icp/api/icrc-ledger.api.ts) already wraps. So the spec's premise for open question 1 — that the field is absent from every -vendored candid in this repo — is wrong. What remains is behavioural: does a -real ledger honour it. +vendored candid in this repo — was wrong twice over. What is left is behaviour, +not interface. + +**Still to prove:** + +- [ ] **Open question 1, behavioural half.** `approve` to + `{owner: backend, subaccount: H(tip_id)}` followed by `icrc2_transfer_from` + with the matching `spender_subaccount`, executed against the local ledger. + A declared field is not a working one. +- [ ] **Open question 3.** `expected_allowance` on replace, so a retried creation + cannot silently destroy a live reservation. +- [ ] **Open question 2, deployment half.** `icrc1_supported_standards` on the + **mainnet** ledgers, read-only. Local proves the wasm supports ICRC-2, not + that mainnet runs a version that does. ## The local rig From a1ddee4ad00124a84bfdb6960dba60d80bc68548 Mon Sep 17 00:00:00 2001 From: Artem Korotkikh <105195369+artkorotkikh-dfinity@users.noreply.github.com> Date: Mon, 24 Aug 2026 14:16:05 +0200 Subject: [PATCH 03/15] docs(ai): record the spike results proven against the local ledger The behavioural half of open question 1 is answered, driven by two dfx identities against the local ckBTC ledger. The load-bearing result is that a spender subaccount genuinely scopes an allowance, three ways: the allowance for the bare spender reads zero, transfer_from without a spender_subaccount is refused, and so is transfer_from with a different one. One tip therefore cannot draw on another tip's allowance even with the same sender and the same spender canister, which is what makes per-tip reservations safe under a single backend principal. Also measured rather than assumed: the payout fee comes out of the allowance rather than on top of it; a stale expected_allowance fails with the current value attached, so a retry can reconcile; approve replaces rather than adds; and expires_at is enforced by the ledger, with a past deadline rejected outright. Only the read-only mainnet check remains. Co-Authored-By: Claude Opus 5 --- .../build-plan.md | 45 ++++++++++++++----- 1 file changed, 35 insertions(+), 10 deletions(-) diff --git a/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md b/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md index 623539339c5..ae4367ae5e0 100644 --- a/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md +++ b/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md @@ -69,17 +69,42 @@ So the spec's premise for open question 1 — that the field is absent from ever vendored candid in this repo — was wrong twice over. What is left is behaviour, not interface. -**Still to prove:** - -- [ ] **Open question 1, behavioural half.** `approve` to - `{owner: backend, subaccount: H(tip_id)}` followed by `icrc2_transfer_from` - with the matching `spender_subaccount`, executed against the local ledger. - A declared field is not a working one. -- [ ] **Open question 3.** `expected_allowance` on replace, so a retried creation - cannot silently destroy a live reservation. +**Proven against the local ckBTC ledger** (`icrc2_approve` / `icrc2_transfer_from` +driven by two dfx identities, sender and spender, with a 32-byte subaccount +standing in for `H(tip_id)`): + +- [x] **Open question 1, behavioural half.** `approve` to + `{owner: spender, subaccount: SUB_A}` then `icrc2_transfer_from` with the + matching `spender_subaccount` pays out: the claimer's balance went from 0 to + exactly the transferred amount. +- [x] **The subaccount genuinely scopes the allowance** — this is the property the + whole no-custody model rests on, and it holds three ways: + `icrc2_allowance` for `{spender, no subaccount}` reads **0** while + `{spender, SUB_A}` reads the full amount; `transfer_from` **without** a + `spender_subaccount` fails `InsufficientAllowance { allowance = 0 }`; and + `transfer_from` with a **different** subaccount (`SUB_B`) fails the same way. + So one tip cannot draw on another tip's allowance, even with the same sender + and the same spender canister. +- [x] **The payout fee comes out of the allowance, not on top of it.** After + transferring 100_000 with a ledger fee of 11_500, the remaining allowance + fell by 111_500. This is what the spec's "allowance covers the amount plus + the payout fee" requires, now measured rather than assumed. +- [x] **Open question 3.** `approve` with a stale `expected_allowance` fails + `AllowanceChanged { current_allowance }` — and the error carries the current + value, so a retry can reconcile instead of guessing. With the correct + `expected_allowance` it succeeds, and it **replaces** rather than adds: the + allowance became exactly the new amount. +- [x] **`expires_at` is enforced by the ledger.** A future deadline round-trips — + `icrc2_allowance` returns it alongside the amount — and an `approve` with a + past deadline is rejected outright with `Expired { ledger_time }`. So the + reservation carries the tip's own deadline; the backend record is the second + line of defence, not the only one. + +**Still open:** + - [ ] **Open question 2, deployment half.** `icrc1_supported_standards` on the - **mainnet** ledgers, read-only. Local proves the wasm supports ICRC-2, not - that mainnet runs a version that does. + **mainnet** ledgers, read-only. Local proves the wasm behaves; it does not + prove mainnet runs a version matching these candids. ## The local rig From ae4d24fc34a39fd4e748b59723dcc8a71e5edb88 Mon Sep 17 00:00:00 2001 From: Artem Korotkikh <105195369+artkorotkikh-dfinity@users.noreply.github.com> Date: Tue, 25 Aug 2026 09:41:08 +0200 Subject: [PATCH 04/15] docs(ai): close the tips spike against the mainnet ledgers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Read-only queries confirm every v1 candidate reports ICRC-2 in production, so the token list is all five the spec named: ICP, ckBTC, ckETH, ckUSDC and ckUSDT. PR-0 is complete and nothing in the stack is blocked on the mechanism. One trap found on the way: dfx.json maps ckusdc_ledger's ic entry to yfumr-cyaaa-aaaar-qaela-cai, which answers icrc1_symbol with ckSepoliaUSDC — the testnet ledger, on mainnet. Production ck-ERC20 ids live in tokens.ckerc20.json, which is what the frontend reads. ICP, ckBTC and ckETH are the ledgers their dfx.json ic entries claim; the ck-ERC20 ones are not, so the token picker must not source them there. Co-Authored-By: Claude Opus 5 --- .../build-plan.md | 32 +++++++++++++++---- 1 file changed, 25 insertions(+), 7 deletions(-) diff --git a/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md b/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md index ae4367ae5e0..19ffa5ca51b 100644 --- a/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md +++ b/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md @@ -27,9 +27,9 @@ blocks nothing; at position 3 it would hold up four UI PRs behind its review. Land bottom-up. Each branch is cut from its parent, and a fix goes to the branch that owns the code, then cascades down. -## PR-0 — the spike +## PR-0 — the spike (complete) -Throwaway, no branch, no PR. Everything above waits on it. It runs entirely +Throwaway, no branch, no PR. Everything above waited on it; nothing does now. It runs entirely against a local replica: the local ck-ledgers are the real `ic-icrc1-ledger.wasm` from `dfinity/ic` at the commit pinned in [`scripts/download.ckbtc.sh`](../../../../../scripts/download.ckbtc.sh), so the @@ -100,11 +100,29 @@ standing in for `H(tip_id)`): reservation carries the tip's own deadline; the backend record is the second line of defence, not the only one. -**Still open:** - -- [ ] **Open question 2, deployment half.** `icrc1_supported_standards` on the - **mainnet** ledgers, read-only. Local proves the wasm behaves; it does not - prove mainnet runs a version matching these candids. +**Closed against mainnet** (read-only `icrc1_supported_standards` / +`icrc1_symbol` queries, `--network ic`): + +- [x] **Open question 2, deployment half.** Every v1 candidate reports ICRC-2 in + production. So the **v1 token list is all five the spec named**: + +| Token | Mainnet ledger | Standards | +| ------ | ----------------------------- | ---------------------------------- | +| ICP | `ryjl3-tyaaa-aaaaa-aaaba-cai` | ICRC-1, **2**, 21 | +| ckBTC | `mxzaz-hqaaa-aaaar-qaada-cai` | ICRC-1, **2**, 3, 10, 21, 103, 106 | +| ckETH | `ss2fx-dyaaa-aaaar-qacoq-cai` | ICRC-1, **2**, 3, 10, 21, 103, 106 | +| ckUSDC | `xevnm-gaaaa-aaaar-qafnq-cai` | ICRC-1, **2**, 3, 10, 21, 103, 106 | +| ckUSDT | `cngnf-vqaaa-aaaar-qag4q-cai` | ICRC-1, **2**, 3, 10, 21, 103, 106 | + +**Do not take ck-ERC20 ledger ids from `dfx.json`.** Its `remote.id.ic` entry for +`ckusdc_ledger` is `yfumr-cyaaa-aaaar-qaela-cai`, which answers `icrc1_symbol` +with **`ckSepoliaUSDC`** — the testnet ledger, on mainnet. Production ck-ERC20 +ids live in +[`tokens.ckerc20.json`](../../../../../src/frontend/src/env/tokens/tokens.ckerc20.json), +which is what the frontend actually reads. ICP, ckBTC and ckETH are the ledgers +their `dfx.json` `ic` entries claim; the ck-ERC20 ones are not. + +**PR-0 is complete.** Nothing in the stack is blocked on the mechanism any more. ## The local rig From aa7b4dc25d85614f2b60bed1ca2234d9ae3cd799 Mon Sep 17 00:00:00 2001 From: Artem Korotkikh <105195369+artkorotkikh-dfinity@users.noreply.github.com> Date: Tue, 25 Aug 2026 13:57:32 +0200 Subject: [PATCH 05/15] docs(ai): record the built stack and what it still owes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tracker still said "not started" for all six branches. It now carries each branch's head, the seventh (the flag flip), and why the claim page had to leave the `(app)` group — the layout guard was discarding it for exactly the visitor it serves. Splits the leftovers by who can act. Analytics and the `Uncovered` history status are code. The fragment surviving Internet Identity in an in-app webview, a fresh anchor seeing its token, and a claim crossing a canister upgrade are assertions nothing has measured yet — kept separate from the sign-offs, because "blocked on a person" and "nobody has checked" fail very differently. Co-Authored-By: Claude Opus 5 --- .../build-plan.md | 50 ++++++++++++++++--- 1 file changed, 42 insertions(+), 8 deletions(-) diff --git a/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md b/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md index 19ffa5ca51b..03817412e78 100644 --- a/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md +++ b/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md @@ -10,14 +10,27 @@ The spec stays the source of truth for **what** to build; this only records ## The stack -| # | Branch | Spec PR | Contains | Status | -| --- | ------------------------------ | ------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | ----------- | -| 1 | `feat/tips-1-backend` | PR-1 | tip store, `create_tip` / `get_tip` / `claim_tip` / `cancel_tip` / `get_my_tips`, claim-code hashing, atomic claim, expiry, rate limiter, pruning, candid + `npm run generate` | not started | -| 2 | `feat/tips-2-service` | PR-2 | `tip.api.ts` + `tip.services.ts`: approve → record with idempotent retry, link + QR construction, claim, cancel | not started | -| 3 | `feat/tips-3-sender-ui` | PR-3 | `Issue Tip` menu entry, intro modal, token picker + empty state, Issue Tip step, share screen, `tip.*` i18n | not started | -| 4 | `feat/tips-4-recipient-ui` | PR-4 | `/tip/` landing, Tip Status modal, II hand-off across the fragment, claim review, success, **unavailable** | not started | -| 5 | `feat/tips-5-history` | PR-5 | History with all five statuses, claimer principal on claimed rows, cancel action | not started | -| 6 | `feat/tips-6-reserved-balance` | PR-2b | subtract reserved amounts once, in the derived store the token list, send, swap and both MAX controls read | not started | +| # | Branch | Spec PR | Head | Contains | Status | +| --- | ------------------------------ | ------- | ----------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------ | +| 1 | `feat/tips-1-backend` | PR-1 | `34b6e076d` | tip store, `create_tip` / `get_tip` / `get_tip_details` / `claim_tip` / `cancel_tip` / `get_my_tips`, claim-code hashing, atomic claim, expiry, rate limiters, pruning, candid | built | +| 2 | `feat/tips-2-service` | PR-2 | `0c6f8f2e1` | `base64url.utils`, `tip.crypto`, `tip.services`, api + canister layer, pinned cross-language hash vectors | built | +| 3 | `feat/tips-3-sender-ui` | PR-3 | `c311f6c4c` | `Issue Tip` menu entry, intro, token picker + empty state, create step, expiry, share screen with QR, `tip.*` i18n, flag off | built | +| 4 | `feat/tips-4-recipient-ui` | PR-4 | `bc3997164` | `/tip/` **standalone landing page**, claim review, success, **unavailable**, **uncovered** | built | +| 5 | `feat/tips-5-history` | PR-5 | `16e88309a` | History with four stored statuses, claimer principal on claimed rows, cancel action | built | +| 6 | `feat/tips-6-reserved-balance` | PR-2b | `d712296f6` | subtract reserved amounts once, in the derived store the token list, send, swap and both MAX controls read | built | +| 7 | `feat/tips-7-enable` | PR-6 | `2654f16aa` | flip `TIPS_ENABLED` to `true` — the release, on its own, after everything above lands | built | + +24 commits over `main`, none pushed. `main` here is the merged spec +([#13768](https://github.com/dfinity/oisy-wallet/pull/13768)). + +**Why the claim page is a standalone route.** It started under `(app)`, which +looked right — the claim needs an authenticated agent. But `AuthGuard` swaps the +whole route out for the marketing landing page whenever there is no identity, +which is exactly the visitor a tip link arrives at, so the claim view never +rendered for them at all. It is now a `+page@` that resets the layout hierarchy, +the same shape as the shared-note recipient page. Component tests could not have +caught this: they render the component directly, above the layout that was +discarding it. **Why reserved-balance sits at the top, not at position 3 as the spec lists it.** It touches the most load-bearing derived state in the app, so it is the slowest @@ -155,3 +168,24 @@ These do not block building, but they block landing. Longest lead time first. | Compliance sign-off on OISY holding a bounded, revocable authorisation over user funds for up to a week (open question 11) | branch 1 | — | | The `Uncovered` wording — it is information about the sender | branch 4 | whoever owns the privacy promise | | The five undrawn states, plus a light theme for every screen (the Figma page is dark-only) | branches 3, 4, 5 | design | + +## Still to build + +Code, not sign-off. Neither blocks the stack from landing; both are worth their +own branch on top. + +| What | Where it goes | +| ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------- | +| **Analytics.** Nothing is instrumented. The spec's [Analytics section](../2026-08-05-feat-tips-via-link.md#analytics-plausible) names the funnel — open → token → created, and landing → sign-in → claimed. That second funnel is the number the feature exists to produce: cold-start conversion of a non-crypto recipient. Follow `personal-notes-analytics.services.ts`. | a branch of its own | +| **`Uncovered` in History.** Criterion 15 lists it as a sender-visible status; `tip-status.utils.ts` deliberately cannot show it, because it is the outcome of a claim attempt rather than a stored state, and History would have to query every tip's allowance on every read. Either accept the deviation and amend the spec, or pay for the query. | decision, then either | + +## Not yet verified + +Facts the stack asserts but nothing has measured. Each is a real risk, not a +formality. + +| What | How to close it | +| -------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------- | +| **The fragment surviving Internet Identity on mobile Safari and in in-app webviews** (open question 8, second half). The entire link model rests on it. Desktop is fine — II opens in a popup, so the page never unmounts — but an in-app webview may not keep it. | a real phone, and a link opened from a DM | +| **A fresh identity seeing the received token without manual setup** (open question 6). The backend half is covered by `a_tip_pays_a_brand_new_principal_…`; the wallet-UI half is not. | claim on a never-before-used anchor, then look | +| **Claim atomicity across a canister upgrade mid-flight** (open question 4). The design answers it — `Claiming` plus a five-minute in-flight timeout — but no test upgrades the canister while a claim is in the air. | a pocket-ic test that upgrades mid-claim | From dc409e1a3150b93d4d42c240589122024f6b7cea Mon Sep 17 00:00:00 2001 From: Artem Korotkikh <105195369+artkorotkikh-dfinity@users.noreply.github.com> Date: Thu, 27 Aug 2026 22:14:53 +0200 Subject: [PATCH 06/15] docs(ai): record the deploy mechanics and where be1 actually stands MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The tracker had the stack table and the spike results but nothing about how the work reaches be1, and the two mechanics that produced wrong diagnoses were only in a conversation. Adds four sections: - **Deploying to be1 / fe1** — the three commits the deploy branch carries and must never leak into a feature PR, the dispatch commands, and why the frontend must go first: adding a case to a variant the canister *returns* is breaking, and three of ours are. - **Mechanics that cost real time** — the vetKD key name frozen by `Cell::init` at first init, `ECDSA_KEY_NAME` doubling as the vetKD key name with `test_be_*` falling through to a local-only key, the cost of swallowing a failure silently, and the tooling that reflows `backend.did` behind your back. - **Open decisions** — five, each with the default that applies if nobody answers. - **Where be1 stands** — deployed, tips working, recovery still blocked on a reserved-cycles limit that needs a controller, with both remedies written out. Also refreshes the branch heads (241 commits over main now), and marks analytics and `Uncovered`-in-History as done and superseded respectively rather than deleting the original notes, so the reasoning stays readable. Co-Authored-By: Claude Opus 5 --- .../build-plan.md | 150 +++++++++++++++--- icrc1-ledger.wasm.gz | 1 + 2 files changed, 131 insertions(+), 20 deletions(-) create mode 120000 icrc1-ledger.wasm.gz diff --git a/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md b/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md index 03817412e78..3d07f061774 100644 --- a/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md +++ b/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md @@ -10,17 +10,17 @@ The spec stays the source of truth for **what** to build; this only records ## The stack -| # | Branch | Spec PR | Head | Contains | Status | -| --- | ------------------------------ | ------- | ----------- | ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ------ | -| 1 | `feat/tips-1-backend` | PR-1 | `34b6e076d` | tip store, `create_tip` / `get_tip` / `get_tip_details` / `claim_tip` / `cancel_tip` / `get_my_tips`, claim-code hashing, atomic claim, expiry, rate limiters, pruning, candid | built | -| 2 | `feat/tips-2-service` | PR-2 | `0c6f8f2e1` | `base64url.utils`, `tip.crypto`, `tip.services`, api + canister layer, pinned cross-language hash vectors | built | -| 3 | `feat/tips-3-sender-ui` | PR-3 | `c311f6c4c` | `Issue Tip` menu entry, intro, token picker + empty state, create step, expiry, share screen with QR, `tip.*` i18n, flag off | built | -| 4 | `feat/tips-4-recipient-ui` | PR-4 | `bc3997164` | `/tip/` **standalone landing page**, claim review, success, **unavailable**, **uncovered** | built | -| 5 | `feat/tips-5-history` | PR-5 | `16e88309a` | History with four stored statuses, claimer principal on claimed rows, cancel action | built | -| 6 | `feat/tips-6-reserved-balance` | PR-2b | `d712296f6` | subtract reserved amounts once, in the derived store the token list, send, swap and both MAX controls read | built | -| 7 | `feat/tips-7-enable` | PR-6 | `2654f16aa` | flip `TIPS_ENABLED` to `true` — the release, on its own, after everything above lands | built | - -24 commits over `main`, none pushed. `main` here is the merged spec +| # | Branch | Spec PR | Head | Contains | Status | +| --- | ------------------------------ | ------- | ----------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | ------ | +| 1 | `feat/tips-1-backend` | PR-1 | `ee4dcab82` | tip store, `create_tip` / `get_tip` / `get_tip_details` / `claim_tip` / `cancel_tip` / `get_my_tips`, claim-code hashing, atomic claim, expiry, rate limiters, pruning, candid | built | +| 2 | `feat/tips-2-service` | PR-2 | `7f34c4674` | `base64url.utils`, `tip.crypto`, `tip.services`, api + canister layer, pinned cross-language hash vectors | built | +| 3 | `feat/tips-3-sender-ui` | PR-3 | `3848ef5e1` | `Issue Tip` menu entry, intro, token picker + empty state, create step, expiry, share screen with QR, `tip.*` i18n, flag off | built | +| 4 | `feat/tips-4-recipient-ui` | PR-4 | `2eb4e9dbd` | `/tip/` **standalone landing page**, claim review, success, **unavailable**, **uncovered** | built | +| 5 | `feat/tips-5-history` | PR-5 | `4d607e162` | History with four stored statuses, claimer principal on claimed rows, cancel action | built | +| 6 | `feat/tips-6-reserved-balance` | PR-2b | `d3b78adb8` | subtract reserved amounts once, in the derived store the token list, send, swap and both MAX controls read | built | +| 7 | `feat/tips-7-enable` | PR-6 | `10666311b` | flip `TIPS_ENABLED` to `true` — the release, on its own, after everything above lands | built | + +241 commits over `main`, none pushed. `main` here is the merged spec ([#13768](https://github.com/dfinity/oisy-wallet/pull/13768)). **Why the claim page is a standalone route.** It started under `(app)`, which @@ -174,18 +174,128 @@ These do not block building, but they block landing. Longest lead time first. Code, not sign-off. Neither blocks the stack from landing; both are worth their own branch on top. -| What | Where it goes | -| ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------- | -| **Analytics.** Nothing is instrumented. The spec's [Analytics section](../2026-08-05-feat-tips-via-link.md#analytics-plausible) names the funnel — open → token → created, and landing → sign-in → claimed. That second funnel is the number the feature exists to produce: cold-start conversion of a non-crypto recipient. Follow `personal-notes-analytics.services.ts`. | a branch of its own | -| **`Uncovered` in History.** Criterion 15 lists it as a sender-visible status; `tip-status.utils.ts` deliberately cannot show it, because it is the outcome of a claim attempt rather than a stored state, and History would have to query every tip's allowance on every read. Either accept the deviation and amend the spec, or pay for the query. | decision, then either | +| What | Where it goes | +| ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | --------------------- | +| ~~**Analytics.**~~ **Done.** One `tip` event with the step in `event_modifier` and the side in `source_location`, per `trackPersonalNoteShare`. Sender steps on branch 3, claimer steps on branch 4. No amounts, ever — the symbol answers "which assets get tipped" without making the stream a spending log. Original note: The spec's [Analytics section](../2026-08-05-feat-tips-via-link.md#analytics-plausible) names the funnel — open → token → created, and landing → sign-in → claimed. That second funnel is the number the feature exists to produce: cold-start conversion of a non-crypto recipient. Follow `personal-notes-analytics.services.ts`. | a branch of its own | +| ~~**`Uncovered` in History.**~~ **Superseded.** The canister now records a failed claim on the tip (`last_claim_failure`) and reports `TipStatus::Failed`, so History shows it without querying any allowance. Original note: `tip-status.utils.ts` deliberately cannot show it, because it is the outcome of a claim attempt rather than a stored state, and History would have to query every tip's allowance on every read. Either accept the deviation and amend the spec, or pay for the query. | decision, then either | ## Not yet verified Facts the stack asserts but nothing has measured. Each is a real risk, not a formality. -| What | How to close it | -| -------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------- | -| **The fragment surviving Internet Identity on mobile Safari and in in-app webviews** (open question 8, second half). The entire link model rests on it. Desktop is fine — II opens in a popup, so the page never unmounts — but an in-app webview may not keep it. | a real phone, and a link opened from a DM | -| **A fresh identity seeing the received token without manual setup** (open question 6). The backend half is covered by `a_tip_pays_a_brand_new_principal_…`; the wallet-UI half is not. | claim on a never-before-used anchor, then look | -| **Claim atomicity across a canister upgrade mid-flight** (open question 4). The design answers it — `Claiming` plus a five-minute in-flight timeout — but no test upgrades the canister while a claim is in the air. | a pocket-ic test that upgrades mid-claim | +| What | How to close it | +| ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------- | +| **The fragment surviving Internet Identity on mobile Safari and in in-app webviews** (open question 8, second half). The entire link model rests on it. Desktop is fine — II opens in a popup, so the page never unmounts — but an in-app webview may not keep it. | a real phone, and a link opened from a DM | +| **A fresh identity seeing the received token without manual setup** (open question 6). The wallet-UI half is now built — `TipClaimModal` calls `autoLoadSingleToken` on the way out, so a claimed ck-asset is enabled — but nobody has claimed on a never-before-used anchor and looked. | claim on a never-before-used anchor, then look | +| **Claim atomicity across a canister upgrade mid-flight** (open question 4). The design answers it — `Claiming` plus a five-minute in-flight timeout — but no test upgrades the canister while a claim is in the air. | a pocket-ic test that upgrades mid-claim | + +## Deploying to be1 / fe1 + +The stack is exercised on the shared test environment from `test/tips-be1`, which +is branch 7 plus three commits that must **never** reach a feature PR: + +- `dfx.json` and `canister_ids.json`, pointing `test_fe_1`'s backend at be1 + (`jloto-byaaa-aaaap-anryq-cai`). Without this fe1 talks to **staging**, which + has no tips endpoints at all. +- `scripts/build.backend.args.sh`, giving `test_be_*` / `test_fe_*` a real vetKD + key name. This one belongs in main as its own reviewed change — it is a shared + script other teams build from. + +`git diff feat/tips-7-enable test/tips-be1 --stat` must show only those three +files. Anything else means work landed on the deploy branch by mistake. + +```bash +gh workflow run deploy-to-environment.yml --ref test/tips-be1 -f network=test_fe_1 -f canister=frontend +gh workflow run deploy-to-environment.yml --ref test/tips-be1 -f network=test_be_1 -f canister=backend -f force-backend=true +``` + +**Frontend first.** Adding a case to a variant the canister _returns_ is a +breaking candid change: a client that does not know the case cannot decode it, and +dfx says so out loud. `TipStatus::Failed`, `TipError::InsufficientFunds` and +`TipClaimFailureReason::InsufficientFunds` are all in return position. Adding an +`opt` record field is safe in either direction. An earlier deploy went +backend-first and only got away with it because the new variants require a failed +claim to appear. + +**Verify from the artefacts, not the green checks.** Grepping the bundle proves +nothing — `app.constants.ts` embeds the staging fallback alongside the selected +id, so both appear. Load `https://fe1.oisy.com/tip/probe/#c=probe` and read which +canister the page actually calls. Expect transient 503 `no_healthy_nodes` right +after an asset deploy; it clears on reload. + +## Mechanics that cost real time + +Each of these produced a wrong diagnosis first. + +**The vetKD key name is frozen at first init.** `KeyManager` keeps its key id in a +`StableCell`, and `Cell::init` _loads_ the stored value whenever the region is +non-empty — it writes the value passed in only when the memory is fresh. So a +store uses whichever key name was configured the first time it was ever touched, +permanently, and correcting the deployment argument does nothing. The fix is to +move the store to a fresh memory region: tip secrets went 22-25 → **26-29**, and +22-25 must never be reused. + +**`ECDSA_KEY_NAME` is also the vetKD key name.** `build.backend.args.sh` picks it +per network and the backend reuses that one field for both. `test_be_*` and +`test_fe_*` were unlisted and fell through to the local default `dfx_test_key`, +which exists only on a local replica, so every derivation on be1 trapped with +`SignCostError(InvalidKeyName)`. It reached the sender as "the link for this tip +is not recoverable" — a confident, invented explanation for a live bug. Personal +notes was unaffected only because its store had captured a valid name earlier. + +**A silently swallowed failure is expensive.** The claim-code write was +best-effort with a `consoleWarn` and no retry, so one transient failure cost a tip +its recoverable link permanently with nothing on screen. It now retries once and +returns `secretStored`, and the share screen says "copy this link now" when it +could not be saved. + +**Tooling quirks.** A plain `dfx deploy backend` _and_ `scripts/lint.did.sh` both +reflow the whole of `backend.did` from tabs to spaces — pre-existing formatter +drift; discard it rather than commit it. `scripts/generate.sh` needs `didc`, which +is not installed, and `dfx generate` writes a different file layout than this +repo's pipeline, so candid declarations are hand-edited and validated by having +dfx parse the interface. + +## Open decisions + +Answers change what gets built; none of them block the rest. + +| Question | Default if nobody answers | +| ----------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------- | +| `Cancelled` is a fifth status. Fold it into the Expired group, give it its own, or hide it? | folded into Expired, row keeps its own label | +| Should the four History groups also appear on the wallet home screen, or only inside the tips modal? | modal only; the home screen gets just the badge | +| Apply the II delegation guard to `claim_tip` too? It would mean turning `cancel_tip(text)` into a record. | sender-side endpoints only | +| Raise the vetKey `caller_hour` (10) and global (100/hour) tiers? They are real cost ceilings at ~26B cycles a derivation, and personal notes shares the exposure. | left alone, needs whoever sized them | +| Reject a self-claim (spec decision 15)? Verified end to end that a self-claim currently succeeds. | not implemented; owner's call | + +## Where be1 stands + +Deployed from `63c2122bd`, both canisters, frontend first. Tips work; **link +recovery does not**, and the remaining blocker is not code: + +``` +Canister cannot grow memory by 8388608 bytes due to its reserved cycles limit. +The current limit (10_000_000_000_000) would be exceeded by 216_531_593_707. +``` + +be1 cannot allocate the 8 MiB the fresh memory region needs. That error is itself +proof the new wasm is live — the old one reused existing regions and needed no +growth. It also means the key-name fix is **untested**, because +`ensure_tip_secrets()` runs before the derivation, so the cost check is never +reached and `InvalidKeyName` disappearing proves nothing yet. + +Needs a controller (be1 has five, none of them ours). Either raise the limit and +keep be1's data: + +```bash +dfx canister update-settings --network ic jloto-byaaa-aaaap-anryq-cai \ + --reserved-cycles-limit 20000000000000 +``` + +or reinstall the backend, which wipes be1 state but needs no limit change: a +reinstall clears stable memory, so the store re-initialises with the corrected +name in the existing region and no new allocation is required. + +Meanwhile be1 is usable for everything except recovery — `create_tip` never +touches the secrets store, and the failure is now graceful rather than silent. diff --git a/icrc1-ledger.wasm.gz b/icrc1-ledger.wasm.gz new file mode 120000 index 00000000000..2828e1620d5 --- /dev/null +++ b/icrc1-ledger.wasm.gz @@ -0,0 +1 @@ +icrc1-ledger.wasm.gz.a98196714c \ No newline at end of file From 4f99f536d7d9a45459751756bdf4a4b03d45955a Mon Sep 17 00:00:00 2001 From: Artem Korotkikh <105195369+artkorotkikh-dfinity@users.noreply.github.com> Date: Sat, 29 Aug 2026 19:00:30 +0200 Subject: [PATCH 07/15] docs(ai): track what still stands between the tips stack and its PRs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three sections, all of them things that were only in my head or in a chat log. **Before these can become PRs** — per-branch gate status, measured branch by branch rather than from the top of the stack, which hides what the lower branches are missing. Branches 1-3 are CI-clean; 4-7 fail `npm run test` at the tsc step, which means CI goes red having run no tests at all. Names the three spec type errors, which branch owns each, and the fix already proven in the tree for one of them. **Merging main** — the memory-id collision and how it was settled, so the next reader does not rediscover that `MemoryId::new(20)` was claimed twice while this branch was away. Three conflicts remain unattempted. **be1's memory pin** — the deploy branch now carries a fourth deploy-only commit, so the invariant to check before pushing it is four files rather than three. Records why: the alternative is a reinstall, and a reinstall wipes every profile, contact and note on a shared environment, not just tips. And that it defers the reinstall rather than removing it. Branch heads refreshed. Co-Authored-By: Claude Opus 5 --- .../build-plan.md | 101 ++++++++++++++++-- 1 file changed, 92 insertions(+), 9 deletions(-) diff --git a/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md b/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md index 3d07f061774..40cfe4802b7 100644 --- a/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md +++ b/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md @@ -12,15 +12,16 @@ The spec stays the source of truth for **what** to build; this only records | # | Branch | Spec PR | Head | Contains | Status | | --- | ------------------------------ | ------- | ----------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------ | ------ | -| 1 | `feat/tips-1-backend` | PR-1 | `ee4dcab82` | tip store, `create_tip` / `get_tip` / `get_tip_details` / `claim_tip` / `cancel_tip` / `get_my_tips`, claim-code hashing, atomic claim, expiry, rate limiters, pruning, candid | built | -| 2 | `feat/tips-2-service` | PR-2 | `7f34c4674` | `base64url.utils`, `tip.crypto`, `tip.services`, api + canister layer, pinned cross-language hash vectors | built | -| 3 | `feat/tips-3-sender-ui` | PR-3 | `3848ef5e1` | `Issue Tip` menu entry, intro, token picker + empty state, create step, expiry, share screen with QR, `tip.*` i18n, flag off | built | -| 4 | `feat/tips-4-recipient-ui` | PR-4 | `2eb4e9dbd` | `/tip/` **standalone landing page**, claim review, success, **unavailable**, **uncovered** | built | -| 5 | `feat/tips-5-history` | PR-5 | `4d607e162` | History with four stored statuses, claimer principal on claimed rows, cancel action | built | -| 6 | `feat/tips-6-reserved-balance` | PR-2b | `d3b78adb8` | subtract reserved amounts once, in the derived store the token list, send, swap and both MAX controls read | built | -| 7 | `feat/tips-7-enable` | PR-6 | `10666311b` | flip `TIPS_ENABLED` to `true` — the release, on its own, after everything above lands | built | - -241 commits over `main`, none pushed. `main` here is the merged spec +| 1 | `feat/tips-1-backend` | PR-1 | `edc500086` | tip store, `create_tip` / `get_tip` / `get_tip_details` / `claim_tip` / `cancel_tip` / `get_my_tips`, claim-code hashing, atomic claim, expiry, rate limiters, pruning, candid | built | +| 2 | `feat/tips-2-service` | PR-2 | `27da853ba` | `base64url.utils`, `tip.crypto`, `tip.services`, api + canister layer, pinned cross-language hash vectors | built | +| 3 | `feat/tips-3-sender-ui` | PR-3 | `dfc880864` | `Issue Tip` menu entry, intro, token picker + empty state, create step, expiry, share screen with QR, `tip.*` i18n, flag off | built | +| 4 | `feat/tips-4-recipient-ui` | PR-4 | `f8ece90d3` | `/tip/` **standalone landing page**, claim review, success, **unavailable**, **uncovered** | built | +| 5 | `feat/tips-5-history` | PR-5 | `1a7f6120d` | History with four stored statuses, claimer principal on claimed rows, cancel action | built | +| 6 | `feat/tips-6-reserved-balance` | PR-2b | `6c61cea8a` | subtract reserved amounts once, in the derived store the token list, send, swap and both MAX controls read | built | +| 7 | `feat/tips-7-enable` | PR-6 | `6401abf47` | flip `TIPS_ENABLED` to `true` — the release, on its own, after everything above lands | built | + +None of the seven are pushed; `test/tips-be1` is, for the shared test +environment only. `main` here is the merged spec ([#13768](https://github.com/dfinity/oisy-wallet/pull/13768)). **Why the claim page is a standalone route.** It started under `(app)`, which @@ -159,6 +160,88 @@ the only way to test without manual setup) for real. No unit test can fake a principal with no profile. +## Before these can become PRs + +Measured per branch, because that is what CI does per PR — not from the top of +the stack, which hides everything the lower branches are missing. + +| # | Branch | Files | lint | check | `npm run test` | +| --- | ------------------------------ | ----- | ---- | ----- | ---------------- | +| 1 | `feat/tips-1-backend` | 31 | ok | ok | ok | +| 2 | `feat/tips-2-service` | 10 | ok | ok | ok | +| 3 | `feat/tips-3-sender-ui` | 42 | ok | ok | ok | +| 4 | `feat/tips-4-recipient-ui` | 42 | ok | ok | **fails at tsc** | +| 5 | `feat/tips-5-history` | 27 | ok | ok | **fails at tsc** | +| 6 | `feat/tips-6-reserved-balance` | 33 | ok | ok | **fails at tsc** | +| 7 | `feat/tips-7-enable` | 1 | ok | ok | **fails at tsc** | + +Sizes are the three-dot diff against each branch's parent — what GitHub shows. + +### 1. Three spec type errors (blocks PRs 4-7) + +`npm run test` is `tsc --project tsconfig.spec.json --noEmit && vitest`, so the +type error stops the suite before a single test runs. CI goes red having +executed nothing. + +- `TipClaimModal.spec.ts` — `vi.mock` overload on the `icrc.derived` partial mock — **PR 4** +- `tip-status.utils.spec.ts` — fixture missing `last_claim_failure` — **PR 5** +- `Menu.spec.ts` — `vi.mock(import('$env/tips.env'))` returns `boolean` where the module's literal type is `false` — **PR 6** + +The third has a proven fix already in the tree: `LoaderTips.spec.ts` uses the +string form of `vi.mock`, which never enters the typed-module overload. + +### 2. Merge `main` (blocks everything) + +Merge base is `883f2c69d` (24 Aug); main has moved ~70 commits. Four files +conflict, all stable-memory: `state/memory.rs`, `state/mod.rs`, `types/maps.rs`, +`types/storable.rs`. + +The dangerous one is settled: main took `MemoryId::new(20)` for +`CONTACT_IMAGE_MEMORY_ID` while this branch was away, and tips had the same id. +Two structures on one region decode into each other's data. Tips moved to 21-26 +(contacts is live on mainnet, tips is not). The remaining three conflicts are +where main's contact-image work meets ours and have not been attempted yet. + +### 3. `build.backend.args.sh` needs its own PR + +The `test_be_*` / `test_fe_*` key-name fix still lives only on `test/tips-be1`. +It is a shared script other teams build from, so it should reach main as its own +reviewed change rather than riding in with tips. + +### Order + +1. Fix the three spec type errors, each on its owning branch, cascade. +2. Merge `main` into branch 1, resolve, cascade, re-gate every branch. +3. Open PR 1 first — 31 files, the heaviest review, and provably untouched by UI + testing: its diff is `src/backend`, `src/shared`, `src/declarations` plus + `.gitignore`, the spec doc and `test.backend.sh`. +4. Hold 2-7 until testing settles. Branch 2 owns `tip.services.ts`, where the + unresolved ICP approve failure would be fixed, and there is no force-push + here — once pushed, every correction is a visible extra commit. +5. Separate PR for the deploy script. + +## be1 is pinned to the old memory ids + +`test/tips-be1` now carries a **fourth** deploy-only commit, beside the canister +ids and the vetKD key name: `state/memory.rs` pinned to tips at 20/21 and +secrets at 26-29, the ids be1 already holds data under. + +Without it, deploying the renumber to be1 as an upgrade would open the +by-sender index's region as the tips map and read one as the other. The fix for +that is a reinstall, and a reinstall wipes **all** stable memory on a shared test +environment — every profile, contact and note, not just tips. + +So the invariant to check before pushing that branch is now **four** files, not +three: + +```bash +git diff feat/tips-7-enable test/tips-be1 --stat +``` + +This defers the reinstall rather than removing it. The day main is merged into +the deploy branch, main's contact images will want region 20 and be1 has tips +records in it. + ## Blocked on people, not on code These do not block building, but they block landing. Longest lead time first. From a402988e89479e6665330b2e9b56ea41c1146c7c Mon Sep 17 00:00:00 2001 From: Artem Korotkikh <105195369+artkorotkikh-dfinity@users.noreply.github.com> Date: Sat, 29 Aug 2026 21:35:23 +0200 Subject: [PATCH 08/15] docs(ai): close out the five open decisions and the two PR blockers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four of the five confirmed what was already built and needed no code. The rate-limit one did: leaving the vetKey tiers alone is only defensible if meeting one is legible, and it was not — a limit read as a generic failure whose copy said 'try again', which is the one thing that cannot work until the window passes. Also records that the type errors and the main merge are done, and that bare 'main' is an ambiguous refname in this repo, since every size in the table depends on which ref was measured. --- .../build-plan.md | 94 +++++++++++++------ 1 file changed, 63 insertions(+), 31 deletions(-) diff --git a/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md b/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md index 40cfe4802b7..42d9ff07c4e 100644 --- a/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md +++ b/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md @@ -165,42 +165,48 @@ profile. Measured per branch, because that is what CI does per PR — not from the top of the stack, which hides everything the lower branches are missing. -| # | Branch | Files | lint | check | `npm run test` | -| --- | ------------------------------ | ----- | ---- | ----- | ---------------- | -| 1 | `feat/tips-1-backend` | 31 | ok | ok | ok | -| 2 | `feat/tips-2-service` | 10 | ok | ok | ok | -| 3 | `feat/tips-3-sender-ui` | 42 | ok | ok | ok | -| 4 | `feat/tips-4-recipient-ui` | 42 | ok | ok | **fails at tsc** | -| 5 | `feat/tips-5-history` | 27 | ok | ok | **fails at tsc** | -| 6 | `feat/tips-6-reserved-balance` | 33 | ok | ok | **fails at tsc** | -| 7 | `feat/tips-7-enable` | 1 | ok | ok | **fails at tsc** | +| # | Branch | Files | lint | check | `npm run test` | +| --- | ------------------------------ | ----- | ---- | ----- | -------------- | +| 1 | `feat/tips-1-backend` | 31 | ok | ok | ok | +| 2 | `feat/tips-2-service` | 10 | ok | ok | ok | +| 3 | `feat/tips-3-sender-ui` | 42 | ok | ok | ok | +| 4 | `feat/tips-4-recipient-ui` | 42 | ok | ok | ok | +| 5 | `feat/tips-5-history` | 27 | ok | ok | ok | +| 6 | `feat/tips-6-reserved-balance` | 33 | ok | ok | ok | +| 7 | `feat/tips-7-enable` | 1 | ok | ok | ok | Sizes are the three-dot diff against each branch's parent — what GitHub shows. +For branch 1 that parent is `origin/main`, not the local `main`, which is 21 +commits behind. A `main` **tag** also exists in this repo, so bare `main` is an +ambiguous refname here and every measurement above spells the ref out. -### 1. Three spec type errors (blocks PRs 4-7) +### ~~1. Three spec type errors (blocks PRs 4-7)~~ — fixed -`npm run test` is `tsc --project tsconfig.spec.json --noEmit && vitest`, so the -type error stops the suite before a single test runs. CI goes red having -executed nothing. +`npm run test` is `tsc --project tsconfig.spec.json --noEmit && vitest`, so a +type error stopped the suite before a single test ran and CI went red having +executed nothing. Each was fixed on its owning branch and cascaded; `tsc` now +passes on all seven. - `TipClaimModal.spec.ts` — `vi.mock` overload on the `icrc.derived` partial mock — **PR 4** - `tip-status.utils.spec.ts` — fixture missing `last_claim_failure` — **PR 5** - `Menu.spec.ts` — `vi.mock(import('$env/tips.env'))` returns `boolean` where the module's literal type is `false` — **PR 6** -The third has a proven fix already in the tree: `LoaderTips.spec.ts` uses the +The third took the fix already proven in the tree: `LoaderTips.spec.ts` uses the string form of `vi.mock`, which never enters the typed-module overload. -### 2. Merge `main` (blocks everything) +### ~~2. Merge `main` (blocks everything)~~ — done -Merge base is `883f2c69d` (24 Aug); main has moved ~70 commits. Four files -conflict, all stable-memory: `state/memory.rs`, `state/mod.rs`, `types/maps.rs`, -`types/storable.rs`. +Merge base was `883f2c69d` (24 Aug); main had moved ~70 commits. Four files +conflicted, all stable-memory: `state/memory.rs`, `state/mod.rs`, `types/maps.rs`, +`types/storable.rs`. Resolved on branch 1 and cascaded; `origin/main` is now an +ancestor of every branch in the stack. The dangerous one is settled: main took `MemoryId::new(20)` for `CONTACT_IMAGE_MEMORY_ID` while this branch was away, and tips had the same id. Two structures on one region decode into each other's data. Tips moved to 21-26 -(contacts is live on mainnet, tips is not). The remaining three conflicts are -where main's contact-image work meets ours and have not been attempted yet. +(contacts is live on mainnet, tips is not), leaving ids 0-26 contiguous with no +duplicates. `storable.rs` was reconstructed by hand after checking that neither +side had _removed_ lines — the conflict was two additions, not a rewrite. ### 3. `build.backend.args.sh` needs its own PR @@ -210,8 +216,8 @@ reviewed change rather than riding in with tips. ### Order -1. Fix the three spec type errors, each on its owning branch, cascade. -2. Merge `main` into branch 1, resolve, cascade, re-gate every branch. +1. ~~Fix the three spec type errors, each on its owning branch, cascade.~~ Done. +2. ~~Merge `main` into branch 1, resolve, cascade, re-gate every branch.~~ Done. 3. Open PR 1 first — 31 files, the heaviest review, and provably untouched by UI testing: its diff is `src/backend`, `src/shared`, `src/declarations` plus `.gitignore`, the spec doc and `test.backend.sh`. @@ -220,6 +226,9 @@ reviewed change rather than riding in with tips. here — once pushed, every correction is a visible extra commit. 5. Separate PR for the deploy script. +Steps 1 and 2 are what the stack was waiting on; what remains is a decision +about when to start pushing, not more work on the branches. + ## be1 is pinned to the old memory ids `test/tips-be1` now carries a **fourth** deploy-only commit, beside the canister @@ -342,15 +351,38 @@ dfx parse the interface. ## Open decisions -Answers change what gets built; none of them block the rest. - -| Question | Default if nobody answers | -| ----------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----------------------------------------------- | -| `Cancelled` is a fifth status. Fold it into the Expired group, give it its own, or hide it? | folded into Expired, row keeps its own label | -| Should the four History groups also appear on the wallet home screen, or only inside the tips modal? | modal only; the home screen gets just the badge | -| Apply the II delegation guard to `claim_tip` too? It would mean turning `cancel_tip(text)` into a record. | sender-side endpoints only | -| Raise the vetKey `caller_hour` (10) and global (100/hour) tiers? They are real cost ceilings at ~26B cycles a derivation, and personal notes shares the exposure. | left alone, needs whoever sized them | -| Reject a self-claim (spec decision 15)? Verified end to end that a self-claim currently succeeds. | not implemented; owner's call | +**All five were answered on 29 Aug 2026.** Four confirmed the standing default +and needed no code; the fifth did. Kept here rather than deleted so a reviewer +asking "was this considered?" gets an answer instead of silence. + +| Question | Answer | +| ----------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------------------------------------- | +| `Cancelled` is a fifth status. Fold it into the Expired group, give it its own, or hide it? | **Confirmed as built.** Folded into Expired; the row keeps its own label. | +| Should the four History groups also appear on the wallet home screen, or only inside the tips modal? | **Confirmed as built.** Modal only; the home screen gets just the badge. | +| Apply the II delegation guard to `claim_tip` too? It would mean turning `cancel_tip(text)` into a record. | **Confirmed as built.** Sender-side endpoints only; no candid change. | +| Raise the vetKey `caller_hour` (10) and global (100/hour) tiers? They are real cost ceilings at ~26B cycles a derivation, and personal notes shares the exposure. | **Tiers left alone, but the limit is no longer invisible** — see below. | +| Reject a self-claim (spec decision 15)? Verified end to end that a self-claim currently succeeds. | **Confirmed as built.** Self-claim stays allowed; it is also a testing path. | + +### What the rate-limit answer required + +Leaving the tiers as they are only works if meeting one is legible, and it was +not: a limit surfaced as an ordinary failure whose copy said "try again" — the +one action that cannot work until the window passes. Two things now happen +wherever a tip call can be turned away. + +`tipRateLimit(err)` reads `max_calls` and `window_ns` off `TipError::RateLimited` +and converts the window to seconds, so every surface can say _how long_ rather +than "too many requests". Wired into all five: create and cancel (toast), link +recovery (in place of `link_recovery_failed`, and it is the likeliest one to be +met — recovery derives a vetKey), and the claimer's read and payout, where it +outranks the other failure copy because "ask the sender for a new link" sends +someone to fix a link that is fine. + +`trackTip` gained a `rateLimited` flag emitting `rate_limited: 'true'`, kept +separate from `outcome` so "how often are we turning people away" is one query +across every step rather than five. Until this, a limit was indistinguishable +from a generic failure in Plausible, which is why nobody could size the tiers +from evidence — that is now collectable. ## Where be1 stands From 00d70652cce6ecb97ea5c74e9f92523d45e1eb01 Mon Sep 17 00:00:00 2001 From: Artem Korotkikh <105195369+artkorotkikh-dfinity@users.noreply.github.com> Date: Sun, 30 Aug 2026 12:46:59 +0200 Subject: [PATCH 09/15] docs(ai): record why the mid-flight upgrade case cannot be tested here MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The upgrade test that landed on branch 1 covers the part that was genuinely unmeasured — that tips survive an upgrade of the regions they were renumbered into — but not the lost callback open question 4 names. pocket-ic 15 cannot reach it: upgrade_canister drives rounds to answer its own install_code ingress and cannot while the claim's ingress is outstanding, and awaiting the claim to unblock it completes the claim. Recorded with the round table and the exact error so the next person does not rediscover it. --- .../build-plan.md | 49 ++++++++++++++++++- 1 file changed, 48 insertions(+), 1 deletion(-) diff --git a/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md b/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md index 42d9ff07c4e..698b53aa09b 100644 --- a/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md +++ b/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md @@ -280,7 +280,54 @@ formality. | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------- | | **The fragment surviving Internet Identity on mobile Safari and in in-app webviews** (open question 8, second half). The entire link model rests on it. Desktop is fine — II opens in a popup, so the page never unmounts — but an in-app webview may not keep it. | a real phone, and a link opened from a DM | | **A fresh identity seeing the received token without manual setup** (open question 6). The wallet-UI half is now built — `TipClaimModal` calls `autoLoadSingleToken` on the way out, so a claimed ck-asset is enabled — but nobody has claimed on a never-before-used anchor and looked. | claim on a never-before-used anchor, then look | -| **Claim atomicity across a canister upgrade mid-flight** (open question 4). The design answers it — `Claiming` plus a five-minute in-flight timeout — but no test upgrades the canister while a claim is in the air. | a pocket-ic test that upgrades mid-claim | +| **Claim atomicity across a canister upgrade mid-flight** (open question 4). Partly closed, and the rest is blocked by the harness — see below. | not reachable in pocket-ic 15; see below | + +### What the upgrade test does and does not reach + +`a_tip_survives_an_upgrade_and_still_pays_exactly_once` (branch 1) upgrades the +canister with a claim outstanding and asserts the tip, its amount and its +deadline all come back, the claimer was paid once, and a second attempt is +refused. That is worth having on its own account: tips took stable-memory +regions of their own and those regions were **renumbered late**, so "the records +still mean the same thing after an upgrade" had been an assumption. A region +reopened as the wrong structure decodes into plausible rubbish rather than +failing outright, which is why the fields are asserted and not just the row. + +What it does **not** reach is the hazard open question 4 actually names: the +upgrade landing between the ledger call and its reply, destroying the callback. +That is not expressible in pocket-ic 15, and the reason is worth recording so +nobody spends another afternoon on it. + +The claim runs over three rounds, confirmed by ticking one at a time and +watching the ledger: + +| round | what happens | allowance | claimer | +| ----- | ----------------------------------------------- | --------- | ------- | +| 1 | `claim_tip` writes `Claiming`, calls the ledger | 510_000 | 0 | +| 2 | the ledger executes `icrc2_transfer_from` | 0 | 500_000 | +| 3 | the callback lands, record becomes `Claimed` | 0 | 500_000 | + +The window is after round 1 and before round 3. But `upgrade_canister` drives +rounds to get its own `install_code` ingress answered, and it cannot while the +claim's ingress is outstanding: + +``` +BadIngressMessage("Failed to answer to ingress 0x772a… after 100 rounds.") +``` + +Awaiting the claim first is what unblocks the upgrade — and awaiting it +completes it. Verified both ways: with the claim awaited first the upgrade +succeeds every time; with it outstanding past round 1 the upgrade always fails +this way. The backend wasm is also over the chunked-install threshold, so +hand-rolling the interleaving means reimplementing `upload_chunk` plus +`install_chunked_code` against raw `submit_call`. + +So the lost-callback path, and the five-minute in-flight timeout that recovers +from it, remain covered only by the unit tests in `tips/model.rs`. Closing it +for real needs either a harness that can interleave `install_code` with an +open call context, or a deliberate test-only endpoint that parks a record in +`Claiming` — and the second is a hole in production code for a test, which is +its own argument. ## Deploying to be1 / fe1 From e2901ac6c0e3a8c75130e1ffe958205b53be966d Mon Sep 17 00:00:00 2001 From: Artem Korotkikh <105195369+artkorotkikh-dfinity@users.noreply.github.com> Date: Sun, 30 Aug 2026 15:46:49 +0200 Subject: [PATCH 10/15] docs(ai): re-measure the stack after the pre-PR fixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sizes and gates re-run per branch, plus the full suite once from the top. The one finding worth carrying: run tsc per branch, not just check and the targeted specs. A spy returning undefined where a symbol was expected passed lint, svelte-check and vitest, and failed only the spec typecheck — on three branches at once. Also retires the be1 invariant as written. Branch 7 has moved far past the point be1 was cut from, so that diff no longer means anything; the check that matters is that no deploy-only content reached the stack, which it has not. --- .../build-plan.md | 60 +++++++++++++++---- 1 file changed, 50 insertions(+), 10 deletions(-) diff --git a/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md b/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md index 698b53aa09b..4c82a6bf4d1 100644 --- a/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md +++ b/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md @@ -165,15 +165,27 @@ profile. Measured per branch, because that is what CI does per PR — not from the top of the stack, which hides everything the lower branches are missing. -| # | Branch | Files | lint | check | `npm run test` | -| --- | ------------------------------ | ----- | ---- | ----- | -------------- | -| 1 | `feat/tips-1-backend` | 31 | ok | ok | ok | -| 2 | `feat/tips-2-service` | 10 | ok | ok | ok | -| 3 | `feat/tips-3-sender-ui` | 42 | ok | ok | ok | -| 4 | `feat/tips-4-recipient-ui` | 42 | ok | ok | ok | -| 5 | `feat/tips-5-history` | 27 | ok | ok | ok | -| 6 | `feat/tips-6-reserved-balance` | 33 | ok | ok | ok | -| 7 | `feat/tips-7-enable` | 1 | ok | ok | ok | +Re-measured 30 Aug 2026, after the five fixes below. + +| # | Branch | Files | lint | check | `tsc` (spec) | +| --- | ------------------------------ | ----- | ---- | ----- | ------------ | +| 1 | `feat/tips-1-backend` | 32 | ok | ok | ok | +| 2 | `feat/tips-2-service` | 10 | ok | ok | ok | +| 3 | `feat/tips-3-sender-ui` | 42 | ok | ok | ok | +| 4 | `feat/tips-4-recipient-ui` | 42 | ok | ok | ok | +| 5 | `feat/tips-5-history` | 27 | ok | ok | ok | +| 6 | `feat/tips-6-reserved-balance` | 33 | ok | ok | ok | +| 7 | `feat/tips-7-enable` | 1 | ok | ok | ok | + +The full `npm run test` was run once from the top of the stack: **1088 files, +18156 passed, 1 skipped, 1 todo**. Per branch only the fast half (`tsc`) was +re-run, which is the half that actually caught something — see below. + +**Run `tsc` per branch, not just `check` and the targeted specs.** A spy on +`toastsError` returning `undefined` instead of the toast's `symbol` passed +lint, passed `svelte-check` and passed vitest, and failed only +`tsc --project tsconfig.spec.json`. Branches 5, 6 and 7 were all red on it +while every other gate was green. Sizes are the three-dot diff against each branch's parent — what GitHub shows. For branch 1 that parent is `origin/main`, not the local `main`, which is 21 @@ -208,7 +220,27 @@ Two structures on one region decode into each other's data. Tips moved to 21-26 duplicates. `storable.rs` was reconstructed by hand after checking that neither side had _removed_ lines — the conflict was two additions, not a rewrite. -### 3. `build.backend.args.sh` needs its own PR +### 3. Fixed on 30 Aug, before any push + +Five changes, each on the branch that owns the code, cascaded by merge: + +| What | Branch | +| --------------------------------------------------------------------- | ------ | +| Generate no longer offered before `$sendBalance` has loaded | 6 | +| The tips-list read failure says so, instead of reusing `claim_failed` | 5 | +| A tip survives an upgrade and still pays exactly once | 1 | +| The overview reports sums rather than a tally | 6 | +| The menu entry reads "Give a tip", grouped with Refer and Support | 3 | + +The first is the interesting one. `icrc2_approve` does **not** check the +allowance against the balance — the ledger debits the approve fee and nothing +else — so approving for more than the sender holds succeeds, and the tip then +dies on the recipient's screen as `shortBalance`. The form's ceiling is the only +thing preventing that, and it is derived from `$sendBalance`; while that was +unknown there was no cap, no validation, and the ledger was the first thing to +push back. + +### 4. `build.backend.args.sh` needs its own PR The `test_be_*` / `test_fe_*` key-name fix still lives only on `test/tips-be1`. It is a shared script other teams build from, so it should reach main as its own @@ -247,6 +279,14 @@ three: git diff feat/tips-7-enable test/tips-be1 --stat ``` +**That check has since expired.** Branch 7 has moved a long way past the point +be1 was cut from — main's merge plus everything since — so the diff is now +hundreds of files and says nothing. The invariant to check before pushing a +feature PR is the other direction: that no deploy-only content has reached the +stack. Verified on 30 Aug and all four are clean — memory ids are 21-26 (not +be1's 20/21 + 26-29), and `canister_ids.json`, `dfx.json` and +`build.backend.args.sh` are all identical to `origin/main`. + This defers the reinstall rather than removing it. The day main is merged into the deploy branch, main's contact images will want region 20 and be1 has tips records in it. From d397e4ce4c5f99de6f106570b061ef513317bdee Mon Sep 17 00:00:00 2001 From: Artem Korotkikh <105195369+artkorotkikh-dfinity@users.noreply.github.com> Date: Sun, 30 Aug 2026 15:58:06 +0200 Subject: [PATCH 11/15] docs(ai): design signed off in both themes Reviewed on 30 Aug and cleared, so branches 3, 4 and 5 are no longer waiting on the undrawn states or the light theme. --- .../specs/2026-08-05-feat-tips-via-link/build-plan.md | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md b/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md index 4c82a6bf4d1..8f0501b4e24 100644 --- a/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md +++ b/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md @@ -295,11 +295,11 @@ records in it. These do not block building, but they block landing. Longest lead time first. -| What | Gates | Owner | -| -------------------------------------------------------------------------------------------------------------------------- | ---------------- | -------------------------------- | -| Compliance sign-off on OISY holding a bounded, revocable authorisation over user funds for up to a week (open question 11) | branch 1 | — | -| The `Uncovered` wording — it is information about the sender | branch 4 | whoever owns the privacy promise | -| The five undrawn states, plus a light theme for every screen (the Figma page is dark-only) | branches 3, 4, 5 | design | +| What | Gates | Owner | +| ----------------------------------------------------------------------------------------------------------------------------- | -------------------- | -------------------------------- | +| Compliance sign-off on OISY holding a bounded, revocable authorisation over user funds for up to a week (open question 11) | branch 1 | — | +| The `Uncovered` wording — it is information about the sender | branch 4 | whoever owns the privacy promise | +| ~~The five undrawn states, plus a light theme for every screen~~ **Cleared 30 Aug** — reviewed in both themes and signed off. | ~~branches 3, 4, 5~~ | design | ## Still to build From e66f53eab3c381125578f9c480f1a987ce9ff71f Mon Sep 17 00:00:00 2001 From: Artem Korotkikh <105195369+artkorotkikh-dfinity@users.noreply.github.com> Date: Sun, 30 Aug 2026 20:23:19 +0200 Subject: [PATCH 12/15] docs(ai): downgrade the compliance item and clear the people blockers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The compliance sign-off was carried from the spec's open questions and repeated as the longest-lead blocker without anyone testing whether it deserved that weight. It does not, and the evidence is in this repo: the frontend already calls approve in swap, ck conversions, trade deposits and crypto-pay, and tips grants a narrower authority than any of them — exact amount, per-tip subaccount, revocable, and capped at seven days by the canister as well as the ledger. Records the one thing that is genuinely different (OISY's own backend as the spender, standing for days rather than seconds), corrects the spec's claim that this backend already uses transfer_from, and names the lever if anyone objects. With design signed off and the Uncovered wording resolved in code, nothing in that section gates a PR any more. --- .../build-plan.md | 69 ++++++++++++++++--- 1 file changed, 58 insertions(+), 11 deletions(-) diff --git a/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md b/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md index 8f0501b4e24..7c16074be15 100644 --- a/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md +++ b/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md @@ -250,11 +250,12 @@ reviewed change rather than riding in with tips. 1. ~~Fix the three spec type errors, each on its owning branch, cascade.~~ Done. 2. ~~Merge `main` into branch 1, resolve, cascade, re-gate every branch.~~ Done. -3. Open PR 1 first — 31 files, the heaviest review, and provably untouched by UI +3. Open PR 1 first — 32 files, the heaviest review, and provably untouched by UI testing: its diff is `src/backend`, `src/shared`, `src/declarations` plus - `.gitignore`, the spec doc and `test.backend.sh`. -4. Hold 2-7 until testing settles. Branch 2 owns `tip.services.ts`, where the - unresolved ICP approve failure would be fixed, and there is no force-push + `.gitignore`, the spec doc and `test.backend.sh`. Nothing gates it any more. +4. Hold 2-7 until testing settles. Branch 2 owns `tip.services.ts`; the ICP + approve failure that was open there traced to the missing balance guard, + fixed on branch 6, and there is no force-push here — once pushed, every correction is a visible extra commit. 5. Separate PR for the deploy script. @@ -293,13 +294,59 @@ records in it. ## Blocked on people, not on code -These do not block building, but they block landing. Longest lead time first. - -| What | Gates | Owner | -| ----------------------------------------------------------------------------------------------------------------------------- | -------------------- | -------------------------------- | -| Compliance sign-off on OISY holding a bounded, revocable authorisation over user funds for up to a week (open question 11) | branch 1 | — | -| The `Uncovered` wording — it is information about the sender | branch 4 | whoever owns the privacy promise | -| ~~The five undrawn states, plus a light theme for every screen~~ **Cleared 30 Aug** — reviewed in both themes and signed off. | ~~branches 3, 4, 5~~ | design | +Both of the original entries are now cleared. Nothing here gates a PR. + +| What | Gates | Status | +| ------------------------------------------------------------------------- | -------------------- | ------------------------------------------------------------ | +| ~~The five undrawn states, plus a light theme for every screen~~ | ~~branches 3, 4, 5~~ | **Cleared 30 Aug** — reviewed in both themes and signed off. | +| ~~The `Uncovered` wording — it is information about the sender~~ | ~~branch 4~~ | **Resolved 30 Aug** in code — see below. | +| ~~Compliance sign-off on the week-long authorisation (open question 11)~~ | ~~branch 1~~ | **Downgraded 30 Aug** — ask in parallel, do not hold PR 1. | + +### Why the compliance item was downgraded + +It was carried from the spec's open-questions list and repeated as the +longest-lead blocker without anyone testing whether it deserved that weight. It +does not, and the evidence is in this repo. + +**Allowances are already core OISY UX.** The frontend calls `approve` in swap +(all three providers), ckBTC/ckETH conversions, trade deposits and crypto-pay. +Every user who has swapped in OISY has already granted one. + +**Tips grants a narrower authority than any of them.** Exactly the amount plus +one ledger fee; scoped to a spender subaccount derived from that single tip's id; +revocable by cancelling; expired by the ledger within seven days, with +`validate_expiry` enforcing `MAX_TIP_EXPIRY_NS` independently so a hand-rolled +client cannot ask for more. What people routinely grant a DEX router is unlimited +and never expires. + +**What is genuinely different, and worth one sentence to whoever is asked.** In +every existing OISY flow the spender is a third-party protocol and the allowance +is consumed inside the same user action — approve and swap, seconds apart. In +tips the spender is OISY's own backend and it stands for days. That moves OISY +from brokering someone else's spend authority to holding its own. + +**One correction to the spec.** It records that `icrc2_transfer_from` is already +used in production by the rewards canister. That is a separate canister — +`grep` this backend and tips is its first use. The precedent is real but +external, so cite it that way rather than as "we already do this here." + +**If anyone does object**, the lever is the seven-day option, not the mechanism: +dropping to 24 hours is one entry in `TIP_EXPIRY_OPTIONS` and the canister cap. + +### The `Uncovered` wording, resolved + +The claim screen said "The sender is out of funds" and "The sender has taken back +the funds they set aside for it" — to whoever holds the link, which is a bearer +token that gets forwarded and screenshotted. It also undid a decision one layer +down, where the canister deliberately answers `NotFound` for an unknown id, an +expired tip, a claimed tip and a wrong code alike so a prober learns nothing. + +The retry advice stayed, since _come back later_ versus _ask for a new link_ is +the only part that changes what the reader does, and neither needs a word about +anyone's balance. The guard asserts against the rendered text rather than the two +strings, so the property survives the next edit. The sender's own screen keeps +its financial detail — `overview_failed_hint` still says topping up is all it +takes, because that is their money on their screen. ## Still to build From ef9a5a87a5380baa2de33c9b3f96f02d37742424 Mon Sep 17 00:00:00 2001 From: Artem Korotkikh <105195369+artkorotkikh-dfinity@users.noreply.github.com> Date: Sun, 30 Aug 2026 21:56:48 +0200 Subject: [PATCH 13/15] docs(ai): correct the pocket-ic claim, and close open question 4 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Recorded that the mid-flight upgrade case was not expressible in pocket-ic 15. It is — CI hits it on every run. It is not controllable, which is a different thing: locally the claim completes before the upgrade, on CI the upgrade lands between the ledger call and its reply and the callback dies. The local evidence was real but was one scheduling, not the only one. The test now asserts what holds on both, and the end state is identical either way, so the lost-callback path is exercised rather than argued about. --- .../build-plan.md | 74 +++++++++---------- 1 file changed, 37 insertions(+), 37 deletions(-) diff --git a/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md b/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md index 7c16074be15..23d5bd845f8 100644 --- a/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md +++ b/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md @@ -367,26 +367,31 @@ formality. | ---------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ---------------------------------------------- | | **The fragment surviving Internet Identity on mobile Safari and in in-app webviews** (open question 8, second half). The entire link model rests on it. Desktop is fine — II opens in a popup, so the page never unmounts — but an in-app webview may not keep it. | a real phone, and a link opened from a DM | | **A fresh identity seeing the received token without manual setup** (open question 6). The wallet-UI half is now built — `TipClaimModal` calls `autoLoadSingleToken` on the way out, so a claimed ck-asset is enabled — but nobody has claimed on a never-before-used anchor and looked. | claim on a never-before-used anchor, then look | -| **Claim atomicity across a canister upgrade mid-flight** (open question 4). Partly closed, and the rest is blocked by the harness — see below. | not reachable in pocket-ic 15; see below | +| ~~**Claim atomicity across a canister upgrade mid-flight** (open question 4)~~ **Closed.** Exercised on every CI run — see below. | done | -### What the upgrade test does and does not reach +### The upgrade test, and a correction -`a_tip_survives_an_upgrade_and_still_pays_exactly_once` (branch 1) upgrades the -canister with a claim outstanding and asserts the tip, its amount and its -deadline all come back, the claimer was paid once, and a second attempt is -refused. That is worth having on its own account: tips took stable-memory -regions of their own and those regions were **renumbered late**, so "the records -still mean the same thing after an upgrade" had been an assumption. A region -reopened as the wrong structure decodes into plausible rubbish rather than -failing outright, which is why the fields are asserted and not just the row. +`a_tip_survives_an_upgrade_and_still_pays_exactly_once` (branch 1) submits a +claim, ticks once so the canister has written `Claiming` and called the ledger, +then upgrades with that call outstanding. -What it does **not** reach is the hazard open question 4 actually names: the -upgrade landing between the ledger call and its reply, destroying the callback. -That is not expressible in pocket-ic 15, and the reason is worth recording so -nobody spends another afternoon on it. +**Correction.** This section previously recorded the mid-flight case — the +upgrade landing between the ledger call and its reply, destroying the callback — +as _not expressible in pocket-ic 15_. That was wrong. It is reachable; it is +simply not **controllable**, and which path you get depends on the machine: -The claim runs over three rounds, confirmed by ticking one at a time and -watching the ledger: +| where | what happens | +| ----- | ----------------------------------------------------------------------------- | +| local | the claim completes first, the upgrade lands after — call returns `Ok` | +| CI | the upgrade gets in between — `CanisterTrapped`, `call_on_cleanup` also fails | + +The first version of the test asserted the claim still answers, so it passed +locally and failed on CI. The local evidence behind the old claim was real — +`upgrade_canister` does fail with `BadIngressMessage("Failed to answer to +ingress … after 100 rounds")` when the claim's ingress is outstanding _past the +ledger's reply_ — but that is one scheduling, not the only one. + +The claim runs over three rounds, confirmed by ticking one at a time: | round | what happens | allowance | claimer | | ----- | ----------------------------------------------- | --------- | ------- | @@ -394,27 +399,22 @@ watching the ledger: | 2 | the ledger executes `icrc2_transfer_from` | 0 | 500_000 | | 3 | the callback lands, record becomes `Claimed` | 0 | 500_000 | -The window is after round 1 and before round 3. But `upgrade_canister` drives -rounds to get its own `install_code` ingress answered, and it cannot while the -claim's ingress is outstanding: - -``` -BadIngressMessage("Failed to answer to ingress 0x772a… after 100 rounds.") -``` - -Awaiting the claim first is what unblocks the upgrade — and awaiting it -completes it. Verified both ways: with the claim awaited first the upgrade -succeeds every time; with it outstanding past round 1 the upgrade always fails -this way. The backend wasm is also over the chunked-install threshold, so -hand-rolling the interleaving means reimplementing `upload_chunk` plus -`install_chunked_code` against raw `submit_call`. - -So the lost-callback path, and the five-minute in-flight timeout that recovers -from it, remain covered only by the unit tests in `tips/model.rs`. Closing it -for real needs either a harness that can interleave `install_code` with an -open call context, or a deliberate test-only endpoint that parks a record in -`Claiming` — and the second is a hole in production code for a test, which is -its own argument. +**So the test now asserts what holds on both paths**, which is a better test than +the one that was aimed at a single scheduling: + +- at most one payout has happened by the time the upgrade returns; +- the record survived with its **amount and deadline** intact — the point that + was never measured, since the memory regions were renumbered late and a region + reopened as the wrong structure decodes into plausible rubbish rather than + failing outright; +- and after the in-flight window, a retry leaves the claimer paid **exactly + once** — either the first transfer landed and the allowance is spent, or it did + not and the retry pays. + +That last assertion is the same on both schedulings, which is what makes it worth +having. Open question 4 is closed by it: the lost-callback path is now exercised +on every CI run, and the `Claiming` state plus its timeout is what recovers from +it. ## Deploying to be1 / fe1 From 7139655759c38a151dc5353cf4bdc6e040364d6c Mon Sep 17 00:00:00 2001 From: Artem Korotkikh <105195369+artkorotkikh-dfinity@users.noreply.github.com> Date: Sun, 30 Aug 2026 22:53:35 +0200 Subject: [PATCH 14/15] chore(ai): drop a build artifact that was swept into the docs branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit icrc1-ledger.wasm.gz is a symlink to a fixture download-immutable.sh caches, and it landed here in dc409e1a3 via a broad git add. The tips backend branch adds it to .gitignore alongside the other ledger fixtures; this branch does not carry that change, so nothing stopped it. Removed rather than rewritten — it is a symlink to a public fixture, not anything that needs history erased. --- icrc1-ledger.wasm.gz | 1 - 1 file changed, 1 deletion(-) delete mode 120000 icrc1-ledger.wasm.gz diff --git a/icrc1-ledger.wasm.gz b/icrc1-ledger.wasm.gz deleted file mode 120000 index 2828e1620d5..00000000000 --- a/icrc1-ledger.wasm.gz +++ /dev/null @@ -1 +0,0 @@ -icrc1-ledger.wasm.gz.a98196714c \ No newline at end of file From eb7efb14ad1b21dab4150f5195b393584e3c224b Mon Sep 17 00:00:00 2001 From: Artem Korotkikh <105195369+artkorotkikh-dfinity@users.noreply.github.com> Date: Mon, 31 Aug 2026 09:57:23 +0200 Subject: [PATCH 15/15] docs(ai): record the eighth PR and what review changed A reviewer asked for the rate-limiter rename to land before the backend, so the plan now has a PR at the bottom that is not tips. Also writes down the squash-merge mechanic that makes a stacked PR replay everything below it unless main is merged in first. Sixteen review findings accepted and two declined, with the four that touch the money path named. Two of them were mistakes this doc asserted: the mid-flight upgrade case is reachable, and the reason given for writing our own ICRC-2 client was untrue. --- .../build-plan.md | 54 +++++++++++++++++++ icrc1-ledger.wasm.gz | 1 + 2 files changed, 55 insertions(+) create mode 120000 icrc1-ledger.wasm.gz diff --git a/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md b/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md index 23d5bd845f8..c1e73015ce6 100644 --- a/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md +++ b/docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link/build-plan.md @@ -248,6 +248,28 @@ reviewed change rather than riding in with tips. ### Order +**Revised 31 Aug, after review.** A human reviewer asked for the rate-limiter +rename to land as its own PR before the backend, so the stack now has an eighth +PR at the bottom that is not tips: + +| PR | Branch | Base | State | +| ----------------------------------------------------------- | ------------------------------ | -------- | ------------------ | +| [#13862](https://github.com/dfinity/oisy-wallet/pull/13862) | `refactor/tiered-rate-limiter` | `main` | open, review first | +| [#13859](https://github.com/dfinity/oisy-wallet/pull/13859) | `feat/tips-1-backend` | `main` | open | +| [#13860](https://github.com/dfinity/oisy-wallet/pull/13860) | `feat/tips-2-service` | branch 1 | open | +| — | branches 3-7 | stacked | local, in waves | + +Then waves: 3 and 4 together (the two ends of one flow), 5 and 6 once those +land, and 7 alone last since it is the switch. `TIPS_ENABLED` stays false +through 3-6, so all four can sit on `main` invisibly — which is what makes +staging safe rather than a race. + +**After each merge, merge `main` into the next branch before opening its PR.** +This repo squash-merges, so a merged branch's commits never become ancestors of +`main` — the content lands as one new commit. A stacked PR retargeted to `main` +then computes its diff from the old fork point and replays everything below it. +Merging `main` in resolves it by content, and it is a merge, not a rebase. + 1. ~~Fix the three spec type errors, each on its owning branch, cascade.~~ Done. 2. ~~Merge `main` into branch 1, resolve, cascade, re-gate every branch.~~ Done. 3. Open PR 1 first — 32 files, the heaviest review, and provably untouched by UI @@ -262,6 +284,38 @@ reviewed change rather than riding in with tips. Steps 1 and 2 are what the stack was waiting on; what remains is a decision about when to start pushing, not more work on the branches. +## What review has changed so far + +Three reviewers on #13859 and #13860: Copilot, `claude[bot]`, and Antonio. Kept +here because most of it is about the money path, and because two findings were +mistakes in things this doc asserted. + +**Sixteen findings accepted, two declined.** The ones worth remembering: + +| Finding | Where | +| -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- | ----- | +| A tip id of 33-64 bytes could never store or read its recovery secret — `MAX_TIP_ID_BYTES` is 64, the map key a `Blob<32>`. Creatable, claimable, cancellable, and the link silently unrecoverable. Key is now the id's SHA-256. | PR 1 | +| `create_tip` validated the expiry against a clock read _before_ two awaited ledger calls, so a deadline could be in the past by the time it was written. | PR 1 | +| `cancel_tip` accepted a timed-out `Claiming` tip, contradicting its own doc. The timeout only means no reply came back; the ledger call may still pay, so cancelling returned `Ok` on a promise the canister could not keep. | PR 1 | +| The claim success branch wrote `Claimed` unconditionally while `release_claim` checked ownership first. A late success could erase who actually received the money. Both now share `claim_is_ours`. | PR 1 | +| Five ICRC types were re-declared when the crate `signer/service.rs` imports them from already had them, identical on the wire. | PR 1 | +| The retry around `setTipSecret` re-ran the vetKD derivation it documented itself as not retrying. | PR 2 | + +**Declined, with reasons on the PRs:** rejecting an allowance greater than +`amount + fee` (a fee change between approve and create would reject a good tip, +and it is not sufficient anyway), and `LocalKey>::set` not existing +(stable since 1.73). + +**Two things this doc had wrong**, both corrected above and in the PRs: the +mid-flight upgrade case is reachable in pocket-ic, and the module comment +justifying a separate ICRC-2 client gave a reason that was simply untrue — the +underscore in `icrc_2_approve` is a Rust method name, not a wire string. + +**Deferred as its own PR:** making reserve idempotent end to end — reconciling +`DuplicateTipId` after an ambiguous `create_tip`, and deduplicating the approve +via `created_at_time`. Both are real, both change the money path, and neither +belongs in a service-layer PR. + ## be1 is pinned to the old memory ids `test/tips-be1` now carries a **fourth** deploy-only commit, beside the canister diff --git a/icrc1-ledger.wasm.gz b/icrc1-ledger.wasm.gz new file mode 120000 index 00000000000..2828e1620d5 --- /dev/null +++ b/icrc1-ledger.wasm.gz @@ -0,0 +1 @@ +icrc1-ledger.wasm.gz.a98196714c \ No newline at end of file