Reward factor state machine - #6
Conversation
Does not include fixing the tests.
meiersi-da
left a comment
meiersi-da
left a comment
There was a problem hiding this comment.
Thanks. Very valuable to have this code to align on the implementation. Directionally this seems right, but it looks like the logic might be simplified.
| -- ^ the weight of an SV for voting, can be zero | ||
| rewardWeight : Int | ||
| -- ^ the weight of an SV for reward minting, can be zero | ||
| rewardFactorState: Optional SvRightOwnerRewardFactorState |
There was a problem hiding this comment.
| rewardFactorState: Optional SvRightOwnerRewardFactorState | |
| rewardAdjustmentState: Optional RewardAdjustmentState |
| permanentWeightFactor : Decimal | ||
| -- ^ the permanent maximum weight factor due to permanent underlocking |
There was a problem hiding this comment.
I'd have expected that we don't store that here at all. Instead just adjust the SvRightOwner.rewardWeight directly.
There was a problem hiding this comment.
I'm wary of not storing this or a copy of the original rewardWeight, because we need to not apply a loss factor repeatedly and the degree of underlocked an SV is does not change at all based on their weight; there's several wrong ways to do this where you can end up multiplying the rewardWeight by a factor repeatedly if we don't have the original number somewhere. The math is also a bit underspecified and if we treat permanent adjustments as updating the root truth, and can be complex just to keep avoiding repeated penalties.
| effectiveTime : Time | ||
| controller dso | ||
| do | ||
| -- Assumes that we can calculate lock state one week in the past for enforcement. |
There was a problem hiding this comment.
polish: push the core logic into Splice.DSO.SvState and add unit tests for it. Thereby improving readability and focus.
There was a problem hiding this comment.
Move is done, unit tests not yet.
| permanentWeightFactor = 1.0 | ||
| recoverableUnderlocks = [] | ||
|
|
||
| let sevenDaysAgo = effectiveTime `addRelTime` days (-7) |
| create svRightOwner with | ||
| info = svRightOwner.info with | ||
| rewardFactorState = Some currentRewardFactorState with | ||
| rewardWeightFactor = nextActive.newFactor |
There was a problem hiding this comment.
Please add a comment clarifying what None means? It presumably means "no adjustment". If that's the case though, then I'd expect that it resolves all underlocks and sets the rewardWeightFactor to 1.0?
Thinking about the logic a bit more it seems to me that there should be an implementation that works with just the new adjustment factor being passed in:
just remove all underlocks with adjustment factors <= the new adjustment factor.
I'd implement adjustment for
The under-locked portion of its SV Weight is removed from the active SV pool within 7 days
as part of reward coupon creation: retrieve the lowest adjustment factor older than 7 days.
The above are just sketches of parts of the full implementation. It does though hint at options to simplify the algorithm. My suspicion is that it should only need the earliest time at which a non-recovered underlock occurred. So a Map Decimal Time might be the right type to drive the logic.
That said: I might well be misunderstanding the problem. Probably best to do a code walkthrough together this afternoon.
There was a problem hiding this comment.
It probably is clearer to just split the None case off into it's own choice or remove the need for it entirely; it's "update the active adjustment to the most recent underlock more than seven days before effectiveTime". It's certainly an option to just not store the "current" value and seek to it for each reward, there was just a stated preference at one point for explicit management of the weight.
2 participants
Does not yet include fixing the tests.
Pull Request Checklist
Cluster Testing
/cluster_teston this PR to request it, and ping someone with access to the DA-internal system to approve it./upgrade_teston this PR to request it, and ping someone with access to the DA-internal system to approve it./hdm_teston this PR to request it, and ping someone with access to the DA-internal system to approve it./lsu_teston this PR to request it, and ping someone with access to the DA-internal system to approve it.PR Guidelines
Fixes #n, and mention issues worked on using#nMerge Guidelines