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

322/store quote info on redux #555

Closed
wants to merge 7 commits into from

Conversation

alfetopito
Copy link
Collaborator

@alfetopito alfetopito commented May 20, 2022

Summary

Part of #332

Making both sell and buy amounts returned with the quote available in redux context

These amounts will later be stored in the order metadata to calculate real surplus (difference between quote prices and executed prices)

To Test

  1. Using redux developer addon/extension, load the page
  2. Go to redux > actions > State
    Screen Shot 2022-05-20 at 13 23 12
  3. Filter by price
  4. Click on a price/refreshQuote action
  5. In the tree navigation, open price -> quotes -> 1 -> <token address> -> price
    Screen Shot 2022-05-20 at 13 20 47
  • You should see the field oppositeAmount populated
  • There should be no changes at all to the behaviour

@alfetopito alfetopito self-assigned this May 20, 2022
@alfetopito alfetopito requested a review from a team May 20, 2022 12:28
@github-actions
Copy link
Contributor

CLA Assistant Lite All Contributors have signed the CLA.

@github-actions
Copy link
Contributor

  • 🔭 GP Swap: CoW Protocol v2 Swap UI

@elena-zh
Copy link
Contributor

Hey @alfetopito , I can't see 'oppositeAmount' for 'refreshQuote' request, but I can see it for 'rupdateQuote' in Mainnet and GC.
Is it intentional that it is missing for Rinkeby?
no amount for rinkeby

Also, I see duplicating requests, and one of them does not contain opposite amount. Is it OK?
image
image

@alfetopito
Copy link
Collaborator Author

Hey @alfetopito , I can't see 'oppositeAmount' for 'refreshQuote' request, but I can see it for 'rupdateQuote' in Mainnet and GC. Is it intentional that it is missing for Rinkeby? no amount for rinkeby

Also, I see duplicating requests, and one of them does not contain opposite amount. Is it OK? image image

You are looking at the diff. This is the redux state management and it only updates a field when it changes.
Check the state instead and it'll be there.

@elena-zh
Copy link
Contributor

@alfetopito , thanks for explanation!
LGTM!

@W3stside
Copy link
Contributor

don't really understand why we need the oppositeAmount. also the issue linked is the wrong one i believe, code looks ok tho!

@alfetopito
Copy link
Collaborator Author

don't really understand why we need the oppositeAmount.

We will need to add to the metadata of every order both sell and buy amounts returned in the quote response.

Right now, only one of those (buy or sell, depending on the order type) is stored.
This change adds the opposite amount (if sell, store the buy, if buy, store the sell).

As said, aware this is not the best name, happy to hear suggestions :)

also the issue linked is the wrong one i believe

You are right, wrong issue id 🤦 . Correct one -> #332

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.

Maybe i did sth wrong but id didn't get the "opposite" field
image

@@ -99,5 +99,5 @@ export function toPriceInformation(priceRaw: CoinGeckoUsdQuote | null): PriceInf
}

const { usd } = priceRaw[token]
return { amount: usd.toString(), token }
return { amount: usd.toString(), token, oppositeAmount: '1' }
Copy link
Contributor

Choose a reason for hiding this comment

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

i find oppositeAmount missliading.

If we can really not add from/to, would it be better amountInputted or something?

@anxolin
Copy link
Contributor

anxolin commented May 24, 2022

What I mentioned yesterday is.

Why do we need to change the quote object here?

Maybe we can avoid modifying it. We already have the state of the original inputed amount from the user in our state. We just need to include that in the quote object of the meta-data of the order when we submit the order.

This metadata could have normal names as suggested in the document (no backward compatibility issues, because is a new metadata)

@alfetopito
Copy link
Collaborator Author

As Anxo points out, the information needed (sell and buy amounts) are already in the context.

https://github.com/cowprotocol/cowswap/blob/1a7b9d4928240d195b40feacbf870eac8c2deda0/src/custom/hooks/useSwapCallback.ts#L156-L157=

Namely inputAmountWithFee and experectedOutputAmount.

Thus this change is unnecessary.

When there's a quoteId coming from the backend though, I'll send a new PR, but likely will have to come from the sdk instead.
See #561 and #567

@alfetopito alfetopito closed this May 26, 2022
@alfetopito alfetopito deleted the 322/store-quote-info-on-redux branch May 26, 2022 11:19
@github-actions github-actions bot locked and limited conversation to collaborators May 26, 2022
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