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 Beethoven X Cache Issue [TKR-486] #519

Merged
merged 4 commits into from
Jul 26, 2022
Merged

Fix Beethoven X Cache Issue [TKR-486] #519

merged 4 commits into from
Jul 26, 2022

Conversation

kyu-c
Copy link
Contributor

@kyu-c kyu-c commented Jul 19, 2022

Description

Bug Fix:

  • Replace Beethoven X subgraph .

Improvements:

  • Do not create BalancerV2PoolsCache on unsupported chains.
  • Add BalancerV2PoolsCache test for Beethoven X.
  • Remove BalancerV2PoolsCache test for Balancer V2 (no longer used in production).
  • Simplify PoolsCacheMap type.

Testing instructions

Types of changes

Checklist:

  • Prefix PR title with [WIP] if necessary.
  • Add tests to cover changes as needed.
  • Update documentation as needed.
  • Add new entries to the relevant CHANGELOG.jsons. (will be updated before this PR is merged)

@kyu-c kyu-c requested review from dekz and dextracker as code owners July 19, 2022 01:13
@kyu-c kyu-c requested review from idokleinman and removed request for dekz and dextracker July 19, 2022 01:14
public static createBeethovenXPoolCache(chainId: ChainId): BalancerV2PoolsCache | undefined {
const subgraphUrl = BEETHOVEN_X_SUBGRAPH_URL_BY_CHAIN.get(chainId);
if (subgraphUrl === undefined) {
return undefined;
Copy link
Member

@dekz dekz Jul 19, 2022

Choose a reason for hiding this comment

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

What are your thoughts on usage of undefined versus a default identity like cache that implements PoolCache but always returns empty results?

It would reduce the flow on effects of it being undefined throughout the rest of the code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I like the approach!

I might need to refactor PoolCache a bit more first.
will update the PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually there are other related cache stuff that I want to modify as well.

Do you mind if I address undefined vs no-op PoolCache in a separate PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

addressed in #525 and #526

@kyu-c kyu-c force-pushed the fix/beets-cache branch from 5a694f0 to 03e9ebc Compare July 21, 2022 21:42
@kyu-c kyu-c mentioned this pull request Jul 22, 2022
4 tasks
kyu-c added 3 commits July 25, 2022 21:22
* `BalancerV2PoolsCache` was created regardless of the chain id resulting in
unecessary cache update.
* Remove `BalancerV2PoolsCache` Balancer mainnet test since it's not used in prod.
@kyu-c kyu-c force-pushed the fix/beets-cache branch from 03e9ebc to 80ebb3a Compare July 26, 2022 04:23
Copy link
Member

@dekz dekz left a comment

Choose a reason for hiding this comment

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

Nice, left some nits, looking good to me.

chainId,
BEETHOVEN_X_SUBGRAPH_URL_BY_CHAIN[chainId],
),
[ERC20BridgeSource.Beethovenx]: BalancerV2PoolsCache.createBeethovenXPoolCache(chainId),
Copy link
Member

Choose a reason for hiding this comment

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

So only Beethovenx uses BalancerV2PoolsCache now? Since BalancerV2 uses the newer BalancerV2SwapInfoCache? If that is the case I'd ultimately recommend a rename.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's the case!

I will rename in a follow up PR.


import { parsePoolData } from './balancer_sor_v2';
import { CacheValue, PoolsCache } from './pools_cache';

const BEETHOVEN_X_SUBGRAPH_URL_BY_CHAIN = new Map<ChainId, string>([
[ChainId.Fantom, 'https://api.thegraph.com/subgraphs/name/beethovenxfi/beethovenx'],
Copy link
Member

Choose a reason for hiding this comment

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

I'm on the fence about having the config/urls/addresses in files outside of packages/asset-swapper/src/utils/market_operation_utils/constants.ts. Do you have a preference?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

For the constants that are only used in a single place, I prefer those constants to be defined in the same file as it's used and not exported.

Ideally, if it's for sampling of a liquidity source, all related constants and logic (router addresses, pool info, gas schedule) are located in a single file specific to that source (when a new source is added, it should be mainly adding a single file vs touching multiple parts of the code base).

Let's discuss more on Slack!

@kyu-c kyu-c merged commit 6ca14ed into development Jul 26, 2022
@kyu-c kyu-c deleted the fix/beets-cache branch July 26, 2022 16:54
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.

2 participants