Skip to content
This repository was archived by the owner on Jun 24, 2022. It is now read-only.

Fix inverted invested amounts #2426

Merged
merged 4 commits into from
Feb 16, 2022
Merged

Conversation

alfetopito
Copy link
Contributor

@alfetopito alfetopito commented Feb 15, 2022

Summary

Closes #2423

Fixing issue that under some circumstances will result in the investment amounts being inverted.

Keep in mind this only happens when there are at least 2 investment options selected.

To Test

  1. With any account that has 2 or more investment options, load it in the claim ui
  2. When selecting the claims, start ticking the boxes from bottom up of at least 2 claims
  3. On investment screen, opt to invest max amount on all
  4. Proceed to review screen
  • Amounts selected should match the previous screen

If you test the same thing on Prod or anywhere else that doesn't have this fix, it'll have the amounts inverted, like in this video

Screen.Recording.2022-02-15.at.13.40.58.mov

@github-actions
Copy link
Contributor

  • 🔭 GP Swap: CoW Protocol v2 Swap UI

@alfetopito alfetopito changed the base branch from master to hotfix/1.10.2 February 15, 2022 21:32
@alfetopito alfetopito self-assigned this Feb 15, 2022
@alfetopito alfetopito requested review from a team February 15, 2022 21:42
@alfetopito alfetopito marked this pull request as ready for review February 15, 2022 21:42
@alfetopito alfetopito changed the title Using latest contract and deployment info for rinkeby Fix inverted invested amounts Feb 15, 2022
Copy link
Contributor

@W3stside W3stside left a comment

Choose a reason for hiding this comment

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

Nice find

Copy link
Contributor

@anxolin anxolin left a comment

Choose a reason for hiding this comment

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

So at the end it was that confusion of using the index of the array instead of the one from the claiming. Not sure if it was conscious decision, but it is a bad idea to rely on the order of the arrays

Nice 🕵️‍♀️ job Leandro!

pls, don't forget to revert the branch name before merging

Copy link

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

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

LGTM!

@alfetopito alfetopito force-pushed the 2423/flipped-invested-amounts branch from 33f9cc6 to 8c65bc8 Compare February 16, 2022 16:24
@alfetopito alfetopito merged commit e9d8d76 into hotfix/1.10.2 Feb 16, 2022
@alfetopito alfetopito deleted the 2423/flipped-invested-amounts branch February 16, 2022 16:50
ramirotw pushed a commit that referenced this pull request Feb 18, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants