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

Should we pass the token address to the callback functions ? #245

Closed
Jean-Grimal opened this issue Aug 8, 2023 · 14 comments · Fixed by #250
Closed

Should we pass the token address to the callback functions ? #245

Jean-Grimal opened this issue Aug 8, 2023 · 14 comments · Fixed by #250

Comments

@Jean-Grimal
Copy link
Contributor

Jean-Grimal commented Aug 8, 2023

As it stands the callback functions don't take the token address as argument, but the flashloan function does.
Do we want to add this argument to these functions or assume the the caller handle it by himself with bytes calldata data ?
I think we should as it doesn't mean adding new arguments to the entry points functions.

@pakim249CAL
Copy link
Contributor

From #238 (comment)

Yes, I think the data field is enough for callers to make use of the callback. This data should be used to store all necessary context that the calling contract needs to do what they need to do. IMO callbacks should only return values that the calling contract does not know. e.g. amount repaid if repaying max.

@MerlinEgalite
Copy link
Contributor

MerlinEgalite commented Aug 8, 2023

I'm in favor

@Rubilmax
Copy link
Collaborator

Rubilmax commented Aug 9, 2023

Are we talking about the token address or the market parameters?

In the case of the token address, it only makes sense to me to pass it into callbacks upon flash loan, which is already what we're doing. For all other callbacks, it seems odd and incomplete to pass the borrowable asset address instead of all the market parameters. I believe passing the whole market struct as parameter of callback functions is not complex and has a relatively low gas cost so I'm ok with doing so

@Jean-Grimal
Copy link
Contributor Author

Jean-Grimal commented Aug 9, 2023

For all other callbacks, it seems odd and incomplete to pass the borrowable asset address instead of all the market parameters.

Yes you are right, I just thought about it while doing the PR and just passing one token address to the callback function doesn't seem appropriate (particularly for the liquidate callback)

@makcandrov
Copy link
Contributor

I think similarly to Patrick. I'm not in favor of passing the token address, and even less of passing the entire market structure – it contains too much unnecessary data (nobody cares about the IRM or the oracles).

I would even be in favor of removing amount in callbacks other than in repay if it didn't disrupt the harmony between them. I believe that attempting to anticipate the needs of integrators will likely result in burdening them with variables they don't care about, and they will still pass the data they truly need.

@MathisGD
Copy link
Contributor

MathisGD commented Aug 9, 2023

For the sake of simplicity and minimalism, I am against adding anything that cannot be know in advance by the user. So at a first sight, everything except amount in repay. One could even argue that since the caller is synchronous, they can compute the amount in repay but I'm ok with keeping it too, and even in supply/supplyCollat for harmony.

@makcandrov
Copy link
Contributor

I am against adding anything that cannot be know in advance by the user

I think you mean anything that can be known in advance by the user.

I agree and I'd even add anything that can be known without any additional gas cost in advance by the user. That way, it excludes the amount to repay.

@MerlinEgalite
Copy link
Contributor

I agree, let's remove any amount except in repay

@MathisGD
Copy link
Contributor

MathisGD commented Aug 9, 2023

But not having the same interface for all callbacks is also a bit weird, that's why I'm ok with keeping amount in other callbacks.

@MerlinEgalite
Copy link
Contributor

As you want I don't have a strong opinion on this tbh

@Jean-Grimal
Copy link
Contributor Author

After thinking about it I agree with you.
Callback users are advanced and experienced users and we can assume that they will handle the data correctly.
Therefore we shouldn’t pass arguments that can be known without any additional gas cost in advance by the user if it can help save some gas.
So probably the only argument that is worth passing to the callbacks is repaid in the liquidate case.

@MathisGD
Copy link
Contributor

MathisGD commented Aug 9, 2023

I'm ok with adding token everywhere too, as requested by @Rubilmax in #250, as it will be used 95% of the time and it's really common.

@MerlinEgalite
Copy link
Contributor

MerlinEgalite commented Aug 9, 2023

I'm ok too, should we close your PR then @MathisGD ?

@MathisGD
Copy link
Contributor

#260 (comment)

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