[Fix] Wallet - Routes not fetched on scanned address

[smohamedjavid]

fixes #18652 (for the second time and hopefully last)

Summary

This PR fixes routes not fetched for scanned addresses.

Review notes

The issue doesn't state it is caused only by scanned addresses. This was discovered on https://github.com/status-im/status-mobile/pull/18850#issuecomment-1948041140.

This bug is caused as a regression by the introduction of :modal-view-ids (to app-db) to store the screen list and to prevent some key data lost on re-render (during development).

In the send flow, the first screen (wallet-select-address) is opened in a modal (:open-modal) and the screens after that are navigated with the help of the :navigate-to-within-stack event. BUT, The scan-address screen is opened with :open-modal (resets :modal-view-ids ) and on successful scan or on navigating back by tapping on the X icon, the :modal-view-ids is removed from app-db (because the navigate-back event clears it) and makes the following condition to fail and not to fetch the route: https://github.com/status-im/status-mobile/blob/be2c697b7cf245f91d593dfda18d0554214ceb3b/src/status_im/contexts/wallet/send/input_amount/view.cljs#L179

We can make the open-modal robust by checking if any :modal-view-ids exists and conj the view-id to it. This would keep the :modal-view-ids intact but may introduce other unwanted bugs as the old data would persist.

So, the proper fix is to add the :scan-address screen in the stack flow on the send flow and the add watch address flow separately. Since the scanner view is common, we need to handle the :navigate-back event as well (to not affect other screens). Hence, we introduce a custom navigation function prop to fix it.

Testing notes

Regression testing on the whole application is appreciated as this PR updates the common scanner screen.

Platforms

• Android
• iOS

Steps to test

• Open Status
• Log in to your profile
• Navigate to the Wallet tab
• Tap on any account
• Tap on the Send button
• Scan any address
• Select any asset to send
• Enter any amount (below the limit)
• Verify the routes are fetched

Additionally, verify the add address to watch flow with the scanner is working as expected

status: ready

[status-im-auto]

Jenkins Builds

:grey_question:	Commit	:hash:	Finished (UTC)	Duration	Platform	Result
:heavy_check_mark:	fb2f3c9d	#1	2024-02-16 18:13:05	~6 min	ios	:iphone:ipa :calling:
:heavy_check_mark:	fb2f3c9d	#1	2024-02-16 18:14:14	~7 min	android	:robot:apk :calling:
:heavy_check_mark:	fb2f3c9d	#1	2024-02-16 18:14:15	~7 min	android-e2e	:robot:apk :calling:

[briansztamfater]

BTW, thanks for finding out the root cause of the issue, it was tricky!

[smohamedjavid]

@briansztamfater! Thanks for the review, Appreciate it.

Let’s say, we conj the screen-id to modal-view-ids on opening the modal (instead of reset). This would ensure the modal-view-ids updates with the latest view-id. But, we need to handle the dismiss modal and back navigation as well (adds a lot of complexity).

In most cases, for dismissing a modal, we don’t use the dismiss-modal event (found only one usage in the whole codebase). We use the navigate-back event to dismiss the whole stack.

With a multi-dimensional approach, it would make things complicated on the navigate-back-to-within-stack event, as we can’t guarantee the screen would be available only once in an array of arrays (to remove from the stack and going back) and could lead to unwanted issues.

Adding options to support opening more modals increases the complexity of maintaining it. It's a nice feature to have, but it requires brainstorming and thorough testing.

The reason why I felt the current solution is good and simple is that the address QR scanner (:scan-address) is part of the add address to watch and send flows. navigate-to-within-stack won’t cause any issues in overall UX and DX for both flows as the screen is intended in those flows. This PR is not intended to introduce anything new in the navigation events or handling.

It’s just we have to update the QR scanner code (status-im.common.scan-qr-code.view ns) view because it is a common view and has a navigate-back event automatically dispatched on the successful scan. Overriding the navigation event via prop would be helpful in this case.

Additionally, I don’t think unit tests could be helpful here as they focus on a small area of code. Even with the screen test, we set the value of subs (and app-db values) as the expected result. https://github.com/status-im/status-mobile/blob/c44ba69676d11e44c644e870f9d48619ee932e44/src/status_im/contexts/wallet/send/input_amount/component_spec.cljs#L56

IMO, we need something like Maestro or Appium to test the flow in holistic as this involves navigation events and re-frame events.

Let me know what you think. I’d be happy to jump on a call if needed.

[briansztamfater]

@smohamedjavid thanks for the explanation. Yep, let's jump into a call. I'd like to see if we can find an approach where the UX doesn't get affected.

[smohamedjavid]

UPDATE: @briansztamfater is doing some R&D on a multi-modal approach. I will keep this PR on hold until then.

[briansztamfater]

As discussed with @smohamedjavid, opened a new PR with a multi-modal approach #19098

[flexsurfer]

feel free to reopen when needed