improve ethereum.send #13724
improve ethereum.send #13724
Conversation
Jenkins BuildsClick to see older builds (6)
|
98% of end-end tests have passedFailed tests (2)Click to expandClass TestSendTxDeviceMerged:
Class TestKeycardTxOneDeviceMerged:
Passed tests (85)Click to expandClass TestCommandsMultipleDevicesMerged:
Class TestEnsStickersMultipleDevicesMerged:
Class TestRestoreOneDeviceMerged:
Class TestOnboardingOneDeviceMerged:
Class TestPublicChatBrowserOneDeviceMerged:
Class TestKeycardTxOneDeviceMerged:
Class TestContactBlockMigrateKeycardMultipleSharedDevices:
Class TestGroupChatMultipleDeviceMerged:
Class TestPublicChatMultipleDeviceMerged:
Class TestSendTxDeviceMerged:
Class TestWalletManagementDeviceMerged:
Class TestPairingSyncMultipleDevicesMerged:
Class TestOneToOneChatMultipleSharedDevices:
|
0% of end-end tests have passedFailed tests (2)Click to expandClass TestSendTxDeviceMerged:
Class TestKeycardTxOneDeviceMerged:
|
|
@qfrank thank you for your contribution. Got a couple of e2e that are failing due to PR changes, please take a look at the ISSUE 1 ISSUE 1 Sign message method does not result in appearing signing pop up in simpldappDescription: for purposes of testing different transaction methods we are using test app https://simpledapp.eth.link/ (this link should be used for Goerli network). Here is also a link to app repo https://github.com/status-im/simple-dapp In a few words, this app gives us a possibility to call different methods via it's interface. So one of such methods which is being tested by these failed e2e is "Sign message". Calling "Sign message" method should result in opening signing pop up which doesn't happen in current PR builds. Steps:
Actual result: signing pop up does not appear. telegram-cloud-document-2-5404564662333216837.mp4Expected result: signing pop up appears. telegram-cloud-document-2-5404564662333216838.mp4I am not sure if this can be considered a bug or our test simpldapp just needs some changes in terms of this PR improvements. Maybe @flexsurfer can assist on this question. |
Hi @pavloburykh , thanks for reporting, i tried remote branch |
|
hi @pavloburykh , pls try the latest PR, thanks! |
|
@qfrank thanx for fixing ISSUE 1. Please, take a look at a new ISSUE 2 ISSUE 2 Sign pop ups stopped appearing in some other appsAs an example of such apps I tested crytokitties.co and opensea.io Steps (cryptokitties.co):
Actual result: Sign pop up does not appear. It is impossible to perform purchases. Tapping on Sign up button does not result in appearing sign up modal (See video below) telegram-cloud-document-2-5406910603535063974.mp4Expected result: after connecting wallet - sign up modal should appear. User fills up and send sign up form (with email/nickname). After sending registration form the sign pop up appears and user is able to confirm wallet connect action (see video below showing expected behaviour). telegram-cloud-document-2-5406910603535063964.mp4Similar issue happens to the opensea.io app. telegram-cloud-document-2-5406910603535063986.mp4Expected result: telegram-cloud-document-2-5406910603535063989.mp4Guys @qfrank @flexsurfer , overall I'd like to discuss if we really need to implement this improvements, because I have some doubts and would like know your opinion. My doubts are based on following points:
So how do you think, are current PR's improvements cover potential risks of regression that can be introduced by it? |
|
i'm always against adding "hacks" in our code for specific dapps, we should drop |
100% of end-end tests have passedPassed tests (87)Click to expandClass TestPublicChatBrowserOneDeviceMerged:
Class TestOnboardingOneDeviceMerged:
Class TestWalletManagementDeviceMerged:
Class TestPublicChatMultipleDeviceMerged:
Class TestCommandsMultipleDevicesMerged:
Class TestPairingSyncMultipleDevicesMerged:
Class TestSendTxDeviceMerged:
Class TestKeycardTxOneDeviceMerged:
Class TestEnsStickersMultipleDevicesMerged:
Class TestContactBlockMigrateKeycardMultipleSharedDevices:
Class TestGroupChatMultipleDeviceMerged:
Class TestOneToOneChatMultipleSharedDevices:
Class TestRestoreOneDeviceMerged:
|
|
i prefer continue improve ... i admit current PR is not good enough, but i promise will change it to better and better, i see the old implementation of |
|
@pavloburykh issue 2 fixed, i guess |
100% of end-end tests have passedPassed tests (87)Click to expandClass TestRestoreOneDeviceMerged:
Class TestContactBlockMigrateKeycardMultipleSharedDevices:
Class TestOneToOneChatMultipleSharedDevices:
Class TestEnsStickersMultipleDevicesMerged:
Class TestPairingSyncMultipleDevicesMerged:
Class TestWalletManagementDeviceMerged:
Class TestGroupChatMultipleDeviceMerged:
Class TestCommandsMultipleDevicesMerged:
Class TestSendTxDeviceMerged:
Class TestKeycardTxOneDeviceMerged:
Class TestPublicChatBrowserOneDeviceMerged:
Class TestPublicChatMultipleDeviceMerged:
Class TestOnboardingOneDeviceMerged:
|
|
https://twitter.com/vitalikbuterin/status/1531771109602799617 |
|
@qfrank ISSUE 2 fixed, thanks. At this moment I haven't found other issues. As I have mentioned, there is no guarantee that we will not have regression in some other apps. So its up to you guys @flexsurfer @qfrank to decide wether to merge this PR or not. |
|
|
agree |
|
@pavloburykh could we check most popular dapps? just make sure they work? without sending transactions |
sure, I will check some more top rated dapps and give feedback |
so, some of apps that have been checked are: Opensea, Cryptokitties, 1inch.exchange, Kyberwap, Zerion, Uniswap etc. I didn't send any trans, just went until signing step. Overall I didn't notice any issues with those apps that I checked. |
|
thanks |
fixes #13121