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

332/slippage bips rather than amounts on metadata #758

Conversation

alfetopito
Copy link
Collaborator

Summary

Part of #332

Waterfalls onto/supersedes #750

Using the latest version of the appData.

Updating metadata way less often by storing only the slippage instead of sell/buyAmounts and quoteId

To Test

  1. Open the console, filter by appData
  2. Play with the slippage in the settings
  • Every update should trigger a new appData being generated
  • The slippage will be stored as BIPs. For example, the default 0.50% will be stored as 50
  1. Place an order
  • Verify it contains the appData hash calculated in the previous step

@alfetopito alfetopito self-assigned this Jun 29, 2022
@alfetopito alfetopito requested review from a team June 29, 2022 16:36
@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

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.

Approved.

A small nitpick. I notice some delay between submitting the tx and seeing the uploading.

I think this is due to the updater checking only from time to time.
I think the:

  • Updater don't need to run so frequently
  • We should trigger an action that tries to upload the document right away. This way, the effect is that we attempt the upload right after posting the order, and if it fails is when the updater takes over and re-attempts

src/custom/hooks/useAppData.ts Outdated Show resolved Hide resolved
src/custom/utils/appData.ts Outdated Show resolved Hide resolved
Base automatically changed from 332/quote-id-on-metadata to 332/generate-appDataHash-for-every-order June 30, 2022 10:04
@alfetopito alfetopito force-pushed the 332/slippageBips-rather-than-amounts-on-metadata branch from e0e13cc to 1788141 Compare June 30, 2022 10:09
@alfetopito alfetopito requested review from a team June 30, 2022 10:28
@alfetopito
Copy link
Collaborator Author

Approved.

A small nitpick. I notice some delay between submitting the tx and seeing the uploading.

I think this is due to the updater checking only from time to time. I think the:

* Updater don't need to run so frequently

* We should trigger an action that tries to upload the document right away. This way, the effect is that we attempt the upload right after posting the order, and if it fails is when the updater takes over and re-attempts

Yes, this is true.

More of a technical reason. I don't want to react to every change as that leads to race conditions since the same state is used to track when something is being updated or not.
Also, not a big concern given that uploading the file is not time sensitive, even more so now that we are back at reduced amount of files.

Maybe what does make sense is to check whether the file exists before trying to pin it 🤔

@alfetopito alfetopito force-pushed the 332/slippageBips-rather-than-amounts-on-metadata branch from 6495a4b to aada118 Compare June 30, 2022 11:16
@elena-zh
Copy link

Changes related to Slippage LGTM besides I regression issue I reported for GA analytics event: #639
So in this PR I see duplicating appdatahash logs when I'm filling in the slippage field
related to

Copy link

@elena-zh elena-zh left a comment

Choose a reason for hiding this comment

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

Approving with the issue above

@alfetopito alfetopito mentioned this pull request Jun 30, 2022
@alfetopito
Copy link
Collaborator Author

Changes related to Slippage LGTM besides I regression issue I reported for GA analytics event: #639 So in this PR I see duplicating appdatahash logs when I'm filling in the slippage field related to

Elena, I don't see a duplication here.
It is regenerating at every keypress, though.
You can check the slippageBips is different, as well as the CID and appDataHash

@alfetopito alfetopito merged commit 2588905 into 332/generate-appDataHash-for-every-order Jun 30, 2022
@alfetopito alfetopito deleted the 332/slippageBips-rather-than-amounts-on-metadata branch June 30, 2022 16:06
@github-actions github-actions bot locked and limited conversation to collaborators Jun 30, 2022
@elena-zh
Copy link

elena-zh commented Jul 4, 2022

Elena, I don't see a duplication here. It is regenerating at every keypress, though. You can check the slippageBips is different, as well as the CID and appDataHash

@alfetopito , I'm sorry, I did not explain it correctly. I meant that the log appears when I'm still typing into the field, so when I want to specify a 2-3 digits slippage value, I will see 2 logs for appdata, and I called it 'duplicating'.
So my point was to log appdata as soon as user removes a cursor/mouse from the Slippage field or so.

@alfetopito
Copy link
Collaborator Author

Elena, I don't see a duplication here. It is regenerating at every keypress, though. You can check the slippageBips is different, as well as the CID and appDataHash

@alfetopito , I'm sorry, I did not explain it correctly. I meant that the log appears when I'm still typing into the field, so when I want to specify a 2-3 digits slippage value, I will see 2 logs for appdata, and I called it 'duplicating'. So my point was to log appdata as soon as user removes a cursor/mouse from the Slippage field or so.

Ah, got it.
Not a problem Elena, as it won't upload the file anywhere by that point, and it's fairly cheap to re-calculate

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.

3 participants