feat(backend)!: record and claim tips through an ICRC-2 allowance - #13859
Open
artkorotkikh-dfinity wants to merge 46 commits into
Open
feat(backend)!: record and claim tips through an ICRC-2 allowance#13859artkorotkikh-dfinity wants to merge 46 commits into
artkorotkikh-dfinity wants to merge 46 commits into
Conversation
Implements the backend half of the tips spec: a sender reserves an amount by approving this canister under a per-tip spender subaccount, and whoever holds the link claims it. The canister never holds the tokens — every payout is an icrc2_transfer_from against the sender's own account, and every failure path leaves the money where it already was. The subaccount is what makes this safe: H(tip_id) scopes the allowance to one tip, so a second tip from the same sender to this same canister cannot draw on it. Verified against a real ledger rather than assumed. Endpoints: create_tip validates that the allowance covers the amount plus the fee the ledger draws from it, and refuses a reservation that lapses before the tip does. get_tip is anonymous and collapses unknown, expired, cancelled and claimed into one NotFound so a prober learns nothing; get_tip_details adds the sender's message but only to someone holding the claim code. claim_tip is guarded on a non-anonymous caller rather than a registered one, because the claimer may be an identity that has never used OISY — that is the point of the feature. A claim flips the record to Claiming before awaiting the ledger, which is what makes a double payout impossible; a claim interrupted by an upgrade is recoverable by timeout rather than stranding the tip. Every failure reverts to Reserved, which is safe even for an ambiguous transport error, because the allowance is the source of truth: if the transfer did happen, the next claim finds it consumed and fails Uncovered rather than paying twice. Two things the spec left open are answered here. The minimum tip is one ledger fee — below the cost of moving it a tip is spam by construction, and the ledger already tells us that number, so no per-token table is needed. And a terminal tip is kept for 30 days before pruning, because unlike a note share it is a History row the sender is entitled to see. Also adjusts the spec's acceptance criterion 9: the ledger charges the transfer fee to the allowance and credits the amount in full, so the claimer receives the whole amount and the sender carries both fees. The "net of the fee" wording described a model the allowance design does not use. Candid is additive only — new types and methods, no changes to existing ones — so this is not a breaking interface change. PRODUCT.md is deliberately not touched: no behaviour is user-visible until the UI branches land, and the workflow asks for PRODUCT.md in the PR that ships the behaviour. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Measuring the claim against a real ledger settled who pays what, and the spec still described the model it replaced in six places — including one line I had just written that said the payout fee comes out of the tip. icrc2_transfer_from debits amount + fee from the sender and credits the claimer the amount in full. So the sender pays twice — once to reserve, once when it is claimed — and the claimer pays nothing. The claim review therefore has no fee line at all, and the creation screen has to quote both fees to the sender rather than one. The consequence worth catching early is in the reserved-balance work: spendable balance has to exclude the whole allowance, amount plus fee. Excluding only the amount would let someone spend down to where their own tip can no longer be claimed — a tip that fails at claim through nobody's fault but our arithmetic. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The interface was refactored so `claim_tip` and `get_tip_details` take a single `TipClaimRequest` instead of two positional arguments, but the candid and the generated declarations were produced before that change. The Rust and the committed interface therefore disagreed: the .did still advertised `(text, text)`. Nothing exercised the gap. The integration tests encode the request as one record and pass, because they talk to the freshly built wasm rather than to the checked-in .did — so this would have surfaced first in CI's generate job, or worse, in the frontend against a signature that does not exist. Still purely additive against main: 0 deletions in the .did, so the interface remains non-breaking. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two unrelated asks, both landing here because both are backend state. **`tips_count` in `Stats`.** One line next to `personal_note_shares_count`, `O(1)` off the map's own length, behind the same `caller_is_allowed` guard as every other stat. Deliberately aggregate: criterion 19 promises no endpoint enumerates another principal's tips, and this one must not become the way around that, so there is a count here and never a row. A per-status or per-ledger breakdown needs either an `O(n)` scan or maintained counters — worth its own change, not smuggled into this one. **Recoverable claim codes.** A sender could cancel a tip but never see its link again: the code is generated in the browser and only its hash reaches the canister, so closing the share screen lost it. That property is what makes a link unforgeable and is not up for negotiation, so the recovery path does not weaken it — the browser encrypts the code under a vetKey only that principal can derive, and stores the ciphertext. The canister moves opaque bytes and can read a claim code exactly as well as before, which is to say not at all. A second `EncryptedMaps` rather than a field on `TipRecord`, mirroring `personal_notes`. `EncryptedMaps` is what enforces that a map belongs to one principal, so "only the sender can read their own codes" is the library's job rather than ours — and an integration test holds it, because that is the kind of guarantee that is worth not taking on faith. Its own map name and its own domain separator, so a fault or a rotation in the notes store cannot reach tips, and its own rate limiters, so recovering links cannot exhaust the budget for reading notes. Cancelling drops the stored code: the link is worthless then, and a recoverable secret should not outlive its usefulness. Best-effort — a tip created before this store existed has none, which is not an error, and failing to delete unreadable bytes must not fail a cancellation that already succeeded. Claimed and expired tips keep theirs until pruned; the cleanup runs as the sender, and neither of those paths has the sender as caller. `set_tip_secret` takes a request struct rather than two arguments, matching `SetPersonalNoteRequest`. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both were added after the first build, so the spec did not describe either. Written as reference rather than as a plan: it says what is in the code and why, including the parts that constrain future changes. The load-bearing sentence is that link recovery does not weaken criterion 16. It is easy to read "the claim code is now stored" as a retreat from "the code never reaches the backend in the clear", and it is not one — the canister holds ciphertext under a key only the sender can derive. The section says so explicitly, next to the two strings that must never change for a deployed canister because they are bound into the key derivation. Also records the thing that is easy to assume backwards: encrypting the codes contributes nothing to OISY-side auditability, because vetKey-encrypted data is opaque to us by construction. It does not need to — amounts, ledgers, statuses and timestamps were never hidden, only unaggregated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The decisions table promises self-claim is rejected. Nothing implements it: against the local ledger, `get_tip_details` and `claim_tip` both succeed for the sender's own tip and the payout goes through as a self-transfer costing one fee. Found while diagnosing why opening a link in the same browser appeared to do nothing — that turned out to be a frontend ordering bug, but this was underneath it. Recorded rather than fixed. Closing it is a breaking candid change and it would remove the only way to exercise a link end to end in one browser, so it is the owner's call — but the sender who tries it today burns a fee and gets a History row saying they claimed their own tip.
…tention window
`active_until` stored `0` in the by-sender index for a terminal tip, as a
sentinel meaning "this no longer occupies a cap slot". The same value is what
the retention check measures age against:
now_ns.saturating_sub(active_until_ns) > TIP_RETENTION_AFTER_TERMINAL_NS
With the sentinel that reads `now > 30 days`, which is true by a factor of
~700 for every nanosecond timestamp there will ever be. So every claimed or
cancelled tip became collectable the instant it went terminal, and disappeared
from the sender's History on the next `create_tip` or the next hourly
housekeeping sweep. Proven against the local ledger: a claimed tip survived the
claim and vanished the moment any new tip was created.
The index value is now the instant the tip *became* terminal, which is already
in the past — so it frees the cap slot exactly as the sentinel did, while giving
the retention arithmetic a real age to measure. `Claimed` already carried
`claimed_at_ns`; `Cancelled` gets `terminal_at_ns` on the record rather than a
field on the variant, so records already in stable memory keep decoding (candid
reads a missing `opt` as `None`). Verified by upgrading a replica holding live
tips and finding them all still there.
Expired-but-unclaimed tips were never affected: they keep their real
`expires_at_ns`.
Why the tests missed it: one asserted `active_until` returns the sentinel, the
other fed the partition only realistic timestamps. Neither passed a terminal
record *through* the function that interprets the value, and the bug lived in
that seam. The new test does, and fails against the old behaviour.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Creating a tip or reopening one from History spends one vetKD derivation per page load, and the browser fetches the verification key alongside it. Both endpoints were metered as paid derivations at 2/min and 10/hour per caller, so two reloads inside a minute made the third fail — and a failure on the create path silently and permanently cost that tip its recoverable link, because the secret write is best-effort. Three things, none of which raise worst-case cycle spend: The verification key is now fetched once per canister lifetime and cached. It is a property of the key name, domain separator and map name; it does not depend on the caller and does not change, so fetching it per session was pure waste. Heap rather than stable memory, since it is derivable and re-fetching once after an upgrade needs no migration. That endpoint therefore comes off `VetKeyRateLimiters` onto an ordinary per-caller limiter. Metering a cached constant like a paid derivation was actively harmful: the browser requests both in one `Promise.all`, so a rejection on the *constant* discarded a derivation that had already been paid for. The derivation's per-caller burst goes from 2/min to 5/min via a new `with_caller_burst`, leaving every other tier alone. That tier is a burst damper; the per-hour tier is the cost control and still binds at 10, so this cannot increase hourly spend for a caller — there is a test asserting exactly that, and another asserting `new()` is unchanged so personal notes keeps the tiers it was sized with. Not addressed here, deliberately: `caller_hour` at 10 and the global 100/hour are real cost ceilings at ~26B cycles a derivation, and personal notes has the identical exposure and is already live. Those numbers want a decision from whoever sized them, not a unilateral change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`create_tip`, `claim_tip` and `cancel_tip` had per-caller limits and no global ceiling. A per-caller limit is blind to one call each from a thousand fresh principals, and identities are free to create — so the tier that matters against a flood was the one missing. The mechanism already existed for the vetKey endpoints: peek every tier before any tier records, so a per-caller rejection never touches the global counters and a globally-rejected call never creates a per-caller entry. That is worth reusing rather than reimplementing, so `VetKeyRateLimiters` is renamed to `TieredRateLimiter` and gains a `with_tiers` constructor. A type called `VetKeyRateLimiters` guarding `create_tip` is the kind of name that costs the next reader an hour. The rename is mechanical: five lines across two files, and `new()` keeps the vetKey defaults so personal notes is untouched. Numbers: create and claim 20/min and 200/hour per caller, 300/min and 3000/hour globally; cancel 30/300 and 400/4000. Generous on purpose — a create makes two ledger *queries*, a few million cycles, three orders of magnitude below a vetKD derivation — so these bound a runaway rather than rationing ordinary use. No cost conversation needed either way: any ceiling is stricter than the none that was there before. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A claim whose payout did not go through reverted the tip to `Reserved` and recorded nothing. So a tip nobody had touched and a tip that had already failed a claimer were indistinguishable in the sender's History — and only the second is something the sender can act on, typically by topping up the account the tip draws from. `TipRecord` gains `last_claim_failure`, set on every failed payout by `release_claim` (which already owned the revert, and already checks the claim still owns the tip — so a late failure from a timed-out claim cannot mark a tip failed while somebody else is mid-payout). `TipStatus` gains `Failed`, reported only while the tip is still live. Expiry outranks it, because past the deadline there is nothing left to act on; `Claimed` and `Cancelled` outrank it too, while the record keeps the failure so a tip that succeeded on the second attempt can still show it was not first time lucky. The reason is a two-variant enum, not the ledger's error text: that text is written for an operator, it can name balances, and it has no business being rendered to a user. Only `Uncovered` and `TransferFailed` for now, which is all the ledger lets us tell apart — separating "the sender's balance is short" out of `TransferFailed` is a further change. Both new record fields are `Option`, so records already in stable memory keep decoding. The candid and its generated declarations are edited by hand rather than regenerated: `scripts/generate.sh` needs `didc`, which is not installed, and `dfx generate` writes a different file layout than this repo's pipeline. Both were validated by having dfx parse the interface. Note `scripts/lint.did.sh` reflows the whole of `backend.did` from tabs to spaces, which is pre-existing formatter drift and deliberately not included here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A failed payout gave the claimer one message for every cause: "nothing was transferred, so try again". But "the sender is temporarily out of funds" and "the call failed" call for very different things from the reader — the first is worth coming back for, and only the sender can fix it. The ledger already distinguishes them. `icrc2_transfer_from` returns `InsufficientFunds` separately from `InsufficientAllowance`, and we were folding the first into a generic `Failed` along with transport errors. It now travels through as `TipError::InsufficientFunds`, and is recorded on the tip as `TipClaimFailureReason::InsufficientFunds` so History can say the same thing. Kept apart from `Uncovered` deliberately: there the reservation itself is gone, so the link may be worthless; here the reservation still stands and the code still works, and topping up makes the same link pay out. Note for deployment: this adds variants to `TipError` and `TipClaimFailureReason`, both in return position. dfx correctly flags that as breaking — a client that does not know a variant cannot decode it — so the frontend must ship before the backend starts emitting these. That is the opposite of the order I used for the last be1 deploy. The candid and its declarations are hand-edited again. Worth noting that a plain `dfx deploy` reflowed the whole of `backend.did` from tabs to spaces as a side effect, which is the same pre-existing formatter drift `lint.did.sh` causes; that reflow is reverted and excluded here. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`KeyManager` keeps its vetKD 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:
pub fn init(memory: M, default_value: T) -> Self {
if memory.size() == 0 { return Self::new(memory, default_value); }
...
Self { value: Self::read_value(&memory, header.value_length), memory }
}
So the key name a store uses is whichever one was configured the first time it was
ever touched, permanently. The be1 test environment initialised this store while
its backend was configured with `dfx_test_key` — a name that exists only on a
local replica, because `test_be_*` is not special-cased in
`scripts/build.backend.args.sh` and falls through to the local default. Every
derivation there trapped with `SignCostError(InvalidKeyName)`, which surfaced to
the sender as "the link for this tip is not recoverable".
Correcting the deployment argument cannot fix that on its own: the bad name was
already frozen in memory 22. Moving to a fresh region is what lets `init` write
the corrected one. 22-25 must never be reused.
The abandoned region holds ciphertexts nobody could ever decrypt — the key to read
them was not derivable — so nothing recoverable is lost.
This is harmless for an environment that has never initialised the store, which
includes production.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Two ways the tip-secrets store could only ever grow. **Nothing released a secret but an explicit cancel.** `remove_tip_secret` resolved the owner from `msg_caller()`, and neither of the other two paths that should release one runs as the sender: a claim runs as the recipient, and the retention sweep runs as the canister on a timer. So a claimed tip's ciphertext stayed behind, and a swept tip's outlived the record that pointed at it — permanently, since nothing else would ever look at that key again. Removal is now addressed by the tip's owner. `EncryptedMaps` checks writes with `ensure_user_can_write(caller, key_id)`, which an owner satisfies implicitly, so passing the owner as both is what lets those paths clean up at all. Wired into the claim success path and the sweep; cancel keeps working and now goes through the same call. Two hazards the sweep had to avoid. It must not run inside `mutate_state`, because dropping a secret takes that borrow itself and nesting it panics on the `RefCell` rather than failing politely — so the ids are collected, the records removed, and the secrets dropped after the borrow closes. And it must not use `with_tip_secrets_mut`, which creates the store on first access: sweeping a canister where nobody ever stored a code would allocate 32 MiB of stable memory purely to find nothing to delete, and on a canister short of reserved cycles it would trap. Hence `with_existing_tip_secrets_mut`, which no-ops when there is no store. **`set_tip_secret` had no rate limit.** Every other tips write has a tiered limiter. This one writes 512 bytes to stable memory, is deliberately not gated on the tip existing, and `MAX_TIPS_PER_USER` bounds active tips rather than stored codes — so one registered caller could grow the store at ingress speed without creating a single tip. Same tiers as `create_tip`, which the browser pairs it with one-for-one. Also corrects a stale expectation in the revoked-allowance test: it asserted `Reserved` after a failed claim, but recording `last_claim_failure` made that `Failed`, a live state meaning "somebody tried and it did not pay out". The re-approve assertion just below already proves the tip is still claimable. That test was red on this branch before this change. No candid change: both endpoints keep their signatures. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`TipClaimFailureReason` was written as a three-line union where prettier puts it on one, so `npm run lint` failed on this branch and the next one. Later branches already carried the formatted version, which is why it only showed up when linting from the bottom of the stack. These declarations are hand-edited rather than generated — `scripts/generate.sh` needs `didc`, which is not installed, and `dfx generate` writes a different layout than this repo's pipeline — so nothing reformats them on the way in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Main took `MemoryId::new(20)` for `CONTACT_IMAGE_MEMORY_ID` while this branch was away, and tips had claimed the same id. Two structures pointing at one region decode into each other's data, and stable memory does not forgive that after a deploy — so this had to be settled before main is merged, not during. Tips moved rather than contacts, because contacts is already live on mainnet and tips is not. Ids run 21-26 now, contiguous, following this file's convention: sequential, with a retired id parked under a `RESERVED_` name rather than skipped, the way id 5 is for the old PoW challenge map. That means reusing 22-25, which an earlier comment here said never to touch. That warning is spent: the store it protected never initialised anywhere. The message that would have written its config cell trapped on the vetKD key name and rolled back, and the later attempt trapped on the canister's reserved-cycles limit before allocating anything. No environment holds a byte of it. be1 holds tips at the old ids and will not be able to read them after this. That is unavoidable — moving the primary map off 20 invalidates its data whatever the new number is — so be1 needs a reinstall rather than an upgrade on the next deploy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four conflicts, all where main's contact-image work meets tips. `state/memory.rs` was the one that mattered. Main took `MemoryId::new(20)` for `CONTACT_IMAGE_MEMORY_ID` while this branch was away, and tips had claimed the same id — two structures on one region decode into each other's data, and stable memory does not forgive that after a deploy. Tips had already been moved to 21-26 in anticipation, so both sides are kept: contacts at 20, tips above it, ids running 0-26 unbroken with none repeated. `state/mod.rs` and `types/maps.rs` were both sides adding names to the same sorted import lists — resolved as the union. `types/storable.rs` looked worse than it was: both sides appended independent types and git interleaved the hunks. Rebuilt from main's file plus our `TipId` and `TipSenderKey`, after checking that neither side removed anything and that our only other change there was widening one import. 286 backend unit tests and 31 tips tests pass, clippy and lint.did clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
… once Tips took stable-memory regions of their own, and those regions were renumbered late — main had claimed MemoryId::new(20) for contact images while this branch was away. Nothing until now upgraded the canister with a tip in it, so 'the records still mean the same thing afterwards' was an assumption. A region reopened as the wrong structure decodes into plausible rubbish rather than failing outright, so the amount and the deadline are asserted, not just that a row came back. The claim is submitted and left unawaited so the upgrade lands on a canister mid-payout. What that does not reach is recorded in the test: the lost-callback case behind open question 4 is not expressible in pocket-ic 15, because driving rounds to answer install_code is blocked while the claim's ingress is outstanding, and awaiting the claim to unblock it completes the claim.
|
✅ No security or compliance issues detected. Reviewed everything up to 453623c. Security Overview
Detected Code ChangesThe diff is too large to display a summary of code changes. |
Contributor
There was a problem hiding this comment.
Pull request overview
This PR introduces a full “tips via link/QR” backend feature built around ICRC-2 allowances (non-custodial), including stable storage/indexing, claim-code recovery via vetKD-encrypted storage, endpoint rate limiting, housekeeping pruning, and an extensive PocketIC test suite against a real ICRC-1/2 ledger.
Changes:
- Added tip domain types + backend API surface (
create_tip,get_tip,get_tip_details,claim_tip,cancel_tip, sender history, vetKD helpers, and encrypted claim-code storage). - Implemented stable-memory persistence (dedicated regions + by-sender index) plus hourly pruning and per-endpoint tiered rate limiting.
- Added integration tests using a real
ic-icrc1-ledgerwasm and upgrade-survival coverage, plus supporting test utilities and scripts.
Reviewed changes
Copilot reviewed 30 out of 32 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/shared/src/types/tip.rs | New shared Candid types/constants for tips (requests, responses, errors, history models). |
| src/shared/src/types/result_types.rs | Adds *Result wrappers for tips endpoints for Candid ergonomics. |
| src/shared/src/types.rs | Exposes the new tip module and extends Stats with tips_count. |
| src/declarations/backend/backend.factory.did.js | Updates JS IDL factory with tips types and methods. |
| src/declarations/backend/backend.factory.certified.did.js | Updates certified JS IDL factory with tips types and methods. |
| src/declarations/backend/backend.did.d.ts | Updates TS declarations with tips types/methods and tips_count. |
| src/declarations/backend/backend.did | Updates declarations DID with tips types/methods and tips_count. |
| src/backend/tests/it/utils/pocketic.rs | Removes dead-code suppression for an upgrade helper now used by tips tests. |
| src/backend/tests/it/utils/mod.rs | Exposes the new real-ledger test utility module. |
| src/backend/tests/it/utils/icrc1_ledger.rs | New PocketIC helper to deploy and interact with a real ICRC-1/2 ledger canister. |
| src/backend/tests/it/tips.rs | New end-to-end tips integration tests (real ledger, upgrade survival, secrets cleanup, rate limits). |
| src/backend/tests/it/stats.rs | Extends stats test expectations with tips_count. |
| src/backend/tests/it/main.rs | Registers the new tips integration test module. |
| src/backend/src/utils/rate_limiter.rs | Introduces generalized TieredRateLimiter and adds tip-specific limiters. |
| src/backend/src/utils/housekeeping.rs | Adds hourly pruning for tips past retention. |
| src/backend/src/types/storable.rs | Adds stable-structure key types for tips (TipId, TipSenderKey). |
| src/backend/src/types/maps.rs | Adds stable maps for tips and tips-by-sender index. |
| src/backend/src/tips/service.rs | New orchestration/storage logic for tips (create/read/claim/cancel/history/prune). |
| src/backend/src/tips/secrets.rs | New encrypted claim-code recovery store using vetKD (EncryptedMaps) + cleanup helpers. |
| src/backend/src/tips/model.rs | New pure-domain model: lifecycle/state, validation, constant-time code checks, status mapping. |
| src/backend/src/tips/mod.rs | New tips module entrypoint wiring submodules. |
| src/backend/src/tips/icrc2.rs | New minimal ICRC-1/2 client for fees, allowances, and transfer_from. |
| src/backend/src/state/mod.rs | Adds tips maps and lazy-initialized tip-secrets EncryptedMaps store to canister state. |
| src/backend/src/state/memory.rs | Allocates stable memory region IDs for tips and tip-secrets stores (and documents renumbering rationale). |
| src/backend/src/lib.rs | Wires tips types/results into the canister crate and includes the new module. |
| src/backend/src/api/tips.rs | New public canister endpoints for tips + rate limiting + guards. |
| src/backend/src/api/personal_notes.rs | Updates vetKey rate limiting calls to use TieredRateLimiter. |
| src/backend/src/api/mod.rs | Exposes the new tips API module. |
| src/backend/backend.did | Updates the canister DID interface with tips types/methods and tips_count. |
| scripts/test.backend.sh | Downloads the real ICRC-1 ledger wasm for integration tests and exports its path. |
| docs/ai/spec-driven-development/specs/2026-08-05-feat-tips-via-link.md | Updates/extends the spec with post-build decisions and link-recovery/auditability notes. |
| .gitignore | Ignores downloaded icrc1-ledger.wasm.gz. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| // Two callers racing the first fetch both write the same bytes, so last | ||
| // write wins is not a race worth guarding. | ||
| VETKEY_PUBLIC_KEY.set(Some(verification_key.clone())); |
Collaborator
Author
There was a problem hiding this comment.
Not taking this one — LocalKey<RefCell<T>>::set has been stable since Rust 1.73, alongside with_borrow / with_borrow_mut. Line 190 just above uses with_borrow from the same stabilisation.
It also compiles: backend-checks / lint is green on this PR, and the tips suite runs against a wasm built from this file.
with(|cell| *cell.borrow_mut() = ...) would be equivalent, but .set is the shorter form for exactly this case.
The store had its own length check instead of going through validate_tip_id, so it agreed on the upper bound and diverged on the lower one: an empty id was accepted. That key matches no tip, and claim, cancel and prune all clean up by tip id, so anything written under it would have outlived every cleanup path. Also unstacks two doc comments that had both ended up above InsufficientFunds, leaving ClaimInProgress undocumented and the public candid docs wrong about which variant is which. Both found by the Copilot review on #13859.
…-wallet into feat/tips-1-backend
The doc said there were only two and that TransferFailed covered a sender whose balance had dropped — which stopped being true when InsufficientFunds was added as its own variant precisely to carry that case. Rewritten around what the sender can do about each: Uncovered means only a new tip helps, InsufficientFunds means the same link works after a top-up, and TransferFailed means retry. Comment-only; the interface is unchanged, and the bindings job regenerates the .did copy. Found by the Copilot review on #13859.
Collaborator
Author
|
It asserted the in-flight claim still answers across the upgrade. Whether it does is up to whichever lands first, the upgrade or the ledger's reply — locally the claim completes, on CI the upgrade gets in between, destroys the callback and the call comes back trapped. So the test passed here and failed there. Which means the case I recorded as unreachable in pocket-ic is reachable after all; it just is not controllable. Rewritten around what holds either way: the record is committed before the await, the allowance is the source of truth for whether the money moved, and no scheduling may pay twice. The end state is the same on both paths and is now what the test pins — retry after the in-flight window and the claimer is paid exactly once, because either the first transfer landed and the allowance is spent, or it did not and this one pays.
Five of the shapes in tips/icrc2.rs already existed, identical on the wire, in
the crate signer/service.rs imports them from — so a second copy bought nothing.
Account, Allowance, AllowanceArgs, TransferFromArgs and TransferFromError now
come from ic_cycles_ledger_client, re-exported here so callers still import
ledger shapes from the module that speaks to the ledger. TransferFromResult is
gone: Result is what candid's variant { Ok; Err } decodes into, so it was a
third copy of one shape. backend.did is byte-identical, which is the check that
matters.
The calls stay local, and the module comment now gives the real reasons rather
than the one it had. It claimed the existing client 'speaks the cycles ledger's
dialect - icrc_2_approve, with underscores'. That was wrong: the underscore is
in the Rust method name and the wire strings there are the standard icrc2_*.
What actually justifies a separate module is that CyclesLedgerService uses
bounded_wait — and a payout is the one call here that must never come back
'maybe' — and that it is bound to one canister and carries cycles-only methods.
TransferFromCallError stays too: it is a tips domain type with no equivalent, and
its three-way split feeds the claim outcome directly.
…sing The regression test was named for four tiers and asserted one. with_tiers takes four positional numbers, so the mistake it exists to catch is a pair swapped — which the old assertion would have passed straight through. It now exercises each tier against the numbers personal notes was sized with, and each of the four assertions was checked by perturbing that tier and confirming the test fails. Writing it turned up that the hour cases are easy to get wrong in the direction of passing: 100 calls one per minute never fill a sliding hour, because the window only holds the last 60. They are packed five per minute over twenty minutes instead. The struct doc claimed the global key is 'never a real registered caller on these endpoints'. That was a statement about two specific callers, written as a property of the type — and now the type is general purpose. On an endpoint the anonymous principal can reach, the per-caller and global tiers are the same bucket and consume each other. The doc now says the guards are what make the shared key safe, and points an anonymous endpoint at RateLimiter instead.
Same correction as #13862, applied here verbatim so the merge is trivial. The struct doc called the global bucket's key 'never a real registered caller on these endpoints' — true of the two personal-notes callers it was written for, and now stated as a property of a general-purpose type that this branch adds six more callers to. On an endpoint the anonymous principal can reach, the per-caller and global tiers are the same bucket and consume each other. Every caller here is behind caller_is_registered_user or caller_is_not_anonymous, so it holds — but as a property of those guards, which is what the doc now says.
…-wallet into feat/tips-1-backend
Makes the stack real. The refactor was extracted to #13862 but both branches sat on main as siblings, so this PR still displayed 203 lines of it — including the personal_notes.rs call sites the reviewer asked to see on their own. Resolution keeps this branch's additions (the six tip limiters, with_caller_burst and its regression test) and takes the refactor's wording and its four-tier default test, which is strictly stronger than the one-tier version here and is what will be on main once #13862 lands.
artkorotkikh-dfinity
changed the base branch from
main
to
refactor/tiered-rate-limiter
August 31, 2026 08:25
Antonio's suggestion, and the right home: it is a types-only crate, no client and no transport, so it carries no opinion about how the calls are made. The previous commit borrowed the same shapes from ic-cycles-ledger-client, which worked but meant reaching through a crate named for the cycles ledger to talk to an arbitrary token ledger. Its Subaccount is [u8; 32] rather than a ByteBuf. Same blob on the wire, and it makes a wrong-length subaccount unrepresentable — so the two call sites now pass the SHA-256 array straight through instead of wrapping it. Verified where it counts: backend.did is byte-identical, and the 15 pocket-ic tips tests still pass against a real ICRC-1/2 ledger, which is what actually proves the encoding did not move. Costs ~35 KB of wasm (6.161 MB to 6.196 MB) for base32, crc32fast and minicbor coming along.
Same five shapes the production module just stopped duplicating: Account, AllowanceArgs, Allowance, ApproveArgs and ApproveError now come from icrc-ledger-types. A second copy in the test helper could drift from the encoding it is meant to be testing and still pass, which is the worst way for a test to be wrong. InitArgs, ArchiveOptions and FeatureFlags stay. They are this ledger wasm's install arguments rather than ICRC standard types, and no published crate ships them. Account::owner and Account::with_subaccount became free functions, since inherent methods cannot be added to a foreign type — six call sites. The subaccount is [u8; 32] now, so spender_subaccount returns the array and it passes straight through rather than being wrapped and unwrapped. Also corrects the module doc, which repeated the same wrong claim the production module already records: the cycles ledger does not speak a different wire protocol, the underscore in icrc_2_approve is a Rust method name.
AntonioVentilii
changed the title
Comment on lines
+1038
to
+1040
| get_tip_encrypted_vetkey: IDL.Func([IDL.Vec(IDL.Nat8)], [TipVetkeyResult], []), | ||
| get_tip_secret: IDL.Func([IDL.Text], [GetTipSecretResult], ['query']), | ||
| get_tip_vetkey_public_key: IDL.Func([], [TipVetkeyResult], []), |
Collaborator
There was a problem hiding this comment.
do we need these 3 methods? instead of open, can we not leave them inside? or are they necessary to be open?
Collaborator
Author
There was a problem hiding this comment.
All three have to be open, and they are the same triple the notes feature already ships:
get_tip_encrypted_vetkey— the vetKD derivation is secured to a browser-supplied transport key. Only the browser can finish the decryption, so the output has to leave the canister. Mirrorsget_personal_notes_encrypted_vetkey.get_tip_vetkey_public_key— the verification key the browser needs to check its derived vetKey. Mirrorsget_personal_notes_vetkey_public_key.get_tip_secret— returns the caller's own ciphertext, which the canister cannot read.EncryptedMapskeys every map by its owner, so it can only ever return the caller's own. Mirrorsget_personal_notes.
All three carry guard = "caller_is_registered_user", and none of them is callable on another principal's data. Keeping them internal would mean the browser can never decrypt a tip link it stored, which is the whole point of the store.
johandelforge
pushed a commit
to yogabuild/oisy-wallet
that referenced
this pull request
Sep 2, 2026
…dfinity#13862) # Motivation Split out of dfinity#13859 at review request: renaming and generalising a core shared rate limiter deserves its own review rather than riding in on a 32-file feature PR. `VetKeyRateLimiters` was built for the two personal-notes vetKey endpoints and had its four tiers hardcoded in the constructor. Nothing else could use it. Other endpoints want the same shape — a per-caller tier plus a global one, so a flood spread across many principals is still caught — with numbers of their own. The call sites this touches in `api/personal_notes.rs` are live code, which is the other reason it should not be buried in a feature branch. # Changes - Renamed `VetKeyRateLimiters` to `TieredRateLimiter`, since it is no longer about vetKeys. - Added `with_tiers(caller_minute, caller_hour, global_minute, global_hour)` so an endpoint can state its own limits. - Kept `new()` as the vetKey defaults, now delegating to `with_tiers(2, 10, 20, 100)`. - Updated the three call sites in `api/personal_notes.rs`. - Added two tests: one pins all four defaults through the move, one covers a global tier catching a flood the per-caller tier cannot see. - Corrected the struct doc, which claimed the global bucket's key is "never a real registered caller on these endpoints" — true of the two callers it was written for, but stated as a property of a type that is now general-purpose. # Tests - **The personal-notes limits are byte-identical.** `new()` produces 2/min and 10/hour per caller and 20/min and 100/hour globally, the same four values `VetKeyRateLimiters::new()` hardcoded, bound to the same windows. - `the_default_tiers_survive_the_move_into_with_tiers` asserts **all four**, because `with_tiers` takes four positional numbers and a swapped pair is the mistake it exists to catch — an earlier version checked only the first tier and would have passed with the hour and global limits reversed. Each assertion was verified by perturbing that tier alone and confirming the test fails. - Worth knowing for anyone writing more of these: the hour tiers fail in the direction of passing. 100 calls one per minute never fill a sliding hour, because the window only holds the last 60. They are packed five per minute across twenty minutes instead. - `cargo test -p backend --lib` — 265 passed (263 before, plus these two). - `./scripts/lint.rust.sh` and `cargo fmt --check` clean. - Pocket-IC suite not run: no endpoint behaviour changes here, only the type the two existing limiters are built from. ## Stack `dfinity#13859` (tips backend) is now based on this branch, so this one lands first. Nothing else depends on it. One deliberate omission: `dfinity#13859` also adds a `with_caller_burst` helper, for a tip endpoint that wants a higher per-minute burst. It has no caller on `main`, so it stays in that PR rather than landing here as dead code. --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: Antonio Ventilii <antonioventilii@gmail.com> Co-authored-by: github-actions <41898282+github-actions[bot]@users.noreply.github.com>
Conflict in `rate_limiter.rs`, from #13862 landing squashed while this branch carried the same refactor as separate commits. Resolved by keeping main's `new() -> with_tiers(2, 10, 20, 100)` and the tips-only additions on top: the two tip vetKD limiters, `with_caller_burst`, and its burst test. Both sides' tests now run and pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Review feedback. The same reasoning is already the module doc of `tips/icrc2.rs`, where it sits next to the code it explains, so the manifest does not need a second copy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AntonioVentilii
approved these changes
Sep 3, 2026
AntonioVentilii
left a comment
AntonioVentilii
left a comment
Collaborator
There was a problem hiding this comment.
Some small comments, but LGTM tks
| /// `TipError::NotFound` for an unclaimable tip or a wrong claim code. | ||
| #[query(guard = "caller_is_not_anonymous")] | ||
| #[must_use] | ||
| pub fn get_tip_details(request: TipClaimRequest) -> GetTipDetailsResult { |
Collaborator
There was a problem hiding this comment.
not for this iteration, but to keep it in mind:
we could put some kind of cap on the methods: get_tip_details, get_my_tips and get_tip_secret. I understand the point that a stateful limiter is a no-op on the query path, but that reasoning is written up only on get_tip, so as it stands the other three read as simply unlimited rather than deliberately so. Even just repeating the justification on them, or capping the rows get_my_tips returns, would make it clear this was a decision.
Collaborator
Author
There was a problem hiding this comment.
Took the documentation half now, in 5769753 — it was a fair hit that the reasoning existed in exactly one place.
get_tip_details, get_my_tips and get_tip_secret each carry the justification themselves now, with what actually bounds them:
get_tip_details— one O(log n) lookup that also has to guess a 128-bit claim code.get_my_tips— the row cap is the bound, and a caller can only read their own tips.get_tip_secret— one keyed lookup scoped to the caller.
On capping get_my_tips: it is already capped. service::get_my_tips truncates to MAX_TIPS_RETURNED, and the endpoint doc said so — but the cap read as a pagination detail rather than as the thing standing in for a limiter, so I have now said which job it is doing.
Leaving the actual limiters out of this iteration as you suggested. If we ever want a real cap on the query path it has to be something that survives a non-persisted query — certified queries, or moving the counter into the update path — and that is a bigger decision than this PR.
Review feedback: "not at install" only held for the lazily-attached stores. `tips` and `tips_by_sender` are built inside the `STATE` thread_local, and `#[init]` reaches it through `set_config` -> `mutate_state`, so their buckets are claimed at install. Only the vetKeys `EncryptedMaps`, held as `Option` and attached on first use, claim theirs lazily. The failure mode differs accordingly: a tight `reserved_cycles_limit` breaks the install for one and first use for the other. Also records why the three remaining tip queries carry no rate limiter, which until now was written up only on `get_tip` — so they read as accidentally unlimited rather than deliberately so. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
artkorotkikh-dfinity
marked this pull request as draft
September 3, 2026 09:18
artkorotkikh-dfinity
marked this pull request as ready for review
September 11, 2026 11:25
AntonioVentilii
approved these changes
Sep 11, 2026
DenysKarmazynDFINITY
approved these changes
Sep 11, 2026
AntonioVentilii
enabled auto-merge
September 11, 2026 13:17
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Sending crypto today needs the other person to already have a wallet and to give you an address. Tips removes both: the sender reserves an amount, gets a link or QR, and whoever opens it can claim into a wallet they create on the spot. This is the canister half — the endpoints, the storage and the payout path.
The design rule throughout is that OISY never takes custody. The tokens stay in the sender's own account until someone claims, and the payout is an
icrc2_transfer_fromagainst an allowance the sender granted for exactly that tip.Changes
create_tip,claim_tip,cancel_tip, plus the reads behind the share link and the sender's history.with_caller_bursthelper: a sender spends one vetKD derivation per page load, so the default 2/min failed a third reload and silently cost that tip its recoverable link. Only the minute tier moves — the hour tier still binds at 10, so worst-case cycle spend is unchanged.mainclaimed id 20 for contact images.every_memory_id_is_claimed_once, which parses the ids back out ofstate/memory.rsand fails on a duplicate. See the note below — this is the one part of the PR that is not about tips, and it is here because this is the PR that hit the problem.docs/ai/backend/workflows/state-and-migrations.md.Failedwhile the link stays live.Stacked on #13862
Based on
refactor/tiered-rate-limiter, notmain. The rename and generalisation of the shared limiter was split out at review request, so review that one first — the diff here no longer shows it, or theapi/personal_notes.rscall sites that came with it.Tests
src/backend/tests/it/tips.rs, run against a real ICRC-1/2 ledger rather than a mock — every guarantee here is a guarantee about what a real ledger does with an allowance../scripts/lint.rust.shandcargo fmtclean.What reviewers should know about the stable memory
Six new regions is about 48 MiB, and it lands on first use. Each
MemoryIdclaims a whole bucket — 128 pages, 8 MiB — the first time it is written, not at install. So the cost appears the first time someone creates a tip, and it appears as a memory grow that needs reserved cycles. On a canister with a tightreserved_cycles_limitthat fails long after the deploy looked fine. This is not hypothetical; it is what be1 did:The id namespace has no compile-time protection, and that nearly bit us. Tips originally took
MemoryId::new(20). While this branch was away,maintook 20 forCONTACT_IMAGE_MEMORY_ID. Both sides compiled, linted and passed their own tests; two structures on one region decode each other's data, and it would only have shown up after both merged and deployed. Tips moved to 21-26 — contacts is live on mainnet, tips is not.That collision was caught by reading a diff, which is not a control, so this PR adds the test. It parses the ids out of
state/memory.rsrather than listing them, so a constant added later is covered automatically. Confirmed it reproduces the original case:Atomicity: the guard and the docs note are not about tips. They are here rather than in a follow-up because this PR is the one that renumbered the ids, and shipping the renumber without the thing that makes the collision impossible leaves the next person in the same position. Happy to split them out if you would rather.
One CI note:
backend-tests / breaking-interfacewill fail, and it is not this branch. The candid diff here is purely additive (no removals), but the check compares against the deployed interface, andmainalready fails it identically —create_active_user_transactiongained anOisyTradevariant that is not live on the canister yet. Reproduced onorigin/mainwith no tips code present.Update (2 Sep): @AntonioVentilii marked this PR breaking to clear the check, so the declaration below is here to satisfy the second half of the job — it needs both the
!in the title and aBREAKING CHANGE:line in the body. Mergedmainin as well, resolving therate_limiter.rsconflict left by #13862 landing squashed.BREAKING CHANGE: the backend candid interface is incompatible with the currently deployed canister. This PR's own candid diff is purely additive; the incompatibility is the
OisyTradevariant added tocreate_active_user_transactionin #13796, which the live canister predates. A backend deploy clears it.