Skip to content

fix(widget): respect tool allow-list on first route request#791

Open
tomiiide wants to merge 2 commits into
mainfrom
fix/emb-450-stale-localstorage-allow-list-returns-disabled-exchange
Open

fix(widget): respect tool allow-list on first route request#791
tomiiide wants to merge 2 commits into
mainfrom
fix/emb-450-stale-localstorage-allow-list-returns-disabled-exchange

Conversation

@tomiiide

Copy link
Copy Markdown
Contributor

Which Linear task is linked to this PR?

https://linear.app/lifi-linear/issue/EMB-450/stale-localstorage-allow-list-returns-disabled-exchange-routes

Why was it implemented this way?

The integrator allow-list (exchanges.allow / bridges.allow) is enforced only through the enabledExchanges / enabledBridges store arrays, which are populated asynchronously by the /tools fetch. Before that resolves the array is empty (cleared or fresh storage) or stale, and useRoutes then either omits the exchanges/bridges filter entirely (an empty array reads as "no restriction") or sends the stale list. So the first /advanced/routes request escapes the allow-list and can return disabled tools (OKX, Kyber, etc.); it self-corrects once /tools resolves, which is why only the first request was affected.

Fix:

  • Seed enabled* from the config allow-list synchronously at store creation (covers empty/cleared localStorage).
  • Intersect any persisted state against the config in the persist merge (covers a stale persisted list).

This makes the store reflect the allow-list from the first render with no dependency on /tools timing. A useRoutes-only fallback was considered but rejected: it scatters the config logic into a second place and leaves the persisted store and settings UI inconsistent.

Known limitation: deny-only / no-allow configs cannot be pre-seeded synchronously (the full tool universe is unknown until /tools). The reporting integrator uses allow, so this is covered.

Visual showcase (Screenshots or Videos)

N/A (request-payload behavior). Repro: with exchanges: { allow: ['fly'] }, clear localStorage and block /tools in DevTools, then trigger a quote. The first POST /advanced/routes body omits options.exchanges before the fix and sends { allow: ['fly'] } after.

Checklist before requesting a review

  • I have performed a self-review and testing of my code.
  • This pull request is focused and addresses a single problem.
  • If this PR modifies the Widget API or adds new features that require documentation, I have updated the documentation in the public-docs repository. (N/A, no API change)

Seed enabled tools from the config allow-list at store creation so the first route
request, before /tools loads, no longer drops the filter and returns disabled tools.
@changeset-bot

changeset-bot Bot commented Jun 15, 2026

Copy link
Copy Markdown

🦋 Changeset detected

Latest commit: fe485f7

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 3 packages
Name Type
@lifi/widget Patch
nft-checkout Patch
tanstack-router-example Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions

github-actions Bot commented Jun 15, 2026

Copy link
Copy Markdown

E2E Examples — all passed

All examples passed in the latest run.

@github-actions

github-actions Bot commented Jun 15, 2026

Copy link
Copy Markdown

E2E Playground results

passed  158 passed

Details

stats  158 tests across 10 suites
duration  2 minutes, 9 seconds
commit  fe485f7

📥 Download full HTML report (open the run → Artifacts → playwright-report)

@lifi-qa-agent

lifi-qa-agent Bot commented Jun 15, 2026

Copy link
Copy Markdown

🔍 QA Review — EMB-450 · PR #791

Ticket: Stale localStorage allow-list returns disabled exchange routes
Status: In Review | Priority: Medium | Labels: Bug, TechSupport, Apyx
PR: fix(widget): respect tool allow-list on first route request · +165 / −12 · 3 files
Review type: 🔁 Re-review (post changes-requested, commit fe485f74)


🔁 Re-Review — Resolution of Previous Change Requests

Three items were flagged in the initial review (2026-06-15T12:40). Developer (tomiiide) addressed all three in commit fe485f74 (2026-06-15T13:46).


✅ Resolution Status

