-
Notifications
You must be signed in to change notification settings - Fork 279
Jon/infinite v2 #5892
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: develop
Are you sure you want to change the base?
Jon/infinite v2 #5892
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the original commit message:
fix(infinite): redirect to KYC webview for PENDING customers
When a customer with PENDING KYC status already exists, skip the KYC
form and redirect directly to the KYC webview. The form is now only
shown for new customers without a customerId.
Extract openKycWebView helper to reuse webview logic in both the
PENDING status handling and new customer form submission flows.
It explained "why" this change.
de05d4d to
6957fef
Compare
6957fef to
638dca7
Compare
56f737d to
cf4e4fc
Compare
9641ee1 to
8aa54bb
Compare
| // Try to parse as HTTP error response | ||
| const httpErrorResponse = asMaybe(asInfiniteHttpErrorResponse)(data) | ||
| if (httpErrorResponse != null) { | ||
| // OTP verification errors should be localized for the modal: | ||
| if ( | ||
| httpErrorResponse.statusCode === 400 && | ||
| urlStr.includes('/v1/headless/customers/verify-otp') | ||
| ) { | ||
| throw new InfiniteApiError( | ||
| 400, | ||
| lstrings.ramp_kyc_error_title, | ||
| lstrings.ramp_otp_invalid_code | ||
| ) | ||
| } | ||
|
|
||
| const detail = Array.isArray(httpErrorResponse.message) | ||
| ? httpErrorResponse.message.join('; ') | ||
| : httpErrorResponse.message | ||
| throw new InfiniteApiError( | ||
| httpErrorResponse.statusCode, | ||
| httpErrorResponse.error, | ||
| detail | ||
| ) | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realize during this review turn that no localized errors should be implemented in the infiniteApi.
How we've handle localization and plan to handle localization is a the ramp plugin level, not the API library level. For example we translate duplicate_record errors in kycWorkflow.ts:
if ('error' in customerResponse) {
if (
customerResponse.error instanceof InfiniteApiError &&
customerResponse.error.detail.includes('duplicate_record')
) {
throw new I18nError(
lstrings.ramp_signup_failed_title,
lstrings.ramp_signup_failed_account_existsmessage
)
}
throw customerResponse.error
}
This means we should always throw an InfiniteApiError from the HTTP response with no translations. Then we handle and translate errors in the workflows using I18nError.
This design is intentional. Localization strings are a GUI concern and ramp plugins are GUI plugins.
| }) | ||
| ) | ||
|
|
||
| export const asInfiniteHttpErrorResponse = asJSON( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have asInfiniteErrorResponse why is this needed? It only seems needed cause we have a special handling for errors for localization? Well if we don't do localization here, then we can perhaps omit this additional cleaner?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because the type differs from asInfiniteErrorResponse.
These HTTP errors were failing the cleaner previously in fetchInfinite so I was unable to catch it later as a recognized InfiniteApiError error to display the appropriate message. Previously, if fetchInfinite did not recognize the error, it only returns a generic Error with response.status, obfuscating the other required data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah. I see.
So what we want is to extend asInfiniteErrorResponse with the new type information. It'd be a union of of the two shapes. Then we'd treat each shape the same (return a infinite error object).
We still want to move the special handling of the verify-otp error at the workflow level.
486e6dc to
84c34e4
Compare
Update challenge and verify endpoints to use the new headless API paths: - /v1/auth/wallet/challenge → /v1/headless/auth/wallet/challenge - /v1/auth/wallet/verify → /v1/headless/auth/wallet/verify
- Flatten customer request schema: remove nested 'data' wrapper - Remove residentialAddress from request (no longer supported) - Remove kycLinkUrl and usedPersonaKyc from response - New customers now start with PENDING status instead of ACTIVE - Add getKycLink API method for obtaining KYC verification links - Update kycWorkflow to use new getKycLink endpoint
- Change status values: PENDING, IN_REVIEW, ACTIVE, NEED_ACTIONS, REJECTED - Add sessionStatus field to KYC status response - Update kycWorkflow status checks for new format - ACTIVE replaces 'approved' as the completed status - PENDING replaces 'not_started'/'incomplete'
Change bridgeAccountId to externalAccountId in customer accounts response to match new API format that abstracts away the underlying provider.
The TOS (Terms of Service) workflow is no longer part of the new Infinite API: - Delete tosWorkflow.ts - Remove tosWorkflow import and call from infiniteRampPlugin.ts - Remove asInfiniteTosStatus, InfiniteTosStatus, asInfiniteTosResponse types - Remove getTos method from InfiniteApi interface and implementation - Remove ramp_tos_* locale strings from en_US.ts and all JSON files
84c34e4 to
bebf020
Compare
For already-KYC'ed emails
01763af to
f83f2bc
Compare
| maxLength={9} | ||
| error={fieldErrors.routingNumber} | ||
| aroundRem={0.5} | ||
| onBlur={handleRoutingNumberBlur} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Routing number input missing maxLength constraint
Low Severity
The routing number FilledTextInput had both minLength={9} and maxLength={9} removed, but only the maxLength constraint should have been replaced by validation. Without maxLength={9}, users can now type more than 9 characters into the routing number field. The validation catches this on blur, but the previous behavior prevented invalid input entirely. This is a UX regression for a field that requires exactly 9 digits.
| } | ||
| if (personalInfoUuid != null) { | ||
| await vault.updatePersonalInfo(personalInfoUuid, personalInfo) | ||
| } else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ExitError shows unfriendly message when user cancels OTP
Medium Severity
When the user cancels the OTP verification modal, an ExitError with the message "User cancelled OTP verification" is thrown from inside the onSubmit callback. This error is caught by RampKycFormScene's try/catch and displayed to the user via ErrorCard. The result is a non-localized, developer-facing message shown for a deliberate user action. ExitError is intended for graceful workflow exits (handled by handleExitErrorsGracefully), but here it's being caught by the form's error handler instead, causing poor UX.
f83f2bc to
223d9d5
Compare
| } | ||
| if (personalInfoUuid != null) { | ||
| await vault.updatePersonalInfo(personalInfoUuid, personalInfo) | ||
| } else { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
User cancellation shows unexpected error in form
Medium Severity
When the user cancels OTP verification, ExitError is thrown inside the form's onSubmit callback. The RampKycFormScene catches all errors and displays them via ErrorCard. Since ExitError is not an I18nError, it triggers the generic "Unexpected error" message with a "Report Error" button. User cancellation is a normal flow and shouldn't show an error. The ExitError pattern expects errors to bubble up to handleExitErrorsGracefully in the plugin, but form scene error handling intercepts it first.
4d8f4b9 to
f354054
Compare
"Missing state reset blocks OTP retry after error"
"REJECTED status throws uncaught error at initialization" "ExitError in onSubmit prevents promise settlement causing confusing UX"
Field focus doesn't properly move the focused field above the floating next button, so just changing it into `SceneButtons`
We should consider adding more information to this mostly blank scene...
Account and routing number length check failures will disable the next button, and length errors in these fields immediately show in the field on blur.
"Routing number field missing maxLength constraint"
f354054 to
d56b9b8
Compare
Pass countryCode through workflow params instead of hardcoding 'US'. Apply US-specific bank validation only for US customers. TODO: Extend validation for other countries when internationalized.
Fix spacing throughout, remove styled(), reuse existing components, add copy functionality.
Update snapshots for themed icon changes in EdgeRow.
The `ErrorCard` solved the situation of user-unfriendly error messages, but hid useful information for quickly debugging from QA and customers without having to deal with logs. Expose a way to show the error if dev mode is enabled as a compromise.
e61f30c to
a069acd
Compare
| city: contactInfo.city, | ||
| state: contactInfo.state, | ||
| postalCode: contactInfo.postalCode, | ||
| country: 'US' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoded address country inconsistent with countryCode parameter
Low Severity
The address.country field is hardcoded to 'US' while the top-level countryCode parameter uses the dynamically passed value from request.regionCode.countryCode. This creates an inconsistency in the createCustomer API request where the top-level countryCode could differ from the nested address.country. If the ramp is extended to support other countries, this would cause the API request to have mismatched country values. The address.country should use the countryCode parameter instead of the hardcoded 'US' string.
CHANGELOG
Does this branch warrant an entry to the CHANGELOG?
Dependencies
noneRequirements
If you have made any visual changes to the GUI. Make sure you have:
Note
Modernizes the Infinite ramp integration and related UI flows.
plugins/ramps/infinite(challenge/verify, quotes/transfers, KYC status/link, accounts); improves error parsing; removes legacy TOS workflowOtpVerificationModaland integrates OTP verification into KYC; updates KYC flow to usegetKycLinkand new status mapping; stores customer info; handles REJECTED withI18nErrorRampBankFormScenegainscountryCode, inline validation and errors;RampBankRoutingDetailsSceneredesigned with copy actions and sectioned rows;RampConfirmationScenelayout polishThemedIcons(Edit/Delete/Question),EdgeRowicon refactor,ErrorCarddev-mode "Show Error" buttonWritten by Cursor Bugbot for commit a069acd. This will update automatically on new commits. Configure here.