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

Pr2256/follow up #2279

Merged
merged 9 commits into from
Jan 25, 2022
Merged

Pr2256/follow up #2279

merged 9 commits into from
Jan 25, 2022

Conversation

alfetopito
Copy link
Contributor

Summary

Follow up to #2256

  • No longer blocking EOAs in case native amount is being fully used. Showing same warning as SC wallets instead.
  • Refactoring how gas is cost is calculated:
    • Using real gas cost based on selected input from the contract call data
    • Storing calculated gas on redux
    • Using fast gas prices for safety
  • Show how much we think the tx will cost in the warning message

Example warning message
Screen Shot 2022-01-24 at 14 16 25

What you'll see if you ignore the message
Screen Shot 2022-01-24 at 14 19 01

To Test

Same steps as PR #2256

@github-actions
Copy link
Contributor

  • 🔭 GP Swap: CoW Protocol v2 Swap UI

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.

It works great, and I will merge to consolidate and do the RC.
The UX is pretty nice! I like is a bit conservative, i manage to test it in the limit where i got the warning but did the TX anyways.

However, I think it wouldn't harm to break down a bit the claim hook into subhooks. I feel it is too big (as a good to have).
I feel there's too many not small auxiliary function defined there that can just be hooks or pure functions receiving their dependencies?

}
if (!vCowToken) {
throw new Error('vCOW token not present')
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't this checks replicated in getClaimArgs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes but if not checked TS complains

@anxolin
Copy link
Contributor

anxolin commented Jan 25, 2022

Others, pls review post-merge

@anxolin anxolin merged commit 9b3cde3 into develop Jan 25, 2022
maria-vslvn pushed a commit that referenced this pull request Jan 25, 2022
@elena-zh
Copy link

Works for me!
The only thing is that it would be nice to change the warning color for the light mode
image

@alfetopito , @biocom , WDYT?

@fairlighteth
Copy link
Contributor

@elena-zh Agreed. Will handle.

@alfetopito alfetopito deleted the pr2256/follow-up branch January 25, 2022 17:48
@alfetopito
Copy link
Contributor Author

However, I think it wouldn't harm to break down a bit the claim hook into subhooks. I feel it is too big (as a good to have). I feel there's too many not small auxiliary function defined there that can just be hooks or pure functions receiving their dependencies?

@anxolin You mean the claim hooks file or the useClaimCallback hook?

@anxolin
Copy link
Contributor

anxolin commented Jan 26, 2022

I was meaning useClaimCallback it seemed to me it has too much logic, and is good to keep the "one thing principle" as much as possible.

Anyways, don't worry if this is not needed right now and the refactor is costly. We can always break it if we need to reuse parts of it or if we need to touch there again.

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