Skip to content

Add callbacks #121

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

Merged
merged 23 commits into from
Jul 28, 2023
Merged

Add callbacks #121

merged 23 commits into from
Jul 28, 2023

Conversation

makcandrov
Copy link
Contributor

I think that callbacks are important for advanced users to perform complex operations. To provide maximum flexibility, we should add one before transferring an asset from the user to Blue (ie in supply, supply collateral, repay, liquidate, and flashloan), and allow reentrancies on purpose. It can seem scary, but it can be done safely if the last two things in the concerned function are the callback and then the transferFrom, with all the checks and storage updates being done before. In fact, it remains safe as long as we never use the contract's balance in the logic.

I made some choices that may be discussed (in addition to the need for callbacks ofc):

  • Should we have a single callback for all operations or a different one for each? I decided to implement a single callback because when I think of how the integrator would use it, I believe the logic will almost always be the same in all cases (at least for supply, supply collateral, and repay). Perhaps only liquidate and flashloan should have separate callbacks.
  • What arguments should we pass to the callback? I made the choice to provide a generic amount, representing the amount to supply or repay, depending on the case

pakim249CAL
pakim249CAL previously approved these changes Jul 12, 2023
@makcandrov makcandrov requested a review from Jean-Grimal July 12, 2023 04:44
Copy link
Contributor

@MerlinEgalite MerlinEgalite left a comment

Choose a reason for hiding this comment

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

I like the idea but need to reflect a bit more on it.

I don't think there's any problem with this PR either. If it does then the problem was already there for any ERC777 tokens.

I just made some suggestions!

@QGarchery QGarchery requested a review from Rubilmax July 17, 2023 13:05
Copy link
Collaborator

@Rubilmax Rubilmax left a comment

Choose a reason for hiding this comment

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

I thought one main usecase would be single-tx leverage, which involves borrowing and making the promise to supply enough collateral by the end of the callback. It seems callbacks are not implemented for withdraw/borrow. Is this intended?

@pakim249CAL
Copy link
Contributor

I thought one main usecase would be single-tx leverage, which involves borrowing and making the promise to supply enough collateral by the end of the callback. It seems callbacks are not implemented for withdraw/borrow. Is this intended?

I don't think callbacks would be useful in those cases, especially before the token transfers. It is likely that whoever is calling the funds wants the funds actually transferred from Blue to the receiver so that they can do something with it. If it is a contract calling Blue, they can simply continue with their logic as normal. As such, I saw it as intended design to not implement callbacks in these functions. Maybe I'm mistaken though.

@Rubilmax
Copy link
Collaborator

I don't think callbacks would be useful in those cases, especially before the token transfers. It is likely that whoever is calling the funds wants the funds actually transferred from Blue to the receiver so that they can do something with it. If it is a contract calling Blue, they can simply continue with their logic as normal. As such, I saw it as intended design to not implement callbacks in these functions. Maybe I'm mistaken though.

Totally makes sense, sorry!

Rubilmax
Rubilmax previously approved these changes Jul 17, 2023
Copy link
Collaborator

@Rubilmax Rubilmax left a comment

Choose a reason for hiding this comment

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

Contango is extremely bullish on this feature. They suggest to have separate callbacks. In any case, I'm ok to include this feature.

@makcandrov makcandrov mentioned this pull request Jul 17, 2023
@makcandrov makcandrov dismissed stale reviews from Rubilmax and pakim249CAL via ead5748 July 18, 2023 13:33
@MerlinEgalite
Copy link
Contributor

I think I'm in favor of introducing callbacks to Blue it would ease a lot some usecases on top of Blue(position migration, liquidations, etc.)

@Jean-Grimal
Copy link
Contributor

I thought about it and changed my mind about callbacks. I now also think callbacks should be implemented.

Copy link
Contributor

@MerlinEgalite MerlinEgalite left a comment

Choose a reason for hiding this comment

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

Can you do a gas diff comparison @makcandrov please?

@makcandrov
Copy link
Contributor Author

supply +841(+0.8%)
supplyCollateral +156 (+0.2%)
repay +595 (+0.7%)
liquidate +472 (+0.4%)

main:
image
feat/callbacks:
image

@Rubilmax
Copy link
Collaborator

It seems to me that defining a specific function for each callback is a better design (suggested by Contango):

  • each callback has a single responsibility
  • it leverages Solidity's compiler's function dispatcher instead of a custom one

@makcandrov
Copy link
Contributor Author

If everyone agrees (or is not opposed, at least), I'll implement that

Jean-Grimal
Jean-Grimal previously approved these changes Jul 24, 2023
Jean-Grimal
Jean-Grimal previously approved these changes Jul 27, 2023
MerlinEgalite
MerlinEgalite previously approved these changes Jul 27, 2023
pakim249CAL
pakim249CAL previously approved these changes Jul 27, 2023
MerlinEgalite
MerlinEgalite previously approved these changes Jul 28, 2023
@MathisGD
Copy link
Contributor

Let's rebase on main and handle this question.

@makcandrov makcandrov requested a review from MerlinEgalite July 28, 2023 08:18
@MathisGD MathisGD merged commit 4a860dc into main Jul 28, 2023
@MathisGD MathisGD deleted the feat/callbacks branch July 28, 2023 15:04
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.

Add callbacks
6 participants