[Feature] Wallet - Handle blockchain status #18850
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
smohamedjavid
commented
Feb 15, 2024
src/status_im/constants.cljs
Outdated
Comment on lines
417
to
426
| (def ^:const ethereum-chain-id 1) | ||
| (def ^:const goerli-chain-id 5) | ||
| (def ^:const arbitrum-chain-id 42161) | ||
| (def ^:const arbitrum-testnet-chain-id 421613) | ||
| (def ^:const optimism-chain-id 10) | ||
| (def ^:const optimism-testnet-chain-id 420) | ||
| (def ^:const ethereum-mainnet-chain-id 1) | ||
| (def ^:const ethereum-goerli-chain-id 5) | ||
| (def ^:const ethereum-sepolia-chain-id 11155111) | ||
| (def ^:const arbitrum-mainnet-chain-id 42161) | ||
| (def ^:const arbitrum-goerli-chain-id 421613) | ||
| (def ^:const arbitrum-sepolia-chain-id 421614) | ||
| (def ^:const optimism-mainnet-chain-id 10) | ||
| (def ^:const optimism-goerli-chain-id 420) | ||
| (def ^:const optimism-sepolia-chain-id 11155420) |
There was a problem hiding this comment.
Just renamed the chain-id names to a bit more specific one.
smohamedjavid
commented
Feb 15, 2024
| @@ -60,8 +60,7 @@ | |||
| :color :colorId}) | |||
| (update :prodPreferredChainIds chain-ids-set->string) | |||
| (update :testPreferredChainIds chain-ids-set->string) | |||
| (dissoc :watch-only?) | |||
| (dissoc :default-account?))) | |||
| (dissoc :watch-only? :default-account? :tokens :collectibles))) | |||
There was a problem hiding this comment.
We have been unnecessarily sending huge amounts of tokens and collectible data whenever we edit an account.
There was a problem hiding this comment.
Nice!
I've faced some performance issues while just updating the profile!
smohamedjavid
commented
Feb 15, 2024
Comment on lines
204
to
220
| (defn- get-chain-id | ||
| [test-net?] | ||
| (if test-net? | ||
| {:eth constants/goerli-chain-id | ||
| :opt constants/optimism-testnet-chain-id | ||
| :arb1 constants/arbitrum-testnet-chain-id} | ||
| {:eth constants/ethereum-chain-id | ||
| :opt constants/optimism-chain-id | ||
| :arb1 constants/arbitrum-chain-id})) | ||
| [testnet-enabled? sepolia-enabled?] | ||
| (cond | ||
| testnet-enabled? | ||
| {:eth constants/ethereum-goerli-chain-id | ||
| :opt constants/optimism-goerli-chain-id | ||
| :arb1 constants/arbitrum-goerli-chain-id} | ||
|
|
||
| (and testnet-enabled? sepolia-enabled?) | ||
| {:eth constants/ethereum-sepolia-chain-id | ||
| :opt constants/optimism-sepolia-chain-id | ||
| :arb1 constants/arbitrum-sepolia-chain-id} | ||
|
|
||
| :else | ||
| {:eth constants/ethereum-mainnet-chain-id | ||
| :opt constants/optimism-mainnet-chain-id | ||
| :arb1 constants/arbitrum-mainnet-chain-id})) |
There was a problem hiding this comment.
Added the Sepolia check.
smohamedjavid
force-pushed
the
feature/blockchain-status
branch
from
313fa66
to
df25759
Compare
Jenkins BuildsClick to see older builds (41)
|
smohamedjavid
commented
Feb 15, 2024
smohamedjavid
commented
Feb 15, 2024
| (if test-networks-enabled? | ||
| mainnet-chain? | ||
| (not mainnet-chain?)))) | ||
| chains-filtered-by-mode (remove filter-fn down-chains) |
There was a problem hiding this comment.
To prevent showing the toast if a testnet chain is down and the user is not in testnet mode.
OmarBasem
reviewed
Feb 16, 2024
src/status_im/constants.cljs
Outdated
Comment on lines
417
to
426
| (def ^:const ethereum-mainnet-chain-id 1) | ||
| (def ^:const ethereum-goerli-chain-id 5) | ||
| (def ^:const ethereum-sepolia-chain-id 11155111) | ||
| (def ^:const arbitrum-mainnet-chain-id 42161) | ||
| (def ^:const arbitrum-goerli-chain-id 421613) | ||
| (def ^:const arbitrum-sepolia-chain-id 421614) | ||
| (def ^:const optimism-mainnet-chain-id 10) | ||
| (def ^:const optimism-goerli-chain-id 420) | ||
| (def ^:const optimism-sepolia-chain-id 11155420) |
There was a problem hiding this comment.
what about calling the test networks ethereum-testnet, optimism-testnet, arbitrum-testnet ?
There was a problem hiding this comment.
I would love to. The reason why we renamed it specifically is that we have two test networks (Goerli and Sepolia) at this moment. In future, One will supersede the old, and another will be in the pipeline for testing/integration (It's a cycle). Additionally, this rename goes well with status-go.
|
Hey @smohamedjavid thank you for PR. Here is the result. Looks like the problem is not provider related Actual result:The routes are not found and the toast is not shown routes.mp4Expected result:The toast is shown if the provider for any of the blockchains is down Logs: |
|
I noticed this issue is reproducible for me only when I am at home wifi. Now, I am in the coworking space, and it is not reproducible for me anymore (now routes are successfully found). I thought it might be a low network issue, but that's strange. I tried to emulate a low network connection using Andoird Studio or mobile network on real device, and the routes were successfully found for me. I will try to reproduce this at home one more time and take the logcat logs |
|
@smohamedjavid just got this issue in the coworking space. Here are the logcat logs: |
|
@VolodLytvynenko - Thank you for debugging the issue. Duly appreciate it! |
smohamedjavid
force-pushed
the
feature/blockchain-status
branch
from
df25759
to
53ed0ba
Compare
J-Son89
approved these changes
Feb 16, 2024
|
@VolodLytvynenko - The fix is on #18879 for grouping any bugs caused by the PR changes (if any). |
smohamedjavid
force-pushed
the
feature/blockchain-status
branch
from
53ed0ba
to
ee5446d
Compare
ulisesmac
approved these changes
Feb 20, 2024
There was a problem hiding this comment.
Hey @smohamedjavid
Sorry for the late review, just added some small comments 👍
OmarBasem
approved these changes
Feb 20, 2024
94% of end-end tests have passedFailed tests (2)Click to expandClass TestCommunityMultipleDeviceMergedTwo:
Class TestCommunityMultipleDeviceMerged:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (45)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
|
|
hey @smohamedjavid could you please rebase and resolve conflicts for this PR? thanx! |
Hey @VolodLytvynenko - If it's not too much trouble, can we deprioritise testing this PR now? We will return to this PR after that as I need to help you in testing as well (creating a temporary status-go to simulate the provider down cases if required). |
Hey @smohamedjavid Sure. Thank you! |
smohamedjavid
force-pushed
the
feature/blockchain-status
branch
from
dea8367
to
e519792
Compare
|
@VolodLytvynenko - This PR is ready for testing |
12% of end-end tests have passedFailed tests (41)Click to expandClass TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityOneDeviceMerged:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePR:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (6)Click to expandClass TestDeepLinksOneDevice:
Class TestCommunityOneDeviceMerged:
|
|
Hey @smohamedjavid, could you please rebase this PR and create a status-go test branch to simulate the chains down? |
smohamedjavid
force-pushed
the
feature/blockchain-status
branch
from
e519792
to
12b3c92
Compare
|
@qoqobolo - In the builds: #18850 (comment) Build #7 is WITHOUT any changes in Build #8 is with changes in Let me know if you need anything. Thanks! |
33% of end-end tests have passedFailed tests (31)Click to expandClass TestDeepLinksOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestCommunityMultipleDeviceMerged:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (16)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
96% of end-end tests have passedFailed tests (1)Click to expandClass TestCommunityMultipleDeviceMerged:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (46)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestActivityMultipleDevicePRTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
Thanks @smohamedjavid! PR looks good to me, but I have a question When I check build #7, where the chains are not supposed to be down, I still see the banner when entering the amount of the asset being sent. 
It seems the banner appears when the user does not have enough ETH on other networks (?) Could you check and confirm if it's not expected for that build, please? For example, this user only has ETH on Arbitrium: 
More examples when user has some ETH on Mainnet, Optimis and Arbitrium: 
|
88% of end-end tests have passedFailed tests (5)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityMultipleDeviceMergedTwo:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (42)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestActivityMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMerged:
|
Checking it with the Desktop team. It should not happen. 🙏 |
smohamedjavid
force-pushed
the
feature/blockchain-status
branch
2 times, most recently
from
eb74a1d
to
b140fd1
Compare
|
@qoqobolo - You are right. It's due to insufficient gas fees (due to no balance) on those chains that make false positive results. There is a fix merged into status-go status-im/status-go@004bd4e by the desktop team. I updated this PR to use that version tag. One known issue in this PR is the Mainnet Sepolia balances are not fetched due to POKT dropping support for it. If that's not a blocker for testing, Please proceed with verifying the fix in this PR. If not, there is a separate PR (#19040) for removing Goerli RPC and I have included the fix for Sepolia as well (including the latest commits from status-go and the existing insufficient gas fee fix). We can wait for that to be merged. My suggestion is to keep this PR on hold until the above mentioned PR (#19040) is merged (to capture and fix any bugs due to status-go changes) and keep this PR from any status-go changes. |
smohamedjavid
force-pushed
the
feature/blockchain-status
branch
from
b140fd1
to
c60a355
Compare
|
@smohamedjavid gotcha, thank you! |
92% of end-end tests have passedFailed tests (3)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Expected to fail tests (1)Click to expandClass TestCommunityOneDeviceMerged:
Passed tests (44)Click to expandClass TestActivityMultipleDevicePR:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePRTwo:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
|
67% of end-end tests have passedFailed tests (1)Click to expandClass TestActivityCenterContactRequestMultipleDevicePR:
Passed tests (2)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
|
@smohamedjavid don't see any problems anymore. |
@qoqobolo - Thank you for testing this PR. This PR (with the latest commit/update - https://github.com/status-im/status-mobile/pull/18850/commits) doesn't update any status-go version. It's all using the existing functionalities/bug fixes from status-go now.
I can push a temp commit to point to the temp status-go branch. But, it will be the same as Build #8. Nothing new to test. We can merge this PR 👍 |
Signed-off-by: Mohamed Javid <19339952+smohamedjavid@users.noreply.github.com>
smohamedjavid
force-pushed
the
feature/blockchain-status
branch
from
c60a355
to
5e60e10
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
fixes #18826
Summary
This PR adds the feature to handle blockchain status signals from the
status-goand shows a toast if the provider for any of the blockchains is down.Additionally, this PR refactors a few areas of the wallet.
Testing notes
To simulate the chains down, we need to create a test branch in status-go with completely wrong network RPC URLs and use it. I will create it when we start testing.
A regression test on the wallet is appreciated as this PR refactors a few areas of the Wallet.
Platforms
Steps to test
status: ready