Testing different go changes required for release 2.31 #21445
Closed
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
Jenkins BuildsClick to see older builds (4)
|
45% of end-end tests have passedFailed tests (30)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestActivityMultipleDevicePR:
Class TestDeepLinksOneDevice:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePRTwo:
Class TestWalletOneDevice:
Class TestCommunityMultipleDeviceMerged:
Passed tests (25)Click to expandClass TestDeepLinksOneDevice:
Class TestGroupChatMultipleDeviceMergedNewUI:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestWalletMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestFallbackMultipleDevice:
Class TestCommunityOneDeviceMerged:
|
|
I'm not sure if this is related to the current PR. I've never swapped SNT on L1; I usually swap tokens with lower fees like DAI, USDC, or ETH on this network. Should it be fixed within the scope of this PR? ISSUE 1: "Spending cap failing" Error is shown when attempting to swap SNTSteps:
Actual Result:"Spending cap failing" Error is shown when attempting to swap SNT snt_swap.mp4Expected Result:The approval should be successful Devices:Pixel 7a, Android 13 Logs: |
pavloburykh
changed the title
55% of end-end tests have passedFailed tests (25)Click to expandClass TestCommunityOneDeviceMerged:
Class TestCommunityMultipleDeviceMerged:
Class TestActivityMultipleDevicePRTwo:
Class TestWalletOneDevice:
Class TestActivityCenterContactRequestMultipleDevicePR:
Class TestCommunityMultipleDeviceMergedTwo:
Class TestActivityMultipleDevicePR:
Class TestDeepLinksOneDevice:
Passed tests (30)Click to expandClass TestWalletMultipleDevice:
Class TestCommunityOneDeviceMerged:
Class TestDeepLinksOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletOneDevice:
Class TestFallbackMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUiTwo:
Class TestGroupChatMultipleDeviceMergedNewUI:
|
|
So let me check what we should expect from these PRs all together:
Overall we consider PR as succesfull:
If someone can help us to analyze logs from kibana or use any other tools to identify how it really works with 3d party services, please, let us know. As now it seems we're just trying to guess whether it works or not. |
|
Yeah sounds correct, RPS limiter removal might help with the circuit breaker problem. And Goerli fix doesn't affect non-testnet.
After changing provider order, if we hit grove, then we should check the logs and see why nodefleet and infura are failing. Please ping me if you need help analysing logs (nginx and status-go) |
|
This issue #21449 is still reproducible in this PR. So we still facing problem when balances are not fetched on certain network. This time it is Arbitrum. Recently we encountered same issue with Optimism. Here are the log from this PR |
Thanks for the logs here is a error log. It could be a combination of client logic bugs: We try to parse prices from a response with error The code is probably called by FetchPrices in market.go: and the second error is related to It's from cryptocompare/client.go FetchPrices The proxy seems working fine: |
|
From the attached logs in this comment #21445 (comment) I see that 2 approvals failed, but the both txs have empty data and doesn't call approve function:
In theory, yes, but as I mentioned in Discord, can be also up to wrongly configured/used hystrix. We need to inspect that.
You can check that in the log, for error that you're getting and see the order of errors. So error in the log should have firstly error from |
|
hi @saledjenic I checked this issue on desktop as well using today's latest master and was able to approve SNT once, but I couldn't approve it a second and more time. Below are the steps I followed Preconditions:Current User has:
Steps:
Actual result:
Expected result:Users can click approve and navigate to approved page Logs:Desktop: geth copy.log |
|
@saledjenic Just FYI. It's very possible that this issue related to the current PR. I successfully approved SNT (this works only if the user does not have approved SNT and this approval is the first) on the latest nightly build on both Desktop and Mobile, but when I tried it again in the build of the current PR, I encountered the same issue again. |
|
this issueis present on the latest master as well. I just tracked it separately |
|
@VolodLytvynenko the log is useless. :( The only I could see there is this error
If it passed once, means the code is good. Also if new approval has the same or lower value than previously approved, then you won't be ask to approve again, but if the value is higher then you should approve that higher value and that's a new tx, and has nothing with previously approved value. Also since the new value is higher than previous, but still less than the total balance, there is no reason for getting an error. I can only blame providers for that. |
|
@saledjenic what can we do to make log not useless? do we have any ideas? @friofry new health monitoring doesn't give us more clarity? because seems we're blind now - and might be |
|
I mean based on #21424 (comment) - it is Grove in Swap. Here we changed the order, why it is still Grove then? |
Hi, @saledjenic @churik perhaps this could be the solution to the issue. When I try to approve ERC-20 assets on other apps (like Uniswap, MetaMask), they often set the approval to "unlimited," meaning you don’t need to approve the same asset again. This could prevent the error on the second approval on our side. However, I’m not sure if it’s technically possible for us to implement unlimited approval on our side. WDYT?
|
|
@saledjenic can you please point us to the same PR in desktop in order to verify? Fo now:
So perhaps we're checking the wrong places, but to my understanding so far we're not able to approve SNT once more (may be not only SNT actually) if the approve value is bigger than the previous one. |
|
Here's a quick summary of the swap feature for the current PR:
Mobile logs:
Desktop logs: |
|
just added logs for both issues in comment above |
I guess yes (we can do that only if user is about to swap SNT).
It can be with lower priority, but we need to keep that in mind and don't log issues for that.
Not exactly like that, the issue is happening if the user already has approval for swaping X SNT, and wants to swap Y now, where X<Y. Now based on that usecaed we should re-think what to do, but setting unlimited approval for SNT only is for sure one of solutions.
As said in comment in that issue, I don't have other explanation atm except that's happening due to some chain issues, cause checking that the tx is packed and sent well, so there's nothing else specially that we can do after that. One possibility could be that gas goes too high in meantime, cause bridging usually takes a bit longer than a normal tx.
If you don't see it, then it's good. :) It means that the changes work. |
|
issue 1 is fixed after latest commit. @dlipicar thank you! |
|
so what we have now:
Should we report it for mobile team only?
Should we consider then to make unlimited approve like Uniswap does(and basically a lot of other DeFi that I saw)? That will resolve the issue by itself.
Can be deprioritized from 2.31 as again can be network or provider related As for functionality:
ConclusionI don't see any blockers for now related to status-go itself. also it seems useless to try to check some funtionalities while one of the chain is out of sync. |
If you don't see it, then it's good. :) It means that changes work. |
|
@churik Checked So, the mobile client seems to be calling |
we generate symbols with this function: I'll try to quickly prepare a fix |
|
This has been tested from deskop app end and looks good. |
Hey @dlipicar @friofry Could you please help to understand how does this issue affect app functionality/UI? Asking, cause it seems for me little bit risky to implement the last minute fixes (before release cut) which we do not know how to test properly and what potential regression we can get. I would prefer to implement those fixes on mobile side after we cut the release branch unless you are sure it is safe to include into upcoming release. Please share your thoughts on that. Thank you! |
@pavloburykh I really don't know... I can only say there must be some place where you're expecting a token price or market data and you're not getting it. There could be other parts of the app that expect a "Symbol" and are getting a "ChainID-Symbol". |
The intention of #21453 was to highlight my findings. I have asked @alwx and @briansztamfater to review this idea. |
|
@churik I've just updated this comment #21445 (comment) please check it again, cause I see that from what I've wrote yesterday just a single thing was saved. |
This was referenced Oct 18, 2024
UPDATE after re-testing today:
|
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.
PR is created to test
806b2587dbbc6730129bab84689559895c3dcaf2commit which includes following changes:status-im/status-go#5950
status-im/status-go#5924
status-im/status-go#5941
status-im/status-go#5945
-o-
Bumped to status-go commit
5a19cc2a6ebc23d5dd8fcac2530a2897b0443e42to include status-im/status-go#5957 as well