-
Notifications
You must be signed in to change notification settings - Fork 301
Conversation
e88de80
to
9d10a1a
Compare
9d10a1a
to
ec3c5c9
Compare
Coverage report
Test suite run success1 tests passing in 1 suite.Report generated by 🧪jest coverage report action from 9cd1bca |
fe73a57
to
fd70a47
Compare
In order to avoid unhandled cancel events
since changes on live-common side have not been merged yet
9e7a61f
to
9cd1bca
Compare
case 0x01: // sell | ||
case 0x02: // fund | ||
return renderSecureTransferDeviceConfirmation({ | ||
exchangeType: exchangeType === 0x01 ? "sell" : "fund", |
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.
Why not in the case statement?
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.
What do you mean exactly?
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.
Just wondering why you have the conditional statement within the 0x02 case
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.
Is it just less lines of code rather than writing the return statement in both the 0x01 and the 0x02 statements?
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.
Yes, it avoids code duplication.
FYI it is not only within the 0x02 case, but also within the 0x01 case (cf. -> https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Statements/switch#methods_for_multi-criteria_case)
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, I was sure I already approved this last week 😕
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 however i suggest you start thinking of moving all of that device action specific towards Live-common
this feels way too logic oriented to be on the "front" part.
especially when you're managing hex codes directly 🙈
* releases/2.38.x: (55 commits) v2.38.2 (LL-MKT): remove 1h graph range and mini graph color while we sort out the data (#4712) LIVE-881 - Fix window not opening for users who have "dusk" theme (#4711) v2.38.1 LL-8791 LL-9146 (Market): release (#4667) v2.38.0 ledger-live-common 21.32.0 (#4696) More padding bottom Add padding bottom Remove developer CTA in platform catalog LL-9044 Change plugin rendering on Manager LL-9023 Remove bold from token-id in nft viewer (#4693) LL-9028 Handle new lastSeenDevice event on DeviceActions (#4659) LL-9176 Add trackers for the NFT feature (#4694) design fix swap: display parent accounts in the target account drawer [LL-8097] sell/fund flow (#4361) LL-7480: swap wording for the exchange drawer (#4377) LL-8982 Add message for NFT 0 qty error (#4682) chore(publish): use the s3 publisher & provider for nightlies (#4685) ...
🦒 Context (issues, jira)
https://ledgerhq.atlassian.net/browse/LL-8097
https://ledgerhq.atlassian.net/browse/LL-8101
https://ledgerhq.atlassian.net/browse/LL-8105
Add the exchange SELL and FUND flow in the context of the live-app-sdk.
It is built on the fondations laid down by this POC PR by @juan-cortes
🔗 LedgerHQ/ledger-live-common#1486
💻 Description / Demo (image or video)
This PR provide the minimal technical requirements to handle exchange-app flow in the SDK context within LLD.
No consideration has been made regarding UI design / modals. As of today the displayed modal is a minimalistic one that could be improved in subsequent iterations.
🖤 Expectations to reach
PR must pass CI, merge develop if conflicts, do not force push. Thanks!
PR must pass CI, merge develop if conflicts, do not force push. Thanks!
🧪 How to test
Sell flow
yarn dev
)Fund flow
Same steps as above but instead of the Coinify live app POC, use this platform-app-test-exchange instead.
PS: You will need a test version of the exchange app to test the fund flow, cf. the platform-app-test-exchange README for details
Here is a reference of the manifest file referenced in previous tests sections
This manifest assumes your local Live App runs on
http://localhost:3000
. If this is not the case, update theurl
field accordingly in your manifestLocal live app manifest (.json file)
☑️ Todo