Refactor from and to pool transfers #13
Conversation
There was a problem hiding this comment.
Thanks for the PR!
The changes specifically for #11 look good, we just need to remove spender and to from the submit function interface.
While reviewing the changes for #4, I think we need to go a slightly different direction for solving this, and it requires waiting for #5 to be completed.
The issue is the transfer's from the user to the pool are appended to the transaction, and signed by the user. When differences are taken/on repayment, the amount coming from the user to the pool can change each block due to interest accrual.
The repayment side can be fixed, but when a full position is withdrawn (consider, user exiting looped XLM collateral and XLM liability position in 1 transaction) cannot be fixed.
I think the best route forward would be to retain the current spender_transfer and pool_transfer actions, and if processing with transfer_from, take the difference for each set of user's and assets after #5 is merged.
pool/src/pool/actions.rs
Outdated
| actions.add_for_spender(&request.asset, &request.user, request.amount); | ||
| if d_tokens_burnt > cur_d_tokens { | ||
| let amount_to_refund = | ||
| request.amount - reserve.to_asset_from_d_token(cur_d_tokens); | ||
| require_nonnegative(e, &amount_to_refund); | ||
| from_state.remove_liabilities(e, &mut reserve, cur_d_tokens); | ||
| actions.add_for_pool_transfer(&reserve.asset, amount_to_refund); | ||
| actions.add_for_pool(&request.asset, &request.user, amount_to_refund); |
There was a problem hiding this comment.
This will make it difficult for users to fully repay loans, as users have to sign for "tokens_in", so it needs to be consistent between blocks.
Every block, interest is accrued, and thus the total loan amount being repaid also changes. So, it a user submits a request of "repay 1000 USDC" and their loan is 900, the protocol will calculate a transfer of 900 USDC (100 refund) and request authorization from the user in the transaction.
If they submit the transaction on the next block, their loan is now 900.00001 USDC and the pool will request 900.00001 USDC (99.99999 refund), which would invalidate their signature.
This is less of an issue when #5 is completed, however we still should support singular repayment operations with transfer.
Hey @brozorec. This opened up a can of worms that we are still figuring out the best way to move forward, which has been slow to iron out due to the holidays. The TL;DR is:
Options are:
In discussion with @heytdep about which options make more sense to ensure we cover all use cases. I'll get back to this / update the issue in a day or two with what makes the most sense. If you have any opinions here as well, would love to hear them. |
Proposing to solve #4 and #11 by refactoring the
Actionsstruct.Instead of having 2 maps: one for to-pool and one for from-pool transfers, I suggest doing the accounting in one place by adding or subtracting. This can result in a positive balance (pool must be debited) or a negative balance (pool must be credited). This balance out the account at the end resulting in only one transfer per asset and per user.
Making a draft PR to temp check whether those changes are going in the right direction. If that's confirmed, more optimization could be done such as reducing the number of params in some functions. Looking forward to your feedback!