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.

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: sell and fund flow [LIVE-784] #91

Merged
merged 9 commits into from
Jun 15, 2022
Merged

feat: sell and fund flow [LIVE-784] #91

merged 9 commits into from
Jun 15, 2022

Conversation

Justkant
Copy link
Contributor

@Justkant Justkant commented May 19, 2022

❓ Context

It's the same as LedgerHQ/ledger-live-desktop#4361 but for mobile.

Add the exchange SELL and FUND flow in the context of the live-app-sdk.

✅ Checklist

  • Test coverage: Did you write any tests to cover the changes introduced by this pull request?
  • Atomic delivery: Is this pull request standalone? In order words, does it depend on nothing else?
  • Breaking changes: Does this pull request contain breaking changes of any kind? If so, please explain why.

📸 Demo

fund-flow.mov

🚀 Expectations to reach

Please make sure you follow these Important Steps.

Pull Requests must pass the CI and be internally validated in order to be merged.

Parts of the app affected / Test plan

  1. Start the platform-app-test-exchange locally (yarn dev)
  2. Start Ledger Live Mobile with envs below
  3. Settings -> Developer -> Load Platform Manifest
  4. Paste manifest below
  5. Test the SELL and FUND flows (only works with BTC as of today)

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.

.env
DISABLE_TRANSACTION_BROADCAST=1
MOCK_EXCHANGE_TEST_CONFIG=1
manifest.json
{
 "id": "test-app",
 "name": "Test app",
 "url": "http://localhost:3000",
 "homepageUrl": "https://developers.ledger.com/",
 "icon": "",
 "platform": "all",
 "apiVersion": "0.0.1",
 "manifestVersion": "1",
 "branch": "debug",
 "categories": ["tools"],
 "currencies": "*",
 "content": {
   "shortDescription": {
     "en": "Test Live App. Use at your own risk."
   },
   "description": {
     "en": "Test Live App. Use at your own risk."
   }
 },
 "permissions": [
   {
     "method": "*"
   }
 ],
 "domains": [
   "https://*",
   "http://*"
 ]
}

handle exchange action

add new tracking events

implement startExchange

[WIP] fix completeExchange error

fix crash on completeExchange

fix crash on completeExchange

use same device for start and complete exchange action

cleanup

show device animation on exchange flow

fix fund device action modal

fix fund action message

remove unnecessary changes

add comments for exchangeType
@Justkant Justkant requested review from IAmMorrow, LFBarreto and a user May 19, 2022 09:41
@Justkant Justkant self-assigned this May 19, 2022
@vercel
Copy link

vercel bot commented May 19, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
live-common-tools ✅ Ready (Inspect) Visit Preview Jun 7, 2022 at 9:41AM (UTC)
3 Ignored Deployments
Name Status Preview Updated
ledger-live-github-bot ⬜️ Ignored (Inspect) Jun 7, 2022 at 9:41AM (UTC)
native-ui-storybook ⬜️ Ignored (Inspect) Jun 7, 2022 at 9:41AM (UTC)
react-ui-storybook ⬜️ Ignored (Inspect) Jun 7, 2022 at 9:41AM (UTC)

@changeset-bot
Copy link

changeset-bot bot commented May 19, 2022

🦋 Changeset detected

Latest commit: bf12e0f

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 2 packages
Name Type
live-mobile Minor
@ledgerhq/live-common Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-actions github-actions bot added mobile Has changes in LLM translations Translation files have been touched labels May 19, 2022
@github-actions
Copy link

github-actions bot commented May 19, 2022

@Justkant

Screenshots: ❌

It seems this PR contains screenshots that are different from the base branch.
If you are sure all those changes are correct, you can comment on this PR with /generate-screenshots to update those screenshots.

Make sure all the changes are correct before running the command, as it will commit and push the new result to the PR.

windows

Actual Diff Expected
market-btc-buy-page-actual market-btc-buy-page-diff market-btc-buy-page-expected
market-btc-buy-page-actual market-btc-buy-page-diff market-btc-buy-page-expected

using slice fixes the issue (needs more digging to understand the root cause)
@codecov
Copy link

codecov bot commented May 24, 2022

Codecov Report

Merging #91 (bf12e0f) into develop (01b136e) will increase coverage by 56.95%.
The diff coverage is 0.00%.

@@             Coverage Diff              @@
##           develop      #91       +/-   ##
============================================
+ Coverage         0   56.95%   +56.95%     
============================================
  Files            0      516      +516     
  Lines            0    21468    +21468     
  Branches         0     5748     +5748     
============================================
+ Hits             0    12228    +12228     
- Misses           0     9194     +9194     
- Partials         0       46       +46     
Flag Coverage Δ
test 56.95% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ve-common/src/exchange/hw-app-exchange/Exchange.ts 25.97% <0.00%> (ø)
...ger-live-common/src/families/algorand/api/index.ts 100.00% <0.00%> (ø)
...mmon/src/families/bitcoin/js-prepareTransaction.ts 29.41% <0.00%> (ø)
...dger-live-common/src/families/cosmos/api/Cosmos.ts 85.41% <0.00%> (ø)
...src/families/solana/api/chain/validators/bignum.ts 50.00% <0.00%> (ø)
...rc/families/tezos/datasets/tezos.scanAccounts.1.ts 100.00% <0.00%> (ø)
...-live-common/src/families/polkadot/test-dataset.ts 60.00% <0.00%> (ø)
libs/ledger-live-common/src/portfolio/v2/range.ts 100.00% <0.00%> (ø)
...ger-live-common/src/migrations/accounts-dataset.ts 100.00% <0.00%> (ø)
...er-live-common/src/families/tezos/signOperation.ts 15.25% <0.00%> (ø)
... and 507 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01b136e...bf12e0f. Read the comment docs.

@ggilchrist-ledger
Copy link
Contributor

Successful Fund on Android (physical) with Nano X

Screen.Recording.2022-06-10.at.13.28.28.mov

@ggilchrist-ledger
Copy link
Contributor

Unsuccessful fund on iOS when it freezes on the 'complete' phase'

Screen.Recording.2022-06-10.at.13.43.18.mov

Successful iOS fund on Nano X:

Screen.Recording.2022-06-10.at.13.46.52.mov

@ggilchrist-ledger
Copy link
Contributor

I was also able to do some funds on both ETH and BTC on LLM on Baanx, however the device interactions were very flaky and on Android the BTC 'confirm exchange' step would not show up on the Nano. See photos of where it stopped:
image

image

image

My recommendation at this stage is we could merge this as the basic flows have been proved to be working (successful funds/sells on the exchange test app, and ETH and BTC funds on Baanx), however the sell is very flaky and requires a lot more testing on a stable environment. If we merge this (as long as it is not usable by users) then we will be able to test this on actual iOS/Android devices with proper builds and without relying on dev builds, http proxies and debug nano apps, which make it tricky to tell if any issues are with the app or with our test setup.

Once this is merged we should test the following on physical iPhones and Android phones:

  • Sell (this will still require the debug exchange app. This needs to be made available for Nano S+ and the crash bug for Nano S 2.0.11-debug needs to be fixed). This only needs to be verified with BTC.
    • Tested on Segwit, Legacy and Native Segwit
    • Tested on Nano S, Nano S+ and Nano X
  • Fund - tested on Baanx
    • Tested on Segwit, Legacy and Native Segwit
    • Tested on Nano S, Nano S+ and Nano X

@desirendr desirendr added release candidate for next release feature ready labels Jun 14, 2022
@Justkant Justkant mentioned this pull request Jun 14, 2022
3 tasks
@valpinkman valpinkman added this to the Ledger Live Mobile 3.3.x milestone Jun 14, 2022
@Justkant Justkant changed the base branch from develop to release June 15, 2022 07:49
@Justkant Justkant merged commit 25b9e29 into release Jun 15, 2022
@Justkant Justkant deleted the feat/LL-8098 branch June 15, 2022 07:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
common Has changes in live-common feature ready mobile Has changes in LLM release candidate for next release translations Translation files have been touched
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants