feat(core): add the non-custodial wallet partner user role - #205
feat(core): add the non-custodial wallet partner user role#205joshuakrueger-dfx wants to merge 2 commits into
Conversation
The API grants `NonCustodialWalletPartner` to employees of a wallet partner (DFXswiss/backend#4587, `src/shared/auth/user-role.enum.ts`). Without the member, every consumer compares the role as a string literal, and a one-character drift silently hides a gated screen from everyone entitled to it. Value is character-identical to the API enum.
The enum is a contract against the API: every value is a string the backend compares literally. A duplicate makes two roles indistinguishable, a stray space or hyphen breaks the comparison, and neither is caught by the compiler. Three invariants over all members rather than an assertion on any single one - no duplicates, anchored PascalCase, and a fixed member count so the other two cannot pass over an empty set.
|
@marassteiner please review This is a PR, not an issue: 2 files against its base branch, head |
|
Sorry — I've reached my hourly job limit and can't take this on right now. I'll pick it up automatically once the limit resets. |
marassteiner
left a comment
There was a problem hiding this comment.
Review summary
Recommendation: approve
Additive UserRole member for the non-custodial wallet partner contract, plus invariant tests. No merge-blockers found. Gate A (two independent Codex reviews: Konformität + Logik) and Gate B (local lint / format / build / test) completed.
What changed
| File | Change |
|---|---|
packages/core/src/definitions/jwt.ts:16 |
NON_CUSTODIAL_WALLET_PARTNER = 'NonCustodialWalletPartner' |
packages/core/src/__tests__/user-role.test.ts (new) |
no duplicates, anchored PascalCase, length 13 |
Gate A — AI diff review
A — Konformität / Qualität
- Scope matches CONTRIBUTING: source-only feature PR, conventional commits (
feat(core),test(core)), branchfeat/…→develop, tests undersrc/__tests__/*.test.ts. - Public API is additive; no version / CHANGELOG / lockfile edits.
- One non-blocking test-strength note (see follow-up).
B — Logik / Korrektheit
- String enum:
Object.valuesyields exactly the 13 value strings (no reverse-mapping half). - Contract string is character-identical to the stated API role name.
- Publishing the string alone does not grant privileges; auth remains server-side.
- No exhaustive
switch/Record<UserRole, …>consumers in this monorepo (only defs, re-exports,Jwt.role/Session.role). - 0 merge-blockers.
Gate B — local software
On head 7ef2f7c after npm ci:
| Gate | Result |
|---|---|
npx lerna run lint |
exit 0 (0 errors; pre-existing warnings only in bip322-multisig tests) |
npx lerna run format:check |
clean |
npx lerna run build |
exit 0 (all 4 packages) |
npx lerna run test |
core: 7 suites / 72 tests pass (includes user-role.test.ts); bip322: 45 pass + 1 todo |
LOCAL_RUN_OK
Merge-blockers
None.
Draft status and empty CI rollup are process notes, not defects in the diff. CI was noted as not yet configured on this draft.
Follow-up (must not block merge)
- UserRole test strength —
user-role.test.ts:18-28accepts any 13 unique PascalCase strings and does not pin'NonCustodialWalletPartner'. A fixed length also collides with parallel role PRs.
Tracked: #223
Release sequencing (not a merge-blocker)
Agree with the PR body: merging is safe; releasing the package version should wait until DFXswiss/api#4587 has a final role name. A published enum member is harder to retract than a frontend string.
Recommendation
Approve. Mark ready and merge when you want; hold the publish/release until the API role is final.
Why
Not symptom-driven: no incident. DFXswiss/api#4587 introduces the
NonCustodialWalletPartnerrole (
src/shared/auth/user-role.enum.ts:15), and DFXswiss/app#1262 gates the partnerdashboard on it. Without the member here, the consumer has to compare
session.roleagainst astring literal — the review on services#1262 blocks on exactly that, and the same rule was applied
to services#1270: the SDK owns the contract, the call site does not re-declare it.
Scale: the role is the entry condition for the partner dashboard. Cake alone has 126,988 users
in production and is the first of several wallet partners; every one of them reaches the screen
through this role.
Smaller fix considered: keep the string allow-list in the consumer
(
PARTNER_DASHBOARD_ROLES = ['NonCustodialWalletPartner'] as const). Rejected — a drift of onecharacter silently hides the page from everyone entitled to it, and the failure is invisible: no
type error, no runtime error, just an empty menu. It is also the only role comparison in the whole
of
services/srcthat would not go throughUserRole; every other one(
labels.ts:173,navigation.tsx:237,navigation.tsx:246,support-dashboard-issue.screen.tsx:34) already uses the enum.What
One line in
packages/core/src/definitions/jwt.ts, appended toUserRolethe same wayMONITORINGwas added in #177:The value is character-identical to the API enum it mirrors.
Consumers
A new enum member is additive, but only if nothing maps the enum exhaustively. Checked, not
assumed:
packages:UserRoleappears injwt.ts,session.tsand the two index re-exports only —no
switch, noRecord<UserRole, …>.services: the single mapping isPartial<Record<UserRole, Department[]>>(
src/util/support-helpers.ts:20) — partial, so a new member does not break the build.dfx-wallet: noRecord<UserRole, …>at all.Verification
tsc -b packages/core/tsconfig.build.json— exit 0. Both commits signed and GitHub-verified.Measured on
7ef2f7cin a clean worktree with its ownnpm ci:7 suites / 72 tests passing,
lintexit 0 with empty output,format:checkclean.The test asserts invariants over the whole enum, not the new line. A test saying
UserRole.NON_CUSTODIAL_WALLET_PARTNER === 'NonCustodialWalletPartner'would restate thedeclaration it tests and could only fail when someone edits that line on purpose. What is worth
pinning is the property every member shares, because the enum is a contract: each value is a string
the backend compares literally, and neither a duplicate nor a stray space is caught by the
compiler. So: no duplicate values, anchored
^[A-Z][A-Za-z]*$(whichVIPandKycClientCompanyboth satisfy), and a fixed member count — the count matters, because without it the other two would
pass over an empty set.
Three mutations, each applied with an asserted hit count of one and reverted afterwards:
'Monitoring x', regex left anchoreduses contiguous PascalCase contract stringsfails — 1 of 3'Admin')has no duplicate valuesandhas exactly 13 membersfail — 2 of 3The suite is green again after each revert.
Final pass (7ef2f7c):
Coherent: one enum member and the test that pins the enum's contract — both about the same
declaration, nothing else in the diff.
Nothing extra: only the member the consumer needs.
ClientCompany, which the API accepts onthe same endpoints, is deliberately not added — no consumer asks for it, and adding a role on
speculation is exactly the surplus this would be. The test covers invariants rather than the new
line, so it does not grow with the next role.
Sources closed: the review comment on services#1262 (
SDK enum member first, then consume) —implemented; the same rule on services#1270 — same shape; api#4587 as the source of the value —
quoted verbatim; the mechanical gate's "no test in the PR" — answered with the invariant test above
rather than a tautological one; no issue, no prior review on this PR.
Sequencing — please do not release this ahead of the API
The value mirrors a role that does not exist on
api/developyet: it lives only in the TypeScriptenum of DFXswiss/api#4587, with no migration and no database row behind it, and that PR is still
open with review points outstanding. The name itself has not been questioned in its review — but it
is code, not data, so nothing would stop it from changing.
A published enum member in a public package is harder to take back than a line in a frontend.
So: merging this is safe at any time, releasing it should follow api#4587. If the role is
renamed there, this PR follows before any version goes out.
After this lands
DFXswiss/app#1262 raises
@dfx.swiss/reactand replaces the string allow-list with the enummember. That step needs a published version, which this PR alone cannot produce.