Implement partial quote size for sushi market price #431
+180
−16
Conversation
This file contains hidden or 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
Contributor
WalkthroughAdded an Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant SS as SharedState
participant Router as Router
participant Sushi as SushiRouter
Client->>SS: getMarketPrice(fromToken, toToken, ...)
SS->>SS: amountIn = 1 unit
SS->>Router: getMarketPrice(amountIn, gasPrice, routeType, skipFetch)
Router->>Sushi: getMarketPrice(...)
Sushi-->>Router: result (isOk? true/false)
Router-->>SS: result
alt initial fetch failed
SS->>Router: findLargestTradeSize(orderDetails, toToken, fromToken, maximumInputFixed, gasPrice, routeType, absolute)
Router->>Sushi: findLargestTradeSize(...)
Sushi-->>Router: partialAmountIn (bigint | undefined)
Router-->>SS: partialAmountIn
alt partialAmountIn present
SS->>Router: getMarketPrice(partialAmountIn, gasPrice, routeType, skipFetch)
Router->>Sushi: getMarketPrice(...)
Sushi-->>Router: retry result
Router-->>SS: retry result
end
end
SS-->>Client: final result
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/state/index.test.ts`:
- Line 444: The test case title in the unit test function it("should call
getMarketPrice with correct params for partial size uhappy all", ...) contains a
typo; change "uhappy" to "unhappy" in the it(...) description in
src/state/index.test.ts so the test reads it("should call getMarketPrice with
correct params for partial size unhappy all", ...); leave the test
implementation and identifiers (e.g., the it(...) call and any describe blocks)
unchanged.
In `@src/state/index.ts`:
- Around line 332-340: Explain in a short inline comment above the
partialAmountIn call that the mocked orderDetails ({ takeOrder: { quote: {
ratio: 0n } } } as any) is safe because findLargestTradeSize is invoked with
absolute=true, which bypasses any ratio-based comparisons; reference
findLargestTradeSize, partialAmountIn and the absolute=true flag so future
readers know why the cast/as any is acceptable and cannot affect logic.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/state/index.ts`:
- Around line 341-352: The retry path currently treats any non-bigint
partialAmountIn as a guard but misses zero or negative bigints; add a check for
partialAmountIn <= 0n before calling this.router.getMarketPrice and skip the
retry (return result or handle as before) to avoid calling getMarketPrice with
amountIn=0n; locate the variable partialAmountIn (and the call to
this.router.getMarketPrice) and short-circuit when partialAmountIn is not a
positive bigint.
Comment on lines
+341
to
352
| if (typeof partialAmountIn !== "bigint") { | ||
| return result; | ||
| } | ||
| const partialResult = await this.router.getMarketPrice({ | ||
| fromToken, | ||
| toToken, | ||
| blockNumber, | ||
| gasPrice: this.gasPrice, | ||
| amountIn: parseUnits("1", fromToken.decimals), | ||
| amountIn: partialAmountIn, | ||
| sushiRouteType: this.appOptions.route, | ||
| skipFetch: !!skipFetch, | ||
| }); |
Contributor
There was a problem hiding this comment.
Guard against zero-sized partial quotes.
If findLargestTradeSize returns 0n (or another non-positive sentinel), the retry will call getMarketPrice with amountIn=0n, which is likely invalid. Add a non‑positive guard to avoid unnecessary/erroneous calls.
🔧 Suggested fix
- if (typeof partialAmountIn !== "bigint") {
+ if (typeof partialAmountIn !== "bigint" || partialAmountIn <= 0n) {
return result;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| if (typeof partialAmountIn !== "bigint") { | |
| return result; | |
| } | |
| const partialResult = await this.router.getMarketPrice({ | |
| fromToken, | |
| toToken, | |
| blockNumber, | |
| gasPrice: this.gasPrice, | |
| amountIn: parseUnits("1", fromToken.decimals), | |
| amountIn: partialAmountIn, | |
| sushiRouteType: this.appOptions.route, | |
| skipFetch: !!skipFetch, | |
| }); | |
| if (typeof partialAmountIn !== "bigint" || partialAmountIn <= 0n) { | |
| return result; | |
| } | |
| const partialResult = await this.router.getMarketPrice({ | |
| fromToken, | |
| toToken, | |
| blockNumber, | |
| gasPrice: this.gasPrice, | |
| amountIn: partialAmountIn, | |
| sushiRouteType: this.appOptions.route, | |
| skipFetch: !!skipFetch, | |
| }); |
🤖 Prompt for AI Agents
In `@src/state/index.ts` around lines 341 - 352, The retry path currently treats
any non-bigint partialAmountIn as a guard but misses zero or negative bigints;
add a check for partialAmountIn <= 0n before calling this.router.getMarketPrice
and skip the retry (return result or handle as before) to avoid calling
getMarketPrice with amountIn=0n; locate the variable partialAmountIn (and the
call to this.router.getMarketPrice) and short-circuit when partialAmountIn is
not a positive bigint.
Contributor
|
@coderabbitai assess this PR size classification for the totality of the PR with the following criterias and report it in your comment: S/M/L PR Classification Guidelines:This guide helps classify merged pull requests by effort and complexity rather than just line count. The goal is to assess the difficulty and scope of changes after they have been completed. Small (S)Characteristics:
Review Effort: Would have taken 5-10 minutes Examples:
Medium (M)Characteristics:
Review Effort: Would have taken 15-30 minutes Examples:
Large (L)Characteristics:
Review Effort: Would have taken 45+ minutes Examples:
Additional Factors to ConsiderWhen deciding between sizes, also consider:
Notes:
|
Contributor
|
The author of this PR is on the CodeRabbit Free Plan. In order to use the Chat feature, please upgrade the PR author to CodeRabbit Pro by visiting https://app.coderabbit.ai/login. |
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.
Motivation
This PR updates the logic for getting market price quote, where if the unit quote size (1 unit token) fails to give any valid result, partial sizes are also tried.
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.