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

Router clean up #21235

Merged
merged 5 commits into from
Sep 11, 2024
Merged

Router clean up #21235

merged 5 commits into from
Sep 11, 2024

Conversation

alwx
Copy link
Contributor

@alwx alwx commented Sep 9, 2024

Works together with status-im/status-go#5778

Summary

Updates router calls to work with changes made in status-im/status-go#5778

@alwx alwx self-assigned this Sep 9, 2024
@alwx alwx marked this pull request as ready for review September 9, 2024 14:00
@status-im-auto
Copy link
Member

status-im-auto commented Sep 9, 2024

Jenkins Builds

Click to see older builds (12)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 5bbc811 #1 2024-09-09 14:04:09 ~5 min tests 📄log
✔️ 5bbc811 #1 2024-09-09 14:07:05 ~8 min android-e2e 🤖apk 📲
✔️ 5bbc811 #1 2024-09-09 14:08:01 ~9 min android 🤖apk 📲
✔️ 5bbc811 #1 2024-09-09 14:17:43 ~19 min ios 📱ipa 📲
✔️ 47c43ca #2 2024-09-10 07:20:49 ~6 min tests 📄log
✔️ 47c43ca #2 2024-09-10 07:24:29 ~9 min android-e2e 🤖apk 📲
✔️ 47c43ca #2 2024-09-10 07:25:40 ~10 min android 🤖apk 📲
✔️ 47c43ca #2 2024-09-10 07:33:43 ~19 min ios 📱ipa 📲
✔️ 795e60d #3 2024-09-10 08:27:04 ~4 min tests 📄log
✔️ 795e60d #3 2024-09-10 08:29:08 ~6 min android-e2e 🤖apk 📲
✔️ 795e60d #3 2024-09-10 08:30:28 ~8 min android 🤖apk 📲
✔️ 795e60d #3 2024-09-10 08:41:43 ~19 min ios 📱ipa 📲
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 156afb2 #4 2024-09-10 10:03:50 ~5 min tests 📄log
✔️ 156afb2 #4 2024-09-10 10:06:54 ~9 min android-e2e 🤖apk 📲
✔️ 156afb2 #4 2024-09-10 10:08:21 ~10 min android 🤖apk 📲
✔️ 156afb2 #4 2024-09-10 10:10:35 ~12 min ios 📱ipa 📲
✔️ 94630ee #5 2024-09-11 07:39:31 ~5 min tests 📄log
✔️ 94630ee #5 2024-09-11 07:43:12 ~8 min ios 📱ipa 📲
✔️ 94630ee #5 2024-09-11 07:43:41 ~9 min android-e2e 🤖apk 📲
✔️ 94630ee #5 2024-09-11 07:44:57 ~10 min android 🤖apk 📲

@Khushboo-dev-cpp
Copy link

Khushboo-dev-cpp commented Sep 9, 2024

@alwx I am working on a PR that has api change https://github.com/status-im/status-go/pull/5564/files

basically removed

        TokenPrice            *float64              `json:"TokenPrice,omitempty"`
	NativeChainTokenPrice *float64              `json:"NativeChainTokenPrice,omitempty"`

and added
UpdatedPrices map[string]float64 `json:"UpdatedPrices,omitempty" `
instead.

I wanted to check if the removed params are being used in the mobile client already? cause if not I'll merge my PR to Sales PR directly.

fyi @saledjenic

@saledjenic
Copy link

saledjenic commented Sep 10, 2024

@alwx for testing this branch could you make it to point to statusgo commit 900ad9be01de8bfce2c229c8fb0222b99606cf35 cause I rebased it today?

@saledjenic
Copy link

@alwx what about Khushboo's question? Do you use those properties or not? If not we can merge here changes in the same go.

@alwx
Copy link
Contributor Author

alwx commented Sep 10, 2024

@saledjenic will do in a sec
Regarding the @Khushboo-dev-cpp's question: no, those parameters are not used anywhere.

@VolodLytvynenko VolodLytvynenko self-assigned this Sep 10, 2024
@saledjenic
Copy link

@churik please let us know when is the right time to merge statusgo PR?

@churik
Copy link
Member

churik commented Sep 10, 2024

when we'll test it, I'll ping you @saledjenic

@VolodLytvynenko
Copy link
Contributor

