feat: build core pools with apiv3 as only source #73
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
gosuto-inzasheru
changed the title
|
blocked by balancer/backend#1519 |
|
this is good to go, just need twap test fixed first |
|
@gosuto-inzasheru i added a temp check to the twap price tests that makes sure the timestamps are less than 100 days old or will be skipped. the fact that the stable pools are <0.01% price diff and the non stable ones are off by a consistent amount means that the twap price function is working fine and just an issue with stale test data. can implement a full solution in #75 lets get this merged in the meantime so that BalancerMaxis/protocol_fee_allocator#337 isnt being blocked. reviewing now... |
jalbrekt85
reviewed
Jan 28, 2025
jalbrekt85
reviewed
Jan 28, 2025
Comment on lines
+374
to
+379
| if ( | ||
| pool_token["priceRateProviderData"].get("address") | ||
| != ZERO_ADDRESS | ||
| ): | ||
| # pools with a non zero rate provider are core pools | ||
| core_pools.append(pool) |
There was a problem hiding this comment.
shouldnt the check be for at least for half of tokens to have rate provider based on BIP-734?
Minimum 50% yield-bearing or boosted tokens for pool types such as weighted or composable stable pools
There was a problem hiding this comment.
yea that is true. in the previous direct subgraph implementation calcing this was practically impossible
i think the apiv3 would make it a lot easier now, but we are also not getting any false positives because of not having it (yet)
created a ticket for it so we can handle it later when it matter/there is more time: #76
|
here is what the core pool list looks like now based on this branch: |
|
@jalbrekt85 yes that all looks good. i did a test run already and the output was reviewed by @Xeonus: BalancerMaxis/bal_addresses#588 all the (new) gnosis ones look good; they have the necessary tvl and have rate providers. im actually suspecting a bug in the old logic---not sure |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
closes #72 and closes #39