fix: only award redeem premium upto the secure threshold

[nakul1010]

Description Imported from Pr 589

2 significant changes:

• Only award a premium for the amount of tokens required to bring the vault back to the secure threshold. Otherwise the vault could end up paying the premium for all of its tokens.
• Rather than only considering issued_tokens to determine whether or not a vault is below the premium redeem threshold, now use the "to_be_backed" tokens, i.e. the tokens if all open issues and redeems execute successfully (= issued + to_be_issued - to_be_redeemed). This is required because otherwise additional redeems would again be able to de a premium redeem.
• Since the vault will lose collateral by giving out premium redeem fees, consider that collateral as well when calculating premium collateral fees for the user.

Since this significantly modifies audited code, I'd like to have 2 people approve this PR before we merge

Example based on test case

• Vault A
  • Thresholds
    • secure: 200%
    • premium: 160%
    • liquidation: 110%
  • secured capacity: 500 Tokens
  • issued_tokens: 650 Tokens
• User redeems 400 tokens
  • Based on first approach: Premium redeem fee should be given on difference_in_tokens_to_reach_secure_threshold + potential_decrease_in_issued_tokens_after_giving_out_collatral_as_premium_fees = 150 tokens + 3.75 tokens = 153.75 tokens equivalent to 307.5 COL tokens if exchange_rate is 2
  • Premium redeem fee should be given on max_premium_amount a vault can pay = 153.84 tokens equivalent to 307.68 COL tokens if exchange_rate is 2
  • Formula for max_premium_amount: SECURE = (oldCol - maxPremium) / ( oldTokens*EXCH - maxPremium/FEE)

[sander2]

The solution is not 100% correct. But maybe it's good enough.. The problem is what you're doing is:

• Like in fix: only award redeem premium upto the secure threshold #589, determine at how many tokens we'd be at the secure threshold. Determine premium based on that
• Additionally, you now do an additional iteration, where you update the collateral to account for the paid premium, and update the premium based on that.
• The problem is that the step above once again pays out additional premium, thus decreasing the collateral once more. You can do the step above again and again to get ever closer to the correct amount.

The "right" way to do this, is to calculate the maximum amount of premium to payout using math:

CollateralizationRate = totalCollateral / convertToCollateral(totalTokens)
// When doing a premium redeem, this changes to:
// NewCollateralization = (oldCol - maxPremium) / ( oldTokens*EXCH - maxPremium/FEE)
// we want NewCollateralization = SECURE threshold, so
SECURE = (oldCol - maxPremium) / ( oldTokens*EXCH - maxPremium/FEE)
// rewriting gives..
maxPremium = (FEE * (oldCol - oldTokens * EXCH * SECURE)) / (FEE -SECURE)
// plugging in number from the test..
maxPremium = (0.05*(2000 - 650 * 2 * 2)) / (0.05 - 2)
= 15.3846153846

In comparison, #589 would award max 15, and this PR awards 15.375. Both are pretty close, so I'm not sure we really need the math based approach. @gregdhill what do you think?

[nakul1010]

The solution is not 100% correct. But maybe it's good enough.. The problem is what you're doing is:

• Like in fix: only award redeem premium upto the secure threshold #589, determine at how many tokens we'd be at the secure threshold. Determine premium based on that
• Additionally, you now do an additional iteration, where you update the collateral to account for the paid premium, and update the premium based on that.
• The problem is that the step above once again pays out additional premium, thus decreasing the collateral once more. You can do the step above again and again to get ever closer to the correct amount.

The "right" way to do this, is to calculate the maximum amount of premium to payout using math:

CollateralizationRate = totalCollateral / convertToCollateral(totalTokens)
// When doing a premium redeem, this changes to:
// NewCollateralization = (oldCol - maxPremium) / ( oldTokens*EXCH - maxPremium/FEE)
// we want NewCollateralization = SECURE threshold, so
SECURE = (oldCol - maxPremium) / ( oldTokens*EXCH - maxPremium/FEE)
// rewriting gives..
maxPremium = (FEE * (oldCol - oldTokens * EXCH * SECURE)) / (FEE -SECURE)
// plugging in number from the test..
maxPremium = (0.05*(2000 - 650 * 2 * 2)) / (0.05 - 2)
= 15.3846153846