VolodLytvynenko commented Sep 10, 2024

hi @alwx thank you for PR. Take a look at the found issue. This issue happen consistently, but unfortunately there are no exact steps to reproduce it. Hopefully, the logs will help

ISSUE 1: [Android] 'Undefined not a function' error is constantly shown after attempt to build the route

Steps:

  1. Go to routes generation page
  2. Build the route
  3. Go to transaction confirmation page
  4. Go back to the wallet main page

Actual result:

'Undefined not a function' error is constantly shown

wallet_new_route.mp4

Expected result:

no errors are shown

Devices:

  • Pixel 7a, Android 13

Logs

Status-debug-logs.zip

@VolodLytvynenko
Copy link
Contributor

PR_ISSUE 2: Endless 'edit' button is shown

Steps:

  1. Navigate to the route generation flow.
  2. Input invalid data, such as:
  • A token count available on multiple networks.
  • A zero value (e.g., 0.00000).
  • A value that cannot be covered due to insufficient fees.
  1. Tap the "Try Again" button.

Actual Result:

The "Edit" button is displayed repeatedly

editbutton.mp4

Expected result:

The "Edit" button should be displayed only once

OS:

IOS, Android

Devices:

  • Pixel 7a, Android 13
  • iPhone 11 Pro Max, IOS 17

Logs

logs (5).zip

@shivekkhurana
Copy link
Contributor

The edit button issue has been logged here already: #21043

I don't think it should be a scope of this PR.

@shivekkhurana
Copy link
Contributor

@alwx Can you check the logs and confirm or deny that this issue is because of the changes you made ?

@VolodLytvynenko
Copy link
Contributor

The edit button issue has been logged here already: #21043

I don't think it should be a scope of this PR.

@shivekkhurana Yes, the first issue and the current one seem similar, but this one appears to be easier to reproduce. Additionally, the endless button keeps appearing even without tapping the 'try again' button, so it might have a different cause.

@VolodLytvynenko
Copy link
Contributor

PR_ISSUE 2: Endless 'edit' button is shown

Steps:

  1. Navigate to the route generation flow.
  2. Input invalid data, such as:
  • A token count available on multiple networks.
  • A zero value (e.g., 0.00000).
  • A value that cannot be covered due to insufficient fees.
  1. Tap the "Try Again" button.

Actual Result:

The "Edit" button is displayed repeatedly

editbutton.mp4

Expected result:

The "Edit" button should be displayed only once

OS:

IOS, Android

Devices:

  • Pixel 7a, Android 13
  • iPhone 11 Pro Max, IOS 17

Logs

logs (5).zip

Here are some additional steps to reproduce this issue, which does not happen in the Develop.

Steps:

  1. Navigate to the route generation flow.
  2. Input invalid data, such as:
  • A zero value (e.g., 0.00000).
  1. Go back
  2. Navigate to routes generation page again
routes.mp4

@VolodLytvynenko
Copy link
Contributor

VolodLytvynenko commented Sep 10, 2024

PR_ISSUE 3: Wrong UI is shown after changing the token on the routes generation page

Steps:

  1. Go to routes generation page
  2. Build the route
  3. Tap the asset to open asset list
  4. Change the asset

Actual result:

The previous route is still shown

asset_route.mp4

Expected result:

The previous route is not shown when asset is changed

OS:

IOS, Android

Devices:

  • Pixel 7a, Android 13
  • iPhone 11 Pro Max, IOS 17

@VolodLytvynenko
Copy link
Contributor

VolodLytvynenko commented Sep 10, 2024

PR_ISSUE 4: The Confirmation Keyboard Auto-Closes After Entering the Password

Steps:

  1. Navigate to the transaction confirmation page.
  2. Attempt to enter the password.

Actual Result:

The confirmation keyboard constantly auto-closes, preventing the user from entering the password properly.

Pixel 7a, Android 13

androidkeybaord.mp4

iPhone 11 Pro Max, IOS 17

ioskeyboard.mp4

Expected Result:

The keyboard should remain open, allowing the user to enter the password without interruptions

Logs:

logs (6).zip

@VolodLytvynenko
Copy link
Contributor

VolodLytvynenko commented Sep 10, 2024

PR_ISSUE 5: "eq() not a number: <0.01" Error shown after transaction confirmation

