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: dangerous small amount warning #6044

Merged
merged 2 commits into from
Jan 18, 2024
Merged

Conversation

gomesalexandre
Copy link
Contributor

@gomesalexandre gomesalexandre commented Jan 18, 2024

Description

Fixes a bug introduced in https://github.com/shapeshift/web/pull/5971/files#diff-b6f4d6672824399e71740531e3b09efe3ad3fabeb3ad6c4ce0763969f9d39ab7R104, where we try and select selectSellAmountCryptoBaseUnit(state), which yields undefined because firstHop is undefined at that time:

image image

Fixed by

  1. using the correct local quote and running the same selector logic against it, instead of introspecting the not-yet-defined hop state
  2. as a paranoia check, not assuming dangerous amount if !sellAmountCryptoBaseUnit which should not happen anymore, but in case it does, we are safu.

Pull Request Type

  • 🐛 Bug fix (Non-breaking Change: Fixes an issue)
  • 🛠️ Chore (Non-breaking Change: Doc updates, pkg upgrades, typos, etc..)
  • 💅 New Feature (Breaking/Non-breaking Change)

Issue (if applicable)

closes #6037

Risk

This isn't high risk - while it does touch quote validation, it is moderate to low risk as we had all the logic baked in already for a long time and stable, but were reacting on the wrong value since #5971 refactor

High Risk PRs Require 2 approvals

Testing

  • Dangerous sell amount warning is still displayed for small amounts
  • Dangerous sell amount warning isn't displayed when the amount is greater than or equal to the minimum THOR-recommended amount

Engineering

☝🏽

Operations

  • ☝🏽

Screenshots (if applicable)

  • Amount smaller than the recommended minimum
Screenshot 2024-01-18 at 12 06 08 Screenshot 2024-01-18 at 12 06 15
  • Amount equal to the recommended minimum
Screenshot 2024-01-18 at 12 06 41 Screenshot 2024-01-18 at 12 06 49

Copy link
Contributor Author

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@gomesalexandre gomesalexandre marked this pull request as ready for review January 18, 2024 11:40
@gomesalexandre gomesalexandre requested a review from a team as a code owner January 18, 2024 11:40
@gomesalexandre gomesalexandre enabled auto-merge (squash) January 18, 2024 11:51
@gomesalexandre gomesalexandre merged commit 5d4208a into develop Jan 18, 2024
7 checks passed
@gomesalexandre gomesalexandre deleted the fix_min_sell_amount branch January 18, 2024 11:52
@0xean 0xean mentioned this pull request Jan 18, 2024
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.

Below minimum warning triggering on trade values above the minimum warning
2 participants