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.

Sign up for GitHub

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

Jump to bottom

[SUPPORT] remove unnecessary check on tx networkInfo #4020

Merged
merged 4 commits into from
Jul 17, 2023
Merged

[SUPPORT] remove unnecessary check on tx networkInfo #4020

merged 4 commits into from
Jul 17, 2023

Conversation

chabroA
Copy link
Contributor

@chabroA chabroA commented Jul 13, 2023 •
edited
Loading

📝 Description

Following this comment #3945 (comment) and the related discussion, proposal to remove what seem to be an unnecessary condition on the existence of a networkInfo property on the transaction object to render (or not) a family custom SendRowsFee component.

Not all crypto transaction types have a networkInfo property and some of them have some custom SendRowsFee component.
If a family SendRowsFee component display is dependent on the presence of a networkInfo property in the transaction object, these families own SendRowsFee component will never be displayed.

This condition seems odd because not all families have a networkInfo property in their transaction. For example algorand, but also the new evm. But these families have their own SendRowsFee component defined in their family folder. Same thing for celo, polkadot, solana (here we even have some hardcoded patch, cf. this).
So, based on this logic, as of today the SendRowsFee for algorand, celo and polkadot would not be displayed.

Furthermore, we don't seem to have similar logic on LLD (cf. here and here) where we don't have an condition on networkInfo.

The only place where I found such condition on LLD are these 2 files (here and here) related to swap, which might probably be some legacy logic from original swap implementation since these 2 conditions control the display of the SendAmountFields file listed just above, and it would not make sense to always display this component on a normal send flow but not on a swap flow (if I can edit fees on a normal send, why could I not do it in a Swap flow?)

❓ Context

✅ Checklist

  • Test coverage No component testing availble on send flows 😞 Video of non regression in the demo part ⬇️
  • Atomic delivery
  • No breaking changes

📸 Demo

develop support/remove-unnecessary-networkInfo-check
develop.mp4
branch.mp4

🚀 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.

@chabroA
remove unnecessary check on tx networkInfo
2d11be3 
not all crypto transaction types have a networkInfo property and some
of them have some custom SendRowsFee component
If a family SendRowsFee component display is dependent on the
presence of a networkInfo property in the transaction object, these
families own SendRowsFee component will never be displayed
@chabroA
remove unnecessary check on tx networkInfo in LLD swap screen
37b0d31
@chabroA
remove solana hardcoded patch
3c5cae4
@chabroA chabroA requested a review from a team as a code owner July 13, 2023 10:12
@changeset-bot
Copy link

changeset-bot bot commented Jul 13, 2023 •
edited
Loading

🦋 Changeset detected

Latest commit: 44df71e

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

This PR includes changesets to release 7 packages
Name Type
ledger-live-desktop Patch
live-mobile Patch
@ledgerhq/live-common Patch
@ledgerhq/live-cli Patch
@ledgerhq/test-utils Patch
dummy-wallet-app Patch
live-common-tools 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

@vercel
Copy link

vercel bot commented Jul 13, 2023 •
edited
Loading

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

Name Status Preview Comments Updated (UTC)
live-common-tools ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 13, 2023 10:30am
3 Ignored Deployments
Name Status Preview Comments Updated (UTC)
ledger-live-github-bot ⬜️ Ignored (Inspect) Jul 13, 2023 10:30am
native-ui-storybook ⬜️ Ignored (Inspect) Jul 13, 2023 10:30am
react-ui-storybook ⬜️ Ignored (Inspect) Jul 13, 2023 10:30am

@github-actions github-actions bot added desktop Has changes in LLD mobile Has changes in LLM common Has changes in live-common labels Jul 13, 2023
@chabroA chabroA mentioned this pull request Jul 13, 2023
Closed
3 tasks
@chabroA chabroA merged commit 37c4e84 into develop Jul 17, 2023
45 of 47 checks passed
@chabroA chabroA deleted the support/remove-unnecessary-networkInfo-check branch July 17, 2023 15:49
ghost
ghost approved these changes Jul 17, 2023
View reviewed changes
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

My main concern is about non-reg:

  • There are changes on LLD but your demo videos only cover LLM
  • There are changes in swap code but your demo videos don't cover that

Still, reading the code, I'm seeing that the single necessary condition before reaching all the parts you changed in your PR, is that prepareTransaction has been called for bitcoin, which I believe is always the case. So it should be safe, but please:

  1. Manually test both items above if you can
  2. Add a comment for QA in the JIRA ticket, to describe all non-reg cases to cover

Thanks

chabroA added a commit that referenced this pull request Jul 17, 2023
@chabroA
Revert "[SUPPORT] remove unnecessary check on tx networkInfo (#4020)"
364cfaf 
This reverts commit 37c4e84.
@chabroA chabroA mentioned this pull request Jul 17, 2023
Closed
chabroA added a commit that referenced this pull request Jul 17, 2023
@chabroA
Revert "[SUPPORT] remove unnecessary check on tx networkInfo (#4020)"
eee44e3 
This reverts commit 37c4e84.
@chabroA chabroA mentioned this pull request Jul 17, 2023
Closed
3 tasks
ghost pushed a commit that referenced this pull request Jul 21, 2023
@chabroA @hakim-adamik
[SUPPORT] remove unnecessary check on tx networkInfo (#4020)
75899a6
ghost pushed a commit that referenced this pull request Jul 21, 2023
@adrienlacombe @sarneijim
@chabroA @lambertkevin @lvndry @nabil-brn @live-github-bot @elbywan @valpinkman @sshmaxime @hzheng-ledger @ggilchrist-ledger @JunichiSugiura @abdurrahman-ledger @cng-ledger @beths-ledger @andreishtamburg @grsoares21 @kallen-ledger @mdeidda-ledger @Justkant @stephane-lieumont-ledger
Support/fix evm tools and domain service publishing (#4094)
15c1348 
* feat(ledgerjs-cal): update CAL (#3984)

* Feat/llm/add staking_completed track llm DOT & XTZ (#3963)

* feat: add cosmos currency to page track

* feat: add sol currency to page track

* feat: add xtz currency to page track

* feat: add near currency to page track

* feat: add dot currency to page track

* feat: add celo currency to page track

* feat: add egld currency to page track

* chore: changeset

* fix: fix xtz comment

* feat: add stake_completed track to tezos llm

* feat: add stake_completed track to dot llm

* chore: add changeset

* fix: add flow to other staking_completed llm tracks

* fix: fix typecheck

* [FEAT][LIVE-7406] create evm send flow (LLD) (#3704)

Co-authored-by: 0xkvn <44363395+lambertkevin@users.noreply.github.com>
Co-authored-by: Landry Monga <lvndry@protonmail.com>

* Account deletion tests (#3975)

* test(lld): account deletion tests

* test(lld): update screenshots (macos-latest)

lld, test, screenshot

* test(lld): update screenshots (ubuntu-latest)

lld, test, screenshot

* test(lld): update screenshots (windows-latest)

lld, test, screenshot

---------

Co-authored-by: live-github-bot[bot] <105061298+live-github-bot[bot]@users.noreply.github.com>

* ci(llm): use proper release env for android

* tagged smoke tests

* chore: prepare test-desktop for smoke tests

* chore: add new smoke and all scripts for playwright

* test(lld): update screenshots (ubuntu-latest)

lld, test, screenshot

* test(lld): update screenshots (macos-latest)

lld, test, screenshot

* chore: update workflow + slack report

* test(lld): update screenshots (windows-latest)

lld, test, screenshot

* chore: format slack message

* chore: remove extra code

* ci(lld): test-desktop-external catch up

* chore: add github actor name in report

* ci(lld): restore slack reporting guards

* chore: update pnpm to 8.6 everywhere

* ci(desktop-external): clean up extra jobs

* test(lld): update screenshots (macos-latest)

lld, test, screenshot

* test(lld): update screenshots (ubuntu-latest)

lld, test, screenshot

* test(lld): update screenshots (windows-latest)

lld, test, screenshot

* chore: fix `if` statement in test desktop

* Feat/llm/remove swap refresh rates (#4003)

* feat: remove extra refresh effect when configuration change in swap llm

* chore: add changeset

* Fix translation keys issue in LLM in the Quick Actions section (#4002)

* Support LIVE-8118 rename optimism (#3964)

* fix: rename optimism

* fix: crypto assets search keys

* fix: update changeset

* fix: update env description

* fix: update cryptoassets list

* chore: fix broken JS in test-desktop for slack message

* test: added framework and test for wallet api/live app detox test

* test: made server.close function wait for  server onclose event to fire to prevent open handles

* test: improved non-android console warnings

* test: fixed ReactNativeWebView typing

* docs: mention useE2EInjection hook in README

* chore: updating desktop docs

* test: updated docs

* fix: remove dup logic

* chore: update readme and skipping test

* feat: created new test-utils directory to contain dummy live apps

* feat: created new test-utils directory to contain dummy live apps

* feat: adding dummy build to workflows

* chore: adding gitignore for dummy builds

* fix: continue test logic

* chore: dev dependency update

* chore: removed redundant scripts from dummy live apps

* chore: making test utils a dev dependency

* chore: update pnpmlock

* fix: downgrade ts/jest packages to lower version to see if it fixes detox issues

* chore: updated dummy server types

* chore: adding types to server start

* Rollback `evm-tools` jest config to pre v29 format

* chore: updating pnpmlock

* chore: updating pnpm filters and documentation for dummy apps

* chore: updating pnpm lock

* chore: fix lockfile

* Update README.md

* test: llm portfolio read only test

* chore: update pnpm-lock file

* chore: fix lint

* chore: implement review suggestions

* fix(LIVE-8530): quotes are not readable in light mode

* fix(LIVE-8530): added changset

* ci: fix concurrency when pushing on dev

* fix(deps): update dependency @abandonware/noble to v1.9.2-21

* feat: add discreet mode param to earn dashboard

* feat(PROTECT-1782): Change the wording around login to recover app

* feat(PROTECT-2171): change wording on recover login CTA

* chore: upgrade babel/core to latest

* chore: fix input checkout in workflows

* chore: add missing ref to test-desktop

* test: update detox and fix animation issues

* fix(LIVE-8530): use colors by color key by default

* Feat/llm/Implement and harmonise tracking for the entry point of each staking flow (#4016)

* feat: add celo currency to screen track

* feat:  cosmos currency to screen track

* feat:  elrond currency to screen track

* feat: near currency to screen track

* feat: polkadot currency to screen track

* feat: solana currency to screen track

* feat: tezos currency to screen track

* feat: algorand currency to screen track

* feat: algorand currency to screen page

* chore: add changeset

* chore: remove debugger

* chore: fix typescript

* chore: add test full suite command to github bot

* [LIVE-7547] Feature - Add telos evm to currencies (#4021)

* chore(deps): update dependency stream-json to v1.7.5

* chore: add command to bot

* chore: change input type in bot

* chore(bot): remove extra input in command

* [SUPPORT] remove unnecessary check on tx networkInfo (#4020)

* chore(actions): add new s3 cache action for turborepo

* ci(s3 cache): use custom s3 caching action with turbo

* chore: remove errors only

* chore(deps): update dependency @types/react-motion to ^0.0.34

* chore(deps): update dependency @react-native-async-storage/async-storage to v1.17.12

* [LIVE-7949] Design polishes for the new firmware update UX (#3792)

* feat(Firmware Update LLM): minor design and wording polishes

* feat(Firmware Update LLM): fix a few wordings and design feedback

* feat(Firmware Update LLM): minor spacing tweaks on the update drawers

* chore(Firmware Update LLM): add changelog

* feat(FirmwareUpdateLLM): add allowmanager lottie on apps backup and minor alignments

* feat(FirmwareUpdateLLM): final minor UI adjustments

* chore(NativeUILibrary): add changelog

* feat: created new test-utils directory to contain dummy live apps

chore: dev dependency update

chore: update pnpmlock

chore: updated dummy server types

* feat: added live app webview page object model

* test: buy/sell test working

* chore: updating liveapp-sdk spec to smoke

* chore: removing redundant screenshots and dep imports

test(lld): update screenshots (macos-latest)

lld, test, screenshot

test(lld): update screenshots (ubuntu-latest)

lld, test, screenshot

* test: improved earn test and skipping stability test

test(lld): update screenshots (ubuntu-latest)

lld, test, screenshot

test(lld): update screenshots (macos-latest)

lld, test, screenshot

test(lld): update screenshots (windows-latest)

lld, test, screenshot

* test: added type for webview

* test: changing dummy filter to see if it fixes windows runs

* test(lld): update screenshots (macos-latest)

lld, test, screenshot

* test(lld): update screenshots (ubuntu-latest)

lld, test, screenshot

* test: changing dummy filter to see if it fixes windows runs

* test(lld): update screenshots (windows-latest)

lld, test, screenshot

* chore: adding console warn for service tests where server fails to start

* chore: adding check for server running before trying to shut it down

* test(lld): update screenshots (windows-latest)

lld, test, screenshot

* chore: remove redundant buy screenshots

* Fix USDC Market Price Component Data (#4048)

* Fix USDC Market Price component data

* Typos

* chore: fix windows git crashing on generate screenshots

* feat: add feature flag to hide or show ptx services in LLM

* fix: update docs on feature flags

* chore: add changeset

* fix: remove unused imports

* fix: update dependency in useAssetAction

* fix: add override on feature flags for e2e test

* [LIVE-6263] Feature - Ledger infrastructure abstraction for the EVM Family (#3827)

Co-authored-by: Alexandre Chabrolin <9203826+chabroA@users.noreply.github.com>

* [support] Minor code improvements on the Celo integration (#3919)

Co-authored-by: Alexandre Chabrolin <9203826+chabroA@users.noreply.github.com>

* test: added test and  elements for market

* test: deleted an unnecessary comment in marketPage.ts

* renaming fonctions and testID and removing async in marketPage.ts

* changes done according to feedbacks on the pr. Wildcard added for the url.

* comments deleted, unusual await deleted, refacto of the url expect assert

* fix made following the raised points

* fix: using previous testID for search bar

* test: adding buy button testid in after rebase

* [LIVE-7445] Filter device selection to only stax when using NFT as CLS (#3999)

* fix(CLS): filter for stax devices when using new device selection screen

* fix(CLS): fix import order

* chore(CLS): add changelog

* feat(CLS LLM): add model id filter to bluetooth pairing flow

* test: added tests for no available swap providers

* chore: improving live sdk warning message

* fix: updated wait in swap page navigate

* feat: logging video path to make it easier to identify a pw video with a test

* chore: added line break to make log more readable

* test: added @smoke tag to tests, removed screenshots for regeneration and renamed files

test(lld): update screenshots (macos-latest)  lld, test, screenshot

test(lld): update screenshots (ubuntu-latest)  lld, test, screenshot

test(lld): update screenshots (windows-latest)  lld, test, screenshot

test: added @smoke tag to manual syncOnboarding

test(lld): update screenshots (macos-latest)  lld, test, screenshot

test(lld): update screenshots (ubuntu-latest)  lld, test, screenshot

test(lld): update screenshots (windows-latest)  lld, test, screenshot

chore: trigger ci

* Bugfix/lld/fix eth stake track (#4073)

* fix: fix Lido/Kiln button analytics not working

* chore: add changeset

* feat(LIVE-6958): package script update

rename shared-ui to ui-shared

* feat(LIVE-6958): add provider icon sizes & cdn url to LLC

feat(LIVE-6958): use new provider icon component from desktop

feat(LIVE-6958): use new provider icon component from mobile

* feat(LIVE-6958): add changeset

* feat(LIVE-6958): fix llm/icons/swap

* Bugfix/fix eth test (#4076)

* fix: fix Lido/Kiln button analytics not working

* chore: add changeset

* fix: fix eth test lld

* feat(PROTECT-2239): rename recover cta

* fix(deps): update dependency @polkadot/react-identicon to v2.11.3

* feat(recover): restore check seeded [PROTECT-1711]

* feat(recover): add translate for error device already setup [PROTECT-1711]

* feat: fix i18n object title

* Remove .gitignores preventing correct publishing

* changeset

---------

Co-authored-by: Adrien Lacombe <adrien.lacombe@ledger.fr>
Co-authored-by: sarneijim <38540290+sarneijim@users.noreply.github.com>
Co-authored-by: Alexandre Chabrolin <9203826+chabroA@users.noreply.github.com>
Co-authored-by: 0xkvn <44363395+lambertkevin@users.noreply.github.com>
Co-authored-by: Landry Monga <lvndry@protonmail.com>
Co-authored-by: Nabil Bourenane <nabil.bourenane@icloud.com>
Co-authored-by: live-github-bot[bot] <105061298+live-github-bot[bot]@users.noreply.github.com>
Co-authored-by: elbywan <julien.el-baz@ledger.fr>
Co-authored-by: Valentin D. Pinkman <valentin.d.pinkman@hey.com>
Co-authored-by: Maxime Aubanel <sshmaxime@gmail.com>
Co-authored-by: hzheng-ledger <71653044+hzheng-ledger@users.noreply.github.com>
Co-authored-by: Gregor Gilchrist <gregor.gilchrist@ledger.fr>
Co-authored-by: JunichiSugiura <jun.sugiura.jp@gmail.com>
Co-authored-by: lambertkevin <kevin.lambert@ledger.fr>
Co-authored-by: Valentin D. Pinkman <valentin.d.pinkman@icloud.com>
Co-authored-by: Abdurrahman SASTIM <abdurrahman.sastim@ledger.fr>
Co-authored-by: Chuck Ng <cheuk-man.ng@ledger.fr>
Co-authored-by: Beth Swingler <beth.swingler-ext@ledger.fr>
Co-authored-by: Andrei Shtamburg <andrei.shtamburg@ledger.fr>
Co-authored-by: Gabriel Restori Soares <gabriel.soares@ledger.fr>
Co-authored-by: Kieran Allen <kieran.allen@ledger.fr>
Co-authored-by: mdeidda-ledger <marco.deidda@ledger.fr>
Co-authored-by: Kant <quentin.jaccarino@ledger.fr>
Co-authored-by: Stephane Lieumont <stephane.lieumont-ext@ledger.fr>
@ghost ghost mentioned this pull request Sep 9, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hedi-edelbloute hedi-edelbloute hedi-edelbloute approved these changes

Assignees

@chabroA chabroA

Labels
common Has changes in live-common desktop Has changes in LLD mobile Has changes in LLM
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@chabroA @hedi-edelbloute