In comparison, #589 would award max 15, and this PR awards 15.375. Both are pretty close, so I'm not sure we really need the math based approach. @gregdhill what do you think?

Changed the approach based on the new formula provided.

[sander2]

We have a vaultRegistry_getPremiumRedeemVaults rpc. @gregdhill I think we should we update that to return only the amount that is premium-redeemable, right?

[sander2]

@gregdhill @nud3l Do we want to award a premium up to the global secure threshold, or to the vault's custom threshold? Personally for my vault I would prefer to only award a premium upto the global secure threshold, otherwise setting a higher threshold seemingly increases the risk of losing money through premium payouts

[gregdhill]

Agree with using the second approach @sander2, as for your other comments we should only return the amount that is premium redeemable in the RPC and I also agree we should award premium up to the global secure threshold since that is always lower.

[nakul1010]

@sander2

• RedeemTransactionSize storage is required to calculate premium_redeem_vaults in vault_registery, but redeem pallet can't be imported in vault_registery as it create cyclic dependency.

1. Migration of storage (move RedeemTransactionSize storage from redeem to vault_registery pallet )

   • Pros
     • no frontend change required
   • Cons
     • migrating storage just for a method used during RPC call doesn't make sense.
     • the storage ideally should be stored in redeem pallet as related to redeem request.

2. Move RPC call (move RPC from vault_registery to redeem pallet )

   • Pros
     • no migration required
   • Cons
     • frontend change required to call a different method

3. Add a router crate, but then will have one more crate to maintain.

went ahead with 2 approach

cc: @gregdhill

[sander2]

I'm ok with moving the rpc to the redeem crate. We could keep the rpc name identical s.t. it doesn't require UI changes. Fwiw there is also an option 3 to add a new getter to the vault_registry config trait, but option 2 works for me as well

[nakul1010]

I'm ok with moving the rpc to the redeem crate. We could keep the rpc name identical s.t. it doesn't require UI changes. Fwiw there is also an option 3 to add a new getter to the vault_registry config trait, but option 2 works for me as well

changed the RPC method name from redeem_getPremiumRedeemVaults to vaultRegistry_getPremiumRedeemVaults in the latest commit, this will not affect the frontend code.

[nud3l]

I'm ok with moving the rpc to the redeem crate. We could keep the rpc name identical s.t. it doesn't require UI changes. Fwiw there is also an option 3 to add a new getter to the vault_registry config trait, but option 2 works for me as well

changed the RPC method name from redeem_getPremiumRedeemVaults to vaultRegistry_getPremiumRedeemVaults in the latest commit, this will not affect the frontend code.

I think it will require an update in the lib maybe?

[sander2]

I'm ok with moving the rpc to the redeem crate. We could keep the rpc name identical s.t. it doesn't require UI changes. Fwiw there is also an option 3 to add a new getter to the vault_registry config trait, but option 2 works for me as well

changed the RPC method name from redeem_getPremiumRedeemVaults to vaultRegistry_getPremiumRedeemVaults in the latest commit, this will not affect the frontend code.

I think it will require an update in the lib maybe?

Yea it for sure affects frontend/lib. Let's keep the old naming as an alias, see https://docs.rs/jsonrpsee-proc-macros/0.20.3/jsonrpsee_proc_macros/attr.rpc.html#method-attribute

[sander2]

I pushed a small change. The is_below_premium_threshold was confusing to me, since unlike the other checks, it uses to_be_backed tokens rather than issued tokens. So in the tests is_below_premium_threshold was true while is_below_secure_threshold was false. Changed the naming to will_be_below_premium_threshold to avoid that confusion. I also removed the check altogether from the non-test code since it wasn't necessary. By removing the check, this code gets hit by more of the tests so I'm a little bit more confident it's correct. Also this way we're sure that the premium calculation gets accounted for in the benchmarks.

I also made some changes to the testing code