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

feat: use WarningAcknowledgement component #6647

Merged
merged 58 commits into from
Apr 16, 2024
Merged

feat: use WarningAcknowledgement component #6647

merged 58 commits into from
Apr 16, 2024

Conversation

0xApotheosis
Copy link
Member

@0xApotheosis 0xApotheosis commented Apr 8, 2024

Notes for reviewer: use "Hide whitespace" when viewing the diff, and ignore the insane commit history (it was started on the old develop branch before I fixed it).

Description

Adds a new, generic, WarningAcknowledgement component that can be used across LP, Swapper, and Saver domains to provide a consistent user experience for acknowledging risks associated with a transaction.

Wires up this component for the following features/flows:

  • Lending borrow on unsafe quote (high slippage)
  • Trade quote input on unsafe quote (wired up, though I don't think we have any unsafe quote logic upstream, so this won't show for now)
  • Trade quote confirm on unsafe price impact/high slippage
  • LP deposits

Still to do (follow-up PR):

  • Savers withdraw (fees higher than withdrawal amount)
  • LP pool halted
  • Multi-hop trade, may end up with different asset if something goes wrong
  • LiFi single-hop trades with intermediary assets

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)

Mostly addresses #6605, follow up PR for remaining places to come.

Risk

High. Whilst this PR should only add an additional step in unsafe transaction situations, getting it wrong could mean we break the flow entirely for all of our key revenue generating flows.

What protocols, transaction types or contract interactions might be affected by this PR?

  • Swaps
  • LP deposits
  • Savers borrow

Testing

Savers borrow

Use frame to quote a borrow that will result in an unsafe amount of slippage. Upon confirmation of the quote a warning should show.

Not able to test or get a screenshot at time or writing, as borrow is currently halted (cap full).

Monkey patched shot:

Screenshot 2024-04-16 at 2 46 57 PM

Trade quote input

Get a THORChain quote from ETH to BTC, enter about 0.05 ETH and the an unsafe warning should show on preview.

Screenshot 2024-04-16 at 2 30 35 PM Screenshot 2024-04-16 at 2 30 19 PM

Trade quote confirm

Use frame to confirm a trade that has an unsafe amount of slippage. Upon confirmation of the trade a warning should show.

Screenshot 2024-04-16 at 2 42 40 PM Screenshot 2024-04-16 at 2 44 42 PM

LP deposit

Use frame to get a quote for a large deposit that cause slippage. Upon confirmation a deposit warning should show.

Screenshot 2024-04-16 at 2 46 02 PM Screenshot 2024-04-16 at 2 46 07 PM

Engineering

☝️

Operations

☝️

Screenshots (if applicable)

high-slippage.mp4

woodenfurniture and others added 22 commits April 2, 2024 19:59
* add bottom sheet dialog

* try out vaul

* hook up send

* Create SubPage.tsx

* adjusting dialog header

* Update DialogCloseButton.tsx

* add new action menu

* updates

* update more height stuff

* use 100dvh instead of vh

* use 100vh

* anchor footer at the bottom of the screen

* update new asset search

* Update SelectAssets.tsx

* add min height for regular modals

* Update MobileNavBar.tsx

* remove extra stuff not needed for PR

* remove more stuff not for this PR

* Update AddressInput.tsx

* Update AddressInput.tsx

* Update Dialog.tsx

* Update SelectAssets.tsx

* fix for dialog height

* Update AssetSearch.tsx

---------

Co-authored-by: gomes <[email protected]>
* feat: use proxy validation endpoint

* chore: improve fn name

* update endpoint

* fix: csp

* fix: tests

* chore: use shapeshift proxy csp

* chore: fail open

---------

Co-authored-by: kaladinlight <[email protected]>
* account down remove portal

* Update StepRow.tsx
* chore: invalidate outdated translations

* Translate main.json via GitLocalize

* Translate main.json via GitLocalize

* Translate main.json via GitLocalize

* Translate main.json via GitLocalize

* Translate main.json via GitLocalize

* Translate main.json via GitLocalize

* Translate main.json via GitLocalize

* Translate main.json via GitLocalize

* Translate main.json via GitLocalize

* Translate main.json via GitLocalize

---------

Co-authored-by: Hellhound13 <[email protected]>
Co-authored-by: Jpanam <[email protected]>
Co-authored-by: yuki <[email protected]>
Co-authored-by: guiribabrb <[email protected]>
Co-authored-by: Markus Meyer <[email protected]>
Co-authored-by: Romko <[email protected]>
Co-authored-by: tonyjiang12399 <[email protected]>
* feat: getFullAppData tests

* feat: cow quote/unsigned order build cleanup
* fix: zapperAssetToMaybeAssetId handle base-token as native asset

* fix: filter unique opportunities by id first

* Revert "fix: zapperAssetToMaybeAssetId handle base-token as native asset"

This reverts commit 1ed1a59.
* feat: uni-v2 pools static generation

* feat: revert DynamicLpAssets flag

* feat: regen

* feat: cleanup asset upserty bits

* feat: regen
@0xApotheosis
Copy link
Member Author

@gomesalexandre & @woodenfurniture all feedback addressed sers 🙏

CI currently failing for unrelated reasons (waiting for my hotfix into release to be merged back into develop).

Copy link
Contributor

@gomesalexandre gomesalexandre left a comment

Choose a reason for hiding this comment

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

Tackled comments look sane - retested at runtime:

Swapper styles

  • Looks sane again! ✅
image

Swapper unsafe lower than minimum

  • Ack is now properly triggered, and captures the original onClick event behavior ✅
unsafe.btc.mov

Swapper Confirm

  • Ack is properly triggered, and captures the original onClick event behavior ✅
swapper.confirm.ack.mov

Lending - high-slippage borrow

Wasn't able to retest as both pools are currently halted, though this was previously tested

THORCHain LP

  • Ack is properly triggered, and captures the original onClick event behavior ✅
Screen.Recording.2024-04-16.at.12.34.55.mov

@gomesalexandre gomesalexandre marked this pull request as ready for review April 16, 2024 08:35
@woodenfurniture
Copy link
Member

Applying the same patch to invoke unsafe trade quote, the layout is greatly improved but is missing a description of the issue such that it just says "Attention!" without further context 😅

image

Copy link
Member

@woodenfurniture woodenfurniture left a comment

Choose a reason for hiding this comment

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

Fallback text now appearing. Looking good to merge
image

@0xApotheosis 0xApotheosis enabled auto-merge (squash) April 16, 2024 23:35
@0xApotheosis 0xApotheosis disabled auto-merge April 16, 2024 23:41
@0xApotheosis 0xApotheosis merged commit e6265e7 into develop Apr 16, 2024
3 checks passed
@0xApotheosis 0xApotheosis deleted the warning-ack branch April 16, 2024 23:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants