Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

added liquidate and redeem (with protocol fee) #75

Merged
merged 7 commits into from
Mar 15, 2022
Merged

Conversation

nope-finance
Copy link
Member

No description provided.

@nope-finance nope-finance changed the title added liquidated and redeem (with protocol fee) added liquidate and redeem (with protocol fee) Feb 23, 2022
.gitignore Outdated Show resolved Hide resolved
Copy link

@jstarry jstarry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My impression of this change is that you want to achieve two things:

  1. Add a convenience method for liquidate + redeem
  2. Add liquidation fees to generate protocol revenue

It looks to me that the convenience method is not much (if at all) more compute efficient than calling the individual instructions separately. However, it seems like you can cut out a refresh reserve ix and you're probably saving on encoded tx size as well, so maybe it's worth it? I would have expected more effort spent on optimizing this instruction to avoid redundant work but maybe you plan to do that later?

For the protocol fees, since there are no fees for the original liquidate instruction, I'm curious what your plans are for enticing liquidators to use the new ix over the old one. Any reason you didn't add a collateral token fee receiver to the reserve to collect fees there too?

token-lending/program/src/instruction.rs Outdated Show resolved Hide resolved
token-lending/program/src/processor.rs Show resolved Hide resolved
token-lending/program/src/state/reserve.rs Outdated Show resolved Hide resolved
token-lending/program/src/processor.rs Show resolved Hide resolved
token-lending/program/src/processor.rs Show resolved Hide resolved
let withdraw_liquidity_amount = _redeem_reserve_collateral(
program_id,
withdraw_collateral_amount,
destination_collateral_info,
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than withdrawing collateral during _liquidate_obligation and then burning the collateral in _redeem_reserve_collateral, you could save a CPI by just directly burning from the reserve's collateral supply. And on this subject, there is some other redundant work done in both _liquidate_obligation and _redeem_reserve_collateral (notably the PDA creation) that could be saved with some more work as well.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeh I think the reasoning i had for doing this way was to be more modular and write as little new math/logic code as possible not optimize compute (beyond sticking to < 200k ish). but perhaps there is some level of optimization that could be added.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, optimizations can easily introduce critical bugs as well, so there's a trade-off

token-lending/program/src/processor.rs Show resolved Hide resolved
@nope-finance
Copy link
Member Author

nope-finance commented Mar 2, 2022

My impression of this change is that you want to achieve two things:

Add a convenience method for liquidate + redeem
Add liquidation fees to generate protocol revenue
It looks to me that the convenience method is not much (if at all) more compute efficient than calling the individual instructions separately. However, it seems like you can cut out a refresh reserve ix and you're probably saving on encoded tx size as well, so maybe it's worth it? I would have expected more effort spent on optimizing this instruction to avoid redundant work but maybe you plan to do that later?

Yeh it's probably less compute efficient (but maybe not since saves a refresh) and saves a few bytes of txn space. also it's slightly challenging for liquidators to know exactly how much ctoken they will receive so this allows us to compute that for them so they don't have to guess and be left with some dust (or do a follow up txn which is what we observe now mostly)

edit:
so i actually tested and it does use slightly less compute total (i guess because not having to unpack the accounts array multiple times?)

For the protocol fees, since there are no fees for the original liquidate instruction, I'm curious what your plans are for enticing liquidators to use the new ix over the old one. Any reason you didn't add a collateral token fee receiver to the reserve to collect fees there too?
didn't want to have to deal with storing a 2nd set of fee receivers on the reserve so thought it made more sense to redeem and take from the final liquidity

the plan is to disable the old method after X amount of time

@jstarry
Copy link

jstarry commented Mar 2, 2022

the plan is to disable the old method after X amount of time

Cool, that's what I expected. I would just be wary of the increased compute cost then, but like I've mentioned a few times, can always do that later as long as there's no big impact to how liquidators are currently operating.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants