Convertible Deposits: Limit Orders - #191
Conversation
Release: Convertible Deposits
Saves a bit of gas
Saves a bit of gas
For front end use
Allows user to change details of an open order. Functionally equivalent to canceling an existing order and opening a new one, with orderID retained and only one transaction required. Difference between existing and new order details is transferred from or to user.
CDAuctioneer Limit Orders
📝 WalkthroughWalkthroughAdds a new CDAuctioneerLimitOrders contract and ILimitOrders interface implementing a limit-order marketplace integrated with the Convertible Deposit system, plus comprehensive unit/fork tests, mocks, deployment scripts, ops batch, and pragma relaxations across tests and mocks. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant LimitOrders
participant CDAuctioneer
participant sUSDS
participant USDS
participant PositionNFT
participant YieldRecipient
rect rgb(220,238,255)
Note over User,LimitOrders: Create order
User->>LimitOrders: createOrder(depositBudget,incentiveBudget,depositPeriod,maxPrice,minFillSize)
LimitOrders->>CDAuctioneer: isDepositPeriodEnabled / getMinimumBid
CDAuctioneer-->>LimitOrders: enabled / minBid
LimitOrders->>USDS: transferFrom(user, depositBudget+incentive)
LimitOrders->>sUSDS: deposit(USDS) -> receive shares
LimitOrders-->>User: OrderCreated event
end
rect rgb(232,248,232)
Note over Filler,LimitOrders: Fill order
Filler->>LimitOrders: fillOrder(orderId, fillAmount)
LimitOrders->>CDAuctioneer: previewBid(depositPeriod, fillAmount)
CDAuctioneer-->>LimitOrders: effectivePrice / expectedOhmOut
LimitOrders->>CDAuctioneer: bid(depositPeriod, amount, minOhmOut, ...)
CDAuctioneer-->>LimitOrders: (ohmOut, positionId, receiptTokenId, actualAmount)
LimitOrders->>sUSDS: withdraw shares (ohmOut + incentive share)
LimitOrders->>PositionNFT: transfer position NFT to filler
LimitOrders-->>Filler: transfer receipt token + incentive, emit OrderFilled
end
rect rgb(255,243,217)
Note over LimitOrders,sUSDS: Yield sweep
LimitOrders->>sUSDS: getAccruedYield / getAccruedYieldShares
LimitOrders->>sUSDS: withdraw/sweep shares to YieldRecipient
sUSDS-->>YieldRecipient: shares transferred
LimitOrders-->>YieldRecipient: YieldSwept event
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
src/policies/deposits/LimitOrders.sol (1)
10-11: Consider exporting the interface alias for test compatibility.The test file imports
ICDAuctioneerfrom this contract, but you importIConvertibleDepositAuctioneer. Either export an alias or update the test to import the interface directly.import {IConvertibleDepositAuctioneer} from "../interfaces/deposits/IConvertibleDepositAuctioneer.sol"; + +// Re-export for convenience +alias ICDAuctioneer = IConvertibleDepositAuctioneer;Or simpler, just have the test import the interface from its source directly.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/policies/deposits/LimitOrders.sol(1 hunks)src/test/policies/ConvertibleDepositAuctioneer/LimitOrders.t.sol(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 29
File: src/modules/TRSRY/TRSRY.v1.sol:2-2
Timestamp: 2025-07-18T00:21:49.138Z
Learning: 0xJem prefers to avoid changing historical/already-deployed contracts to minimize risk and maintain stability, even when it would improve consistency across the codebase.
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 29
File: remappings.txt:35-40
Timestamp: 2025-07-18T00:22:32.511Z
Learning: 0xJem prefers to keep OpenZeppelin 4.8.0 as the standard/default dependency in remappings.txt for stability, with newer versions like 5.3.0 available only through explicit versioned dependency paths (e.g., openzeppelin-5.3.0/) to ensure intentional opt-in to newer versions.
📚 Learning: 2025-08-08T07:33:13.575Z
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 86
File: src/policies/deposits/DepositManager.sol:271-320
Timestamp: 2025-08-08T07:33:13.575Z
Learning: DepositManager (src/policies/deposits/DepositManager.sol) is deployed as non-upgradeable; when releasing a new version, existing receipt tokens and underlying deposits remain with the previous DepositManager instance. No migration path is expected or required when introducing facility scoping.
Applied to files:
src/policies/deposits/LimitOrders.sol
📚 Learning: 2025-08-08T11:14:54.317Z
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 88
File: src/test/policies/ConvertibleDepositAuctioneer/ConvertibleDepositAuctioneerTest.sol:413-416
Timestamp: 2025-08-08T11:14:54.317Z
Learning: In ConvertibleDepositAuctioneer (src/policies/deposits/ConvertibleDepositAuctioneer.sol), bid() reverts with ConvertibleDepositAuctioneer_ConvertedAmountZero when ohmOut == 0, before evaluating the minOhmOut slippage check. Therefore, setting minOhmOut=0 does not guarantee acceptance in zero-output scenarios; the zero-output revert triggers first.
Applied to files:
src/policies/deposits/LimitOrders.sol
🪛 GitHub Actions: Build Size
src/test/policies/ConvertibleDepositAuctioneer/LimitOrders.t.sol
[error] 11-11: Source 'src/CDAuctioneerLimitOrders.sol' not found: File not found. ParserError: Source 'src/CDAuctioneerLimitOrders.sol' not found. Searched locations: '/home/runner/work/olympus-v3/olympus-v3'.
🪛 GitHub Actions: Lint Check
src/test/policies/ConvertibleDepositAuctioneer/LimitOrders.t.sol
[warning] 1-1: Prettier formatting issues detected in 'src/test/policies/ConvertibleDepositAuctioneer/LimitOrders.t.sol'. Run 'prettier --write' to fix code style issues.
src/policies/deposits/LimitOrders.sol
[warning] 1-1: Prettier formatting issues detected in 'src/policies/deposits/LimitOrders.sol'. Run 'prettier --write' to fix code style issues.
🪛 GitHub Actions: Test Code Coverage
src/test/policies/ConvertibleDepositAuctioneer/LimitOrders.t.sol
[error] 11-11: Source 'src/CDAuctioneerLimitOrders.sol' not found. Import statement references a missing file. Searched paths: /home/runner/work/olympus-v3/olympus-v3
🪛 GitHub Actions: Tests - Cross-Chain
src/test/policies/ConvertibleDepositAuctioneer/LimitOrders.t.sol
[error] 11-11: Compiler run failed: Source "src/CDAuctioneerLimitOrders.sol" not found: File not found. Searched locations: .../olympus-v3
🪛 GitHub Actions: Tests - Fork
src/test/policies/ConvertibleDepositAuctioneer/LimitOrders.t.sol
[error] 11-11: Source 'src/CDAuctioneerLimitOrders.sol' not found: File not found. Searched the following locations: '/home/runner/work/olympus-v3/olympus-v3'. Import in LimitOrders.t.sol:11.
🪛 GitHub Actions: Tests - OCG Proposals
src/test/policies/ConvertibleDepositAuctioneer/LimitOrders.t.sol
[error] 11-11: Compiler run failed: Source "src/CDAuctioneerLimitOrders.sol" not found. Searched locations include project root.
🪛 GitHub Actions: Tests - Unit
src/test/policies/ConvertibleDepositAuctioneer/LimitOrders.t.sol
[error] 11-11: Compiler run failed: Source "src/CDAuctioneerLimitOrders.sol" not found. ParserError: Source not found for import in LimitOrders.t.sol (import {CDAuctioneerLimitOrders, ICDAuctioneer} from "src/CDAuctioneerLimitOrders.sol").
🔇 Additional comments (14)
src/policies/deposits/LimitOrders.sol (6)
114-143: Constructor validation is thorough.Good input validation for all addresses. The max approval to SUSDS is a common pattern for vault interactions and is safe since SUSDS is trusted.
164-218: Order creation logic is sound.Proper validation, reentrancy protection, and accounting. The flow (transfer → deposit to sUSDS → track owed) correctly maintains yield accounting.
227-274: Change order logic correctly handles fund adjustments.The reset of
depositSpentandincentiveSpentto zero is documented behavior. The calculation of fund differences handles both increase and decrease cases properly.
371-374: Defensive underflow checks are good practice.The ternary operators on lines 372-373 prevent underflow in edge cases. This defensive coding is appropriate for a financial contract.
4-10: Mixed OpenZeppelin versions are intentional and correctly configured.The use of
@openzeppelin-5.3.0forReentrancyGuardTransientand@openzeppelin(4.8.0) for other contracts follows the project's preference for explicit version opt-in. The interface import path is correct:../interfaces/deposits/IConvertibleDepositAuctioneer.solpoints to the existing file atsrc/policies/interfaces/deposits/IConvertibleDepositAuctioneer.sol.
319-341: Accounting mismatch betweenfillAmount_andactualAmountcan occur.The order's
depositSpentis incremented byfillAmount_(line 319), but the receipt tokens transferred are based onactualAmount(line 340), which corresponds todepositInfrom the auctioneer. InConvertibleDepositAuctioneer._previewBid()(line 406),depositIn = deposit_ - remainingDeposit, meaning the actual deposit used can be less thanfillAmount_if the bid loop exits early whenconvertibleAmount == 0. While the check at line 309 prevents zero output, it does not guaranteedepositIn == fillAmount_. This creates an inconsistency: the order tracks spendingfillAmount_but receives receipt tokens representing onlydepositIn, potentially understating what was actually spent for the order.⛔ Skipped due to learnings
Learnt from: 0xJem Repo: OlympusDAO/olympus-v3 PR: 88 File: src/test/policies/ConvertibleDepositAuctioneer/ConvertibleDepositAuctioneerTest.sol:413-416 Timestamp: 2025-08-08T11:14:54.317Z Learning: In ConvertibleDepositAuctioneer (src/policies/deposits/ConvertibleDepositAuctioneer.sol), bid() reverts with ConvertibleDepositAuctioneer_ConvertedAmountZero when ohmOut == 0, before evaluating the minOhmOut slippage check. Therefore, setting minOhmOut=0 does not guarantee acceptance in zero-output scenarios; the zero-output revert triggers first.src/test/policies/ConvertibleDepositAuctioneer/LimitOrders.t.sol (8)
27-47: MockSUSDS correctly simulates yield accrual.The exchange rate manipulation via
setExchangeRateallows testing yield scenarios. The override of_convertToSharesand_convertToAssetsproperly uses theMath.Roundingparameter from the parent ERC4626.
69-144: MockCDAuctioneer adequately simulates the real auctioneer.The mock correctly:
- Returns 0 from
previewBidwhen below minimum (testing ZeroOhmOut path)- Mints tokens to
msg.sender(the limit orders contract)- Uses controllable price and minimum bid for various test scenarios
166-210: Test setup is well-structured.Good separation of concerns with distinct addresses for owner, users (alice, bob), filler, and yieldRecipient. The setup properly initializes multiple deposit periods.
226-239: Tests use undefined functions.These tests call
getOrder()(line 226) andgetSUsdsBalance()(line 238) which are not defined in the contract. See the contract review for the missing function implementations.
347-367: Good test for final fill incentive logic.This test validates the important dust-avoidance behavior where the final fill receives all remaining incentive rather than a proportional amount.
473-489: Correct partial fill refund calculation.The test correctly calculates the expected refund: 7000 deposit remaining + 35 incentive remaining (50 - 15 spent on 3000 fill).
545-551: Test expects revert but contract returns 0.As flagged in the contract review,
sweepYield()returns 0 when there's no yield, but this test expectsNoYieldToSweeprevert. Align the contract implementation with this test expectation.
818-847: Good coverage of change order after partial fill.This test validates the documented behavior that
changeOrderresets spent amounts, allowing flexible order modification even after partial fills.
fix(cd-limit-orders): inherit `IERC721Receiver`, correct `getRemaining`, optimize
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/policies/deposits/LimitOrders.sol (1)
500-509:sweepYieldreturns 0 on no yield — past comment suggested revert.A previous review noted that tests expect a
NoYieldToSweeprevert when there's no yield, but line 502 returns 0 instead. If tests have been updated to match this behavior, this is fine. Otherwise, consider:function sweepYield() external nonReentrant onlyEnabled returns (uint256 shares) { shares = getAccruedYieldShares(); - if (shares == 0) return 0; + if (shares == 0) revert NoYieldToSweep();Note: The
NoYieldToSweeperror would need to be added to the interface if this change is made.
🧹 Nitpick comments (11)
src/test/mocks/MockDepositManager.sol (1)
2-2: Verify the higher minimum pragma version requirement.This mock uses
pragma solidity >=0.8.20, which is higher than other files in this PR (>=0.8.0forIVersioned.sol,>=0.8.15for others). Confirm whether this higher requirement is necessary or if it should be aligned with the rest of the codebase for consistency.src/test/mocks/MockPrice.sol (1)
2-2: Verify pragma version consistency across the PR.This file uses
>=0.8.15, but other new files in this PR use different minimum versions:IVersioned.soluses>=0.8.0andMockDepositManager.soluses>=0.8.20. Ensure these differences are intentional and align with project standards.#!/bin/bash # Description: Check pragma versions across all Solidity files in this PR # Search for pragma declarations to identify version inconsistencies rg -n "pragma solidity" --type solsrc/test/lib/bonds/BondAggregator.sol (1)
2-2: Consider adding an upper bound to the pragma.While broadening the Solidity version for test code is acceptable, using
>=0.8.15without an upper bound could introduce unexpected behavior with future major versions (e.g., 0.9.0+). Consider using>=0.8.15 <0.9.0to avoid potential breaking changes.🔎 Proposed fix
-pragma solidity >=0.8.15; +pragma solidity >=0.8.15 <0.9.0;src/test/lib/bonds/BondFixedTermSDA.sol (1)
2-2: Consider adding an upper bound to the pragma.Same as BondAggregator.sol, consider using
>=0.8.15 <0.9.0to avoid potential breaking changes in future major compiler versions.src/test/lib/bonds/bases/BondBaseSDA.sol (1)
2-2: Consider adding an upper bound to the pragma.Consider using
>=0.8.15 <0.9.0to avoid potential breaking changes in future major compiler versions.src/test/lib/bonds/bases/BondBaseTeller.sol (1)
2-2: Consider adding an upper bound to the pragma.Consider using
>=0.8.15 <0.9.0to avoid potential breaking changes in future major compiler versions.src/test/policies/EmissionManager.t.sol (1)
3-3: Pragma bump and MockConvertibleDepositAuctioneer ctor updates look safe; consider a clearer dummy for the new arg.
- Bumping the test pragma to
>=0.8.20is fine as long as your global toolchain/Foundry config already targets 0.8.20+.- The updated
MockConvertibleDepositAuctioneerconstructor calls all passaddress(0)for the new middle parameter and vary only the final asset argument. That keeps existing test intent intact (only the deposit asset is relevant), but it’s a bit opaque.If the new ctor parameter represents something meaningful (e.g., facility/manager), you might optionally:
- Introduce a dedicated dummy address/contract in tests instead of
address(0)to make misuse more obvious if the mock starts reading that field later.Functionally everything still aligns with the previous expectations around asset-mismatch and decimal handling.
Also applies to: 317-321, 845-845, 3265-3266
src/scripts/deploy/DeployV3.s.sol (1)
799-850: Limit-orders deploy function is wired correctly; consider a small sanity check on arg arrays.The new
deployConvertibleDepositAuctioneerLimitOrders():
- Resolves dependencies via
_getAddressNotZerousing keys that matchenv.json(DepositManager,ConvertibleDepositAuctioneer,USDS,sUSDS,OlympusDepositPositionManager,OlympusTreasury).- Reads
depositPeriodsandreceiptTokensfrom the sequence under"ConvertibleDepositAuctioneerLimitOrders", then passes them directly into theCDAuctioneerLimitOrdersconstructor.- Returns
"olympus.periphery"as the prefix, matching the env/deployments keys.Two optional robustness tweaks you might consider:
- Require non-empty arrays and matching lengths before deployment to catch misconfigured sequence files early:
Optional precondition guard
uint8[] memory depositPeriods = _readDeploymentArgUint8Array( "ConvertibleDepositAuctioneerLimitOrders", "depositPeriods" ); address[] memory receiptTokens = _readDeploymentArgAddressArray( "ConvertibleDepositAuctioneerLimitOrders", "receiptTokens" ); + + require(depositPeriods.length > 0, "LimitOrders: no depositPeriods"); + require( + depositPeriods.length == receiptTokens.length, + "LimitOrders: periods/receipts length mismatch" + );Not mandatory, but it would make misconfigured JSON fail fast at script time rather than later.
src/test/policies/ConvertibleDepositAuctioneer/LimitOrdersFork.t.sol (1)
1-181: Fork test wiring looks solid; you may want to assert on NFT/receipt token outcomes as well.
- Mainnet constants for
DEPOSIT_MANAGER,CD_AUCTIONEER,USDS,SUSDS,POSITION_NFT, andCD_FACILITYall line up with the addresses inenv.jsonat the time of this PR._deployLimitOrderscorrectly discovers enabled deposit periods and corresponding receipt tokens fromDepositManagerand wires them into a freshCDAuctioneerLimitOrdersinstance, then enables it and funds the test user.test_createAndFillOrderexercises a realistic create→fill path (including apreviewBid-based price sanity check and a full fill).Optional coverage improvement:
- After the fill, you could explicitly assert that:
- The user’s
POSITION_NFTbalance increased and/or the specific position token was minted.- The expected receipt token balance (for the chosen period) is held by the user.
That would make the fork test validate not just order accounting and incentives, but also that the on-chain CD pipeline delivered the expected position assets.
src/policies/deposits/LimitOrders.sol (2)
221-248: Consider warning users when incentive budget is reduced to zero.The
_depositfunction correctly handles sUSDS rounding, but ifactualDeposit <= depositBudget_, the entireincentiveBudget_is silently discarded (line 238-240). Users who intended to offer incentives might not realize this happened.Consider either:
- Emitting an event when the actual budgets differ from requested
- Documenting this behavior prominently in the
createOrderNatSpecThis is a design choice rather than a bug, so flagging for awareness.
675-693: Consider validatingindex1 >= index0to provide clearer error.Line 680 would revert with an arithmetic underflow if
index1 < index0. While this is safe (reverts), a clearer error message would improve UX:function _getFillableOrders( uint8 depositPeriod_, uint256 index0, uint256 index1 ) internal view returns (uint256[] memory) { + if (index1 < index0) revert InvalidParam("index range"); uint256[] memory tmp = new uint256[](index1 - index0);This is a minor improvement for clarity.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (25)
deployments/.sepolia-1766489520.jsondeployments/.sepolia-1766582124.jsonsrc/interfaces/IVersioned.solsrc/modules/CHREG/OlympusClearinghouseRegistry.solsrc/modules/RANGE/OlympusRange.solsrc/modules/RANGE/RANGE.v2.solsrc/policies/deposits/LimitOrders.solsrc/policies/interfaces/deposits/ILimitOrders.solsrc/scripts/deploy/DeployV3.s.solsrc/scripts/deploy/savedDeployments/convertible_deposit_limit_orders.jsonsrc/scripts/deploy/savedDeployments/convertible_deposit_limit_orders_sepolia.jsonsrc/scripts/env.jsonsrc/scripts/ops/batches/PeripheryEnable.solsrc/scripts/ops/batches/args/ConvertibleDepositLimitOrders.jsonsrc/test/lib/bonds/BondAggregator.solsrc/test/lib/bonds/BondFixedTermSDA.solsrc/test/lib/bonds/BondFixedTermTeller.solsrc/test/lib/bonds/bases/BondBaseSDA.solsrc/test/lib/bonds/bases/BondBaseTeller.solsrc/test/mocks/MockConvertibleDepositAuctioneer.solsrc/test/mocks/MockDepositManager.solsrc/test/mocks/MockPrice.solsrc/test/policies/ConvertibleDepositAuctioneer/LimitOrders.t.solsrc/test/policies/ConvertibleDepositAuctioneer/LimitOrdersFork.t.solsrc/test/policies/EmissionManager.t.sol
✅ Files skipped from review due to trivial changes (4)
- src/scripts/ops/batches/args/ConvertibleDepositLimitOrders.json
- src/modules/RANGE/RANGE.v2.sol
- src/test/lib/bonds/BondFixedTermTeller.sol
- deployments/.sepolia-1766489520.json
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 29
File: src/policies/deposits/BaseDepositFacility.sol:631-639
Timestamp: 2025-11-07T11:09:53.808Z
Learning: OlympusDAO/olympus-v3: BaseDepositFacility.setAssetPeriodReclaimRate now uses a dedicated reclaim-rate error (not InvalidAddress) when reclaimRate_ > ONE_HUNDRED_PERCENT, fixed in a follow-up PR (#180). Avoid re-flagging this in earlier PRs.
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 86
File: src/policies/deposits/DepositManager.sol:271-320
Timestamp: 2025-08-08T07:33:13.575Z
Learning: DepositManager (src/policies/deposits/DepositManager.sol) is deployed as non-upgradeable; when releasing a new version, existing receipt tokens and underlying deposits remain with the previous DepositManager instance. No migration path is expected or required when introducing facility scoping.
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 29
File: src/modules/TRSRY/TRSRY.v1.sol:2-2
Timestamp: 2025-07-18T00:21:49.138Z
Learning: 0xJem prefers to avoid changing historical/already-deployed contracts to minimize risk and maintain stability, even when it would improve consistency across the codebase.
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 29
File: remappings.txt:35-40
Timestamp: 2025-07-18T00:22:32.511Z
Learning: 0xJem prefers to keep OpenZeppelin 4.8.0 as the standard/default dependency in remappings.txt for stability, with newer versions like 5.3.0 available only through explicit versioned dependency paths (e.g., openzeppelin-5.3.0/) to ensure intentional opt-in to newer versions.
📚 Learning: 2025-08-08T07:33:13.575Z
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 86
File: src/policies/deposits/DepositManager.sol:271-320
Timestamp: 2025-08-08T07:33:13.575Z
Learning: DepositManager (src/policies/deposits/DepositManager.sol) is deployed as non-upgradeable; when releasing a new version, existing receipt tokens and underlying deposits remain with the previous DepositManager instance. No migration path is expected or required when introducing facility scoping.
Applied to files:
src/scripts/deploy/savedDeployments/convertible_deposit_limit_orders.jsonsrc/scripts/deploy/savedDeployments/convertible_deposit_limit_orders_sepolia.jsonsrc/test/policies/ConvertibleDepositAuctioneer/LimitOrdersFork.t.solsrc/test/policies/EmissionManager.t.solsrc/scripts/deploy/DeployV3.s.solsrc/test/mocks/MockConvertibleDepositAuctioneer.solsrc/test/mocks/MockDepositManager.solsrc/policies/interfaces/deposits/ILimitOrders.solsrc/policies/deposits/LimitOrders.sol
📚 Learning: 2025-08-08T11:14:54.317Z
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 88
File: src/test/policies/ConvertibleDepositAuctioneer/ConvertibleDepositAuctioneerTest.sol:413-416
Timestamp: 2025-08-08T11:14:54.317Z
Learning: In ConvertibleDepositAuctioneer (src/policies/deposits/ConvertibleDepositAuctioneer.sol), bid() reverts with ConvertibleDepositAuctioneer_ConvertedAmountZero when ohmOut == 0, before evaluating the minOhmOut slippage check. Therefore, setting minOhmOut=0 does not guarantee acceptance in zero-output scenarios; the zero-output revert triggers first.
Applied to files:
src/test/policies/EmissionManager.t.solsrc/scripts/deploy/DeployV3.s.solsrc/test/mocks/MockConvertibleDepositAuctioneer.solsrc/policies/interfaces/deposits/ILimitOrders.solsrc/policies/deposits/LimitOrders.sol
📚 Learning: 2025-11-07T11:09:53.808Z
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 29
File: src/policies/deposits/BaseDepositFacility.sol:631-639
Timestamp: 2025-11-07T11:09:53.808Z
Learning: OlympusDAO/olympus-v3: BaseDepositFacility.setAssetPeriodReclaimRate now uses a dedicated reclaim-rate error (not InvalidAddress) when reclaimRate_ > ONE_HUNDRED_PERCENT, fixed in a follow-up PR (#180). Avoid re-flagging this in earlier PRs.
Applied to files:
src/test/mocks/MockConvertibleDepositAuctioneer.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Cursor Bugbot
🔇 Additional comments (21)
src/interfaces/IVersioned.sol (1)
1-14: LGTM!The
IVersionedinterface is well-defined and documented. The use of>=0.8.0provides maximum compatibility for an interface, and the forge-lint directives appropriately handle the mixed-case function naming convention.src/test/mocks/MockDepositManager.sol (2)
26-38: Verify test coverage handles the simplified mock logic.The
deposit()andwithdraw()functions perform actual token transfers but return hardcoded values (e.g.,receiptTokenId = 1) and lack validation logic present in real implementations. Since this is a test mock, the simplified logic is acceptable, but ensure that:
- Tests using this mock don't rely on receipt token ID generation logic
- Tests cover edge cases that would normally be caught by validation (e.g., zero amounts, unauthorized callers)
- The actual
DepositManagercontract is tested separately with full validation
40-161: LGTM!The stub implementations returning zero/empty values are appropriate for a test mock. This allows tests to focus on the specific interactions they care about without requiring full DepositManager behavior.
src/modules/CHREG/OlympusClearinghouseRegistry.sol (1)
2-2: Contract is already deployed in production—clarify if pragma change is necessary and whether the contract has been tested with compiler versions beyond 0.8.15.OlympusClearinghouseRegistry is deployed on mainnet (0x69a3E97027d21a5984B6a543b36603fFbC6543a4), Sepolia (0x38038bdd78602e5AA2accd0Ce07557369e21a6c1), and Goerli. The current pragma (
>=0.8.15) is already permissive and accepts any version >= 0.8.15. However:
- Confirm whether this pragma represents an actual change and, if so, why it is necessary for the limit-orders feature
- The codebase supports compilation with 0.8.24 (for some contracts), but CHREG is compiled with 0.8.15 per foundry.toml defaults. Confirm whether CHREG has been tested with 0.8.24 before deploying
src/modules/RANGE/OlympusRange.sol (1)
2-2: The pragma is already broadened to>=0.8.15—this change has already been applied.OlympusRange is deployed on mainnet (0xb212D9584cfc56EFf1117F412Fe0bBdc53673954 and newer v2 at 0x399cD3685912bb56aAeD0949119dB6cE5Df60FB5). The RANGE v2 architecture uses
>=0.8.15consistently across RANGEv2 and OlympusRange.However, there is a build-system constraint:
foundry.tomlforces all code to compile with Solidity 0.8.15 specifically ({ paths = "**", version = "0.8.15" }), despite the pragma declaring compatibility with newer versions. No testing with compiler versions >0.8.15 has been performed. The permissive pragma should either be tightened topragma solidity 0.8.15;to match the actual build environment, or the foundry restriction should be relaxed and testing conducted with newer versions if forward compatibility is intended.src/scripts/deploy/savedDeployments/convertible_deposit_limit_orders.json (1)
1-13: Verify the receipt token address against the deployed DepositManager state on the target network.The configuration structure is valid: the
depositPeriodsandreceiptTokensarrays have matching lengths (both length 1) as required by the constructor, anddepositPeriods: [3]is the established 3-month deposit period constant used throughout the system. However, the receipt token address0x03d95a2f9A29228ea6D0690f2d1Bcd15973d926Fmust be confirmed to correspond to the existing receipt token created by the DepositManager for the USDS/3-month/ConvertibleDepositFacility combination on mainnet. Since the DepositManager is non-upgradeable and receipt tokens persist across new ConvertibleDepositAuctioneerLimitOrders deployments, ensure this address matches the actual token state on the target network before deployment.src/scripts/deploy/savedDeployments/convertible_deposit_limit_orders_sepolia.json (1)
1-13: Limit-orders saved deployment JSON is consistent with DeployV3 wiring.
namematches the DeployV3 entry anddepositPeriods/receiptTokensare aligned (both length 1). No issues from a scripting perspective.src/scripts/env.json (1)
641-641: New env.json periphery entry matches deployment keying.The
ConvertibleDepositAuctioneerLimitOrdersaddress undercurrent.sepolia.olympus.peripherylines up with the"olympus.periphery.ConvertibleDepositAuctioneerLimitOrders"key returned fromDeployV3. Looks consistent.deployments/.sepolia-1766582124.json (1)
1-3: Deployment mapping is correctly keyed for the new periphery contract.The mapping key
olympus.periphery.ConvertibleDepositAuctioneerLimitOrdersand address match the env.json entry and DeployV3 prefixing, so downstream resolution should work.src/scripts/deploy/DeployV3.s.sol (1)
30-30: Helper array readers are consistent with existing deploy argument helpers.The new
_readDeploymentArgUint8Array/_readDeploymentArgAddressArrayhelpers follow the same pattern as the other_readDeploymentArg*utilities and align with the saved deployment JSON (uint arrays fordepositPeriods, address arrays forreceiptTokens). No functional concerns here.Also applies to: 293-319
src/scripts/ops/batches/PeripheryEnable.sol (1)
1-35: Periphery enable batch script is minimal and correctly targets IEnabler.The script cleanly:
- Pulls a contract key from the batch args.
- Resolves the address via
_envAddressNotZero.- Adds a single
IEnabler.enable(bytes(""))call and forwards viaproposeBatch().This is a nice generic helper for enabling periphery contracts (including the new limit-orders periphery). No changes needed.
src/test/mocks/MockConvertibleDepositAuctioneer.sol (2)
62-112: Mockbid()implementation looks reasonable for testing.The function correctly simulates the real auctioneer behavior with deposit period validation, OHM calculation, and token interactions. The use of low-level calls with success checks (lines 97-100, 106-107) is acceptable for a mock where you control the target contracts.
One minor observation: the
actualAmountcalculation on line 80 uses subtraction which could underflow ifactualAmountDifference > depositAmount_, though this is controlled by test setup.
236-255: Test helpersetDepositPeriodEnabledis functional for test scenarios.The swap-and-pop pattern for array removal is correct. Note that calling
setDepositPeriodEnabled(period, true)twice for the same period would add duplicates toenabledPeriodssince the mapping would already be true andfoundwould be set. This is acceptable for a test mock if tests avoid this pattern.src/policies/deposits/LimitOrders.sol (7)
87-121: Constructor implementation is well-structured.All immutable addresses are validated for non-zero values, array length mismatch is checked, and deposit periods are initialized in a loop. The max approval to sUSDS (line 118) is standard for ERC4626 vault interactions.
451-473:cancelOrdercorrectly allows cancellation even when contract is disabled.The function intentionally omits
onlyEnabledto allow users to withdraw funds if the contract is disabled. The use ofsaturatingSub(lines 461-462) is defensive programming—while the revert on line 458 should prevent underflow, the saturating subtraction provides an extra safety layer.
555-566:calculateIncentivenow correctly handles final fills.The function delegates to
_calculateFillAndIncentive(line 564) which properly returns all remaining incentive on final fills (lines 346-350). This addresses the previous review comment about inconsistency withfillOrder.
517-519:getOrdergetter is now implemented as requested.This addresses the previous review comment about needing a helper function to return the
LimitOrderstruct rather than the tuple from the public mapping.
761-768: Thesuper.supportsInterface(interfaceId)call is correctly supported.PeripheryEnablerimplementssupportsInterface(returning true for ERC-165 andIEnablerinterface), so the code compiles and functions properly. No changes needed.Likely an incorrect or invalid review comment.
283-285: The use oftokenId=0as a sentinel value to verify the caller can receive ERC721 tokens is standard practice and not problematic. OpenZeppelin'scheckOnERC721Receivedforwards the exact tokenId provided, and using 0 as a dummy value for capability checks is the intended pattern (also confirmed in the test suite). The LimitOrders contract'sonERC721Receivedimplementation ignores the tokenId parameter and simply returns the selector, so any value works. No action needed.
408-418: No issues found. The approval to DEPOSIT_MANAGER is correct and intentional—DEPOSIT_MANAGER is the contract that ultimately pulls USDS tokens from LimitOrders via safeTransferFrom during the deposit process.src/policies/interfaces/deposits/ILimitOrders.sol (1)
9-40: Error definitions are comprehensive.All necessary errors are defined for the contract's revert conditions. Note that if
sweepYieldis later changed to revert on zero yield (as suggested in past reviews), aNoYieldToSweeperror would need to be added here.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/scripts/deploy/DeployV3.s.sol:
- Around line 813-821: The deployment reads depositPeriods via
_readDeploymentArgUint8Array and receiptTokens via
_readDeploymentArgAddressArray but doesn't check their lengths; add a validation
right after reading these arrays that requires depositPeriods.length ==
receiptTokens.length (use require or revert with a clear message referencing
depositPeriods and receiptTokens) before constructing the
ConvertibleDepositAuctioneerLimitOrders/CDAuctioneerLimitOrders to ensure the
arrays correspond 1:1 and prevent confusing constructor failures.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/scripts/deploy/DeployV3.s.sol
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 29
File: src/policies/deposits/BaseDepositFacility.sol:631-639
Timestamp: 2025-11-07T11:09:53.808Z
Learning: OlympusDAO/olympus-v3: BaseDepositFacility.setAssetPeriodReclaimRate now uses a dedicated reclaim-rate error (not InvalidAddress) when reclaimRate_ > ONE_HUNDRED_PERCENT, fixed in a follow-up PR (#180). Avoid re-flagging this in earlier PRs.
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 29
File: src/modules/TRSRY/TRSRY.v1.sol:2-2
Timestamp: 2025-07-18T00:21:49.138Z
Learning: 0xJem prefers to avoid changing historical/already-deployed contracts to minimize risk and maintain stability, even when it would improve consistency across the codebase.
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 29
File: remappings.txt:35-40
Timestamp: 2025-07-18T00:22:32.511Z
Learning: 0xJem prefers to keep OpenZeppelin 4.8.0 as the standard/default dependency in remappings.txt for stability, with newer versions like 5.3.0 available only through explicit versioned dependency paths (e.g., openzeppelin-5.3.0/) to ensure intentional opt-in to newer versions.
📚 Learning: 2025-08-08T07:33:13.575Z
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 86
File: src/policies/deposits/DepositManager.sol:271-320
Timestamp: 2025-08-08T07:33:13.575Z
Learning: DepositManager (src/policies/deposits/DepositManager.sol) is deployed as non-upgradeable; when releasing a new version, existing receipt tokens and underlying deposits remain with the previous DepositManager instance. No migration path is expected or required when introducing facility scoping.
Applied to files:
src/scripts/deploy/DeployV3.s.sol
📚 Learning: 2025-08-08T11:14:54.317Z
Learnt from: 0xJem
Repo: OlympusDAO/olympus-v3 PR: 88
File: src/test/policies/ConvertibleDepositAuctioneer/ConvertibleDepositAuctioneerTest.sol:413-416
Timestamp: 2025-08-08T11:14:54.317Z
Learning: In ConvertibleDepositAuctioneer (src/policies/deposits/ConvertibleDepositAuctioneer.sol), bid() reverts with ConvertibleDepositAuctioneer_ConvertedAmountZero when ohmOut == 0, before evaluating the minOhmOut slippage check. Therefore, setting minOhmOut=0 does not guarantee acceptance in zero-output scenarios; the zero-output revert triggers first.
Applied to files:
src/scripts/deploy/DeployV3.s.sol
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: run-ci
- GitHub Check: Cursor Bugbot
- GitHub Check: run-ci
- GitHub Check: run-ci
- GitHub Check: run-ci
- GitHub Check: run-ci
- GitHub Check: run-ci
- GitHub Check: run-ci
🔇 Additional comments (4)
src/scripts/deploy/DeployV3.s.sol (4)
30-30: LGTM!The import is correctly placed and follows the established pattern for policy imports.
293-309: LGTM!The helper function correctly reads and converts uint256 arrays from JSON to uint8 arrays, using SafeCast for safe type conversion that will revert on overflow.
311-319: LGTM!The helper function follows the established pattern for reading deployment arguments and correctly handles address arrays.
823-850: Deployment flow follows established patterns.The logging, broadcast, and return structure align well with other deployment functions in this file. The owner configuration to the DAO multisig is appropriate for governance control.
| depositSpent: 0, | ||
| incentiveSpent: 0, | ||
| maxPrice: maxPrice_, | ||
| minFillSize: minFillSize_ |
There was a problem hiding this comment.
minFillSize validation bypassed due to rounding loss
Medium Severity
The minFillSize validation on line 281 checks against the input depositBudget_, but the order is created with actualDepositBudget which can be less due to sUSDS rounding losses. When actualDepositBudget < minFillSize_ <= depositBudget_, the order gets created with minFillSize > depositBudget. This causes the minimum fill check in fillOrder (line 393) to always evaluate as false since remainingDeposit < minFillSize, treating every fill as a "final fill" and allowing any fill size. This defeats the anti-griefing protection that minFillSize is intended to provide.
|
Deployed to mainnet, awaiting activation by DAO MS |
3 participants
Summary
Introduces a limit order system for the Convertible Deposit Auctioneer, enabling users to place persistent buy orders that execute when the auction price reaches their specified maximum. MEV bots are incentivized to fill orders, creating a permissionless execution layer.
Motivation
Currently, users must actively monitor CDAuctioneer prices and manually execute bids. This creates friction and may result in missed opportunities when prices briefly dip to favorable levels. Limit orders allow users to set-and-forget, capturing favorable prices without active management.
Features
Limit Orders
MEV Incentives
Yield Generation
sweepYield()transfers accrued yield as sUSDS sharesPrice Validation
previewBid()to check actual execution price including slippage across ticksUser Flow
createOrder()with:depositPeriod: CD term lengthdepositBudget: USDS to spend on bidsincentiveBudget: USDS to pay fillersmaxPrice: Maximum USDS per OHMminFillSize: Minimum fill per transactionfillOrder()when profitablecancelOrder()anytime to reclaim remaining fundsBot Flow
getFillableOrders()or indexOrderCreatedeventscanFillOrder()for fillability and effective pricefillOrder()with order ID and fill amountContract Architecture
Security Considerations
nonReentrantOwnablefor yield recipient management; order cancellation restricted to ownerpreviewBid()for actual execution price, not tick pricetotalUsdsOwedto isolate user principal from yieldTesting
Comprehensive test suite covering:
Files
src/CDAuctioneerLimitOrders.sol— Main contracttest/CDAuctioneerLimitOrders.t.sol— Test suiteDeployment Parameters
owner_cdAuctioneer_usds_sUsds_positionNft_yieldRecipient_depositPeriods_receiptTokens_Summary by CodeRabbit
New Features
Interfaces
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.
Note
Introduces a limit-order layer for convertible deposits and wires it into deployment/ops.
CDAuctioneerLimitOrderswithcreateOrder/fillOrder/cancelOrder, max-price enforcement viapreviewBid, proportional filler incentives, sUSDS yield accounting (sweepYield), and per-period receipt token support; exposesIVersionedandILimitOrdersinterfacesdeployConvertibleDepositAuctioneerLimitOrders()and helpers to read array args; adds saved deployment configs and records deployed addresses (mainnet/sepolia); updatesenv.jsonPeripheryEnableand args to enable the new periphery contract>=0.8.15in RANGE/CHREG modules and various test libs; adds/extends mocks for testing (auctioneer, deposit manager, price)Written by Cursor Bugbot for commit a33d3e5. This will update automatically on new commits. Configure here.