fix: feegrant grant period not resetting #9450
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9450 +/- ##
==========================================
+ Coverage 60.39% 60.50% +0.10%
==========================================
Files 589 590 +1
Lines 37099 37342 +243
==========================================
+ Hits 22405 22592 +187
- Misses 12715 12769 +54
- Partials 1979 1981 +2
|
x/feegrant/periodic_fee.go
Outdated
| // if Basic.SpendLimit is not set, reset to PeriodSpendLimit | ||
| if a.Basic.SpendLimit.Empty() { | ||
| a.PeriodCanSpend = a.PeriodSpendLimit | ||
| } |
There was a problem hiding this comment.
I can add this check to the condition above but I thought it might be nice to separate for clarity.
There was a problem hiding this comment.
Isn't it a logic error to put here?
Say I create a grant of 200token total, with a max periodic spend limit of 100token/mo, and the grantee uses everything.
- Expected at beginning of 1st month:
a.Basic.SpendLimit == 200, a.PeriodCanSpend == 100 - Expected at end of 1st month:
a.Basic.SpendLimit == 100, a.PeriodCanSpend == 0 - Expected at beginning of 2nd month:
a.Basic.SpendLimit == 100, a.PeriodCanSpend == 100 - Expected at end of 2nd months:
a.Basic.SpendLimit == 0, a.PeriodCanSpend == 0 - But with this code, after two months:
a.Basic.SpendLimit == 0, a.PeriodCanSpend == 100
There was a problem hiding this comment.
At the end of the second month, the grant would be removed. I will take another look and double check.
There was a problem hiding this comment.
Just to note, we're not actually updating the value of a.Basic.SpendLimit within the tryResetPeriod method; we are doing that within the Accept method. Maybe this is causing some confusion.
If a.Basic.SpendLimit is set and the result of a.Basic.SpendLimit.SafeSub(a.PeriodSpendLimit) is negative, we will set a.PeriodCanSpend to a.Basic.SpendLimit.
If a.Basic.SpendLimit is not set, the result of a.Basic.SpendLimit.SafeSub(a.PeriodSpendLimit) is always negative, and a.PeriodCanSpend is always set to empty. This pull request fixes this issue.
I believe this is covered in the tests. I also manually tested the above scenario and the grant is removed after the second month (2 minutes in my test). Let me know if you think there is still an edge case I am missing.
There was a problem hiding this comment.
I consolidated this into the condition that was already there. The same logic applies so let me know if you think I might still be overlooking something.
ryanchristo
force-pushed
the
ryan/9439-feegrant-reset
branch
from
41e0146 to
14630ad
Compare
amaury1093
added
the
A:automerge
Description
This pull request fixes grant period reset to account for an ongoing periodic grant with no total limit.
closes: #9439
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorerCodecov Reportin the comment section below once CI passes