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

Cancel cowswap orders #196

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Cancel cowswap orders #196

wants to merge 3 commits into from

Conversation

vrtnd
Copy link
Collaborator

@vrtnd vrtnd commented Apr 24, 2023

No description provided.

@vercel
Copy link

vercel bot commented Apr 24, 2023

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

Name Status Preview Comments Updated (UTC)
llamaswap ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jun 5, 2023 3:48pm

@cloudflare-workers-and-pages
Copy link

cloudflare-workers-and-pages bot commented Apr 24, 2023

Deploying with  Cloudflare Pages  Cloudflare Pages

Latest commit: ac9dece
Status: ✅  Deploy successful!
Preview URL: https://e35e0dfb.llamaswap.pages.dev
Branch Preview URL: https://cow-cancel.llamaswap.pages.dev

View logs

@@ -776,8 +788,6 @@ export function AggregatorContainer({ tokenList, sandwichList }) {
.wait?.()
?.then((final) => {
if (final.status === 1) {
forceRefreshTokenBalance();
Copy link
Contributor

Choose a reason for hiding this comment

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

why change the order here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

need to refresh after swap, so we get cowswap order from db

() => getSwapsHistory({ userId, chain, tokensUrlMap, tokensSymbolsMap }),
{
enabled: !!userId && !!chain && isOpen,
Copy link
Contributor

Choose a reason for hiding this comment

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

why remove isOpen?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

to fetch this on every cowswap order, so we can show loader near to history btn

?.map((swap) => {
const rawQuote = swap?.route?.price?.rawQuote;
if (swap.aggregator === 'CowSwap') {
const cowOrder = cowData?.data?.find((order) => order?.ethflowData?.userValidTo === rawQuote?.quote?.validTo);
Copy link
Contributor

Choose a reason for hiding this comment

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

why match using validTo? if there are 2 orders that expire at the same time wont this cause the matching to be incorrect?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

tried to find another way, but this will work since issa timestamp and 2 orders couldnt be placed at the same time

Copy link
Contributor

@0xngmi 0xngmi left a comment

Choose a reason for hiding this comment

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

looks good, but i think users wont be able to find that they have to go to order history in order to cancel the order

instead it would be better to change the post-tx dialog so that if its an order against cowswap we display a message explaining to the user that:

  • if its an eth order it may take up to 30 minutes to execute and the eth is sitting in a contract, it didnt disappear. They can get the ETH back immediately by cancelling the order
  • for other types of others they can cancel it by going to the history tab

the differentiation between eth orders and other orders should be automatic btw, so we only display the message that the user needs

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.

2 participants