Conversation
- Implemented `encodeV2RouteData` function to encode route data based on pool variant. - Enhanced V2 pool metadata discovery to include Solidly pool variants and their fees. - Updated V2 quote functions to handle Solidly stable swaps and introduced stable pool calculations. - Refactored event monitoring to reconcile addresses and manage hydration floor for logs. - Added tests for V2 metadata discovery, Solidly pool handling, and updated existing tests to include new variants. - Introduced new V3 pools and updated event handling to support new features.
Reviewer's GuideAdds Solidly stable/volatile V2 pool support end‑to‑end (discovery, quoting, execution, event monitoring, persistence), improves V3/Carbon callbacks and cursoring, and wires refreshed opportunities into execution with expanded token/uniswap V3 coverage. Sequence diagram for Solidly stable V2 swap executionsequenceDiagram
participant OpportunityEngine
participant ArbitrageExecutor
participant IBaseV1Pair
OpportunityEngine->>OpportunityEngine: encodeV2RouteData(variant)
OpportunityEngine->>ArbitrageExecutor: executeArbitrage(params) with FlashData.data including quoteData
loop circular route
ArbitrageExecutor->>ArbitrageExecutor: _executeCircularRoute(loan)
alt protocol is V2
ArbitrageExecutor->>ArbitrageExecutor: _swapV2(tokenIn, amountIn, pairAddr, fee, quoteData, recipient, inputAlreadySent)
ArbitrageExecutor->>ArbitrageExecutor: _quoteV2(pair, tokenIn, amountIn, fee, quoteData)
alt quoteData indicates stable (0x01)
ArbitrageExecutor->>ArbitrageExecutor: _quoteStableV2(pairAddr, tokenIn, amountIn, fee)
ArbitrageExecutor->>IBaseV1Pair: metadata()
IBaseV1Pair-->>ArbitrageExecutor: StablePairState
ArbitrageExecutor->>ArbitrageExecutor: _stableAmountOut(amountIn, reserveIn, reserveOut, scaleIn, scaleOut, fee)
ArbitrageExecutor-->>ArbitrageExecutor: tokenOut, amountOut, zeroForOne
else volatile or uniswap-v2
ArbitrageExecutor->>ArbitrageExecutor: getReserves(), swapV2 formula
ArbitrageExecutor-->>ArbitrageExecutor: tokenOut, amountOut, zeroForOne
end
ArbitrageExecutor->>IUniswapV2Pair: swap(amount0Out, amount1Out, recipient, hex"")
else protocol is V3 or carbon
ArbitrageExecutor->>ArbitrageExecutor: _swapV3 or _swapCarbon
end
end
ArbitrageExecutor-->>OpportunityEngine: flash loan repaid via _v2RepayAmount and profits realized
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 4 issues, and left some high level feedback:
- The Uniswap V3
slot0ABI inUniswapFlashQuery.solwas reduced to only two return values; for compatibility with real pools and other tooling you should keep the full 7-field return tuple and just ignore unused fields in your code. - The Solidly stable math is now duplicated between Solidity (
_stableAmountOut/_stableY) and TypeScript (swapSolidlyStable/stableY); consider centralizing constants and adding a clearly shared spec/helper to reduce the risk of the two implementations drifting over time.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The Uniswap V3 `slot0` ABI in `UniswapFlashQuery.sol` was reduced to only two return values; for compatibility with real pools and other tooling you should keep the full 7-field return tuple and just ignore unused fields in your code.
- The Solidly stable math is now duplicated between Solidity (`_stableAmountOut` / `_stableY`) and TypeScript (`swapSolidlyStable` / `stableY`); consider centralizing constants and adding a clearly shared spec/helper to reduce the risk of the two implementations drifting over time.
## Individual Comments
### Comment 1
<location path="test/v2.test.ts" line_range="59-68" />
<code_context>
)).toBe(26036525510536776183643n);
});
+test("stable V2 quote matches the historical Yaka pool output", () => {
+ expect(swapSolidlyStable(12_271_683n, {
+ variant: 'solidly-stable',
+ reserveIn: 21_501_234n,
+ reserveOut: 149_089_569n,
+ scaleIn: 1_000_000n,
+ scaleOut: 1_000_000n,
+ fee: 4,
+ })).toBe(22_886_491n);
+ expect(encodeV2RouteData('solidly-stable')).toBe('0x01');
+ expect(encodeV2RouteData('solidly-volatile')).toBe('0x');
+});
+
</code_context>
<issue_to_address>
**suggestion (testing):** Add tests for Solidly stable quote edge cases and solver failure modes
Currently `swapSolidlyStable` is only covered by a single happy‑path case. Given the guard clauses and the iterative `stableY` solver that can throw, please add tests that:
- Verify inputs where any of `amountIn`, `reserveIn`, `reserveOut`, `scaleIn`, or `scaleOut` are `0n` return `0n` as expected.
- Cover a configuration where `stableD` returns `0n` and assert that `swapSolidlyStable`/`quoteV2ExactInput` throws the expected error.
- Cover a configuration where the solver hits the iteration cap and throws `"Stable pool solver did not converge"`.
These cases help prevent future changes from introducing silent misquotes or unhandled runtime errors.
Suggested implementation:
```typescript
test("stable V2 quote matches the historical Yaka pool output", () => {
expect(
swapSolidlyStable(12_271_683n, {
variant: "solidly-stable",
reserveIn: 21_501_234n,
reserveOut: 149_089_569n,
scaleIn: 1_000_000n,
scaleOut: 1_000_000n,
fee: 4,
}),
).toBe(22_886_491n);
expect(encodeV2RouteData("solidly-stable")).toBe("0x01");
expect(encodeV2RouteData("solidly-volatile")).toBe("0x");
});
test("stable V2 quote returns 0n when any numeric input is zero", () => {
const baseConfig = {
variant: "solidly-stable" as const,
reserveIn: 21_501_234n,
reserveOut: 149_089_569n,
scaleIn: 1_000_000n,
scaleOut: 1_000_000n,
fee: 4,
};
// amountIn guard
expect(swapSolidlyStable(0n, baseConfig)).toBe(0n);
// reserveIn guard
expect(
swapSolidlyStable(12_271_683n, {
...baseConfig,
reserveIn: 0n,
}),
).toBe(0n);
// reserveOut guard
expect(
swapSolidlyStable(12_271_683n, {
...baseConfig,
reserveOut: 0n,
}),
).toBe(0n);
// scaleIn guard
expect(
swapSolidlyStable(12_271_683n, {
...baseConfig,
scaleIn: 0n,
}),
).toBe(0n);
// scaleOut guard
expect(
swapSolidlyStable(12_271_683n, {
...baseConfig,
scaleOut: 0n,
}),
).toBe(0n);
});
test("stable V2 quote throws when stableD returns 0n", () => {
const config = {
variant: "solidly-stable" as const,
reserveIn: 1n,
reserveOut: 1n,
// A configuration that should drive the invariant computation to D = 0n.
// This may need tuning against the implementation of `stableD`.
scaleIn: 0n,
scaleOut: 0n,
fee: 4,
};
expect(() => swapSolidlyStable(1_000n, config)).toThrow(
"Stable pool invariant D is zero",
);
});
test("stable V2 quote throws when the stable solver does not converge", () => {
const config = {
variant: "solidly-stable" as const,
// Use extreme reserves/scales to force the iterative solver to hit its cap.
// Exact values may need to be tuned alongside the implementation of `stableY`.
reserveIn: 10_000_000_000_000_000_000_000n,
reserveOut: 1n,
scaleIn: 1_000_000n,
scaleOut: 1_000_000n,
fee: 4,
};
expect(() =>
swapSolidlyStable(10_000_000_000_000_000_000_000n, config),
).toThrow("Stable pool solver did not converge");
});
```
- The `"Stable pool invariant D is zero"` error message in the second new test must match the actual error string thrown when `stableD` returns `0n`. If the implementation uses a different message, adjust the `toThrow(...)` argument accordingly.
- The non-convergence test uses extreme numeric values to try to force the solver to hit its iteration cap. Depending on the concrete implementation of `stableY` and the cap, you may need to tweak `reserveIn`, `reserveOut`, `scaleIn`, `scaleOut`, and `amountIn` to reliably trigger the `"Stable pool solver did not converge"` path.
- If `quoteV2ExactInput` is the higher-level entry point that wraps `swapSolidlyStable`, consider adding analogous `toThrow(...)` expectations in these tests using the correct route/config structure required by `quoteV2ExactInput` (e.g., building a V2 route with `variant: "solidly-stable"` and the same reserves/scales).
</issue_to_address>
### Comment 2
<location path="test/v2-metadata.test.ts" line_range="38" />
<code_context>
+test('Solidly discovery reads factory fees once and persists stable metadata', async () => {
</code_context>
<issue_to_address>
**suggestion (testing):** Exercise Solidly metadata normalization for tuple ABI and reverted ranges
This test exercises the object-shaped `metadata()` and fee lookup, but it doesn’t cover the tuple ABI form or the Solidly-specific revert handling in `getPairsInRange`.
Please add:
- A variant where `metadata` returns the tuple ABI (`readonly [bigint, ...]`) to verify index-to-field mapping and persistence of `scale0/scale1`.
- A Solidly revert scenario (analogous to the Dragon test) to confirm recursive range splitting behaves correctly when `fees` are non-null and that stable/volatile classification remains correct after the split.
These additions would more fully cover the new Solidly discovery logic and both ABI shapes.
```suggestion
test('Solidly metadata normalizes tuple ABI and persists scale fields', async () => {
const client = {
async readContract({ functionName, args }: any): Promise<unknown> {
// Solidly factory length + range (simple, non-splitting case)
if (functionName === 'getPairsLength') return [0n, 2n];
if (functionName === 'getPairsByIndexRange') {
expect(args).toEqual([0n, 2n]);
// tuple ABI form: readonly [token0, token1, stable, scale0, scale1]
// NOTE: adapt tuple layout to match the actual Solidly metadata ABI.
return [
// volatile pair (stable = false), scale0/scale1 preserved
[address(1), address(2), false, 18n, 18n],
// stable pair (stable = true), asymmetric scale
[address(3), address(4), true, 6n, 18n],
];
}
if (functionName === 'filterVolatileHermesPairs') {
// only the volatile pair (index 0) is considered volatile Hermes
return [true, false];
}
throw new Error(`Unexpected ${functionName}`);
},
};
const pools = await discoverV2PoolMetadata(client);
// We expect two pools discovered: one volatile, one stable,
// with scale0 / scale1 propagated from the tuple ABI.
expect(pools).toHaveLength(2);
const volatile = pools.find((p) => !p.stable);
const stable = pools.find((p) => p.stable);
expect(volatile).toBeDefined();
expect(stable).toBeDefined();
// Tuple ABI index-to-field mapping: scale0/scale1 must be persisted.
expect(volatile?.scale0).toBe(18n);
expect(volatile?.scale1).toBe(18n);
expect(stable?.scale0).toBe(6n);
expect(stable?.scale1).toBe(18n);
});
test('Solidly discovery handles reverted ranges with fees and preserves stable metadata', async () => {
const feeCalls: boolean[] = [];
let firstRangeCall = true;
const client = {
async readContract({ functionName, args }: any): Promise<unknown> {
if (functionName === 'getPairsLength') {
// 3 pairs: indices 0..2
return [0n, 3n];
}
if (functionName === 'getPairsByIndexRange') {
const [start, end] = args as [bigint, bigint];
// Simulate a Solidly-specific revert when querying the full [0, 3) range,
// forcing recursive splitting into smaller subranges.
if (firstRangeCall && start === 0n && end === 3n) {
firstRangeCall = false;
const error = new Error('Solidly: range too large');
// If your Solidly adapter uses error codes / specific properties,
// set them here (e.g. (error as any).code = 'CALL_EXCEPTION').
throw error;
}
// After the split, we expect the adapter to query [0, 1), [1, 2), [2, 3)
if (start === 0n && end === 1n) {
return [[address(1), address(2), address(11)]];
}
if (start === 1n && end === 2n) {
return [[address(3), address(4), address(12)]];
}
if (start === 2n && end === 3n) {
return [[address(5), address(6), address(13)]];
}
throw new Error(`Unexpected getPairsByIndexRange args: ${args}`);
}
if (functionName === 'filterVolatileHermesPairs') {
// First two pairs are volatile, third is stable
return [true, true, false];
}
if (functionName === 'getFee') {
feeCalls.push(true);
// Non-null fees for all pairs to ensure the revert path is exercised
// with fee lookup active.
return 3n;
}
throw new Error(`Unexpected ${functionName}`);
},
};
const pools = await discoverV2PoolMetadata(client);
// We still discover all three pools, even though the initial range reverted.
expect(pools).toHaveLength(3);
// Fees should be fetched once per discovered pair.
expect(feeCalls).toHaveLength(3);
// Stable/volatile classification must remain correct after the split.
const stable = pools.filter((p) => p.stable);
const volatile = pools.filter((p) => !p.stable);
expect(stable).toHaveLength(1);
expect(volatile).toHaveLength(2);
// Ensure metadata (including stability) is persisted correctly for the stable pool.
expect(stable[0].token0).toBe(address(5));
expect(stable[0].token1).toBe(address(6));
});
test('Solidly discovery reads factory fees once and persists stable metadata', async () => {
```
</issue_to_address>
### Comment 3
<location path="test/execution-lock.test.ts" line_range="36-45" />
<code_context>
await first;
});
+
+test("submits the refreshed opportunity", async () => {
+ let submittedProfit = 0n;
+ const manager = new OpportunityManager(
+ {} as never,
+ async (_graph, refreshed) => {
+ submittedProfit = refreshed.profit;
+ return true;
+ },
+ async stale => ({ ...stale, profit: 2n })
+ );
+
+ await manager.processOpportunities({} as never, [opportunity]);
+
+ expect(submittedProfit).toBe(2n);
+});
</code_context>
<issue_to_address>
**suggestion (testing):** Consider a test where the refresh hook returns null to assert pairs are released and nothing is submitted
To fully cover the refresh flow, this should be complemented with a test where the refresh callback returns `null`, asserting that `submitOpportunity` is not called and, if easy to verify, that `releasePairs` is invoked for the stale opportunity’s pairs. This will validate behavior for the "no longer valid" case and help prevent regressions where stale opportunities are accidentally submitted.
Suggested implementation:
```typescript
test("submits the refreshed opportunity", async () => {
let submittedProfit = 0n;
const manager = new OpportunityManager(
{} as never,
async (_graph, refreshed) => {
submittedProfit = refreshed.profit;
return true;
},
async stale => ({ ...stale, profit: 2n })
);
await manager.processOpportunities({} as never, [opportunity]);
expect(submittedProfit).toBe(2n);
});
test("does not submit and releases pairs when refreshed opportunity becomes invalid", async () => {
let submitCalled = false;
const graph = {
releasePairs: jest.fn(),
} as never;
const manager = new OpportunityManager(
graph,
async () => {
submitCalled = true;
return true;
},
async () => null
);
await manager.processOpportunities(graph, [opportunity]);
expect(submitCalled).toBe(false);
expect(graph.releasePairs).toHaveBeenCalled();
});
```
- Adjust the shape of `graph` to match your actual execution graph type: e.g. if `releasePairs` expects specific arguments (such as `opportunity.pairs` or an ID), update the `toHaveBeenCalled` assertion accordingly (e.g. `toHaveBeenCalledWith(opportunity.pairs)`).
- If `OpportunityManager.processOpportunities` does not take the graph as the first argument (or if the graph is stored internally), adapt the calls to `new OpportunityManager(...)` and `processOpportunities(...)` to match the real signature.
- If `submitOpportunity` is expected to return a boolean that affects further behavior, and your manager relies on that, ensure the test's submit callback still respects the expected contract (e.g. keep `return true;` if needed).
</issue_to_address>
### Comment 4
<location path="src/runtime/event-monitor.ts" line_range="27" />
<code_context>
private firstBufferedBlock: bigint | null = null;
private lastBufferedBlock: bigint | null = null;
private bufferedLogCount = 0;
+ private hydrationFloor = 0n;
constructor(
</code_context>
<issue_to_address>
**issue (complexity):** Consider introducing shared helpers for cursor updates and hydration-floor checks to avoid duplicated logic and make state transitions easier to reason about.
The main added complexity comes from:
- `hydrationFloor` checks being duplicated in `activate` and `freshLogs`
- multiple cursor mutation paths (`markApplied`, `markReconciled`, implicit `advanceCursor` calls)
You can reduce complexity without changing behavior by centralizing:
1. cursor updates into a single helper
2. hydration-floor filtering into a single helper
### 1. Unify cursor updates
Introduce a single `updateCursorForAddress` helper that both log-based and reconciliation-based paths use, instead of manual `this.cursors.set(...)` and `markReconciled` having its own logic:
```ts
private updateCursorForAddress(
adapter: ProtocolEventAdapter,
address: string,
blockNumber: bigint,
transactionIndex: number,
logIndex: number
): void {
const key = `${adapter.id}:${address.toLowerCase()}`;
const existing = this.cursors.get(key);
const next: ChainCursor = { blockNumber, transactionIndex, logIndex };
if (!existing || isLogAfterCursor(next, existing)) {
this.cursors.set(key, next);
}
}
```
Then:
```ts
private markReconciled(adapter: ProtocolEventAdapter, address: Address, blockNumber: bigint): void {
this.updateCursorForAddress(
adapter,
address,
blockNumber,
Number.MAX_SAFE_INTEGER,
Number.MAX_SAFE_INTEGER
);
}
// Replace markApplied's cursor logic:
private markApplied(adapter: ProtocolEventAdapter, log: any): void {
const address = log.address?.toLowerCase();
if (!address) return;
this.updateCursorForAddress(
adapter,
address,
chainLogBlockNumber(log),
log.transactionIndex,
log.logIndex
);
}
```
And in `freshLogs`:
```ts
const address = log.address?.toLowerCase();
if (!address) continue;
const blockNumber = chainLogBlockNumber(log);
if (!this.shouldProcessBlock(blockNumber)) continue;
this.updateCursorForAddress(adapter, address, blockNumber, log.transactionIndex, log.logIndex);
logs[count++] = log;
```
This removes the separate cursor key shape and logic from `markReconciled`, and makes cursor invariants easier to reason about because there’s exactly one place where they’re updated.
### 2. Centralize hydration-floor filtering
Right now you manually check `chainLogBlockNumber(...) <= this.hydrationFloor` in two places. Extract that into a helper so the “hydration floor” concept is defined once:
```ts
private shouldProcessBlock(blockNumber: bigint): boolean {
return blockNumber > this.hydrationFloor;
}
```
Then in `activate`:
```ts
async activate(hydrationFloor = 0n): Promise<void> {
if (!this.running || !this.buffering) return;
if (hydrationFloor > this.hydrationFloor) this.hydrationFloor = hydrationFloor;
while (this.buffered.size > 0) {
const entries = [...this.buffered.values()].sort((a, b) => compareChainLogs(a.log, b.log));
this.buffered.clear();
const byAdapter = new Map<ProtocolEventAdapter, any[]>();
for (const entry of entries) {
if (!this.shouldProcessBlock(chainLogBlockNumber(entry.log))) continue;
this.markApplied(entry.adapter, entry.log);
const logs = byAdapter.get(entry.adapter);
if (logs) logs.push(entry.log);
else byAdapter.set(entry.adapter, [entry.log]);
}
await Promise.all([...byAdapter].map(([adapter, logs]) => adapter.reconcile(logs)));
}
this.buffering = false;
// ...
}
```
And in `freshLogs`:
```ts
private freshLogs(adapter: ProtocolEventAdapter, logs: any[]): any[] {
let count = 0;
for (const log of logs) {
const blockNumber = chainLogBlockNumber(log);
if (!this.shouldProcessBlock(blockNumber)) continue;
const address = log.address?.toLowerCase();
if (!address) continue;
this.updateCursorForAddress(adapter, address, blockNumber, log.transactionIndex, log.logIndex);
logs[count++] = log;
}
logs.length = count;
return logs;
}
```
This keeps the hydration behavior intact but makes it explicit and localized, reducing the “global-ish” feel and making future changes to floor semantics safer.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+59
to
+68
| test("stable V2 quote matches the historical Yaka pool output", () => { | ||
| expect(swapSolidlyStable(12_271_683n, { | ||
| variant: 'solidly-stable', | ||
| reserveIn: 21_501_234n, | ||
| reserveOut: 149_089_569n, | ||
| scaleIn: 1_000_000n, | ||
| scaleOut: 1_000_000n, | ||
| fee: 4, | ||
| })).toBe(22_886_491n); | ||
| expect(encodeV2RouteData('solidly-stable')).toBe('0x01'); |
There was a problem hiding this comment.
suggestion (testing): Add tests for Solidly stable quote edge cases and solver failure modes
Currently swapSolidlyStable is only covered by a single happy‑path case. Given the guard clauses and the iterative stableY solver that can throw, please add tests that:
- Verify inputs where any of
amountIn,reserveIn,reserveOut,scaleIn, orscaleOutare0nreturn0nas expected. - Cover a configuration where
stableDreturns0nand assert thatswapSolidlyStable/quoteV2ExactInputthrows the expected error. - Cover a configuration where the solver hits the iteration cap and throws
"Stable pool solver did not converge".
These cases help prevent future changes from introducing silent misquotes or unhandled runtime errors.
Suggested implementation:
test("stable V2 quote matches the historical Yaka pool output", () => {
expect(
swapSolidlyStable(12_271_683n, {
variant: "solidly-stable",
reserveIn: 21_501_234n,
reserveOut: 149_089_569n,
scaleIn: 1_000_000n,
scaleOut: 1_000_000n,
fee: 4,
}),
).toBe(22_886_491n);
expect(encodeV2RouteData("solidly-stable")).toBe("0x01");
expect(encodeV2RouteData("solidly-volatile")).toBe("0x");
});
test("stable V2 quote returns 0n when any numeric input is zero", () => {
const baseConfig = {
variant: "solidly-stable" as const,
reserveIn: 21_501_234n,
reserveOut: 149_089_569n,
scaleIn: 1_000_000n,
scaleOut: 1_000_000n,
fee: 4,
};
// amountIn guard
expect(swapSolidlyStable(0n, baseConfig)).toBe(0n);
// reserveIn guard
expect(
swapSolidlyStable(12_271_683n, {
...baseConfig,
reserveIn: 0n,
}),
).toBe(0n);
// reserveOut guard
expect(
swapSolidlyStable(12_271_683n, {
...baseConfig,
reserveOut: 0n,
}),
).toBe(0n);
// scaleIn guard
expect(
swapSolidlyStable(12_271_683n, {
...baseConfig,
scaleIn: 0n,
}),
).toBe(0n);
// scaleOut guard
expect(
swapSolidlyStable(12_271_683n, {
...baseConfig,
scaleOut: 0n,
}),
).toBe(0n);
});
test("stable V2 quote throws when stableD returns 0n", () => {
const config = {
variant: "solidly-stable" as const,
reserveIn: 1n,
reserveOut: 1n,
// A configuration that should drive the invariant computation to D = 0n.
// This may need tuning against the implementation of `stableD`.
scaleIn: 0n,
scaleOut: 0n,
fee: 4,
};
expect(() => swapSolidlyStable(1_000n, config)).toThrow(
"Stable pool invariant D is zero",
);
});
test("stable V2 quote throws when the stable solver does not converge", () => {
const config = {
variant: "solidly-stable" as const,
// Use extreme reserves/scales to force the iterative solver to hit its cap.
// Exact values may need to be tuned alongside the implementation of `stableY`.
reserveIn: 10_000_000_000_000_000_000_000n,
reserveOut: 1n,
scaleIn: 1_000_000n,
scaleOut: 1_000_000n,
fee: 4,
};
expect(() =>
swapSolidlyStable(10_000_000_000_000_000_000_000n, config),
).toThrow("Stable pool solver did not converge");
});- The
"Stable pool invariant D is zero"error message in the second new test must match the actual error string thrown whenstableDreturns0n. If the implementation uses a different message, adjust thetoThrow(...)argument accordingly. - The non-convergence test uses extreme numeric values to try to force the solver to hit its iteration cap. Depending on the concrete implementation of
stableYand the cap, you may need to tweakreserveIn,reserveOut,scaleIn,scaleOut, andamountInto reliably trigger the"Stable pool solver did not converge"path. - If
quoteV2ExactInputis the higher-level entry point that wrapsswapSolidlyStable, consider adding analogoustoThrow(...)expectations in these tests using the correct route/config structure required byquoteV2ExactInput(e.g., building a V2 route withvariant: "solidly-stable"and the same reserves/scales).
| expect(filterCalls).toBe(0); | ||
| }); | ||
|
|
||
| test('Solidly discovery reads factory fees once and persists stable metadata', async () => { |
There was a problem hiding this comment.
suggestion (testing): Exercise Solidly metadata normalization for tuple ABI and reverted ranges
This test exercises the object-shaped metadata() and fee lookup, but it doesn’t cover the tuple ABI form or the Solidly-specific revert handling in getPairsInRange.
Please add:
- A variant where
metadatareturns the tuple ABI (readonly [bigint, ...]) to verify index-to-field mapping and persistence ofscale0/scale1. - A Solidly revert scenario (analogous to the Dragon test) to confirm recursive range splitting behaves correctly when
feesare non-null and that stable/volatile classification remains correct after the split.
These additions would more fully cover the new Solidly discovery logic and both ABI shapes.
Suggested change
| test('Solidly discovery reads factory fees once and persists stable metadata', async () => { | |
| test('Solidly metadata normalizes tuple ABI and persists scale fields', async () => { | |
| const client = { | |
| async readContract({ functionName, args }: any): Promise<unknown> { | |
| // Solidly factory length + range (simple, non-splitting case) | |
| if (functionName === 'getPairsLength') return [0n, 2n]; | |
| if (functionName === 'getPairsByIndexRange') { | |
| expect(args).toEqual([0n, 2n]); | |
| // tuple ABI form: readonly [token0, token1, stable, scale0, scale1] | |
| // NOTE: adapt tuple layout to match the actual Solidly metadata ABI. | |
| return [ | |
| // volatile pair (stable = false), scale0/scale1 preserved | |
| [address(1), address(2), false, 18n, 18n], | |
| // stable pair (stable = true), asymmetric scale | |
| [address(3), address(4), true, 6n, 18n], | |
| ]; | |
| } | |
| if (functionName === 'filterVolatileHermesPairs') { | |
| // only the volatile pair (index 0) is considered volatile Hermes | |
| return [true, false]; | |
| } | |
| throw new Error(`Unexpected ${functionName}`); | |
| }, | |
| }; | |
| const pools = await discoverV2PoolMetadata(client); | |
| // We expect two pools discovered: one volatile, one stable, | |
| // with scale0 / scale1 propagated from the tuple ABI. | |
| expect(pools).toHaveLength(2); | |
| const volatile = pools.find((p) => !p.stable); | |
| const stable = pools.find((p) => p.stable); | |
| expect(volatile).toBeDefined(); | |
| expect(stable).toBeDefined(); | |
| // Tuple ABI index-to-field mapping: scale0/scale1 must be persisted. | |
| expect(volatile?.scale0).toBe(18n); | |
| expect(volatile?.scale1).toBe(18n); | |
| expect(stable?.scale0).toBe(6n); | |
| expect(stable?.scale1).toBe(18n); | |
| }); | |
| test('Solidly discovery handles reverted ranges with fees and preserves stable metadata', async () => { | |
| const feeCalls: boolean[] = []; | |
| let firstRangeCall = true; | |
| const client = { | |
| async readContract({ functionName, args }: any): Promise<unknown> { | |
| if (functionName === 'getPairsLength') { | |
| // 3 pairs: indices 0..2 | |
| return [0n, 3n]; | |
| } | |
| if (functionName === 'getPairsByIndexRange') { | |
| const [start, end] = args as [bigint, bigint]; | |
| // Simulate a Solidly-specific revert when querying the full [0, 3) range, | |
| // forcing recursive splitting into smaller subranges. | |
| if (firstRangeCall && start === 0n && end === 3n) { | |
| firstRangeCall = false; | |
| const error = new Error('Solidly: range too large'); | |
| // If your Solidly adapter uses error codes / specific properties, | |
| // set them here (e.g. (error as any).code = 'CALL_EXCEPTION'). | |
| throw error; | |
| } | |
| // After the split, we expect the adapter to query [0, 1), [1, 2), [2, 3) | |
| if (start === 0n && end === 1n) { | |
| return [[address(1), address(2), address(11)]]; | |
| } | |
| if (start === 1n && end === 2n) { | |
| return [[address(3), address(4), address(12)]]; | |
| } | |
| if (start === 2n && end === 3n) { | |
| return [[address(5), address(6), address(13)]]; | |
| } | |
| throw new Error(`Unexpected getPairsByIndexRange args: ${args}`); | |
| } | |
| if (functionName === 'filterVolatileHermesPairs') { | |
| // First two pairs are volatile, third is stable | |
| return [true, true, false]; | |
| } | |
| if (functionName === 'getFee') { | |
| feeCalls.push(true); | |
| // Non-null fees for all pairs to ensure the revert path is exercised | |
| // with fee lookup active. | |
| return 3n; | |
| } | |
| throw new Error(`Unexpected ${functionName}`); | |
| }, | |
| }; | |
| const pools = await discoverV2PoolMetadata(client); | |
| // We still discover all three pools, even though the initial range reverted. | |
| expect(pools).toHaveLength(3); | |
| // Fees should be fetched once per discovered pair. | |
| expect(feeCalls).toHaveLength(3); | |
| // Stable/volatile classification must remain correct after the split. | |
| const stable = pools.filter((p) => p.stable); | |
| const volatile = pools.filter((p) => !p.stable); | |
| expect(stable).toHaveLength(1); | |
| expect(volatile).toHaveLength(2); | |
| // Ensure metadata (including stability) is persisted correctly for the stable pool. | |
| expect(stable[0].token0).toBe(address(5)); | |
| expect(stable[0].token1).toBe(address(6)); | |
| }); | |
| test('Solidly discovery reads factory fees once and persists stable metadata', async () => { |
Comment on lines
+36
to
+45
| test("submits the refreshed opportunity", async () => { | ||
| let submittedProfit = 0n; | ||
| const manager = new OpportunityManager( | ||
| {} as never, | ||
| async (_graph, refreshed) => { | ||
| submittedProfit = refreshed.profit; | ||
| return true; | ||
| }, | ||
| async stale => ({ ...stale, profit: 2n }) | ||
| ); |
There was a problem hiding this comment.
suggestion (testing): Consider a test where the refresh hook returns null to assert pairs are released and nothing is submitted
To fully cover the refresh flow, this should be complemented with a test where the refresh callback returns null, asserting that submitOpportunity is not called and, if easy to verify, that releasePairs is invoked for the stale opportunity’s pairs. This will validate behavior for the "no longer valid" case and help prevent regressions where stale opportunities are accidentally submitted.
Suggested implementation:
test("submits the refreshed opportunity", async () => {
let submittedProfit = 0n;
const manager = new OpportunityManager(
{} as never,
async (_graph, refreshed) => {
submittedProfit = refreshed.profit;
return true;
},
async stale => ({ ...stale, profit: 2n })
);
await manager.processOpportunities({} as never, [opportunity]);
expect(submittedProfit).toBe(2n);
});
test("does not submit and releases pairs when refreshed opportunity becomes invalid", async () => {
let submitCalled = false;
const graph = {
releasePairs: jest.fn(),
} as never;
const manager = new OpportunityManager(
graph,
async () => {
submitCalled = true;
return true;
},
async () => null
);
await manager.processOpportunities(graph, [opportunity]);
expect(submitCalled).toBe(false);
expect(graph.releasePairs).toHaveBeenCalled();
});- Adjust the shape of
graphto match your actual execution graph type: e.g. ifreleasePairsexpects specific arguments (such asopportunity.pairsor an ID), update thetoHaveBeenCalledassertion accordingly (e.g.toHaveBeenCalledWith(opportunity.pairs)). - If
OpportunityManager.processOpportunitiesdoes not take the graph as the first argument (or if the graph is stored internally), adapt the calls tonew OpportunityManager(...)andprocessOpportunities(...)to match the real signature. - If
submitOpportunityis expected to return a boolean that affects further behavior, and your manager relies on that, ensure the test's submit callback still respects the expected contract (e.g. keepreturn true;if needed).
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+115
to
+122
| }) as RawBaseV1Metadata) | ||
| : null; | ||
| return { | ||
| ...pair, | ||
| fee: isStable ? fees!.stable : fees!.volatile, | ||
| variant: isStable ? 'solidly-stable' : 'solidly-volatile', | ||
| scale0: metadata?.scale0 ?? 1n, | ||
| scale1: metadata?.scale1 ?? 1n, |
There was a problem hiding this comment.
The string-match !String(error).toLowerCase().includes('revert') reclassifies any error that doesn't mention "revert" as fatal. Network timeouts, "Failed to fetch", and provider-specific errors like "JSON-RPC error -32000" will be re-thrown and propagate through discoverV2PoolMetadata — which has no outer try/catch — all the way up to bot startup, causing a hard crash. The prior code swallowed every error and returned [], keeping discovery resilient against transient RPC failures.
The same applies to errors thrown inside the solidly-specific block (e.g., a filterVolatileHermesPairs or metadata() network failure on a specific pair): those errors are also caught here, and if their message doesn't contain "revert" they propagate immediately rather than being retried or skipped.
Comment on lines
207
to
217
| } else if (amount1Delta > 0 && amount0Delta <= 0) { | ||
| _safeTransfer(tokenIn, msg.sender, uint256(amount1Delta)); | ||
| _safeTransfer(IUniswapV3Pool(msg.sender).token1(), msg.sender, uint256(amount1Delta)); | ||
| } else { | ||
| revert InvalidV3SwapDelta(); | ||
| } | ||
| } | ||
|
|
||
| function _finishFlashLoan(FlashData memory loan, uint256 repayAmount) internal { | ||
| uint256 finalAmount = _executeCircularRoute( | ||
| loan.borrowedToken, | ||
| loan.borrowedAmount, | ||
| loan.pools, | ||
| loan.protocols, | ||
| loan.fees, | ||
| loan.data | ||
| ); | ||
| uint256 finalAmount = _executeCircularRoute(loan); | ||
|
|
||
| if (finalAmount < repayAmount) revert InsufficientFlashLoanRepayment(); |
There was a problem hiding this comment.
uint128 truncation
In the old code both paths shared a post-branch if (totalActionAmount != amountIn) revert InvalidCarbonAmount() check that caught silent uint128 truncation of amountIn. After the refactor that check was moved inside the multi-strategy else branch; the single-strategy path (96-byte data) just writes uint128(amountIn) directly into the action without verifying the value wasn't narrowed.
…ntMonitor - updateCursorForAddress() as the single cursor key, ordering, and update path. - Added isAtOrBelowHydrationFloor() for both hydration checks. - Removed markApplied(), markReconciled(), and cursorKey().
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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.
encodeV2RouteDatafunction to encode route data based on pool variant.Summary by Sourcery
Extend V2 protocol support to handle Solidly stable/volatile pools and improve event-driven arbitrage execution robustness.
New Features:
Bug Fixes:
Enhancements:
Tests:
Confidence Score: 4/5
Safe to merge with low risk; no execution path has a correctness regression that could cause fund loss or a hard crash.
The stable swap math is implemented consistently and validated by a historical-output test. V3 accounting is tightened. Event-monitor cursor handling is logically sound. Two findings: the marginal-rate probe for stable pools can zero out an edge price for thin pools, and the shared cursor object in reconcileMarkets is a latent fragility.
src/protocols/v2/quote.ts (marginal-rate probe) and src/runtime/event-monitor.ts (shared cursor object in reconcileMarkets).
Comments Outside Diff (1)
src/protocols/v2/execution.ts, line 8-11 (link)v2FlashLoanFeealways adds+1neven when the division is exact, so TypeScript consistently estimates the flash-loan cost asfloor(amount × fee / denominator) + 1. The Solidity_v2RepayAmountuses ceiling division: whenamount × feeis exactly divisible bydenominator, it returns one wei less than TypeScript expects. Keeping both formulas aligned avoids subtle mismatches in profitability accounting.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
Reviews (2): Last reviewed commit: "refactor: improve cursor management and ..." | Re-trigger Greptile