# Item Severity Status Evidence
1 Test gap — zero-intersection rehydration 🟠 Medium ✅ Fixed (code + test) merge now overlays allowMap onto persistedEnabled before buildToolState; new test 'keeps the allow-list when the persisted map has no overlap' asserts enabledExchanges = ['fly'] for _enabledExchanges: { okx, kyber } + allow: ['fly']
2 Test gap — stale bridges merge path 🟠 Medium ✅ Fixed (test added) New test 'intersects a stale persisted bridges list with the current config' seeds { across, hop, stargate } and asserts enabledBridges = ['across']
3 Deny-only limitation undocumented in code 🟢 Low ✅ Fixed (comment added) // Deny-only configs can't be seeded; the tool universe is unknown until /tools. placed inline near initialSettings

Additional notes from prior review — also addressed:

  • STORE_VERSION = 6 now carries comment: // Must match the persist version in createSettingsStore, else merge tests silently become migration tests.
  • afterEach now restores original globalThis.localStorage

🔍 Re-Review Code Analysis

Item 1 — zero-intersection fix (code verified):

The prior merge called buildToolState(toolType, persistedEnabled) directly. With a zero-overlap persisted map (e.g. _enabledExchanges: { okx: true, kyber: true } and allow: ['fly']), buildToolState filters out all keys → enabledExchanges = []. In useRoutes, the guard allowedExchanges?.length || disabledExchanges.length evaluates to falsy, so options.exchanges is omitted entirely — the original timing-gap bug survives the rehydration path.

The new merge:

const allowMap = toEnabledMap(configItemsByToolType[toolType]?.allow)
Object.assign(state, buildToolState(toolType, { ...allowMap, ...persistedEnabled }))

Spreading allowMap ({ fly: true }) first, then persistedEnabled ({ okx: true, kyber: true }), produces { fly: true, okx: true, kyber: true }. buildToolState filters to only flyenabledExchanges = ['fly']. The allow-list is preserved even with zero-overlap. The developer's analysis of the useRoutes empty-array semantics is accurate and the code fix is correct. ✅

No regressions from the new merge logic:

  • Partial-overlap case (fly + stale extras): stale extras filtered, fly retained ✅
  • Fresh store (no localStorage): persistedEnabled branch not entered; initialSettings seed applies ✅
  • No-config integrator: toEnabledMap(undefined)[] → empty allowMap; { ...{}, ...persistedEnabled } = persistedEnabled; isItemAllowedForSets(key, undefined) = true → full persisted state passes through ✅

Items 2 & 3 are straightforward additions (test + comment). No logic changes. ✅


🚦 Verdict: Pass ✅

All three flagged items are resolved — two with both code changes and new tests, one with an inline documentation comment. The additional test-hygiene notes (version comment, afterEach restore) were also actioned. No new issues identified in the updated diff.

Ticket coverage: Complete — the explicit AC is met, the known limitation is documented in code, and the key edge cases are covered by unit tests.


Reviewed by lifi-qa-agent · EMB-450 · 2026-06-15

@lifi-qa-agent lifi-qa-agent Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Requesting changes on 3 items — each requires either a code fix or an explicit acceptance comment with justification before this review is considered complete.

# Severity Type Issue / File
1 🟠 Medium Test gap packages/widget/src/stores/settings/createSettingsStore.test.ts — zero-intersection rehydration
2 🟠 Medium Test gap packages/widget/src/stores/settings/createSettingsStore.test.ts — stale bridges merge path
3 🟢 Low Documentation Deny-only known limitation undocumented in code

1. [Medium] Test gap — createSettingsStore.test.ts — zero-intersection rehydration

The stale-rehydration test (Test 2) seeds _enabledExchanges: { fly: true, okx: true, kyber: true } — which deliberately includes the currently-allowed tool (fly). There is no test for the case where the persisted map has zero intersection with the current allow-list (e.g. returning user who was using okx before the integrator tightened to allow: ['fly']).

