make getWeeklySpend more sensitive to future programs. #40
Conversation
Include future spend when checking, to ensure that at no point will more than the max amount ever be spent.
|
Not sure this is needed but if you think it is better/more clear we can merge it. |
| @@ -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)) { | |||
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
| 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)) { |
There was a problem hiding this comment.
feel free to include our @dev comment suggestion or use different language as you may see best fit to highlight this small intricacy
There was a problem hiding this comment.
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.
Tritium-VLK
requested review from
petrovska-petro and
gosuto-inzasheru
and removed request for
gosuto-inzasheru
Include future spend when checking, to ensure that at no point will more than the max amount ever be spent.
Fixes #33