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

fix(wallet): fix bridge transactions #20902

Merged
merged 2 commits into from
Jul 30, 2024
Merged

Conversation

briansztamfater
Copy link
Member

@briansztamfater briansztamfater commented Jul 26, 2024

fixes #20900
fixes #20534
fixes #20850

Summary

This PR fixes bridge transactions and UI fix on transaction confirmation page for bridge transactions as well. Also minor fix on navigation on bridging flow (dynamic set start-flow? param to wizard depending on where it is called).

Platforms

  • Android
  • iOS

Areas that maybe impacted

Functional
  • wallet / transactions

Steps to test

  • Open Status
  • Go to bridge flow
  • Enter a valid amount
  • Go to transaction confirmation page
  • Verify UI is not broken as in the screenshot of the linked issue
  • Slide to confirm transaction
  • Verify transaction is executed correctly

status: ready

Risk

Described potential risks and worst case scenarios.

Tick one:

  • Low risk: 2 devs MUST perform testing as specified above and attach their results as comments to this PR before merging.
  • High risk: QA team MUST perform additional testing in the specified affected areas before merging.

@status-im-auto
Copy link
Member

status-im-auto commented Jul 26, 2024

Jenkins Builds

Click to see older builds (12)
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ d8bdbd4 #1 2024-07-26 19:19:03 ~4 min tests 📄log
✔️ d8bdbd4 #1 2024-07-26 19:21:19 ~6 min android-e2e 🤖apk 📲
✔️ d8bdbd4 #1 2024-07-26 19:21:44 ~7 min android 🤖apk 📲
✔️ d8bdbd4 #1 2024-07-26 19:27:14 ~12 min ios 📱ipa 📲
✔️ b69a6a1 #4 2024-07-26 23:06:46 ~4 min tests 📄log
✔️ b69a6a1 #4 2024-07-26 23:09:45 ~7 min android-e2e 🤖apk 📲
✔️ b69a6a1 #4 2024-07-26 23:10:11 ~7 min android 🤖apk 📲
✔️ b69a6a1 #4 2024-07-26 23:12:00 ~9 min ios 📱ipa 📲
✔️ c86d25f #5 2024-07-29 10:08:23 ~5 min tests 📄log
✔️ c86d25f #5 2024-07-29 10:09:18 ~6 min android-e2e 🤖apk 📲
✔️ c86d25f #5 2024-07-29 10:11:58 ~8 min ios 📱ipa 📲
✔️ c86d25f #5 2024-07-29 10:12:05 ~8 min android 🤖apk 📲
Commit #️⃣ Finished (UTC) Duration Platform Result
✔️ 4510a80 #7 2024-07-29 19:23:06 ~3 min tests 📄log
✔️ 4510a80 #7 2024-07-29 19:26:04 ~7 min android-e2e 🤖apk 📲
✔️ 4510a80 #7 2024-07-29 19:26:46 ~7 min android 🤖apk 📲
✔️ 4510a80 #7 2024-07-29 19:27:45 ~8 min ios 📱ipa 📲
✔️ e9fa40f #8 2024-07-30 13:49:52 ~4 min tests 📄log
✔️ e9fa40f #8 2024-07-30 13:52:08 ~6 min android-e2e 🤖apk 📲
✔️ e9fa40f #8 2024-07-30 13:53:58 ~8 min android 🤖apk 📲
✔️ e9fa40f #8 2024-07-30 13:54:27 ~8 min ios 📱ipa 📲

@briansztamfater briansztamfater force-pushed the fix/bridge-tx-not-working branch 2 times, most recently from f1cba9f to 3c915fe Compare July 26, 2024 22:59
@briansztamfater briansztamfater marked this pull request as ready for review July 26, 2024 23:00
@J-Son89
Copy link
Contributor

J-Son89 commented Jul 27, 2024

Awesome work @briansztamfater! I tested this out.

I could bridge Ether from Mainnet to Optimism
I also tried Mainnet to Arbitrum, first time I confirmed it showed some error in the toast.
I tried again and it worked then too 🙌

@J-Son89
Copy link
Contributor

J-Son89 commented Jul 29, 2024

@briansztamfater - I updated branch in UI so QA have recent changes to block some assets from being bridged. Hope that's all good 👍
cc @status-im/mobile-qa

@VolodLytvynenko VolodLytvynenko self-assigned this Jul 29, 2024
@status-im-auto
Copy link
Member

75% of end-end tests have passed