In this case buildToolState correctly filters out all keys, producing enabledExchanges = []. In useRoutes, allowedExchanges = enabledExchanges = [] — an empty array sent to the SDK. This is the strictest-correct interpretation of the AC ("no disabled tools allowed through"), but creates a transient "no routes found" state until /tools resolves. Without a test, this behaviour is invisible and any future refactor of buildToolState could silently change it.

  • Missing: test with seedPersistedState({ _enabledExchanges: { okx: true, kyber: true } }) + config: { exchanges: { allow: ['fly'] } } asserting enabledExchanges = [] and _enabledExchanges = {}

2. [Medium] Test gap — createSettingsStore.test.ts — stale bridges merge path

SettingsToolTypes = ['Bridges', 'Exchanges'] — the merge function loops over both. Test 2 only seeds _enabledExchanges; there is no test for the bridges merge path. The code paths are symmetric, but an uncovered path is a silent regression target.

  • Missing: test with seedPersistedState({ _enabledBridges: { across: true, hop: true, stargate: true } }) + config: { bridges: { allow: ['across'] } } asserting enabledBridges = ['across'] and _enabledBridges = { across: true }

3. [Low] Deny-only known limitation undocumented in code

The PR description clearly states: "Known limitation: deny-only / no-allow configs cannot be pre-seeded synchronously." However, this is not reflected in the source code itself. A reader of createSettingsStore.ts seeing the initialSettings seeding block has no indication that deny-only configs are intentionally not seeded, or what the first-request behaviour will be (allowedExchanges = [] until /tools resolves — stricter than before the fix).

Add a brief inline comment near the buildToolState calls in initialSettings (e.g. // deny-only configs cannot be seeded synchronously — the full tool universe is unknown until /tools resolves) so future maintainers understand the constraint without reading the PR description.


💡 Once you've addressed the items above, re-apply the "Agent Review Request" label to trigger an automated re-review.

Overlay persisted toggles on the config allow seed in merge so a returning user whose
persisted tools don't intersect the new allow-list still sends it, not an unrestricted
request. Adds bridges and zero-intersection tests; documents the deny-only limitation.
@tomiiide

tomiiide commented Jun 15, 2026

Copy link
Copy Markdown
Contributor Author

Addressed in fe485f7.

1. Zero-intersection rehydration (Medium) — Fixed in code, not just tested. The empty-array case is actually unsafe: useRoutes treats enabledExchanges = [] as "no filter" (the allowExchanges?.length || disabledExchanges.length guard is falsy), so it omits options.exchanges and the request goes out unrestricted, i.e. the original bug. So merge now overlays persisted toggles on the config allow-list seed before intersecting; a zero-overlap persisted map keeps the allow-list. New test asserts enabledExchanges = ['fly'] (not []) for seedPersistedState({ _enabledExchanges: { okx, kyber } }) + allow: ['fly'].

2. Stale bridges merge path (Medium) — Added a test seeding _enabledBridges: { across, hop, stargate } + bridges: { allow: ['across'] }, asserting enabledBridges = ['across'].

3. Deny-only limitation (Low) — Inline comment added next to initialSettings: deny-only configs can't be seeded since the tool universe is unknown until /tools.

Also from the notes: afterEach now restores the original globalThis.localStorage, and STORE_VERSION carries a comment that it must match the store's persist version.

@lifi-qa-agent lifi-qa-agent Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

QA re-review complete — all 3 change requests resolved in fe485f7. Zero-intersection fix verified correct, bridges test added, deny-only comment added. ✅ Pass.

@vinzenzLIFI vinzenzLIFI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested this by programatically pre-loading the browser with a saved session containing a broad list of allowed exchanges while the app was configured to only allow fly. Before the fix the first swap route request sent all pre-loaded exchanges to the API (disabled exchanges could appear in results. After the fix the first request correctly sends only the configured exchange regardless of what was saved in the browser.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants