Skip to content
This repository has been archived by the owner on Jun 16, 2022. It is now read-only.

LIVE-784 Sell and Fund flow #2232

Open
wants to merge 16 commits into
base: develop
Choose a base branch
from
Open

LIVE-784 Sell and Fund flow #2232

wants to merge 16 commits into from

Conversation

JunichiSugiura
Copy link
Contributor

@JunichiSugiura JunichiSugiura commented Feb 22, 2022

Type

Feature

Context

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

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://*"
 ]
}
fund-flow.mov

@JunichiSugiura JunichiSugiura requested review from a team as code owners February 22, 2022 15:54
@juan-cortes
Copy link
Contributor

Is this supposed to still be a draft @JunichiSugiura ? If not, you have failing CI (unused imports and whatnot) and should fill all the description so I know what this is about 🙏🏼

@JunichiSugiura JunichiSugiura marked this pull request as draft February 22, 2022 17:46
@JunichiSugiura JunichiSugiura changed the title Sell and Fund flow LIVE-784 Sell and Fund flow Feb 23, 2022
@JunichiSugiura JunichiSugiura marked this pull request as ready for review March 2, 2022 15:20
@chabroA chabroA self-requested a review March 4, 2022 09:37
Copy link

@chabroA chabroA left a comment

Choose a reason for hiding this comment

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

⚠️ Disclaimer ⚠️
I was not able to run and test these changes locally on my computer due to unrelated S1 performance issues / throttling

ios/ledgerlivemobile.xcodeproj/project.pbxproj Outdated Show resolved Hide resolved
src/components/DeviceAction/rendering.js Outdated Show resolved Hide resolved
Comment on lines +120 to +128
const exchangeAction = createAction(completeExchange);
const sendAction = txCreateAction(connectApp);

const styles = StyleSheet.create({
root: {
flex: 1,
padding: 32,
},
});
Copy link

Choose a reason for hiding this comment

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

Couldn't we move the definition of these variables before the declaration of the PlatformCompleteExchange function (l 23) to follow the no-use-before-define rule and therefore reduce confusion?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To me the most important part of the file is its props and functional component and these should be defined at the top most. It's the same reason as defining styles in the bottom. If you think no-use-before-define rule is the best bet, then it should be forced by a linter. Forcing without linter would become unmanageable eventually.

Comment on lines +48 to +55
const action = createAction(connectApp, startExchange);

const styles = StyleSheet.create({
root: {
flex: 1,
padding: 32,
},
});
Copy link

Choose a reason for hiding this comment

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

Couldn't we move the definition of these variables before the declaration of the PlatformStartExchange function (l 18) to follow the no-use-before-define rule and therefore reduce confusion?

@chabroA
Copy link

chabroA commented Mar 4, 2022

@JunichiSugiura FYI I updated the test plan section in the description to reference the platform-app-test-exchange app and add info regarding the test version of the nano app-exchange needed

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants