feat(connect): dispatch call in progress event - needed for immediate… #14717
Conversation
mroz22
force-pushed
the
connect-call-progress-event
branch
3 times, most recently
from
a2cb343 to
8da4a49
Compare
| @@ -45,7 +46,9 @@ export const connectInitThunk = createThunk( | |||
| }); | |||
|
|
|||
| TrezorConnect.on(UI_EVENT, ({ event: _, ...action }) => { | |||
| // dispatch event as action | |||
| if (action.type === 'ui-call_in_progress') { | |||
There was a problem hiding this comment.
Hmmm maybe it should be DEVICE_EVENT 🤔
There was a problem hiding this comment.
I agree that DEVICE_EVENT sounds a bit better.
There was a problem hiding this comment.
Also, I thought it's more obvious not to edit Connect events here in connectInitThunks but pass them directly as redux actions whenever possible. So I tried to catch this event in Suite middleware and transform it to SUITE.LOCK_DEVICE there, but then it probably wouldn't work in native, because they don't use the same middlewares? So I'm not sure about that now.
There was a problem hiding this comment.
Sure, middleware sounds like a place to put it. I just don't like to use middleware if there is only a single use of something. In that case, it feels more readable without it.
🤔 but there are also ui-* device-related events
'ui-device_needs_backup'
'ui-request_passphrase_on_device'
'ui-request_pin'
There was a problem hiding this comment.
I just don't like to use middleware if there is only a single use of something
Yes, I agree, that's why my first idea was to directly replace SUITE.LOCK_DEVICE with UI.CALL_IN_PROGRESS everywhere. But that would be a bigger change.
|
not finished, to be discussed, I'd like to nominate @marekrjpolak 🎖️ |
packages/connect/src/core/index.ts
Outdated
| sendCoreMessage(createUiMessage(UI.CALL_IN_PROGRESS, { value: true })); | ||
|
|
||
| try { | ||
| return await onCallDevice(context, message, method); | ||
| } finally { | ||
| sendCoreMessage(createUiMessage(UI.CALL_IN_PROGRESS, { value: false })); | ||
| } |
There was a problem hiding this comment.
I believe that the lock should be removed at all cases, even when the method throws or returns prematurely. So I proposed a fixup.
| if (selectIsDeviceLocked(getState())) { | ||
| return await synchronize(() => original(params)); | ||
| } |
There was a problem hiding this comment.
I'm afraid this won't work as expected. When the first locking call is executed, there's no device lock yet so it would be out of synchronize() and the next call in synchronize() will run in parallel.
There should be something like when locked is true, wait until locked is false and go? But when you accidentally start two methods at once, you're still fucked.
Maybe, if we want the lock to be unpenetrable, it has to be defined at the same place as consumed? Meaning that if you want to await the lock from Connect in Suite, it will always be prone to race condition?
There was a problem hiding this comment.
Sounds right. Maybe I can sort of go back to the previous behaviour, but still more robust simply by defining a new utility method on connect, something like
TrezorConnect.getMethods
Which would return method details, in this case I would be interested in all methods that have method.useDevice=true and create a wrapper for them.
There was a problem hiding this comment.
Yeah, that would be an improvement.
And why do we even need these locks on Suite side? If it is impossible to do two calls in parallel because of Connect, shouldn't Connect handle it? Maybe we can add a new general method param, something like allowQueue, which will cause synchronization on Connect's side instead of throwing call in progress?
| @@ -45,7 +46,9 @@ export const connectInitThunk = createThunk( | |||
| }); | |||
|
|
|||
| TrezorConnect.on(UI_EVENT, ({ event: _, ...action }) => { | |||
| // dispatch event as action | |||
| if (action.type === 'ui-call_in_progress') { | |||
There was a problem hiding this comment.
Also, I thought it's more obvious not to edit Connect events here in connectInitThunks but pass them directly as redux actions whenever possible. So I tried to catch this event in Suite middleware and transform it to SUITE.LOCK_DEVICE there, but then it probably wouldn't work in native, because they don't use the same middlewares? So I'm not sure about that now.
mroz22
force-pushed
the
connect-call-progress-event
branch
5 times, most recently
from
2f90c34 to
4a7866d
Compare
… feedback in suite when called from outside
mroz22
force-pushed
the
connect-call-progress-event
branch
from
4a7866d to
f4fa8b0
Compare
"call in progress"eventsuite.locks[device lock]in suite and get rid of the whitelist of wrapped methods. From now on, the lock is set whenever the connect method that is called works with a device and released when this method posts response.cherry-picked from #14690