Improve kyc level 2 ux#677
Conversation
✅ Deploy Preview for pendulum-pay ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
| case 'face match failure': | ||
| return KycFailureReason.FACE; | ||
| case 'name does not match': | ||
| return KycFailureReason.NAME; |
There was a problem hiding this comment.
There may be others, I'm not entirely sure as we don't see much failures. There is one with the birthdate but I believe we agreed at some point not to show too much.
| useEffect(() => { | ||
| if (error) { | ||
| handleError(error.message); | ||
| const threshold = 60000; |
There was a problem hiding this comment.
This effect is for not failing immediately if a connection error happens. But > 60 seconds it will in fact show a network error failure.
We can adjust this threshold.
gianfra-t
changed the title
| assetSymbol: fromToken?.assetSymbol, | ||
| }); | ||
| } | ||
| // if (Big(userInputTokenBalance).lt(inputAmount ?? 0)) { |
There was a problem hiding this comment.
Leaving this for testing. So it's easier to trigger a level 2.
There was a problem hiding this comment.
Okay, you can uncomment it again now.
There was a problem hiding this comment.
Pull Request Overview
This PR improves the KYC level 2 user experience by introducing a new taxId field, updating failure handling, and modifying the KYC validation and UI interactions. Key changes include:
- Adding a new taxId field and corresponding validation logic in the KYC form.
- Updating failure handling and status messaging logic in the KYC process.
- Adjusting API response payloads and mapping of KYC failure reasons.
Reviewed Changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| shared/src/endpoints/brla.endpoints.ts | Added KycFailureReason enum and updated GetKycStatusResponse properties. |
| frontend/src/translations/pt.json | Added taxId translations and KYC failure reason messages in Portuguese. |
| frontend/src/translations/en.json | Added taxId translations and KYC failure reason messages in English. |
| frontend/src/services/signingService.tsx | Updated type usage for KYC status response. |
| frontend/src/hooks/** | Adjusted form schema, state synchronization, and status update logic for KYC process. |
| frontend/src/components/BrlaComponents/** | Updated components to integrate the new taxId field and failure messaging UI. |
| api/src/api/controllers/brla.controller.ts | Updated event threshold and added failure reason mapping for API responses. |
Comments suppressed due to low confidence (1)
api/src/api/controllers/brla.controller.ts:350
- The event threshold was changed from 60000ms to 2000ms; please confirm that this reduction is intentional and review its impact on event freshness criteria.
if (lastInteraction && lastEventCached.createdAt <= lastInteraction - 2000) {
There was a problem hiding this comment.
Great stuff 👌
When testing locally using the BRLA sandbox environment, I did the following:
- Generated a random CPF and changed the last digit to a different one.
- I was able to proceed to the KYC form 1 (totally fine)
- I filled in all the other fields with dummy data and proceeded by clicking on the button.
- I was quickly taken back to KYC form 1 telling me that my CPF was invalid.
Perfect flow, I love it.
I noticed you changed the form field validation to trigger onChange and not onBlur. Is there a reason for this change? I think onBlur is better because otherwise the user would often times see an error already when they try to fill in the field for the first time.
I noticed one bug though. Once I used a 'valid' dummy CPF, the KYC verification for level 1 succeeded, at least the UI told me so, and I clicked on the 'Confirm' button. I was taken back to the base swap page and immediately presented with the ramp summary dialog. All good, so far. Now the proplem is that I never saw a pix key and the button stays at processing because apparently the registration of the ramp failed because the subaccount does not exist. This doesn't really make sense because in the KYC verification, the subaccount must have been created a very long time ago. My assumption would be that for some reason, not an up-to-date subacount or taxID was used, maybe the invalid one I originally entered with the other digit. I only corrected it to a 'correct' CPF on the KYC form 1 and not the base swap page, so maybe it was not correctly applied to the overall state again? It worked fine after I closed and reopened the ramp summary dialog again.
| if (lastInteraction && lastEventCached.createdAt <= lastInteraction - 2000) { | ||
| // If the last event is older than 2 seconds from the last interaction, we assume it's not a new event. |
There was a problem hiding this comment.
Why are we reducing this to 2 seconds?
There was a problem hiding this comment.
This one is tricky. Because of our choice and how both KYC levels use the same endpoint, we need to ensure they don't get mixed, that's the whole idea of this "last interaction" thing.
The delay was initially added because sometimes, there is a small delay between the call and creation of this state. But 60 seconds was too much, and I experienced a bug where a previous event was taken as valid. I agree 2 seconds is rather arbitrary.
| assetSymbol: fromToken?.assetSymbol, | ||
| }); | ||
| } | ||
| // if (Big(userInputTokenBalance).lt(inputAmount ?? 0)) { |
There was a problem hiding this comment.
Okay, you can uncomment it again now.
ebma
left a comment
ebma
left a comment
There was a problem hiding this comment.
I didn't test it again but looks good to me 👍
Co-authored-by: Marcel Ebert <mail@marcel-ebert.de>
Issues: #630 and #673.
Changes
taxIdfield to KYC level 1 form that is connected to the maintaxIdstate. IftaxIdis detected invalid, it displays a message on the field and allows for change.