[fallback rolling] refactor: config struct - #1104
Conversation
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Claude Code Review
This repository is configured for manual code reviews. Comment @claude review for a one-time review, or @claude review always to subscribe this PR to a review on every future push.
Tip: disable this comment in your organization's Code Review settings.
![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: 0f36cbef1b
ℹ️ 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".
| address user, | ||
| uint64 start, | ||
| uint64 incentive, | ||
| Config memory config, |
There was a problem hiding this comment.
this is actually more complex no? you have to check that the ids match
There was a problem hiding this comment.
yes you have two extra check (not sure if removing the check and fetching the markets from their respective contract would be better)
There was a problem hiding this comment.
that would be very expensive! I don't think that's a good idea
There was a problem hiding this comment.
we could also remove midnightId and blueId from Config to prevent this check (but then the struct becomes useless except if we add more auction parameters)
MathisGD
changed the title
Signed-off-by: MathisGD <74971347+MathisGD@users.noreply.github.com>
| override | ||
| { | ||
| require(incentive <= WAD, IncentiveTooHigh()); | ||
| function setConfig(Config memory config, bool enabled) external override { |
There was a problem hiding this comment.
here it's great but I would rollback in roll
There was a problem hiding this comment.
wdyt now ? kept midnight and blue id out of config and changed start for rollWindow. Config can now be re-used across markets
There was a problem hiding this comment.
we could even go further and allow only one config per user. Users would then just have to enableRolling on a given pair (midnight id, blue id) and then keeper would be able to roll on this specific user config
Move the midnight and blue ids out of the Config struct and into the config id hash. `roll` derives both ids from the markets it is given, so the binding to the right markets is enforced by the mapping lookup itself and the explicit id checks are no longer needed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6cd0c61643
ℹ️ 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".
| import {MarketParams} from "../../../lib/morpho-blue/src/interfaces/IMorpho.sol"; | ||
| import {Market} from "../../interfaces/IMidnight.sol"; | ||
|
|
||
| /// @param rollWindow The duration before the Midnight market's maturity from which rolling is allowed. |
There was a problem hiding this comment.
The interface now accepts a duration relative to market maturity, but src/periphery/README.md lines 26–32 still instruct users to configure an absolute start timestamp. A caller following that documentation could pass a Unix timestamp as rollWindow; subtracting it from maturity will generally produce a timestamp near the epoch, making the roll immediately available instead of opening at the intended time. Update both README references to describe the maturity-relative window.
AGENTS.md reference: AGENTS.md:L72-L75
Useful? React with 👍 / 👎.
MathisGD
left a comment
MathisGD
left a comment
There was a problem hiding this comment.
not fan tbh..
I found the previous version simpler and not sure about the benefit of the approach here
peyha
left a comment
peyha
left a comment
There was a problem hiding this comment.
benefit would be mostly operational, users would have an easier time using the same config everywhere but it's not such an issue
2 participants
looks cleaner and would be better to handle some kind of auction