[rolling fallback] incentive dutch auction - #1127
Conversation
![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 automated review suggestions for this pull request.
Reviewed commit: 35cc852e60
ℹ️ 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
- 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 address that feedback".
| require( | ||
| blueMarketParams.collateralToken == midnightMarket.collateralParams[collateralIndex].token, | ||
| InconsistentCollateralToken() | ||
| ); | ||
| require(midnightMarket.collateralParams[collateralIndex].lltv <= blueMarketParams.lltv, BlueLltvTooLow()); |
There was a problem hiding this comment.
these checks (as well as the loan token check) are not perfect because the users could set non-valid configs
There was a problem hiding this comment.
potential solution, check these in setConfig and use a midnight and blue market instead of their respective ids (it would be more expensive in calldata though)
There was a problem hiding this comment.
it's not too bad that one can set an invalid config, but IMO we should do either all checks in setConfig (meaning you can't enable an invalid config) or all checks in roll
| bool enabled | ||
| ) external override { | ||
| require(start <= end, EndBeforeStart()); | ||
| require(incentiveAtStart <= incentiveAtEnd, IncentiveNotIncreasing()); |
There was a problem hiding this comment.
is this check really necessary ? would a decreasing incentive make sense ? my gut feeling is that it does not
There was a problem hiding this comment.
but at the same time nothing bad happens. we can keep or remove for me, not a big deal
| // Round in favor of the Midnight position. | ||
| uint256 collateralAssets = IMidnight(MIDNIGHT).collateral(midnightId, user, collateralIndex) | ||
| .mulDivDown(assets, IMidnight(MIDNIGHT).debt(midnightId, user)); | ||
| // Round in favor of the borrower. |
There was a problem hiding this comment.
I'm realizing that this is not really well said. Less incentive might not always be good for the borrower. In doubt I would remove since this is not very important (unlike the one above). same in incentive() btw
| // Round in favor of the borrower. |
| ); | ||
| require(midnightMarket.collateralParams[collateralIndex].lltv <= blueMarketParams.lltv, BlueLltvTooLow()); | ||
|
|
||
| // Round in favor of the Midnight position. |
There was a problem hiding this comment.
the rounding direction has been questioned by @chrmatt and I currently don't have a compelling reason for choosing this direction
There was a problem hiding this comment.
let's not tackle that in this PR
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0e54c34760
ℹ️ 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
- 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 address that feedback".
|
|
||
| /// @param start The start time of the rolling period. | ||
| /// @param incentive The caller incentive as a WAD-scaled percentage of the debt rolled. | ||
| /// @param end The end time of the rolling period. |
There was a problem hiding this comment.
Update src/periphery/README.md:26-34 alongside this new end parameter: it still describes configurations as having only a start timestamp and one fixed incentive, and says rolling becomes available after the start without documenting that it expires at end. Integrators following that description will construct the wrong configuration and miss the auction deadline semantics.
AGENTS.md reference: AGENTS.md:L72-L77
Useful? React with 👍 / 👎.
| /// @param incentiveAtStart The caller incentive at `start`, as a WAD-scaled percentage of the debt rolled. | ||
| /// @param incentiveAtEnd The caller incentive at `end`, as a WAD-scaled percentage of the debt rolled. The |
There was a problem hiding this comment.
clear enough we don't need these
There was a problem hiding this comment.
| /// @param incentiveAtStart The caller incentive at `start`, as a WAD-scaled percentage of the debt rolled. | |
| /// @param incentiveAtEnd The caller incentive at `end`, as a WAD-scaled percentage of the debt rolled. The |
2 participants
No description provided.