-
Notifications
You must be signed in to change notification settings - Fork 10
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
Add support for FX pools #29
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall nice changes! I wonder whether we should extract the json thing to it's own PR and make it for all pool types, maybe even including pool specific dynamic data.
modules/pool/subgraph-mapper.ts
Outdated
nestedPools: { id: string; address: string }[], | ||
) => { | ||
const type = mapSubgraphPoolTypeToPoolType(pool.poolType!); | ||
const version = mapPoolTypeVersion(type, pool.poolTypeVersion!); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
need to pass pool.poolType
other wise it wont ever have phantonStable as pool type
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed
modules/pool/subgraph-mapper.ts
Outdated
import { Chain, PrismaPoolType } from '@prisma/client'; | ||
import { BalancerPoolFragment } from '../subgraphs/balancer-subgraph/generated/balancer-subgraph-types'; | ||
import { AddressZero } from '@ethersproject/constants'; | ||
import * as dataMappers from './pool-data'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we explicitely import the dataMappers instead of doing it with wildcards and exports?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
absolutely, we have the option to individually import all the dataMappers. before I change that, let me ask what you think. my rationale for using a namespace is to keep it clear where these functions are coming from. especially around handling types. to make it work with individual imports i'd probably need to add a suffix, like fxDataMapper
and it would end up quite similar to the current dataMappers.fx
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated to have all the imported functions in the header.
modules/pool/subgraph-mapper.ts
Outdated
const prismaPoolRecordWithAssociations = { | ||
data: { | ||
...dbData.base, | ||
data: dbData.data, // DISCUSS: simplify DB schema by migrating from individual tables to a JSON column with types enforced on read. And same with dynamic data. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few thoughts:
- instead if calling it
data
we should call itpoolTypeSpecificData
- If I understand this correctly, fxPool type data is only under
data
but linear, stable etc. speicifc data is under their own tables AND underdata
? I wonder if we should remove thedata
from this PR and export that change into its own PR, also removing the pool type specific tables. - Adding to above suggested to extract that change to its own PR: It could then also get rid of the pool type specific dynamic data tables and have a
poolTypeSpecificDynamicData
column (or table? thinking about update patterns) - Are we sure that we dont use/aggregate the data in the pool type specific tables? (or dynamic data tables?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
data column in now renamed to poolTypeSpecificData. Ready to start the work on migrating other tables in the new PR.
@@ -192,18 +132,13 @@ export class PoolCreatorService { | |||
await prismaBulkExecuteOperations(operations); | |||
} | |||
|
|||
private async createPoolRecord(pool: BalancerPoolFragment, blockNumber: number) { | |||
const poolType = this.mapSubgraphPoolTypeToPoolType(pool.poolType || ''); | |||
private async createPoolRecord( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
also needs to pass the nestedPools when called in syncNewPoolsFromSubgraph
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point, fixed
expect(result.data.type).toBe('WEIGHTED'); | ||
}); | ||
|
||
it('should return correct object for stable pool', () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
add test for phantom stable, will probably fail
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added one and it was failing indeed.
@@ -65,7 +66,7 @@ model PrismaPoolLinearData { | |||
|
|||
id String | |||
poolId String | |||
pool PrismaPool @relation(fields:[poolId, chain], references: [id, chain]) | |||
pool PrismaPool @relation(fields:[poolId, chain], references: [id, chain], onDelete: Cascade) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since there are delete cascade now, maybe we can also simplify poolService.deletePool()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true, cleaned it up a bit
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall nice changes! I wonder whether we should extract the json thing to it's own PR and make it for all pool types, maybe even including pool specific dynamic data.
covered by syncAllPoolsFromSubgraph
covered by syncAllPoolsFromSubgraph
covered by syncAllPools
to have references in the header
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shall we leave out the dynamicData for now?
Also, we are now saving the poolSpecificData for all pool types, but are not reading it. Only for fx pools. Are you planning to extract the 'jsob' from this PR or are you creating a new PR with that uses the data on the 'jsonb' column? I would not add it here, as this PR is already big enough..
export type StableDynamicData = ReturnType<typeof stableDynamic>; | ||
export type LinearDynamicData = ReturnType<typeof linearDynamic>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we remove this? it's unused afaik.
modules/pool/subgraph-mapper.ts
Outdated
const dynamicMapper = { | ||
STABLE: dataMappers.stableDynamic, | ||
COMPOSABLE_STABLE: dataMappers.stableDynamic, | ||
META_STABLE: dataMappers.stableDynamic, | ||
LINEAR: dataMappers.linearDynamic, | ||
STABLE: stableDynamic, | ||
COMPOSABLE_STABLE: stableDynamic, | ||
META_STABLE: stableDynamic, | ||
LINEAR: linearDynamic, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we remove this? it's unused afaik.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mapper is subgraph schema related, so more data we parse to DB schema the better. think about it as a layer. it makes it easy to compose what is needed for any pool syncing we would want to do.
we still read these data from subgraph in pool create jobs, once removed where would it go? just keep it blank until onchain calls fill it in?
I'd like to merge it as it is. Next steps:
Perhaps in 2 PRs, to keep it easy on us. |
This pull request adds:
This helps with testing and adding updates, or upserts will be easier, since we have the clean data object.