fix: remove expired check in checkUpkeep

[gosuto-inzasheru]

closes #88

since cleaning the queue is permissionless, there is no need to pass it through the timelock. it can simply be called by the vmodule while queueing up a tx

[codecov]

Codecov Report

Attention: Patch coverage is 69.23077% with 4 lines in your changes missing coverage. Please review.

Project coverage is 97.20%. Comparing base (acbfe14) to head (6835704).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main      #98      +/-   ##
==========================================
- Coverage   99.02%   97.20%   -1.82%     
==========================================
  Files           2        2              
  Lines         205      215      +10     
  Branches       33       37       +4     
==========================================
+ Hits          203      209       +6     
- Misses          2        6       +4     

Files	Coverage Δ
src/RoboSaverVirtualModule.sol	97.38% <69.23%> (-2.07%)	⬇️

[petrovska-petro]

good shuffle of condition checks for triggering clean up when another normal action will get queue

nice test addition test_When_QueueHasInternalExpiredTxs. not sure if that ever will happen in prod tho, otherwise i think we have a bigger problem w/ the keeper system allowing an internal tx being expired

imagine that actually it happens should not at some point actually happen also a storage clean-up, meaning delete queuedTx;? otherwise when calling _executeQueuedTx will be trying to exec an expired payload, no?

[gosuto-inzasheru]

imagine that actually it happens should not at some point actually happen also a storage clean-up, meaning delete queuedTx;? otherwise when calling _executeQueuedTx will be trying to exec an expired payload, no?

i fixed this by only returning EXEC_QUEUE_POOL_ACTION if !_isCleanQueueRequired(). otherwise, it will look for an actual pool action. this in turn will call _queueTx with the new action, overwriting the exipred one. ie cleanup should only occur as part of a (new) action, not by itself

lmk if you agree :D

[gosuto-inzasheru]

hmm what do you think of this

Warning (8760): This declaration has the same name as another declaration.
   --> src/RoboSaverVirtualModule.sol:266:[13](https://github.com/onchainification/robosaver/actions/runs/9939898449/job/27456020086#step:7:14):
    |
266 |             uint256 deficit = dailyAllowance - balance + buffer;
    |             ^^^^^^^^^^^^^^^
Note: The other declaration is here:
   --> src/RoboSaverVirtualModule.sol:300:5:
    |
300 |     function deficit() external view returns (uint256 deficit_) {
    |     ^ (Relevant source part starts here and spans across multiple lines).

[petrovska-petro]

re the warning of duplication for surplus and deficit

i like those methods deficit() & surplus(), but why not make them instead public view? 🤔

as a simple solution is that those internal variables used in checkUpkeep could be added a prefix/suffix to avoid the duplication. e.g: _deficit, deficitShadow

[petrovska-petro]

i fixed this by only returning EXEC_QUEUE_POOL_ACTION if !_isCleanQueueRequired(). otherwise, it will look for an actual pool action. this in turn will call _queueTx with the new action, overwriting the exipred one. ie cleanup should only occur as part of a (new) action, not by itself

i think is a valid solution

really nice test flow in test_When_QueueHasInternalExpiredTxs mirroring actual actions triggering each behaviour 🔥