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

Disable claim review on zero or not approved #2250

Merged
merged 13 commits into from
Jan 22, 2022

Conversation

nenadV91
Copy link
Contributor

Summary

Adding on top or @alfetopito PR #2241

  • Disable review button if some option is not approved
  • Disable and show error msg if some selected option has value of 0

@nenadV91 nenadV91 changed the base branch from develop to disable-claim-review-on-errors January 21, 2022 13:31
@github-actions
Copy link
Contributor

  • 🔭 GP Swap: CoW Protocol v2 Swap UI

@elena-zh
Copy link

@nenadV91 , great job!
But 'Review' button enables when I type '0' into the field. Could you please fix it?
image

@nenadV91
Copy link
Contributor Author

@elena-zh Yep, I'm on it

@elena-zh
Copy link

@nenadV91 , I'm not sure if it is related to the current PR or not, but I'm not show any validation messages until a token is approved
image
WDYT?

@alfetopito alfetopito force-pushed the disable-claim-review-on-errors branch from c280566 to 5bbe2d5 Compare January 21, 2022 16:37
Base automatically changed from disable-claim-review-on-errors to develop January 21, 2022 18:08
Copy link
Contributor

@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.

Very good, working great!

I've only pushed one tiny change to reset percentage on error and one suggestion below.

@fairlighteth
Copy link
Contributor

Looks good to me 👍🏼

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.

Works great! nicely done

@anxolin anxolin merged commit 4603884 into develop Jan 22, 2022
@alfetopito alfetopito deleted the disable-claim-review-on-zero-or-not-approved branch January 22, 2022 18:06
mergify bot pushed a commit that referenced this pull request Jan 22, 2022
# Summary

Waterfalls onto #2250 

Hide/show investment error msg when there are errors
Also updated the msg to be more general.

![Screen Shot 2022-01-21 at 15 15 47](https://user-images.githubusercontent.com/43217/150612049-ec644178-2dce-4823-9f6d-566659883f0f.png)

Of course feel free to suggest something else

  # To Test

1. Try to claim something paid and make sure to fail the validation
@elena-zh
Copy link

LGTM!

mergify bot pushed a commit that referenced this pull request Jan 24, 2022
# Summary

Waterfalls into #2250

Show warning/error when using too much native currency to invest

Uses current gas estimation for 1 approval. Might need to adjust that for a higher amount

If it's a Smart contract wallet, show a warning and let user proceed.
If it's an EOA wallet, show an error and block from moving forward until amount is corrected.

## Smart contract:

![Screen Shot 2022-01-21 at 18 14 02](https://user-images.githubusercontent.com/43217/150620950-7f19b526-ab55-4ee8-b7c5-2b05ddc1be53.png)

## EOA:

![Screen Shot 2022-01-21 at 18 13 44](https://user-images.githubusercontent.com/43217/150620964-30a16c42-e9a0-4523-8e20-5393047f17a0.png)

  # To Test

1. With and EOA, load one account that has a ETH investment option
2. Fund the account with not enough ETH to cover the full investment
3. On approvals page, select to invest max
* You'll see the error as you are using everything in your wallet
4. With a Smart contract wallet, load one account that has a ETH investment option
5. Fund the account with exactly the amount needed to cover the full investment (this step is only need if claiming on behalf of an EOA. If you have a Safe with claims do the same as step `2`)
* On approvals page, you should see the warning and be able to move forward

  # Background

**Note:** Color is of course dumb and doesn't quite work on light more.
Neither does the error color for that matter.
Counting on @biocom to make it look good
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.

5 participants