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

feat(yield): return liquidity pools info #99

Merged
merged 8 commits into from
Dec 13, 2024

Conversation

shoom3301
Copy link
Contributor

@shoom3301 shoom3301 commented Nov 25, 2024

There are two new endpoints:

https://bff.barn.cow.fi/42161/yield/pools-average-apr - returns average APR per provider
https://bff.barn.cow.fi/42161/yield/pools - returns pools info (tvl, apr, etc.)

BFF endpoint instead of this mock.
This data is used for Vampire attack.

image

@shoom3301 shoom3301 requested a review from a team November 25, 2024 14:12
@shoom3301 shoom3301 self-assigned this Nov 25, 2024
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.

nice one!

Just a couple of nits

@anxolin
Copy link
Contributor

anxolin commented Dec 2, 2024

I stil don't get what's the API naming convention here. Maybe we can discuss it synchronously while we deploy this

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.

Other than the endpoint names, where to be honest I don't care too much anymore I have some small concern :)

One concern with this PR is that, so far we made that BFF can be easily cloned and launched locally.

This PR introduces one dependency. We need to provide the CoW AMM DB details, or it will try to connect to 127.0.0.1:5432

image

For example, for coingecko proxy we made the env optional, and we just printed an error if its missconfigured

https://github.com/cowprotocol/bff/blob/feat/com-amm-competitor-api/apps/api/src/app/routes/proxies/coingecko/index.ts#L24

Would it make sense to do something like this here?
We can throw an error if the env is not set up for example and show in the logs what's the issue

I will give you the soft approve as I think this PR is quite good.

@@ -20,6 +20,7 @@
"strict": true,
"strictNullChecks": true,
"alwaysStrict": true,
"strictPropertyInitialization": false,
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

@shoom3301 shoom3301 Dec 3, 2024

Choose a reason for hiding this comment

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

image

It works well in twap app, because it has strict: false in its tsconfig.
The only better soultion I have is to move strictPropertyInitialization: false to the api app tsconfig.

@shoom3301
Copy link
Contributor Author

image

@anxolin I've enhanced the db connection error handling

@shoom3301 shoom3301 merged commit 6e1e26a into main Dec 13, 2024
6 checks passed
@shoom3301 shoom3301 deleted the feat/com-amm-competitor-api branch December 13, 2024 09:12
@anxolin
Copy link
Contributor

anxolin commented Dec 19, 2024

@anxolin I've enhanced the db connection error handling

Thanks a lot, looks way better now. Sorry it took me a while to re-review this

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants