feat(kyc-controller): generalize UKYC vendor APIs and add status polling - #9908
feat(kyc-controller): generalize UKYC vendor APIs and add status polling#9908georgeweiler wants to merge 10 commits into
Conversation
Parameterize vendor HTTP and the consents path so KYC stays vendor-neutral, and add refreshKycStatus / statusChanged for user-keyed KYC status. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
…tus polling Stored terms acceptance now records the vendor it was given for, so one vendor's disclaimer ids can no longer satisfy another vendor's flow. `refreshKycStatus` no longer restarts polling when a `reset()` landed mid-request, and `acceptTermsAndStartSession` accepts a zero-argument call again. Co-authored-by: Cursor <cursoragent@cursor.com>
…s to null The optional `accessToken` and `country` params could be `undefined`, which is not assignable to TanStack's `Json`-constrained `QueryKey`, breaking the build. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@cursoragent review this PR |
|
Now I'll provide a comprehensive review of this PR. Let me examine the Bugbot findings and conduct my own analysis. Code Review: KYC Controller Generic UKYC Vendor APIsSummary of ChangesThis PR generalizes the KYC controller to support multiple identity vendors (currently MoonPay and Iron) by:
Analysis of Bugbot IssuesIssue #1: Cross-vendor terms reuse ✅ VALID HIGH SEVERITY The root cause is a state migration problem. Terms accepted before this PR have #hasTermsForVendor(vendor: KycVendor): boolean {
return (this.state.termsAcceptedVendor ?? 'moonpay') === vendor;
}When users upgrade and call Fix: Clear terms if #hasTermsForVendor(vendor: KycVendor): boolean {
if (this.state.termsAcceptedVendor === null) {
return false; // Force reacceptance for pre-migration state
}
return this.state.termsAcceptedVendor === vendor;
}Issue #2: Polling restarts after reset The concern is that
This seems like a design question rather than a bug. If Issue #3: Zero-arg session start crash ❌ INVALID The code uses optional chaining correctly ( Additional Issues Found1. Race condition in vendor customer creation (Medium) In The issue: if customer creation fails AND reset happens during the failure, the error won't be recorded. While not critical (reset clears error anyway), it's inconsistent with the pattern elsewhere. 2. Hardcoded T&C flags in Lines 737-740: await this.#startConsentsSession({
sumsubTncSigned: true,
idosTncSigned: true,
});When Recommendation: Persist 3. Inconsistent vendor field naming (Low)
Positive Aspects✅ Excellent generation-based async guard pattern prevents state corruption RecommendationsMust Fix (before merge):
Should Fix: Nice to Have: Test Plan Verification
Would you like me to implement fixes for the identified issues?   |
There was a problem hiding this comment.
Pull request overview
This PR expands @metamask/kyc-controller to support multiple UKYC identity vendors (MoonPay + Iron) behind a unified service surface, adds a non-MoonPay “consents path” flow, and introduces user-keyed KYC status polling for Money UI surfaces.
Changes:
- Generalize vendor HTTP endpoints in
KycService(vendor-parameterized disclaimers/KYC-required plus new customer/consents/status endpoints). - Add non-MoonPay (
iron) controller flow (customer → disclaimers → consents → SumSub) and exposerefreshKycStatus+statusChanged+getCustomerIdentity. - Extend shared types/docs/changelog for
money,iron, and simplified/kyc/statususer status.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/kyc-controller/src/types.ts | Adds money product, iron vendor, customer identity type, and simplified user-status types. |
| packages/kyc-controller/src/KycService.ts | Vendor-parameterized endpoints, optional injected fetch, consents + status APIs, improved HttpError detail. |
| packages/kyc-controller/src/KycService.test.ts | Adds coverage for new vendor APIs, 204 handling, error-detail parsing, and optional fetch behavior. |
| packages/kyc-controller/src/KycService-method-action-types.ts | Exposes new service methods via messenger action types. |
| packages/kyc-controller/src/KycController.ts | Implements consents-path vendor flow, vendor-scoped terms acceptance, status polling + events, identity accessor. |
| packages/kyc-controller/src/KycController.test.ts | Adds extensive tests for iron flow, vendor-scoped terms behavior, status polling, and identity behavior. |
| packages/kyc-controller/src/KycController-method-action-types.ts | Exposes new controller methods via messenger action types. |
| packages/kyc-controller/src/index.ts | Exports new public types and action types. |
| packages/kyc-controller/CHANGELOG.md | Documents new vendor-generalization, iron flow, and status polling additions. |
| packages/kyc-controller/ARCHITECTURE.md | Updates architecture docs to reflect vendor parameterization and consents-path flow. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
- Fix cross-vendor terms reuse by invalidating null termsAcceptedVendor (Bugbot #1) - Add validation for MoonPay checkKycRequired params (accessToken, country required) - Check fetch availability before binding in KycService constructor - Reorder bearer token check before assert() for better error messages - Add T&C2 flag persistence (sumsubTncAccepted, idosTncAccepted) for consents-path resume - Standardize vendor parameter naming (vendorId → vendor in createUkycSession) - Add comprehensive test coverage for new validation paths Co-authored-by: George Weiler <georgejweiler@gmail.com>
Co-authored-by: George Weiler <georgejweiler@gmail.com>
Do not default omitted sumsubTncSigned/idosTncSigned to true when starting a non-MoonPay session. MoonPay callers may still omit them. Co-authored-by: Cursor <cursoragent@cursor.com>
Make sumsubTncSigned and idosTncSigned required for every vendor. Zero-argument calls and omitted flags fail instead of defaulting to true. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@metamaskbot publish-previews |
| // ids to the wrong vendor. | ||
| if (!this.#hasTermsForVendor(vendor)) { | ||
| this.#clearAcceptedTerms(state); | ||
| } |
There was a problem hiding this comment.
Terms wiped before vendor switch succeeds
Medium Severity
initialize and createVendorCustomer clear persisted termsAcceptedAt / acceptedDisclaimerIds as soon as the requested vendor differs, before KycService:createVendorCustomer succeeds. A transient Iron customer-creation failure (or a reset during that request) permanently drops the other vendor's stored acceptance, so ramps/card users must re-accept terms after a failed Money start.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 5cf810c. Configure here.
There was a problem hiding this comment.
probably shouldn't permanently drop
| if (params.vendor !== 'moonpay') { | ||
| state.moonpayCustomerId = null; | ||
| } | ||
| }); |
There was a problem hiding this comment.
In-flight frames recapture wrong identity
Medium Severity
createVendorCustomer changes activeVendor and clears moonpayCustomerId without the IN_PROGRESS_PHASES guard used by initialize. A MoonPay Check/Auth complete still writes moonpayCustomerId whenever phase is check/auth, so getCustomerIdentity() can return that MoonPay id labeled as iron after Money starts a customer on a shared controller.
Additional Locations (2)
Reviewed by Cursor Bugbot for commit 5cf810c. Configure here.
|
Preview builds have been published. Learn how to use preview builds in other projects. Expand for full list of packages and versions. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
There are 3 total unresolved issues (including 2 from previous reviews).
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, have a team admin enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit a3cea00. Configure here.
| state.phase = 'done'; | ||
| state.statusMessage = 'KYC submitted.'; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Failed SumSub still completes consents
Medium Severity
The consents path only rewinds when startSumSub returns a string error. If the SDK closes without completion or resolves a non-error result, sumsub.status can be failed while #startConsentsSession still refreshes status and forces phase to done.
Reviewed by Cursor Bugbot for commit a3cea00. Configure here.
Summary
fetchDisclaimers/checkKycRequired/createVendorCustomer) so identity vendors share one client surface instead of Iron-branded public methods (createIronCustomer,fetchIronDisclaimers, etc.).initialize({ vendor: 'iron' })→ empty-shell customer → consents → SumSub, skipping MoonPay Check/Auth), plusrefreshKycStatus/statusChangedandgetCustomerIdentity.mainalternative to stacking on #9852; that PR should not merge as-is.Test plan
yarn workspace @metamask/kyc-controller run test(100% coverage)yarn workspace @metamask/kyc-controller run changelog:validateyarn workspace @metamask/kyc-controller run messenger-action-types:checkvendor: 'moonpay'(nocreateIron*on the public surface)initialize({ vendor: 'iron', product: 'money' })andrefreshKycStatusMade with Cursor