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 / Significant Balance Decrease Banner #1303

Merged
merged 7 commits into from
Feb 18, 2025

Conversation

jordan-enev
Copy link
Member

@jordan-enev jordan-enev commented Feb 11, 2025

Closes https://github.com/AmbireTech/ambire-app/issues/3821.

The problem:

When we perform a SWAP (or any other activity that results in a new portfolio token), the BalanceDecreaseBanner was shown if the output token was new to the portfolio. This happened because the newly acquired token hadn't yet been added to the portfolio through our learning/discovery process.

To fix this, we introduced a new flag called traceCallDiscoveryStatus, which tracks whether the discovery logic has been completed.

How we fixed it:

  1. On sign an acc op initialization or when a new transaction is added to an existing acc op (which wasn't handled before this PR), we call main.traceCall.
  2. If traceCall completes successfully within 2 seconds and there is a significant balance decrease, we display the regular banner. (Screenshot attached)
  3. If traceCall takes more than 2 seconds to respond or fails, we display a similar banner indicating that the balance may decrease, but we can't be certain due to temporary issues with the token discovery logic. (Screenshot attached)
  4. If traceCall is slow to respond, the second banner will be shown initially, and once resolved, the main banner will be displayed.

Significant Account Balance Decrease (traceCall completes successfully)
image

Significant Account Balance Decrease (Possibly Inaccurate) (traceCall fails or it's slow)
image

…esponse from debug_traceCall (token discovery logic). If debug_traceCall fails or its response is delayed, we show `PossibleBalanceDecrease` banner, in place of `SignificantBalanceDecrease`.
… a new request is added to an existing AccOp. Previously, we invoked it only on initialization at the frontend level. However, for earlier invocation and improved reliability, we will now invoke it as early as possible in the background - both on initialization and when a new request is added.
Copy link
Member

@PetromirDev PetromirDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good job. I have some minor comments

src/controllers/main/main.ts Outdated Show resolved Hide resolved
src/interfaces/signAccountOp.ts Outdated Show resolved Hide resolved
src/controllers/main/main.ts Outdated Show resolved Hide resolved
@Ivshti
Copy link
Member

Ivshti commented Feb 12, 2025

@jordan-enev can we please increase the 2 secs to 5 secs

@PetromirDev
Copy link
Member

@Ivshti @jordan-enev In my opinion, if we decide to increase the timeout there should be some sort of loading state or warning that we are not sure if the user's balance will decrease substantially. I'm not a degen by any means and don't operate with large amounts but I don't take too much time on the sign account op screen- it loads, I scan everything, select a fee speed and sign, which may take less than 5 seconds.

This problem is still present but not that prominent with a 2-second timeout.

@Ivshti
Copy link
Member

Ivshti commented Feb 12, 2025

ok @PetromirDev then I agree with two secs

@jordan-enev jordan-enev merged commit f1e885b into v2 Feb 18, 2025
1 check failed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants