-
Notifications
You must be signed in to change notification settings - Fork 90
fix(keycard): Improve exception handling #19546
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: feat/keycard-nfc
Are you sure you want to change the base?
Conversation
✔️ status-desktop/prs/android/arm64/package/PR-19546#1 🔹 ~9 min 51 sec 🔹 00978e3e 🔹 📦 android/arm64 package |
✔️ status-desktop/prs/linux/x86_64/tests-ui/PR-19546#1 🔹 ~13 min 🔹 3eacf69 🔹 📦 tests/ui package |
✔️ status-desktop/e2e/prspr19546 🔹 ~14 min 🔹 3eacf69 🔹 📦 tests/e2e package |
3eacf69 to
1bb9a1e
Compare
✔️ status-desktop/prs/linux/x86_64/tests-ui/PR-19546#2 🔹 ~15 min 🔹 1bb9a1e 🔹 📦 tests/ui package |
1bb9a1e to
e9d79a4
Compare
|
|
||
| method onUserAuthenticated*(self: Module, password: string, pin: string) = | ||
| if password.len == 0: | ||
| if password.len == 0 and pin.len == 0: |
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.
A password should be set in both cases, regular/keycard user, while pin only for the keycard user, that's why only password was checked. It's not incorrect this way ofc.
| if keycardFlowType == ResponseTypeValueKeycardFlowResult: | ||
| if keycardEvent.error.len > 0: | ||
| if keycardEvent.error == ErrorOk: | ||
| if keycardEvent.error == ErrorOk or keycardEvent.error.len == 0: |
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're here only if the keycardEvent.error.len > 0 parent condition, so not needed toc check if keycardEvent.error.len == 0 here, cause will always be false. The same in 4 conditions below.
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.
removed these changes. Thanks!
- src/app/modules/main/wallet_section/send_new/module.nim Verify if both password and pin are empty before emitting the `authenticationCancelled` signal - handle card disconnect/reconnect when the user input is needed (enter pin, puk etc) - treat keycard message with empty error as successful - src/app_service/service/keycard/service.nim - avoid modifying the json container with itself. We're currently iterating the container and modifying the service member - leading to a freeze.
e9d79a4 to
58d65b8
Compare
✔️ status-app/prs/linux/x86_64/tests-nim/PR-19546#4 🔹 ~8 min 29 sec 🔹 58d65b8 🔹 📦 tests/nim package |
✔️ status-app/prs/linux/x86_64/tests-ui/PR-19546#4 🔹 ~18 min 🔹 58d65b8 🔹 📦 tests/ui package |
What does the PR do
Iterates #19545
Preparing the nim code to work with
status-keycard-qt. Had a few cases where the current nim code had to be updated. Probably because thestatus-keycard-qtinternals are not identical tostatus-keycard-go.src/app/modules/main/wallet_section/send_new/module.nimVerify if both password and pin are empty before emitting theauthenticationCancelledsignalsrc/app/modules/shared_modules/keycard_popup/internal/insert_keycard_state.nim(and the other changes in states) handle card disconnect/reconnect when the user input is needed (enter pin, puk etc)src/app_service/service/keycard/service.nim- avoid modifying the json container with itself. We're currently iterating the container and modifying the service member - leading to a freeze.NOTE: Tests can be done in the final PR #19549