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

Update flipping logic in SwapInputsController #5948

Merged
merged 9 commits into from
Jul 29, 2024
Merged

Conversation

jinchung
Copy link
Member

@jinchung jinchung commented Jul 23, 2024

Fixes APP-1675
Fixes APP-1699

More comments are inline in the code files changed below.

What changed (plus any additional context for devs)

  • Moved a couple functions to a separate file to make setting up testing easier later
  • Moved the animatedReaction for reacting to asset changes / flips to be done before the animatedReaction that changes the maxSwappableAmount value as this was causing the reliance of "did the user intend to use max" check unreliable
  • Fixes an issue where from wallet list for testmar27.eth: select BOOMER and swap → the output amount is just 0.0 and when you flip, the input amount goes to "7.5e-8" bc of an unnecessary Number conversion

Overall, the flipping logic will try to figure out which values to used in this priority order:

  • base the updated values on the native input amount (requires that there is a valid price for both input and output assets, and that the balance won't exceed the max balance, and that it isn't the user's intent to select the max balance)
  • base the updated values on the current slider position: if we can't do the above, or if the calculated amount would result in an automatic "Insufficient Balance" for the input, or if the user intended to select max, then we use the slider (requires the previous output has a balance)
  • otherwise, we use the previous output amount as the new input amount and vice versa
  • worst case scenario, we just zero out the values

Screen recordings / screenshots

  • Max scenario:
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-07-26.at.00.26.03.mp4
  • Example of when you have enough balance after the flip to keep using the native input or what happens when you don't (and it falls back to using the current slider position):
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-07-26.at.00.29.00.mp4
Simulator.Screen.Recording.-.iPhone.15.Pro.-.2024-07-26.at.00.23.37.mp4

What to test

  • I've added a full breakdown of combinations in the Linear ticket, but generally flipping should "behave as expected" for max as well as non-max
  • when flipping assets, it should avoid getting into a state of "Insufficient Funds" when possible

Copy link

linear bot commented Jul 23, 2024

@jinchung jinchung force-pushed the @jin/flipping-max branch 3 times, most recently from 3d07ca9 to dc8068a Compare July 25, 2024 03:24
@jinchung jinchung marked this pull request as ready for review July 26, 2024 04:27
@@ -640,6 +611,74 @@ export function useSwapInputsController({
}
);

/**
Copy link
Member Author

Choose a reason for hiding this comment

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

NOTE: this is not all brand new logic - this is mostly moving this animatedReaction from line 812 to here so that it's ahead of another animatedReaction that reacts to maxSwappableAmount changes

});
} else {
// If the assets were flipped
updateInputValuesAfterFlip({
Copy link
Member Author

Choose a reason for hiding this comment

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

this is the "main" change for this function for when the assets are flipped - the logic is in the other file

@@ -30,36 +30,7 @@ import { analyticsV2 } from '@/analytics';
import { divWorklet, equalWorklet, greaterThanWorklet, isNumberStringWorklet, mulWorklet } from '@/__swaps__/safe-math/SafeMath';
import { SPRING_CONFIGS } from '@/components/animations/animationConfigs';
import { triggerHapticFeedback } from '@/screens/points/constants';

function getInputValuesForSliderPositionWorklet({
Copy link
Member Author

Choose a reason for hiding this comment

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

I just moved this out to a separate file to make it easier for testing later - no changes

import { ExtendedAnimatedAssetWithColors } from '@/__swaps__/types/assets';
import { divWorklet, equalWorklet, greaterThanWorklet, mulWorklet } from '@/__swaps__/safe-math/SafeMath';

export function getInputValuesForSliderPositionWorklet({
Copy link
Member Author

Choose a reason for hiding this comment

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

this is the function that was just directly moved over from the useSwapInputsController - no changes

};
}

export const updateInputValuesAfterFlip = ({
Copy link
Member Author

Choose a reason for hiding this comment

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

this is the main logic that is new / updated for how to handle input values after a flip

inputNativeValue: mulWorklet(newInputAmount, inputNativePrice),
};
});
return;
Copy link
Member Author

Choose a reason for hiding this comment

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

only for slider do we return early since we do not want to update the outputAmount and outputNativeValue like all the other flows; we just want to modify the inputAmount and inputNativeValue

});
newInputAmount = inputAmountBasedOnSlider;
inputMethod.value = 'slider';
lastTypedInput.value = 'inputAmount';
Copy link
Member Author

Choose a reason for hiding this comment

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

only for the slider do we need to update the lastTypedInput and focusedInput back to be input based

@jinchung jinchung requested a review from walmat July 26, 2024 04:48
@jinchung jinchung changed the title Update to flipping logic in SwapInputsController Update flipping logic in SwapInputsController Jul 26, 2024
Copy link
Contributor

@walmat walmat left a comment

Choose a reason for hiding this comment

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

🔥

Copy link

linear bot commented Jul 29, 2024

@jinchung jinchung merged commit 09bafbc into develop Jul 29, 2024
6 checks passed
@jinchung jinchung deleted the @jin/flipping-max branch July 29, 2024 17:39
BrodyHughes added a commit that referenced this pull request Aug 1, 2024
…inbow into brody/enable-android-tests

* 'brody/enable-android-tests' of github.com:rainbow-me/rainbow:
  degen mode feature flag (#5969)
  Fix Android dapp browser gestures (#5964)
  Add Degen Mode, swap settings panel (#5963)
  Update flipping logic in SwapInputsController (#5948)
  Hold to swap button (#5920)
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.

2 participants