lisk-mobile icon indicating copy to clipboard operation
lisk-mobile copied to clipboard
verified publisher icon LiskHQ

Resolve todo comments

Open reyraa opened this issue 4 years ago • 1 comments

Description

Todos are one of the issues is marked by DeepScan as critical so it’s recommended to resolve it as soon as possible. In the project there are 10 TODO’s in 6 files. Resolving the issue will increase code clarity, unit test coverage, improve error handling and much more.

Motivation

  • Improve maintainability
  • Prevent unhandeled errors

Additional Information

Search for TODO comments inside the code.

reyraa avatar Oct 11 '21 08:10 reyraa

Price ticker:

  • On error: Show the local fee calculation

dynamicFeesRetrieved

  • On error: Show local fee calculation

settingsRetrieved

  • On error: Reset to default (create a default setting object)

Unmapped deep link

  • Design to be provided

Notifications

  • Discuss separately

onBackButtonPressedAndroid

  • Get more details on what should be fixed here

Balanced02 avatar Jan 06 '22 16:01 Balanced02

For the current state of the app in development, we have the following TODOs:

  1. e2e/packages/Auth.e2e.js: 1.1. Download passphrase before selecting account. 1.2. Test tokens are displayed: We need to write tests to ensure tokens are fetched and displayed correctly on the home screen (token title, amount, symbol)
  2. e2e/packages/Settings.e2e.js: Fix settings end to end test.
  3. libs/wcm/utils/connectionCreator.js: Replace WC client icon with Lisk Service provided assets by #4465 / @reyraa looks like this issue doesn't exist on mobile neither in desktop.
  4. libs/wcm/utils/requestHandlers.js: Add test coverage by #4418 / @reyraa looks like this issue on mobile is not related to this.
  5. libs/wcm/utils/requestHandlers.test.js: Add tests by #4418 / @reyraa same
  6. src/BootstrapApp.js: Replace dummy loading fallback with <LoadingFallbackScreen /> component when working on https://github.com/LiskHQ/lisk-mobile/issues/1587.
  7. src/actions/service.js: Redux error catches needs to be handled properly. Currently using clgs.
  8. src/assets/svgs/CaretSvg.js: Add missing SVGs according to directions.
  9. src/components/shared/BottomModal/index.js: Replace {children} container with another VirtualizedList-backed container. VirtualizedLists should never be nested inside plain ScrollViews with the same orientation because it can break windowing and other functionality.
  10. src/components/shared/DataRenderer/index.js: Add skeletons and error general component for loading and error states.
  11. src/components/shared/imessage/confirm/index.js: Consume transactions hooks instead when they are created / @reyraa can you provide more context on this?
  12. src/components/shared/loading/index.js: Reinstate lottie after upgrade / Not sure if this component stills being used.
  13. src/modules/Accounts/index.js: 13.1. Implement release notification. 13.2. Implement custom message: this can be used in case we need to notify the user about any unforeseen issue/change.
  14. src/modules/Accounts/components/AccountItem/index.js: Implement backup action. onPress callback is mocked.
  15. src/modules/Accounts/components/AccountList/index.js: Integrate pagination props when useAccounts is refactored to use react-query.
  16. src/modules/Auth/AuthMethod/index.js: 16.1. Confirm valid file and show necessary error if any after calling selectEncryptedFile on selectEncryptedJSON function. 16.2. Handle error message on selectEncryptedJSON error catch.
  17. src/modules/BlockchainApplication/api/useApplicationSupportedTokensQuery.js: Query for each token the GET /meta/tokens endpoint when service solves tokenID inconsistency between GET /tokens/summary and GET /meta/tokens responses.
  18. src/modules/BlockchainApplication/hooks/useApplicationsExplorer.js: Pass as CSV of chainIDs when backend supports feature. e.g.: applicationsData?.data.map((app) => app.chainID).
  19. src/modules/SendToken/components/SelectApplicationsStep/components.js: Integrate pagination props on applications InfiniteScrollList using react-query.
  20. src/modules/SendToken/components/SelectTokenStep/components.js: 20.1. Read token symbol from account info when backend send the data on TokenSelectField picker options. 20.2. Integrate pagination props on tokens picker options using react-query.
  21. src/modules/Settings/actions/settings.js: Rejection must is not handled on settingsRetrieved Redux dispatcher.
  22. src/modules/Settings/components/ItemTitle/index.js: Update to use own defined icons (remove react-native-vector-icons) to solve current inconsistencies.
  23. src/modules/Transactions/hooks/useCCMFeeCalculator.js: Implement real calculation business logic.
  24. src/modules/Transactions/hooks/useInitializationFeeCalculator.js: Implement real calculation business logic.
  25. src/modules/Transactions/hooks/useTransactionAssets.js: Add custom images for each transaction type image.
  26. src/modules/Transactions/hooks/useTransactionList.js: Remove when wallet module is refactored and stop using it (deprecated - Use useAccountTransactionsQuery instead.).
  27. src/modules/Transactions/utils/constants.js: For BASE_TRANSACTION_SCHEMA, we need to use the object fetched from service endpoint/elements.
  28. src/modules/Transactions/utils/helpers.js: For getDryRunTransactionError, calculate properly the error message and integrate it with UI. See https://github.com/LiskHQ/lisk-mobile/issues/1578 for details.
  29. src/modules/Transactions/utils/Transaction.js: On the sign method, update networkIdentifier to chainID once service endpoint is updated.
  30. src/modules/Wallet/index.js: Implement saved language detection, release notification, custom message, use useAccountTransactionsQuery instead and use InfiniteScrollList instead. @Balanced02 I think this component will be deprecated right?
  31. src/navigation/index.js: Move useWalletConnectEventsManager() to BootstrapApp component.

@Balanced02 lets discuss here which ones need a separate issue to be solved.

I think 1., 2., needs a separate issue.

clemente-xyz avatar Jan 11 '23 12:01 clemente-xyz

Most of these would have their separate issues.

We don't need 12 again, we can remove everything related to Lottie on the mobile app 30. InfiniteScrollList should not be depreciated... we'll need this along with the query to fetch more data 26. We need to confirm if we're removing the explorer from mobile completely. It's removed atm.

Balanced02 avatar Jan 12 '23 11:01 Balanced02

Can we create these as sub-tasks so we can work on them individually?

Balanced02 avatar Jan 12 '23 11:01 Balanced02

@Balanced02 , I removed the old Loading component (point 12).

Regarding to 30. , I didn't mean that InfiniteScrollList is deprecated, I meant that the Wallet component is.

Regarding to 26., the component useTransactionList is only being used by Wallet component (which we want to remove). Also, useTransactionList is not doing nothing. It's return is mocked.

clemente-xyz avatar Jan 12 '23 11:01 clemente-xyz

Looks good then, we can create the subtasks and proceed

Balanced02 avatar Jan 12 '23 13:01 Balanced02