-
Notifications
You must be signed in to change notification settings - Fork 302
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
Feat/LIVE-12193 track swap cancel accept #6799
Conversation
The latest updates on your projects. Learn more about Vercel for Git βοΈ 5 Ignored Deployments
|
5e551b4
to
2e26f0d
Compare
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.
Nice, looks good to me. Let's wait for the wallet api team to take a look before merging πͺ
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.
I think we can move the logic from LLD and LLM to LLC and remove the uiHook for device as it doesn't really need ui in your context
apps/ledger-live-desktop/src/renderer/components/WebPTXPlayer/CustomHandlers.ts
Outdated
Show resolved
Hide resolved
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.
I think we have the main logic good here just on desktop I'd like to modify it a bit to make sure we have a device in the end
@@ -57,10 +59,10 @@ export function usePTXCustomHandlers(manifest: WebviewProps["manifest"]) { | |||
...exchangeParams, | |||
exchangeType: ExchangeType[exchangeParams.exchangeType], | |||
onResult: (nonce: string) => { | |||
onSuccess(nonce); | |||
onSuccess(nonce, device); |
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.
Not sure how much it keeps the device in sync here because it will be hoisted when the custom.exchange.start
is called it could be the old value
It would probably be better if onResult
and onCancel
could return the device as it's done for LLM
I checked the code and it seems pretty straightforward to add some data:
- Inputs: https://github.com/LedgerHQ/ledger-live/blob/develop/libs/ledger-live-common/src/exchange/platform/startExchange.ts#L16
- Success: https://github.com/LedgerHQ/ledger-live/blob/develop/libs/ledger-live-common/src/exchange/platform/startExchange.ts#L49-L52
- Error: https://github.com/LedgerHQ/ledger-live/blob/develop/libs/ledger-live-common/src/exchange/platform/startExchange.ts#L62-L65
- StartExchange inputs: https://github.com/LedgerHQ/ledger-live/blob/develop/libs/ledger-live-common/src/hw/actions/startExchange.ts#L150-L155
- startExchange mapResult: https://github.com/LedgerHQ/ledger-live/blob/develop/libs/ledger-live-common/src/hw/actions/startExchange.ts#L37-L49
Another solution could be to simply use const device = useSelector(getCurrentDevice);
in apps/ledger-live-desktop/src/renderer/components/LiveAppDrawer.tsx
because if we do the changes above we might want to update LLM so result.device comes from the action and not a state.
You can also cleanup this types that are not needed anymore
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.
I feel this is a very deep understanding of the code and product to produce such detailed comment. Do you mind sharing any piece of documentation for future use cases? Thank you, @Justkant! ππ½
P.S. I'll update accordingly
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.
Hey not sure we have a lot of documentation on the device actions and exchange part to be honest
I'm pretty familiar with this because I worked on some issues around it
My main concern here is to avoid having to much differences between LLD and LLM and sharing as much as possible, especially because you have a lot of ways to pass the data in the end correctly
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.
Updated
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.
LGTM on the device experience team's scope π
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.
LGTM just some typings cleanup and we are good, good job
apps/ledger-live-desktop/src/renderer/components/LiveAppDrawer.tsx
Outdated
Show resolved
Hide resolved
apps/ledger-live-desktop/src/renderer/components/LiveAppDrawer.tsx
Outdated
Show resolved
Hide resolved
apps/ledger-live-mobile/src/components/Web3AppWebview/PlatformAPIWebview.tsx
Outdated
Show resolved
Hide resolved
apps/ledger-live-mobile/src/components/Web3AppWebview/helpers.ts
Outdated
Show resolved
Hide resolved
onCancel( | ||
result.startExchangeError.error, | ||
result.startExchangeError.device || device, | ||
); | ||
} | ||
|
||
if (result.startExchangeResult) { | ||
setDevice(result.device); |
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'll have to look at this result.device
later on to see how we can remove this and instead use result.startExchangeResult.device
11c839e
β Checklist
npx changeset
was attached.π Description
Updates
custom.exchange.start
custom handler to return the device too.β Context
π§ Checklist for the PR Reviewers