MASP rewards controller using token amounts instead of ratios #2460
Conversation
brentstone
force-pushed
the
brent/refactor-shielded-inflation
branch
from
c650800
to
d315396
Compare
brentstone
force-pushed
the
brent/refactor-shielded-inflation
branch
from
508fe9c
to
ad8b1cd
Compare
brentstone
force-pushed
the
brent/refactor-shielded-inflation
branch
from
f67c267
to
0cdca59
Compare
| /// Locked ratio for the given token | ||
| pub locked_ratio_target: Dec, | ||
| /// Target amount for the given token that is locked in the shielded pool | ||
| /// TODO: should this be a Uint or DenominatedAmount??? |
There was a problem hiding this comment.
It should be a uint, I think. Whatever the max supply of a token is (probably u256?)
There was a problem hiding this comment.
Ah but we have the weird MASP denom splitting thing, better think carefully about this
There was a problem hiding this comment.
Uint (or Amount in the worst case) should be fine here. The inflation computations are done at a high level, and the low-level MASP denom splitting is taken care of by the MASP conversion logic.
There was a problem hiding this comment.
Yeah I think Uint is best, as it is clearly readable and digestible by humans, whereas Amount will not be in this scenario.
| // Max inflation amount for this epoch | ||
| let max_inflation = total_native * max_reward_rate / epochs_py; | ||
|
|
||
| // Intermediate values | ||
| let p_gain = p_gain_nom * max_reward_rate / epochs_py; | ||
| let d_gain = d_gain_nom * max_reward_rate / epochs_py; | ||
| let error = locked_amount_target - locked; | ||
| let delta_error = locked_amount_last - locked; | ||
| let control_val = p_gain * error - d_gain * delta_error; |
There was a problem hiding this comment.
review these computations, to make sure they don't have values of 0
* origin/brent/refactor-shielded-inflation: fixup! rebuild masp proofs changelog: add #2460 fix masp tests rebuild masp proofs need denom to write correct target inflation amount fix `Amount` param parsing in toml for PoS masp locked amount param: use u64 in config tomls and `Amount` in storage and usage separate masp and pos rewards controllers
| /// Locked ratio for the given token | ||
| pub locked_ratio_target: Dec, | ||
| /// Target amount for the given token that is locked in the shielded pool | ||
| /// TODO: should this be a Uint or DenominatedAmount??? |
There was a problem hiding this comment.
Uint (or Amount in the worst case) should be fine here. The inflation computations are done at a high level, and the low-level MASP denom splitting is taken care of by the MASP conversion logic.
| storage.write(&masp_kp_gain_key(address), kp_gain_nom)?; | ||
| storage.write(&masp_kd_gain_key(address), kd_gain_nom)?; | ||
|
|
||
| let raw_target = Uint::from(*locked_amount_target) |
There was a problem hiding this comment.
locked_amount_target should probably be a Uint or Amount. If that is the case, then this multiplication (or any other scaling operation) should not be needed. Such a change would probably imply that the denom parameter to this function is unnecessary.
brentstone
force-pushed
the
brent/refactor-shielded-inflation
branch
from
6632297
to
a7be303
Compare
* origin/brent/refactor-shielded-inflation: changelog: add #2460 fix masp tests rebuild masp proofs need denom to write correct target inflation amount fix `Amount` param parsing in toml for PoS masp locked amount param: use u64 in config tomls and `Amount` in storage and usage separate masp and pos rewards controllers
* origin/brent/refactor-shielded-inflation: changelog: add #2460 fix masp tests rebuild masp proofs need denom to write correct target inflation amount fix `Amount` param parsing in toml for PoS masp locked amount param: use u64 in config tomls and `Amount` in storage and usage separate masp and pos rewards controllers
Describe your changes
Two slightly different rewards PD controllers are used for PoS and MASP now. The PoS one remains the same as before, while the MASP one no longer uses target ratios of locked tokens and instead relies on an absolute number of locked tokens.
Indicate on which release or other PRs this topic is based on
v0.30.2
Checklist before merging to
draft