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

Fix swap.cow.fi appData bug #698

Merged
merged 8 commits into from
Jun 20, 2022
Merged

Conversation

W3stside
Copy link
Contributor

Summary

Fixes issue raised by @nlordell here

Regex test:
image

image

@github-actions
Copy link
Contributor

  • 🔭 GP Swap: CoW Protocol v2 Swap UI

.env Outdated Show resolved Hide resolved
Copy link
Contributor

@nlordell nlordell left a comment

Choose a reason for hiding this comment

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

🎉

Just one concern about additional spaces in the regexp.

.env.production Outdated Show resolved Hide resolved
.env.production Outdated Show resolved Hide resolved
W3stside and others added 4 commits June 17, 2022 19:10
Co-authored-by: Nicholas Rodrigues Lordello <[email protected]>
Co-authored-by: Leandro Boscariol <[email protected]>
Co-authored-by: Nicholas Rodrigues Lordello <[email protected]>
@W3stside W3stside changed the base branch from main to develop June 18, 2022 15:11
@W3stside W3stside requested review from alfetopito and nlordell June 18, 2022 15:11
@W3stside W3stside changed the base branch from develop to hotfix/1.15.5 June 18, 2022 15:24
@W3stside W3stside added the RELEASE Included in the release that is being closed label Jun 18, 2022
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.

Great! although I would think that the problem was also that we are sending 0x0 if there's not a known pattern in the URL

I would add some metadata as the default, so we differenciate between us sending orders from "known" domains and others sending orders with all 0s

.env Outdated Show resolved Hide resolved
.env.production Outdated Show resolved Hide resolved
Co-authored-by: Anxo Rodriguez <[email protected]>
@github-actions
Copy link
Contributor

CLA Assistant Lite All Contributors have signed the CLA.

Co-authored-by: Nicholas Rodrigues Lordello <[email protected]>
.env.production Outdated Show resolved Hide resolved
@anxolin
Copy link
Contributor

anxolin commented Jun 20, 2022

I just had a nice idea for using as a default.

Because @alfetopito is making the meta-info dynamic, maybe we will want to use always the domain in the appCode field of the meta-information.

This way, this info will be already part of the meta-info, making it more informative. We don't need to do patter matching any more to decide the appData, its always dynamic.
So now, on our upcoming release of the "slippage tolerance" we can potentially change this easily.
Nothing to address in this PR

WDYT?

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.

I think the underlying issue was not solved as explained here #698 (review)

However, I think is not worth doing it, if we do what I suggest here:
#698 (comment)

@alfetopito
Copy link
Collaborator

I just had a nice idea for using as a default.

Because @alfetopito is making the meta-info dynamic, maybe we will want to use always the domain in the appCode field of the meta-information.

This way, this info will be already part of the meta-info, making it more informative. We don't need to do patter matching any more to decide the appData, its always dynamic. So now, on our upcoming release of the "slippage tolerance" we can potentially change this easily. Nothing to address in this PR

WDYT?

In my PR, I still rely on these mappings as fallback, and also use that to set environment key

@W3stside
Copy link
Contributor Author

@anxolin maybe make an issue for your concern, @alfetopito can coordinate as I'm not familiar with that code so much. merging this

@W3stside W3stside merged commit cae969f into hotfix/1.15.5 Jun 20, 2022
@github-actions github-actions bot locked and limited conversation to collaborators Jun 20, 2022
@nlordell
Copy link
Contributor

nlordell commented Jun 21, 2022

I would add some metadata as the default, so we differenciate between us sending orders from "known" domains and others sending orders with all 0s

The advantage of making it all 0s is that it is easier to notice issues. If the app_data had been some random bytes, I probably wouldn't have noticed that we were sending "unknown domain" app data.

I agree that the "holy grail" would be to make it entirely dynamic and compute some app data based on the current domain.

@alfetopito alfetopito deleted the fix-swap-cow-fi-prod-appData branch June 21, 2022 10:36
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
RELEASE Included in the release that is being closed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants