feat(sdp-web): move token/RPC actions off the playground proxy - #1064
Conversation
Route mint/burn/seize/freeze/pause/authority/allowlist/RPC-test calls through dedicated dashboard API wrapper routes instead of the generic /api/playground/execute proxy, so that route's only remaining consumer is the API playground shell. Reuses the shared dashboardFetch helper (extended with status/body) instead of hand-rolled fetch parsing.
Greptile SummaryThe PR moves token mutations and RPC testing from the generic playground executor to dedicated authenticated dashboard routes.
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains. Important Files Changed
Sequence DiagramsequenceDiagram
participant UI as Dashboard UI
participant Route as Dedicated /api/dashboard route
participant Proxy as proxyToSdpApi
participant API as sdp-api
UI->>Route: Mutation or RPC-test request
Route->>Proxy: Fixed upstream path
Proxy->>API: Project-scoped authenticated request
API-->>Proxy: Status and response body
Proxy-->>Route: Pass-through response
Route-->>UI: Status and response body
Reviews (3): Last reviewed commit: "Merge branch 'main' into feat/move-issua..." | Re-trigger Greptile |
There was a problem hiding this comment.
I have looked through all 16 files. I think the direction here is right and the security shape is a real improvement: fixing
the upstream path server-side instead of accepting it from the request body is the actual win, and
proxyToSdpApi adding a required project scope makes these routes tighter than the playground path they
replace. Object.hasOwn for the action guard rather than in is the right call too.
Holding on the two red checks, because one of them is caused by this change.
1. The E2E helper still targets /api/playground/execute. issuance.e2e.spec.ts:153 matches on response.url().endsWith(“/api/playground/execute”) plus the old {“method”:…} envelope in the post body, then asserts payload.ok === true. This PR moves those actions off that URL and removes that envelope, so the predicate can never match and it times out at 180s — that’s the failing
“7. user can update an authority” shard.
Worth flagging that the helper is stale in three ways, not one: the URL, the postData.includes on the sdp-api path (which is no longer in the body), and the payload.ok assertion. So it needs updating to the new contract rather than a matcher tweak.
Also worth knowing the red test understates it. e2e-issuance.sh runs the shards sequentially under set -euo pipefail, so when authority and supply fails, freeze controls, pause controls and allowlist never run at all. Test 6 (allowlist) uses the same helper at :467 and :486, so it’s broken by this change too and is currently masked. Three shards of coverage are silently skipped.
Until that’s fixed, authority/mint/burn/seize/freeze/allowlist have no end-to-end cover: the new unit test mocks proxyToSdpApi entirely, so it proves the action→path map and the 404 guard and nothing about whether a request actually reaches sdp-api authenticated.
2. Translation Catalog Policy. check-catalog-change-policy.mjs rejects any PR touching both messages/en/** and a localized catalog, so the four en + fr edits trip it. Dropping the two fr files clears it.
Also, the policy’s rationale is that the translation agent adds missing keys on the release PR. These are deletions. If the automation doesn’t prune removed keys, dropping the fr edits leaves five orphaned French keys behind. @GuiBibeau owns #1022, you might want to confirm which way that cuts.
3. Non-blocking, on the new unit test. It covers the 404 path with “not-a-real-action”. The reason the route uses Object.hasOwn rather than in is #926 , with in, __proto__, constructor and toString all match and would proxy through. Worth adding those three to the 404 case so the guard is pinned and a later refactor back to in fails the test instead of quietly reopening it.
4. Non-blocking. In organization-rpc-settings-form.tsx the old code checked !envelope.body?.data before destructuring; the new one checks only !result.ok then reads result.data.data. A 200 with a malformed body now throws instead of returning the friendly message. It’s inside the existing try/catch so it degrades to a generic error, just noting it since rpcTestFailedStatus went away with it.
Happy to re-review as soon as the E2E is pointed at the new routes.
Rewrite waitForActionResponse for the new contract: match the dashboard route URL + real HTTP method instead of the retired /api/playground/execute envelope, and assert on the passed-through upstream status rather than the playground's ok flag. Covers the allowlist shard (test 6) as well as the authority shard that surfaced the failure. Drop the fr catalog edits per the catalog change policy (translation automation owns localized files), and pin the Object.hasOwn action guard with __proto__/constructor/toString 404 cases.
|
Addressed in 7954ba0: 1. E2E helper — 2. Translation Catalog Policy — dropped the two 3. Action guard test — added 4. Agreed it degrades to the generic catch message on a malformed 200; leaving as is since the route now passes through the upstream status, so a non-2xx upstream surfaces its real error via |
tobySolutions
left a comment
tobySolutions
left a comment
There was a problem hiding this comment.
path fixed server side instead of read off the body is the real win here, and the project scope check makes these tighter than the playground route they replace.
Two things for follow up: result.data.data in the rpc form can still throw on a malformed 200, and there are 7 orphaned fr keys to prune not 5 (the two rpcTestFailed ones in dashboard-custody).
4 participants
Route mint/burn/seize/freeze/pause/authority/allowlist/RPC-test calls through dedicated dashboard API wrapper routes instead of the generic /api/playground/execute proxy, so that route's only remaining consumer is the API playground shell. Reuses the shared dashboardFetch helper (extended with status/body) instead of hand-rolled fetch parsing.