Total executed tests: 8
Failed tests: 1
Expected to fail tests: 1
Passed tests: 6
IDs of failed tests: 702843 
IDs of expected to fail tests: 727232 

Failed tests (1)

Click to expand
  • Rerun failed tests

  • Class TestCommunityMultipleDeviceMerged:

    1. test_community_message_edit, id: 702843

    Device 2: Looking for a message by text: Message AFTER edit 2 (Edited)
    Device 2: Find `ChatElementByText` by `xpath`: `//*[starts-with(@text,'Message AFTER edit 2 (Edited)')]/ancestor::android.view.ViewGroup[@content-desc='chat-item']`

    critical/chats/test_public_chat_browsing.py:379: in test_community_message_edit
        self.channel_2.set_reaction(message_text_after_edit)
    ../views/chat_view.py:1054: in set_reaction
        self.chat_element_by_text(message).long_press_until_element_is_shown(element)
    ../views/base_element.py:327: in long_press_until_element_is_shown
        element = self.find_element()
    ../views/chat_view.py:116: in find_element
        self.wait_for_visibility_of_element(20)
    ../views/base_element.py:147: in wait_for_visibility_of_element
        raise TimeoutException(
     Device 2: ChatElementByText by xpath:`//*[starts-with(@text,'Message AFTER edit 2 (Edited)')]/ancestor::android.view.ViewGroup[@content-desc='chat-item']` is not found on the screen after wait_for_visibility_of_element
    



    Device sessions

    Expected to fail tests (1)

    Click to expand

    Class TestWalletOneDevice:

    1. test_wallet_add_remove_watch_only_account, id: 727232

    Device 1: Find EditBox by accessibility id: add-address-to-watch
    Device 1: Type 0x8d2413447ff297d30bdc475f6d5cb00254685aae to EditBox

    critical/test_wallet.py:249: in test_wallet_add_remove_watch_only_account
        self.wallet_view.add_watch_only_account(address=address_to_watch, account_name=new_account_name)
    ../views/wallet_view.py:163: in add_watch_only_account
        self.account_has_activity_label.wait_for_visibility_of_element()
    ../views/base_element.py:147: in wait_for_visibility_of_element
        raise TimeoutException(
     Device 1: Text by accessibility id:`account-has-activity` is not found on the screen after wait_for_visibility_of_element 
    

    [[Missing networks in account address, https://github.com//issues/20166]]

    Device sessions

    Passed tests (6)

    Click to expand

    Class TestWalletMultipleDevice:

    1. test_wallet_send_asset_from_drawer, id: 727230
    2. test_wallet_send_eth, id: 727229

    Class TestCommunityOneDeviceMerged:

    1. test_community_copy_and_paste_message_in_chat_input, id: 702742
    Device sessions

    2. test_restore_multiaccount_with_waku_backup_remove_switch, id: 703133
    Device sessions

    Class TestWalletOneDevice:

    1. test_wallet_add_remove_regular_account, id: 727231
    Device sessions

    Class TestOneToOneChatMultipleSharedDevicesNewUi:

    1. test_1_1_chat_non_latin_messages_stack_update_profile_photo, id: 702745
    Device sessions

    @churik
    Copy link
    Member

    churik commented Jul 29, 2024

    @briansztamfater
    Found a bug at an attempt to bridge 0.1 USD from Arbiscan to Optimism

    Consequences:

    1. was not redirected to activity page
    2. tx is failed and reverted with "Fail with error 'ERC20: transfer amount exceeds allowance", example https://arbiscan.io/tx/0xa834e4772e11daf3cfe0a9afba521e7fb0c4c2280bca6483fa27bbe99c99a5f2
    3. Wallet freeze at update and eventually shows 0
      Re-login helps.

    We need to find out what is this error about (I believe some tx limits in Hop might not be met) and prevent users from making faulty txs.

    Video:
    https://github.com/user-attachments/assets/953e125f-2fe4-4487-a20b-52bdc8eb04e9

    Logs:

    logs (103).zip

    What I get after pull-to-refresh:
    IMAGE 2024-07-29 13:49:35

    Update: reproduced with USDc (any amount) at attempt to bridge to Optimizm

    @churik
    Copy link
    Member

    churik commented Jul 29, 2024

    reported separately, other than that PR is ready.

    @Cuteivist
    Copy link

    Cuteivist commented Jul 29, 2024

    @briansztamfater @OmarBasem Looks like it might fix #20850 (multi-tx-type has value 7 instead of 0 when payload is sent to status-go)

    @churik
    Copy link
    Member

    churik commented Jul 29, 2024

    @briansztamfater based on discussion I think it should be fixed inside PR

    @churik churik self-assigned this Jul 29, 2024
    @briansztamfater briansztamfater force-pushed the fix/bridge-tx-not-working branch 2 times, most recently from a321b6d to 4510a80 Compare July 29, 2024 19:18
    @briansztamfater
    Copy link
    Member Author

    @briansztamfater based on discussion I think it should be fixed inside PR

    Thanks @churik @VolodLytvynenko for testing. Support for approval transactions has been added in this PR here 4510a80

    Another thing to note: even if we approve the token amount before executing the bridge transaction, bridge transactions can still fail if bonderFees are higher than the bridging amount. Check this transaction to see the error: https://arbiscan.io/tx/0x75cffa208239be4706a26a4c7b0b728ac18e48969ddc107d8d1577ed398f4f86
    I think we can add this check in the clients to prevent failing transactions, IMO we can implement that in a follow up PR for mobile in #20914.

    Signed-off-by: Brian Sztamfater <brian@status.im>
    Signed-off-by: Brian Sztamfater <brian@status.im>
    @ilmotta ilmotta merged commit 59ceddb into develop Jul 30, 2024
    5 checks passed
    @ilmotta ilmotta deleted the fix/bridge-tx-not-working branch July 30, 2024 14:02
    ilmotta pushed a commit that referenced this pull request Jul 30, 2024
    * fix(wallet): fix bridge transactions
    
    Signed-off-by: Brian Sztamfater <brian@status.im>
    
    * add support for approve transactions
    
    Signed-off-by: Brian Sztamfater <brian@status.im>
    
    ---------
    
    Signed-off-by: Brian Sztamfater <brian@status.im>
    ilmotta added a commit that referenced this pull request Jul 30, 2024
    Revisions from develop:
    
    - 59ceddb develop origin/develop fix(wallet): fix bridge transactions (#20902)
    - 99ccbc3 Cover wallet send events with tests Part 2 #20411 #20533 (#20721)
    - 8c2d539 Enabling WalletConnect feature flag (#20906)
    - 67c83b1 fix(wallet): remove edit routes button in bridging (#20874)
    - 11a84ba feat(wallet): disable complex routing (#20901)
    - 1f5bb57 chore(wallet): disable bridging on unsupported tokens (#20846)
    - 4586f80 Add toggle in advanced settings for mobile data
    - 55c620e fix: create password for small screen (#20645)
    - 525609f Wallet Activity: transactions are not sorted by time #20808 (#20862)
    - 9065395 chore(settings): Disable telemetry option (#20881)
    - d27ab75 fix_:display group message using the new ui (#20787)
    - c6a1db6 ci: enable split apks & build only for arm64-v8a (#20683)
    - 73777e0 Ensure keycard account can send transaction after upgrading from v1 to v2 #20552 (#20845)
    - a6d3fc3 [#20524] fix: the missed keypairs are shown in the key pair list screen (#20888)
    - a671c70 fix broken screen and navigation when syncing fails (#20887)
    - a45991b 🥅 Filter connected dapps based on testnet mode, reject proposals and requests gracefully (#20799)
    - 2e9fa22 feat: wallet router v2 (#20631)
    - 737d8c4 rename sub to fix error when requesting to join community (#20868)
    - 3aa7e10 Sync process is blocked on Enabled notifications screen (#20883)
    - c1d2d44 perf: Fix app freeze after login (#20729)
    - 0fed811 e2e: updated testnet switching and added one test into smoke
    - 53c35cb fix(wallet): Linear gradient exception on invalid colors for watched account cards (#20854)
    - be82365 chore(settings)_: Remove testnet toggle from legacy advanced settings (#20875)
    - eae8a65 feat(wallet)_: Add beta info box in activity tab (#20873)
    - fe54a25 fix: not clearing network & web3-wallet on logout (#20886)
    - 15a4219 Reject wallet-connect request by dragging the modal down (#20763) (#20836)
    - 2ffbdac WalletConnect show expired toast (#20857)
    - 402eb83 fix Issue with scrolling WalletConnect transaction on Android (#20867)
    - ff88049 Fix WalletConnect header alignment on Android (#20860)
    - cee2124 WalletConnect no internet edge-cases (#20826)
    - 60ad7c8 chore(tests): New match-strict? cljs.test directive (#20825)
    - 4989c92 fix_: Adding own address as saved addresses (#20839)
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    Archived in project
    9 participants