Steps:

  1. Open account
  2. Go to the routes generation page -> confirm a transaction

Actual result:

The user is redirected to the activity page, where the error message "eq() not a number: <0.01" is displayed.

image

Expected result:

The transaction should be confirmed without showing any error messages

OS:

IOS, Android

Devices:

  • Pixel 7a, Android 13
  • iPhone 11 Pro Max, IOS 17

Logs

logs (7).zip

@saledjenic
Copy link

@VolodLytvynenko @alwx I'm wondering if all those issues come from the updated statusgo or they were in the mobile app even before statusgo changes?

@smohamedjavid
Copy link
Member

@VolodLytvynenko

PR_ISSUE 2: Endless 'edit' button is shown

Regarding additional steps posted in #21235 (comment), The routes are not supposed to be fetched if the user enters 0 or 0.000000. It's fixed here: #21209 (regression from #21136)

PR_ISSUE 3: Wrong UI is shown after changing the token on the routes generation page

This issue is fixed here: #21201

@alwx
Copy link
Contributor Author

alwx commented Sep 11, 2024

@saledjenic I don't think this issues have to do something with the status-go update. Your changes done there are a refactoring, you haven't changed the functionality, and what's broken here is mostly UI issues which have nothing to do with either your changes or this particular PR.

It's hard for me to imagine how status-go changes might lead to some issues with edit button or keyboard hiding automatically.

@saledjenic
Copy link

@churik @VolodLytvynenko @alwx can we merge then the statusgo changes, cause we've been waiting for them 3 weeks already?

Then you can merge this PR that handles endpoint names changes for the mobile app, and you can address all those above-mentioned issues (since they were here even before statusgo changes) through other PRs and don't block the desktop app by waiting for them to be fixed in this PR?

@VolodLytvynenko
Copy link
Contributor

PR_ISSUE 6: "Native token not found" error message randomly occurs

Unfortunately, this issue doesn't occur consistently, and there are no exact steps to reproduce it. However, it seems to happen after attempting to find routes upon entering valid data into the input field. Hopefully, the logs will help.

Steps:

  1. Enter valid data into routes generation page to build the routes

Actual result:

"Native token not found" error message is shown
image

Expected result:

No errors are shown, routes are build

Devices:

  • Pixel 7a, Android 13
  • iPhone 11 Pro Max, IOS 17

Logs

Status-debug-logs (2).zip

@VolodLytvynenko
Copy link
Contributor

@churik @VolodLytvynenko @alwx can we merge then the statusgo changes, cause we've been waiting for them 3 weeks already?

Hey @saledjenic, I need some additional time to finish testing. Just 2-3 more hours

@VolodLytvynenko
Copy link
Contributor

PR_ISSUE 7: "Not enough native balance, token ETH, chain id " Toast Shown When There Is Insufficient ETH to Cover the Fee

Steps:

  1. Go to the transaction confirmation page.
  2. Enter the valid amount of ERC-20 requiring ETH to cover the fee.
  3. Ensure the ETH balance is insufficient to cover the transaction fee.

Actual Result:

A toast message is displayed: "Not enough native balance, token ETH, chain id ."

image

Expected Result:

The toast message is not shown

Devices:

  • Pixel 7a, Android 13
  • iPhone 11 Pro Max, IOS 17

@VolodLytvynenko
Copy link
Contributor

VolodLytvynenko commented Sep 11, 2024

@saledjenic Thank you for your work and patience. I've finished testing. If issue 6 isn't Status Go-related, then the status go PR can be merged. I agree with @alwx that the other issues don't seem to be related to status go

@alwx alwx merged commit 72a02df into develop Sep 11, 2024
6 checks passed
@alwx alwx deleted the router-changes branch September 11, 2024 12:20
@churik
Copy link
Member

churik commented Sep 11, 2024

@alwx
not great that it was merged

  1. without e2e
  2. without clear desicions what to do with issues
  3. without Tested-OK

I understand that it will be not working without new router calls but we should decide how to deal with issues before merging the PR.

@alwx
Copy link
Contributor Author

alwx commented Sep 11, 2024

@churik my mistake, sorry — I somehow read the comment from @VolodLytvynenko above as an invitation to merge this one ☹️ Would be the right way to proceed with that now? Revert?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: DONE
Development

Successfully merging this pull request may close these issues.

10 participants