Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

make getWeeklySpend more sensitive to future programs. #40

Closed
wants to merge 2 commits into from
Closed

make getWeeklySpend more sensitive to future programs. #40

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7698893
make getWeeklySpend more sensitive to future programs.
Tritium-VLK Sep 23, 2024
c1777ad
Merge branch 'main' into issue/33/getWeeklySPend
gosuto-inzasheru Sep 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 7 additions & 4 deletions contracts/ChildChainGaugeInjectorV2.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -270,8 +270,8 @@ KeeperCompatibleInterface
target.periodNumber = 0;
target.programStartTimestamp = doNotStartBeforeTimestamp;
GaugeConfigs[recipients[i]] = target;
if (MaxGlobalAmountPerPeriod > 0 && MaxGlobalAmountPerPeriod < getWeeklySpend()) {
revert ExceedsWeeklySpend(getWeeklySpend());
if (MaxGlobalAmountPerPeriod > 0 && MaxGlobalAmountPerPeriod < getWeeklySpend(true)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

overall it is a great addition, but it is unclear why would you include here also the future schedules?

mind elaborating why you think on the comparison against MaxGlobalAmountPerPeriod is important to also consider the future ones?. since the future ones could be in 2 weeks or in a month time etc

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is important that getGlobalAmountSchedule ensures that more than that amount is never paid. It would be too complex to write logic to ensure that otherwise. This may be a little bit "over-protective" but that's ok. If someone get's stuck they can figure that out.

Future scheduling isn't something we expect to do often, and one should think more about the whole thing if you're gtting close to a defined max anyway.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, can see that this may fall more into corner case and that future scheduling of injections is something internally to be considered rare

Only consideration to keep in mind at least while operating as an admin this contract is that on the rare case that future schedules are set for new gauges/recipients besides whatever injections schedules may be running in parallel, that it could lead into revert in case that it's bigger that defined MaxGlobalAmountPerPeriod

As long as internally there is awareness of that should not lead into reverts while operating the injector

Worth adding a minor note/comment around that line of code, from an outside pov it seems a very intricacy detail that can be forgotten

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
if (MaxGlobalAmountPerPeriod > 0 && MaxGlobalAmountPerPeriod < getWeeklySpend(true)) {
/// @dev adding recipients which have starting date in the future and summing up those with current running schedules
/// @dev if greater than `MaxGlobalAmountPerPeriod` may lead to operational reverts
if (MaxGlobalAmountPerPeriod > 0 && MaxGlobalAmountPerPeriod < getWeeklySpend(true)) {

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

feel free to include our @dev comment suggestion or use different language as you may see best fit to highlight this small intricacy

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the point of the whole thing is for the contract to break/revert before sending out more money than intended.

The point of the revert is to make someone think. The error message clearly explains why this configuration action is being taken. I don't think that the @dev notice above helps to clarify things. This is better documented in user guides/in the UI.

In the end after further assessment I think it would make the most sense to not merge this PR. It just adds more complexity to the code without really adding clarity for the folks that would be using it or functionality that I can see likely to be used.

revert ExceedsWeeklySpend(getWeeklySpend(true));
}
if (MaxTotalDue > 0 && MaxTotalDue < getTotalDue()) {
revert ExceedsTotalInjectorProgramBudget(getTotalDue());
Expand Down Expand Up @@ -389,14 +389,17 @@ KeeperCompatibleInterface
}
/**
* @notice Gets the total weekly spend
* @param includeFutureSchedules If true, the total spend will include future schedules, if false will only include schedules with start time before now
* @return weeklySpend The total amount of tokens required to fulfil all active programs for 1 period.
*/
function getWeeklySpend() public view returns (uint256 weeklySpend){
function getWeeklySpend(bool includeFutureSchedules) public view returns (uint256 weeklySpend){
address[] memory gauges = getActiveGaugeList();
for (uint256 i = 0; i < gauges.length; i++) {
Target memory target = GaugeConfigs[gauges[i]];
if (target.periodNumber < target.maxPeriods) {
weeklySpend += target.amountPerPeriod;
if(includeFutureSchedules || target.programStartTimestamp <= block.timestamp) {
weeklySpend += target.amountPerPeriod;
}
}
}
return weeklySpend;
petrovska-petro marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
Loading