handle negative amountOut error#452
Conversation
Walkthrough
ChangesSushi Router Negative Output Detection and Reset Signaling
probablyPicksFrom Offset Refactor
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/router/sushi/index.ts (1)
284-295: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClear
signalResetbefore re-initialization to prevent sticky flag on failure.If
RainDataFetcher.init()throws an exception (lines 286-290), execution jumps to the empty catch block at line 293, andsignalResetis never cleared. This leaves the flag permanentlytrue, causing the CLI to attempt a reset every round.Move the flag reset before the risky re-initialization so it's always cleared when a reset is attempted.
🔧 Proposed fix to clear flag before re-initialization
async reset(): Promise<boolean> { try { + this.signalReset = false; this.dataFetcher = await RainDataFetcher.init( this.chainId as ChainId, this.client as any, this.liquidityProviders, ); - this.signalReset = false; return true; } catch {} return false; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/router/sushi/index.ts` around lines 284 - 295, The signalReset flag is set to false after the RainDataFetcher.init() call in the reset() method, but if that initialization throws an exception, the flag is never cleared due to the empty catch block, leaving it permanently true and causing repeated reset attempts. Move the line that sets signalReset to false to execute before the try block containing RainDataFetcher.init(), ensuring the flag is always cleared when a reset is attempted regardless of whether the initialization succeeds or fails.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/router/sushi/index.ts`:
- Line 104: The signalReset property is currently public, which could allow
external code to modify internal router state. Rename the signalReset property
to _signalReset to make it private, then create a public getter method that
returns its value. Update all internal references to signalReset to use the
private _signalReset property instead, including the assignments and reads that
occur throughout the router implementation.
---
Outside diff comments:
In `@src/router/sushi/index.ts`:
- Around line 284-295: The signalReset flag is set to false after the
RainDataFetcher.init() call in the reset() method, but if that initialization
throws an exception, the flag is never cleared due to the empty catch block,
leaving it permanently true and causing repeated reset attempts. Move the line
that sets signalReset to false to execute before the try block containing
RainDataFetcher.init(), ensuring the flag is always cleared when a reset is
attempted regardless of whether the initialization succeeds or fails.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4244b5e3-139e-49fe-b6af-3186eaaaf96f
📒 Files selected for processing (4)
src/cli/index.tssrc/router/sushi/error.tssrc/router/sushi/index.tssrc/rpc/helpers.ts
|
|
||
| /** The sushi router data fetcher instance for interacting */ | ||
| dataFetcher: RainDataFetcher; | ||
| signalReset = false; |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Consider encapsulating signalReset as a private property.
The signalReset flag is currently public, allowing external code to accidentally modify it. Since this is internal router state consumed only by the CLI reset guard, consider making it private with a public getter.
♻️ Proposed encapsulation
-signalReset = false;
+private _signalReset = false;
+
+get signalReset(): boolean {
+ return this._signalReset;
+}Then update lines 240 and 291 to use this._signalReset.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/router/sushi/index.ts` at line 104, The signalReset property is currently
public, which could allow external code to modify internal router state. Rename
the signalReset property to _signalReset to make it private, then create a
public getter method that returns its value. Update all internal references to
signalReset to use the private _signalReset property instead, including the
assignments and reads that occur throughout the router implementation.
1 participant
Motivation
handles negative amountOut error from router
Solution
Checks
By submitting this for review, I'm confirming I've done the following:
Summary by CodeRabbit
Bug Fixes
Refactor