Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(connect): Device cancelable prompts (pin, word, passphrase) #13925

Merged
merged 4 commits into from
Sep 27, 2024
Merged

fix(connect): Device cancelable prompts (pin, word, passphrase) #13925

merged 4 commits into from
Sep 27, 2024

Conversation

szymonlesisz
Copy link
Contributor

@szymonlesisz szymonlesisz commented Aug 21, 2024 •
edited by mroz22
Loading

Description

Fix unreported (?) behavior: calling Cancel on device when promptPin, promptPassphrase, promptWord is rejected or TrezorConnect.cancel was invoked.

Changes:

  • unify all mentioned prompt functions and moved to standalone module.
  • each prompt calls Cancel when rejected
  • each prompt assigns cancelableRequest to the Device
  • when prompt is resolved/rejected cancelableRequest is cleared from the Device
  • when TrezorConnect.cancel is called cancelableRequest is invoked (call Cancel then reject prompt promise)

Lets test and discuss :)

resolve #14507 (maybe it won't resolve everything, I just want to trigger QA :P)

mroz22
mroz22 reviewed Aug 26, 2024
View reviewed changes
packages/connect/e2e/tests/device/cancel.test.ts Outdated
const getAddressCall = getAddress(true);
await new Promise(resolve => {
TrezorConnect.on('UI_EVENT', event => {
if (event.type === 'ui-request_passphrase') {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • add ui-response with nullable value (effectivelly cancels)

mroz22
mroz22 reviewed Aug 26, 2024
View reviewed changes
packages/connect/e2e/tests/device/cancel.test.ts Outdated
});

test('Synchronous Cancel', async () => {
await setupTest({
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

maybe automatic button rensposes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is a deeper rabbit hole, briefly there are 2 problems:

  • core methodSynchronize is async (loading method) and it is resolved in the next tick, before method is assigned there is nothing to cancel
  • even if here wouldnt be race condition mentioned above there is no possibility to stop current "acquire" process from the outside, therefore it continues, device gets acquired and workflow continues

since it's not related to my current changes i'm leavaing this for later with todo's and comments

@szymonlesisz szymonlesisz force-pushed the fix/connect-cancel-prompt branch 2 times, most recently from 21d6067 to 71b9973 Compare September 5, 2024 18:25
szymonlesisz
szymonlesisz commented Sep 5, 2024
View reviewed changes
packages/connect/src/device/DeviceCommands.ts Outdated Show resolved Hide resolved
packages/connect/src/device/DeviceCommands.ts Outdated Show resolved Hide resolved
packages/connect/src/device/Device.ts Outdated Show resolved Hide resolved
@szymonlesisz szymonlesisz added the build-desktop This will trigger the build of desktop apps for your PR label Sep 10, 2024
This was referenced Sep 10, 2024
Closed
Closed
@szymonlesisz szymonlesisz force-pushed the fix/connect-cancel-prompt branch 2 times, most recently from d3f4805 to 5410bfe Compare September 16, 2024 18:52
@mroz22 mroz22 mentioned this pull request Sep 17, 2024
Closed
@szymonlesisz szymonlesisz force-pushed the fix/connect-cancel-prompt branch 6 times, most recently from 73e4030 to f2827d7 Compare September 19, 2024 19:43
@szymonlesisz szymonlesisz removed the build-desktop This will trigger the build of desktop apps for your PR label Sep 19, 2024
@szymonlesisz szymonlesisz marked this pull request as ready for review September 20, 2024 07:19
@szymonlesisz
Copy link
Contributor Author

/rebase

@github-actions GitHub Actions
Copy link

Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/10992660973

@mroz22 mroz22 mentioned this pull request Sep 24, 2024
Closed
mroz22
mroz22 approved these changes Sep 24, 2024
View reviewed changes
Copy link
Contributor

@mroz22 mroz22 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good and should be fixing stuff. Also it doesn't seem to be conflicting with @marekrjpolak's views of life (or at least I hope it doesn't).

I created #14507 to let QA know what should be tested. I'd say we may merge it now so that it receives proper testing long enough before the freeze that is scheduled in a week or so.

@mroz22
Copy link
Contributor

mroz22 commented Sep 24, 2024

/rebase

@github-actions GitHub Actions
Copy link

Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/11015634972

@mroz22
Copy link
Contributor

mroz22 commented Sep 24, 2024

It seems to fail for some popup tests. I will investigate it tomorrow

@marekrjpolak
Copy link
Contributor

@mroz22 some popup test was failing even before in one of my PR. We've briefly discussed it with @martykan and came to conclusion that the test is probably somehow faulty, dunno.

@mroz22
Copy link
Contributor

mroz22 commented Sep 25, 2024 •
edited
Loading

There is indeed one difference in behaviour caught by tests

develop

image image

this branch

it doesn't kill the method upon disconnection of the device, it lets the permissions screen active
image

and when I approve it, only after that I would see this error screen

image image

@szymonlesisz
Copy link
Contributor Author

/rebase

@github-actions GitHub Actions
Copy link

Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/11036794841

szymonlesisz and others added 4 commits September 26, 2024 16:59
@szymonlesisz
fix(connect): Device prompts cancel action (pin, word, passphrase)
8322a5c
@szymonlesisz
chore(connect): use cancelableAction instead of _cancelableRequestBySend
930d7de
@mroz22 @szymonlesisz
test(connect): add tests for TrezorConnect.cancel
f420334
@szymonlesisz
test(connect): fix karma setup
07438f6 
- parametrize karma singleRun
- remove unused (mocha, browserConsoleLogOptions) from karma config
- karma fallback for `it.skip` and `it.only`
- fix import in fixtures
- fix filter and accept `testName.test` pattern
szymonlesisz
szymonlesisz commented Sep 26, 2024
View reviewed changes
packages/connect/src/device/Device.ts
delete this.runPromise;
this.transportSession = null; // set to null to prevent transport.release and cancelableAction

return this.interruptionFromUser(ERRORS.TypedError('Device_Disconnected'));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this was the reason of failing tests

interruptionFromUser was not awaited and in the same time runPromise was deleted therefore if couldn't be rejected by interruptionFromUser

@mroz22 mroz22 merged commit fb888be into develop Sep 27, 2024
87 checks passed
@mroz22 mroz22 deleted the fix/connect-cancel-prompt branch September 27, 2024 06:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mroz22 mroz22 mroz22 approved these changes

@martykan martykan Awaiting requested review from martykan martykan is a code owner

@Nodonisko Nodonisko Awaiting requested review from Nodonisko Nodonisko is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cancel from host - meta issue
3 participants
@szymonlesisz @mroz22 @marekrjpolak