-
Notifications
You must be signed in to change notification settings - Fork 9
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 add pool with hook consider flag #1142
base: v3-canary
Are you sure you want to change the base?
Conversation
🦋 Changeset detectedLatest commit: d200415 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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.
LGTM!
@@ -66,6 +66,7 @@ export class SorService { | |||
wethIsEth: wethIsEth, | |||
} | |||
: undefined, | |||
considerPoolsWithHooks: args.considerPoolsWithHooks ? args.considerPoolsWithHooks : false, |
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.
This works, but just sharing that there are simpler ways of doing this, such as:
args.considerPoolsWithHooks ?? false // this returns the first argument if defined, otherwise false
or
!!args.considerPoolsWithHooks // this is called falsy, which returns false if undefined
@@ -391,7 +391,7 @@ export const prismaPoolBatchSwapWithSwaps = Prisma.validator<Prisma.PrismaPoolBa | |||
|
|||
export type PrismaPoolBatchSwapWithSwaps = Prisma.PrismaPoolBatchSwapGetPayload<typeof prismaPoolBatchSwapWithSwaps>; | |||
|
|||
export const prismaPoolWithDynamic = Prisma.validator<Prisma.PrismaPoolArgs>()({ | |||
export const prismaPoolWithDynamicAndHook = Prisma.validator<Prisma.PrismaPoolArgs>()({ |
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.
I'd say prismaPoolAndHookWithDynamic
follows the existing pattern a bit better.
@franzns - since this is more of a backend/db thing than an SOR thing, do you have a preference?
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.
No strong opinion
)) as PathWithAmount[]; | ||
expect(paths.length).toBeGreaterThan(0); | ||
}); | ||
test('SOR does not consider pools with hooks - when hook available', async () => { |
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.
These tests are not yet done, right?
This pr adds optionality for pools with hooks to be either considered or not considered when the api uses the sor