Add partner fee meta-data #53

anxolin · 2024-02-15T16:57:10Z

Add a new type of metadata: partnerFee

This meta-data will allow anyone to encode the intent to charge a fee for a given order.

Add new datata type
~~Version schema: v0.12.0~~ Version schema: v1.0.0 (changes are now breaking changes)
Build new types and adapt compile.ts to export the new metadata schema
Build tests: happy path, not-so-happy path

Validations

The metadata is optional, as all other metadatas
If present, all the props are mandatory: bip, recipient
bip is an integer (not a string)
bip should be greater than zero, and less than 10,000 (the protocol might limit further the maximum, for example 300, or 3%)

See unit tests

Example

Example on the new metadata:

partnerFee: {
   bips: 50,
   recipient: '0xb6BAd41ae76A11D10f7b0E664C5007b908bC77C9'
}

github-actions · 2024-02-15T16:57:25Z

CLA Assistant Lite bot All contributors have signed the CLA ✍️ ✅

anxolin · 2024-02-15T16:59:11Z

README.md

+4. Modify the `compile.ts` script
+
+- Add the exported constant with the latest version in, and the new metadata:
+  - For example: https://github.com/cowprotocol/app-data/pull/44/commits/aeef8a58e7bbd2a53664ce396011cb157a18406d


unrelated: this link was just broken

alfetopito

Other than the comments, the build is currently broken

src/scripts/compile.ts

alfetopito · 2024-02-15T17:20:44Z

src/schemas/definitions.json

      "type": "string"
+    },
+    "bips": {
+      "$id": "#/definitions/bips",


Can you use this definition in the quote slippageBips ?

I did the other way around here: #55

test/schema.spec.ts

anxolin · 2024-02-15T18:03:41Z

The build was broken before. Fixed in #54

alfetopito · 2024-02-16T10:37:50Z

test/schema.spec.ts

@@ -627,6 +628,142 @@ describe('Schema v0.11.0', () => {
  )
 })

+describe('Schema v0.12.0', () => {


As mentioned in #55, v0.12 won't be created. It'll be v1.0 instead

Okay, but the changes are independent and I did the version here. I thought is ok to version it still, as those are 2 different changes and the one in this PR is backward compatible

I will delete the 0.12.0 in #54 instead as it seems you prefer 0.12.0 don't exist. If fine with that

fleupold · 2024-02-19T14:28:36Z

Can we call it bps. Bips is very colloquial and not the official abbreviation: https://www.investopedia.com/ask/answers/what-basis-point-bps

anxolin · 2024-02-19T18:10:14Z

@fleupold no problem with that on my end. The only issue will be that for coherence would be great if I change it also in the quote information. I could make it part of the breaking change #55

This would require @olgafetisova to adapt the queries, and I'm unsure if that's problematic or not. Maybe she could confirm if it's ok to rename.

alfetopito · 2024-02-19T18:15:37Z

@fleupold no problem with that on my end. The only issue will be that for coherence would be great if I change it also in the quote information. I could make it part of the breaking change #55

This would require @olgafetisova to adapt the queries, and I'm unsure if that's problematic or not. Maybe she could confirm if it's ok to rename.

Keep in mind this change would require analytics to handle both cases for quotes

anxolin · 2024-02-19T18:20:23Z

Keep in mind this change would require analytics to handle both cases for quotes

yes, that was my point. This is why I bring it up:

Option 1: keep all as defined in Add partner fee meta-data #53 and Fix build #54 (coherent, using the colloquial term)
Option 2: use bps in parnerFee but not in quote (incoherent, using the right term at least in the partner fee)
Option 3: use bps in both parnerFee and quote quote (coherent, using the right term, potentially adding trouble for analytics to adapt queries or discard older data in some queries)

olgafetisova · 2024-02-20T02:05:14Z

This would require @olgafetisova to adapt the queries, and I'm unsure if that's problematic or not. Maybe she could confirm if it's ok to rename.

We are currently changing slippageBips to integer in the code right away before doing any other adjustments, so I think casting both string and integer to integer should not break anything.

alfetopito · 2024-02-20T08:58:19Z

Keep in mind this change would require analytics to handle both cases for quotes

yes, that was my point. This is why I bring it up:

* Option 1: keep all as defined in [Add partner fee meta-data #53](https://github.com/cowprotocol/app-data/pull/53) and [Fix build #54](https://github.com/cowprotocol/app-data/pull/54) (coherent, using the colloquial term)

* Option 2: use bps in `parnerFee` but not in `quote`  (incoherent, using the right term at least in the partner fee)

* Option 3: use bps in both `parnerFee` and quote `quote` (coherent, using the right term, potentially adding trouble for analytics to adapt queries or discard older data in some queries)

Due to historical reasons and consistency, I prefer option 1.
Unless @olgafetisova doesn't mind the additional change/handling to go for option 3

olgafetisova · 2024-02-20T11:19:50Z

* Option 1: keep all as defined in [Add partner fee meta-data #53](https://github.com/cowprotocol/app-data/pull/53) and [Fix build #54](https://github.com/cowprotocol/app-data/pull/54) (coherent, using the colloquial term)

* Option 2: use bps in `parnerFee` but not in `quote`  (incoherent, using the right term at least in the partner fee)

* Option 3: use bps in both `parnerFee` and quote `quote` (coherent, using the right term, potentially adding trouble for analytics to adapt queries or discard older data in some queries)

If you change slippageBips to bips then it will break some queries, not a big work to adjust it but might take 1-3 days from Dune side to approve the PR, so we might be experiencing some issues there. So I would prefer the name to stay as it is for slippage.

If you change slippageBips from string to integer, it won't break the code. Everything will remain as is.

anxolin · 2024-02-20T13:33:19Z

I would personally not add you 1-3 days of work for this name change (discard option 3) @fleupold do you agree?

If we can't change it for the quote, I would slightly prefer option 2, yes it's not coherent but at least we don't keep using the wrong term for some metadata that is more relevant for integrators.

Otherwise, we can go with option 1.

olgafetisova · 2024-02-21T17:05:21Z

I went over the documentation again and I do not see any simple way to allow a non-existing column in the code (as it will happen when we add the change). The change has to happen on the ETL side which is written in a flat way (same as in our backend), hence, there is no consideration for columns.
To conclude, my strong preference would be to keep the name as is as I did not find an easy way to adjust the code.

Use bips for slippage

anxolin · 2024-02-22T19:23:07Z

Okay, I believe we arrive to consensus over option 2. I applied the changes. Will create a release of this library

alfetopito

👍

anxolin force-pushed the add-partner-fee branch from b128922 to 0243cdb Compare February 15, 2024 16:57

anxolin commented Feb 15, 2024

View reviewed changes

anxolin marked this pull request as ready for review February 15, 2024 17:08

alfetopito requested changes Feb 15, 2024

View reviewed changes

anxolin mentioned this pull request Feb 15, 2024

Use bips for slippage #55

Merged

anxolin added 5 commits February 15, 2024 18:34

docs: improve docs

212e638

Add partner fee

f2ca95c

Add new schema exported types

7633d85

Add unit tests

ce4e81a

Assert decimal values are not allowed

5c027da

anxolin force-pushed the add-partner-fee branch from e5fbb8f to 5c027da Compare February 15, 2024 18:34

anxolin requested a review from alfetopito February 15, 2024 18:37

anxolin added 4 commits February 15, 2024 18:38

Fix issue with versions

d759d4e

Use bips definition for the slippage too

d600861

Use the new bips definition also for the slippageBips in the quote

5b7ca81

Add new breaking changes

1255e24

anxolin mentioned this pull request Feb 16, 2024

[widget] Add UI fee cowprotocol/cowswap#3804

Closed

9 tasks

anxolin requested a review from a team February 16, 2024 10:20

alfetopito reviewed Feb 16, 2024

View reviewed changes

anxolin requested review from olgafetisova and harisang February 19, 2024 18:40

chore: pretend 0.12.0 never existed

a4789f0

anxolin requested a review from alfetopito February 19, 2024 18:48

anxolin added 2 commits February 22, 2024 19:14

Merge pull request #55 from cowprotocol/use-bips-for-slippage

1ca53bb

Use bips for slippage

chore: rename BIPS to BPS

da6b8b1

chore: change version

b0b3d93

anxolin merged commit 8cc394c into main Feb 22, 2024
5 checks passed

anxolin deleted the add-partner-fee branch February 22, 2024 19:25

github-actions bot locked and limited conversation to collaborators Feb 22, 2024

alfetopito reviewed Feb 23, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add partner fee meta-data #53

Add partner fee meta-data #53

anxolin commented Feb 15, 2024 •

edited

Loading

github-actions bot commented Feb 15, 2024 •

edited

Loading

anxolin Feb 15, 2024

alfetopito left a comment

alfetopito Feb 15, 2024

anxolin Feb 15, 2024

anxolin commented Feb 15, 2024 •

edited

Loading

alfetopito Feb 16, 2024

anxolin Feb 19, 2024

anxolin Feb 19, 2024

fleupold commented Feb 19, 2024

anxolin commented Feb 19, 2024

alfetopito commented Feb 19, 2024

anxolin commented Feb 19, 2024

olgafetisova commented Feb 20, 2024

alfetopito commented Feb 20, 2024

olgafetisova commented Feb 20, 2024

anxolin commented Feb 20, 2024

olgafetisova commented Feb 21, 2024

anxolin commented Feb 22, 2024

alfetopito left a comment

Add partner fee meta-data #53

Add partner fee meta-data #53

Conversation

anxolin commented Feb 15, 2024 • edited Loading

Validations

Example

github-actions bot commented Feb 15, 2024 • edited Loading

anxolin Feb 15, 2024

Choose a reason for hiding this comment

alfetopito left a comment

Choose a reason for hiding this comment

alfetopito Feb 15, 2024

Choose a reason for hiding this comment

anxolin Feb 15, 2024

Choose a reason for hiding this comment

anxolin commented Feb 15, 2024 • edited Loading

alfetopito Feb 16, 2024

Choose a reason for hiding this comment

anxolin Feb 19, 2024

Choose a reason for hiding this comment

anxolin Feb 19, 2024

Choose a reason for hiding this comment

fleupold commented Feb 19, 2024

anxolin commented Feb 19, 2024

alfetopito commented Feb 19, 2024

anxolin commented Feb 19, 2024

olgafetisova commented Feb 20, 2024

alfetopito commented Feb 20, 2024

olgafetisova commented Feb 20, 2024

anxolin commented Feb 20, 2024

olgafetisova commented Feb 21, 2024

anxolin commented Feb 22, 2024

alfetopito left a comment

Choose a reason for hiding this comment

anxolin commented Feb 15, 2024 •

edited

Loading

github-actions bot commented Feb 15, 2024 •

edited

Loading

anxolin commented Feb 15, 2024 •

edited

Loading