Conversation
…handling for delayed retries. Introduce new properties in Position interface for execution timing and liquidatability status. Refactor Redis operations for position management.
…ore checking for new ones
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
Codex Review: Here are some suggestions.
About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you open a pull request for review, mark a draft as ready, or comment "@codex review". If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex fix this CI failure" or "@codex address that feedback".
| export async function checkIfPositionStillLiquidatable(position: Position): Promise<boolean> { | ||
| try { | ||
| const { marketAddress, network, positionId, owner } = position | ||
|
|
||
| const provider = new ethers.providers.JsonRpcProvider(networksConfig[network].rpc_url) | ||
|
|
||
| // Use the same market state contract that liquidation-checker uses | ||
| const ovlMarketStateContract = new ethers.Contract( | ||
| '0x10575a9C8F36F9F42D7DB71Ef179eD9BEf8Df238', // ovl_state_address from config | ||
| [ | ||
| 'function liquidatable(address market, address owner, uint256 positionId) view returns (bool)' | ||
| ], | ||
| provider | ||
| ) | ||
|
|
||
| // Check if position is still liquidatable | ||
| const isLiquidatable = await ovlMarketStateContract.liquidatable(marketAddress, owner, parseInt(positionId)) |
There was a problem hiding this comment.
[P1] Avoid parsing positionId into a JavaScript number
The new liquidatability check converts position.positionId with parseInt before calling the contract. parseInt returns a JavaScript number, which loses precision once the value exceeds Number.MAX_SAFE_INTEGER. For large position IDs the contract is called with a truncated ID, so valid positions may be reported as non‑liquidatable and removed from Redis while still being liquidatable on chain. The call can take the string or a BigNumber directly—no conversion is needed.
Useful? React with 👍 / 👎.
| describe('liquidablePositionsListener - Real Flow Tests', () => { | ||
| it('should process delayed positions first, then check for new positions', async () => { | ||
| // Mock delayed positions ready to retry | ||
| vi.mocked(redis.zrangebyscore).mockResolvedValue([JSON.stringify(mockPosition)]) | ||
| vi.mocked(redis.zrem).mockResolvedValue(1) | ||
|
|
||
| // Mock no new positions available | ||
| vi.mocked(redis.brpop).mockResolvedValue(null) | ||
|
|
||
| // Mock successful liquidation for delayed position | ||
| const mockReceipt = { status: 1, transactionHash: '0xhash' } | ||
| const mockTransaction = { | ||
| wait: vi.fn().mockResolvedValue(mockReceipt), | ||
| } | ||
| mockMarketContract.liquidate.mockResolvedValue(mockTransaction) | ||
|
|
||
| vi.mocked(redis.hdel).mockResolvedValue(1) | ||
| vi.mocked(redis.incr).mockResolvedValue(1) | ||
|
|
||
| // Start the listener (it will run one iteration) | ||
| const listenerPromise = liquidablePositionsListener() | ||
|
|
||
| // Wait a bit for the first iteration | ||
| await new Promise(resolve => setTimeout(resolve, 100)) | ||
|
|
||
| // Verify both queues were processed | ||
| expect(redis.zrangebyscore).toHaveBeenCalledWith('delayed_positions', 0, expect.any(Number), 'LIMIT', 0, 1) | ||
| expect(redis.brpop).toHaveBeenCalledWith('liquidatable_positions', 1) | ||
|
|
||
| // Clean up | ||
| vi.mocked(redis.brpop).mockRejectedValue(new Error('Stop listener')) | ||
| await new Promise(resolve => setTimeout(resolve, 100)) | ||
| }) | ||
|
|
||
| it('should process new positions when available', async () => { | ||
| // Mock no delayed positions ready | ||
| vi.mocked(redis.zrangebyscore).mockResolvedValue([]) | ||
|
|
||
| // Mock new position available | ||
| vi.mocked(redis.brpop).mockResolvedValue(['liquidatable_positions', JSON.stringify(mockPosition)]) | ||
|
|
||
| // Mock successful liquidation | ||
| const mockReceipt = { status: 1, transactionHash: '0xhash' } | ||
| const mockTransaction = { | ||
| wait: vi.fn().mockResolvedValue(mockReceipt), | ||
| } | ||
| mockMarketContract.liquidate.mockResolvedValue(mockTransaction) | ||
|
|
||
| vi.mocked(redis.hdel).mockResolvedValue(1) | ||
| vi.mocked(redis.incr).mockResolvedValue(1) | ||
|
|
||
| // Start the listener (it will run one iteration) | ||
| const listenerPromise = liquidablePositionsListener() | ||
|
|
||
| // Wait a bit for the first iteration | ||
| await new Promise(resolve => setTimeout(resolve, 100)) | ||
|
|
||
| // Verify new position was processed | ||
| expect(mockMarketContract.liquidate).toHaveBeenCalledWith( | ||
| mockPosition.owner, | ||
| mockPosition.positionId | ||
| ) | ||
|
|
||
| // Clean up | ||
| vi.mocked(redis.brpop).mockRejectedValue(new Error('Stop listener')) | ||
| await new Promise(resolve => setTimeout(resolve, 100)) | ||
| }) | ||
|
|
||
| it('should handle both queues independently every second', async () => { | ||
| // Mock delayed positions ready | ||
| vi.mocked(redis.zrangebyscore).mockResolvedValue([JSON.stringify(mockPosition)]) | ||
| vi.mocked(redis.zrem).mockResolvedValue(1) | ||
|
|
||
| // Mock no new positions available | ||
| vi.mocked(redis.brpop).mockResolvedValue(null) | ||
|
|
||
| // Mock successful liquidation for delayed position | ||
| const mockReceipt = { status: 1, transactionHash: '0xhash' } | ||
| const mockTransaction = { | ||
| wait: vi.fn().mockResolvedValue(mockReceipt), | ||
| } | ||
| mockMarketContract.liquidate.mockResolvedValue(mockTransaction) | ||
|
|
||
| vi.mocked(redis.hdel).mockResolvedValue(1) | ||
| vi.mocked(redis.incr).mockResolvedValue(1) | ||
|
|
||
| // Start the listener (it will run one iteration) | ||
| const listenerPromise = liquidablePositionsListener() | ||
|
|
||
| // Wait a bit for the first iteration | ||
| await new Promise(resolve => setTimeout(resolve, 100)) | ||
|
|
||
| // Verify both queues were processed independently | ||
| expect(redis.zrangebyscore).toHaveBeenCalledWith('delayed_positions', 0, expect.any(Number), 'LIMIT', 0, 1) | ||
| expect(redis.brpop).toHaveBeenCalledWith('liquidatable_positions', 1) | ||
|
|
||
| // Clean up | ||
| vi.mocked(redis.brpop).mockRejectedValue(new Error('Stop listener')) |
There was a problem hiding this comment.
[P2] Tests never stop the infinite listener
The tests invoke liquidablePositionsListener() but do not provide any mechanism to cancel the while (true) loop. After the expectations run, the listener continues scheduling setTimeout callbacks forever, so the Vitest process will not exit and the test suite hangs. Consider adding a way to signal the listener to stop or refactor the tests to avoid spawning a non‑terminating async loop.
Useful? React with 👍 / 👎.
2 participants
No description provided.