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

[1] fix(twap): display fallback handler banner when it's relevant #2757

Merged
merged 6 commits into from
Jul 4, 2023

Conversation

shoom3301
Copy link
Collaborator

@shoom3301 shoom3301 commented Jun 30, 2023

Summary

Not in Safe In Safe In Safe + checked
Screenshot 2023-06-30 at 19 31 55 Screenshot 2023-06-30 at 19 32 45 Screenshot 2023-06-30 at 19 32 50
  1. Removed state "Unsupported Safe"
  2. Display checkbox "Modify safe" fallback handler is not set and approval is needed
  3. Added a line break for "Unsupported wallet detected" banner

To Test

  1. Open Advanced orders NOT in Safe
  • The button is disabled
  • "Unsupported Wallet detected" banned is displayed
  1. Open Advanced orders in Safe without Fallback handler
  • The button is disabled
  • "Unsupported Safe detected" banned is displayed
  1. "Modify Safe's fallback handler" checkbox works well

@shoom3301 shoom3301 added the TWAP label Jun 30, 2023
@shoom3301 shoom3301 requested review from a team June 30, 2023 13:28
@shoom3301 shoom3301 self-assigned this Jun 30, 2023
@vercel
Copy link

vercel bot commented Jun 30, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
swap-dev ✅ Ready (Inspect) Visit Preview 💬 Add feedback

🌃 Cosmos ↗︎

@shoom3301 shoom3301 changed the title fix(twap): display fallback handler banner when it's relevant [1] fix(twap): display fallback handler banner when it's relevant Jun 30, 2023
const { showPriceImpactWarning } = useTwapWarningsContext()
const isFallbackHandlerRequired = useIsFallbackHandlerRequired()

if (showPriceImpactWarning) return isPriceImpactAccepted
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure this is what you wanted to do

Didn;t you mean sth like if (showPriceImpactWarning && !isPriceImpactAccepted) return false

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In this hook, I like to take into account isPriceImpactAccepted value only when showPriceImpactWarning is true.
So, I can translate it as:

When price impact is shown, then take the "Price impact accepted" checkbox value into account

Copy link
Contributor

Choose a reason for hiding this comment

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

But can it be showPriceImpactWarning and isFallbackHandlerRequired

in my proposal above it says, "if we show the price impact and is not accepted, return false, otherwise continue with the following logic" this way we consider other checks. If all check pass then we return true

Maybe I'm reading it wrong?

Copy link
Contributor

@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! However, I have found #2771 issue while testing the PR

Copy link
Collaborator

@alfetopito alfetopito left a comment

Choose a reason for hiding this comment

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

Some minor questions/concerns

@shoom3301 shoom3301 merged commit 19ed7ee into develop Jul 4, 2023
@github-actions github-actions bot locked and limited conversation to collaborators Jul 4, 2023
@alfetopito alfetopito deleted the fix/twap-handler-banner branch July 4, 2023 14:55
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants