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.

Sign up for GitHub

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

Jump to bottom

Fix issue with deadline #621

Merged
merged 1 commit into from
Jun 2, 2022
Merged

Fix issue with deadline #621

merged 1 commit into from
Jun 2, 2022

Conversation

anxolin
Copy link
Contributor

@anxolin anxolin commented Jun 2, 2022 •
edited
Loading

Summary

Sets the deadline to 10min in the future instead of the max value (in 84 years!)

This is just a temporary fix while this is an optional field in the backend.

@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2022

CLA Assistant Lite All Contributors have signed the CLA.

josojo
josojo approved these changes Jun 2, 2022
View reviewed changes
Copy link
Contributor

@josojo josojo 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!

@github-actions
Copy link
Contributor

github-actions bot commented Jun 2, 2022

  • 🔭 GP Swap: CoW Protocol v2 Swap UI

@anxolin anxolin marked this pull request as ready for review June 2, 2022 17:07
@anxolin anxolin merged commit 687861c into hotfix/1.14.2 Jun 2, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jun 2, 2022
W3stside
W3stside reviewed Jun 2, 2022
View reviewed changes
src/custom/hooks/useUSDCPrice/index.ts
@@ -101,7 +100,8 @@ export default function useUSDCPrice(currency?: Currency) {
toDecimals: stablecoin.decimals,
userAddress: account,
// we dont care about validTo here, just use max
validTo: MAX_VALID_TO_EPOCH,
// FIXME: I guess we care now, using 10min. Future versions of the API will make it optional
Copy link
Contributor

Choose a reason for hiding this comment

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

Alt Text

@nlordell
Copy link
Contributor

nlordell commented Jun 3, 2022

Here is a PR in the backend which makes validTo optional: cowprotocol/services#249

Also - I would suggest, even if it is optional, to set the validTo here to whatever value is configured in the UI here:

image

The reason for this is that it is plausible (although we have no immediate plans to do this) that the fee be dependent on the duration of validity for the order (for example, we could add a premium for orders with higher validity than 1hour to account for gas price fluctuation risk). The intention with the quote API is that it should be as close as possible to the actual order you plan on placing, and setting a validFor: getTransactionDeadlineSetting() would be more indicative of what the trade intends to place.

W3stside
W3stside reviewed Jun 3, 2022
View reviewed changes
Copy link
Contributor

@W3stside W3stside left a comment

Choose a reason for hiding this comment

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

Approved

@anxolin anxolin deleted the fix-max-date branch August 16, 2023 11:10
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@W3stside W3stside W3stside left review comments

@josojo josojo josojo approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@anxolin @nlordell @josojo @W3stside