Changing the PIN of the Keycard #22008
Conversation
Jenkins BuildsClick to see older builds (44)
|
| (rf/reg-event-fx :keycard/connect | ||
| (rf/reg-event-fx :keycard/update-event-vector-and-dispatch | ||
| (fn [{:keys [db]} [{:keys [key-uid on-success on-error on-connect-event-vector]}]] | ||
| (let [event-vector | ||
| (or on-connect-event-vector |
There was a problem hiding this comment.
As mentioned in this comment, we store the on-connect-event-vector at the start of the flow when calling :keycard/connect, but we don’t update this event vector. As a result, when the card is reconnected, we dispatch the stored event instead of the event corresponding to the current flow position. To address this, I created a separate event for updating and dispatching the current keycard event, and used for the change-pin flow.
There was a problem hiding this comment.
sorry @Parveshdhull i somehow missed this, could you please explain why is this needed this for the change-pin flow. thanks
There was a problem hiding this comment.
Hi Andrey,
When we call :keycard/connect, we store the corresponding event in the on-card-connected-event-vector in the database. Later, if the card gets disconnected and reconnected, we use the stored value and dispatch it again. This is problematic, as I mentioned in #21918 and in our Discord DM, because we only create the on-card-connected-event-vector at the start of the flow and don't update it afterward.
For example, in the change-pin flow:
-
On
:keycard/connect, we store theverify-pin-and-continueevent and dispatch it, which is then used to open the changing-pin modal (although this is no longer the case). Afterward, we dispatchchange-pin-and-continue. -
If the card gets reconnected before
change-pin-and-continuis dispatched/completed, it will re-trigger the storedverify-pin-and-continueevent and open the changing-pin modal again (no longer the case, will just redispatchchange-pin-and-continueagain but using modal example to highlight the issue) -
A similar issue occurred in "keycard is not empty" screen shown if keycard moved during create profile flow with keycard migration #21818
We need to account for scenarios where the keycard is moved or reconnected and ensure that we update the on-card-connected-event-vector with the latest position in the keycard flow, instead of dispatching the original event again.
Please Let me know if my explanation is unclear, and please search for on-card-connected-event-vector in our codebase; its lifecycle might be more helpful.
There was a problem hiding this comment.
When we call
:keycard/connect, we store the corresponding event in theon-card-connected-event-vectorin the database. Later, if the card gets disconnected and reconnected, we use the stored value and dispatch it again.
yes that's expected, and that's how it should work, if you need to update on-card-connected-event-vector something wrong with the flow then, and flow should be fixed instead, if on-card-connected-event-vector will be changed in the middle of connection flow it will be really hard to develop and maintain
There was a problem hiding this comment.
btw i don't see in the change-pin flow there on-card-connected-event-vector update would be needed
There was a problem hiding this comment.
Thank you @flexsurfer for the discussion and your help.
Review Note: Reverted related change, these will be further discussed in #21971
Parveshdhull
force-pushed
the
feat/keycard-change-ping
branch
2 times, most recently
from
e098ec3 to
b28a7d2
Compare
Parveshdhull
force-pushed
the
feat/keycard-change-ping
branch
from
9b6a9a7 to
a27e688
Compare
There was a problem hiding this comment.
hey @Parveshdhull thank you for PR, great job, i left a few comments, major conern about modal logic changes, i would revert this changes, im not sure why we need to have two modals. Also you can ask Alisher to remove Changing Keycard PIN from figma, because this screen is not needed actually
Parveshdhull
force-pushed
the
feat/keycard-change-ping
branch
from
a27e688 to
d793970
Compare
|
Thank you, @flexsurfer, for reviewing the PR.
|
Parveshdhull
force-pushed
the
feat/keycard-change-ping
branch
2 times, most recently
from
2364278 to
6e230f4
Compare
| :type :modal}))) | ||
| options/sheet-options))}}]}}) | ||
| (state/navigation-state-push {:id component | ||
| :type :modal}))))) |
There was a problem hiding this comment.
yes,
Removed navigation state management code, except for one small instance, which I believe will help avoid potential bugs in the future (only push the modal to the state when opening it).
64% of end-end tests have passedFailed tests (4)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestWalletOneDevice:
Passed tests (7)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletMultipleDevice:
Class TestWalletOneDevice:
|
Parveshdhull
force-pushed
the
feat/keycard-change-ping
branch
from
5e17cc1 to
3d468b6
Compare
|
Thank you very much, @mariia-skrypnyk, for taking the PR for testing. Apologies for adding another commit. It was related to a small latest review change request and shouldn't have affected the PR. I will also test it manually to ensure everything is fine |
Parveshdhull
force-pushed
the
feat/keycard-change-ping
branch
from
3d468b6 to
495967e
Compare
There was a problem hiding this comment.
Thank you for working on this PR @Parveshdhull. I have a few concerns and suggestions, but hopefully nothing major.
| :on-press cleanup-change-pin-data}] | ||
| [quo/page-top | ||
| {:title (i18n/label :t/ready-to-change-pin)}] | ||
| [rn/view {:style {:flex 1 :align-items :center :justify-content :center}} |
There was a problem hiding this comment.
This line is being duplicated too much. As of this PR, I see it duplicated 12 times across keycard namespaces. We can simply create a style namespace at contexts/keycard/ for example and reuse. Or even simpler have a common component using this style map and reuse it across keycard views.
Not a blocker for this PR, but I think we can do better in a subsequent PR.
| @@ -335,6 +335,9 @@ | |||
| "change-password-old-password-placeholder": "Enter current password", | |||
| "change-password-repeat-password-placeholder": "Repeat new password", | |||
| "change-pin": "Change 6-digit passcode", | |||
| "change-pin-keycard": "Change PIN", | |||
| "change-pin-keycard-description": "Use different PIN for this Keycard", | |||
| "change-pin-keycard-message": "It’s very important that you will not forget new PIN. Status is unable to restore PIN. If you forget it, Keycard will be blocked and you will need to restore it via recovery phrase or PUK.", | |||
There was a problem hiding this comment.
Although I'm not a native speaker and I make plenty of mistakes when I write, I see a few small English issues in this sentence. I prefer if we nail the text straight from the very first PR doing the implementation (also to avoid rework).
Personally, I immediately raise a red flag when I use apps with certain grammatical mistakes. I've seen other translations with less than ideal English text in the Keycard screens or which could feel more natural (separate problem).
We can further discuss this with @benjthayer outside this PR. Not a blocker for this PR @Parveshdhull, but something we should improve across all features we work on. cc @xAlisher
|
Thank you, @ilmotta, for reviewing the PR. I have made the required changes. |
|
Hi @Parveshdhull ! Can you mention what design version did you use during development? |
Hi @mariia-skrypnyk, the issue #21951 refers to the design being used. I've also updated the PR description with the link. Thank you! |
|
Please, take a look at the issue below. Issue 1: PIN of another keycard can be changed during the flowPrecondition: Keycard 1 - contains key pair of Profile 1 Steps:
Actual result: Keycard 2 PIN has changed and user can successfully use it during login https://www.figma.com/design/YGm3igIOAcwMqUVJWCJ6f1/Keycard?node-id=3796-43009&m=dev 
Question 1Is there any reason that User Name didn't included into your implementation? 
https://www.figma.com/design/YGm3igIOAcwMqUVJWCJ6f1/Keycard?node-id=3770-54000&m=dev |
|
Thank you, @mariia-skrypnyk, for finding this issue. It should be fixed now. Note: It will now display "Keycard is not empty" (known issue - #21647).
I’m not sure if this information or feature is unavailable, but since I didn’t see it in the other keycard flows, I skipped it here as well. @flexsurfer, could you please clarify whether we should retrieve this name from the keycard or use the profile name? |
we need to discuss this with Alisher, currently we show profile's name, but it doesn't make any sense, i would remove this, because we don't have name for the card |
|
@Parveshdhull checking your fix, thanks!
Would be great, tag me please in that conversation. |
|
Thanks for fix, still testing but found extra one. ISSUE 2: Second scanning stucks after user scaned not a KeycardPrecondition: Keycard 1 - contains key pair of Profile 1 Steps:
Result: scanning stucks, after retry user sees Oops screen again IMG_5925.MP4Expected result: X or Try again should give an ability to user to continue flow successfully |
Thank you, should be fixed now |
Parveshdhull
force-pushed
the
feat/keycard-change-ping
branch
from
f6d433f to
2f78ee0
Compare
Issue 3: Enter PIN screen appears if Keycard scanning was interrupted during PIN changeSteps:
Result: Reported issue starts from 0:23 sec IMG_5931.MP4Expected result: scanner should not be closed and Enter PIN screen should not appear |
|
Hey @Parveshdhull! Quick question—do I need to check the biometrics flow? The only way I see it turning on is by creating a Profile, migrating the key pair to a Keycard, and then changing the PIN. However, the result isn’t exactly as shown in Figma, so I’m wondering if this might be out of scope. What do you think? 😊 |
@mariia-skrypnyk @Parveshdhull we deprioritized support for biometrics with keycards in 2.32 and 2.33 to give room for other features. Right now if we migrate a profile which had biometrics enabled, we incorrectly verify via biometrics before the keycard is scanned to log in (logged in #21956). The only meaningful thing a user can do is disable biometrics from settings after migrating profile to keycard to mitigate the bug. |
Thank you @mariia-skrypnyk for finding this interesting issue. Should be fixed now. |
Parveshdhull
force-pushed
the
feat/keycard-change-ping
branch
from
ebb49f6 to
a26be7c
Compare
55% of end-end tests have passedFailed tests (5)Click to expandClass TestWalletOneDevice:
Passed tests (6)Click to expandClass TestWalletMultipleDevice:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
|
|
Hi @Parveshdhull ! Thanks for the final fix. Platforms: iOS & Android Everything worked as expected unless specified otherwise. Let me know if further details are needed! 🚀 Failed E2E are not related |
Parveshdhull
force-pushed
the
feat/keycard-change-ping
branch
from
a26be7c to
091dda9
Compare
fixes #21951
design: https://www.figma.com/design/YGm3igIOAcwMqUVJWCJ6f1/Keycard?version-id=2180813677654249055&node-id=3-89231&p=f&viewport=12277%2C12303%2C0.24&t=on1Bz6OzgOy5dE0v-0
Video
signal-2025-02-02-174544.mp4
Testing Note:
status: ready
Found Issues
PIN of another keycard can be changed during the flow
Second scanning stucks after user scaned not a Keycard
Enter PIN screen appears if Keycard scanning was interrupted during PIN change