Solver crate forwards flashloans back to driver #3283
Conversation
|
Reminder: Please consider backward compatibility when modifying the API specification.
Caused by: |
sunce86
requested review from
MartinquaXD,
squadgazzz,
mstrug and
m-lord-renkse
| @@ -161,7 +162,16 @@ impl Auction { | |||
| ), | |||
| app_data: AppDataHash(order.app_data.hash().0.into()), | |||
| flashloan_hint: flashloans_enabled | |||
| .then(|| order.app_data.flashloan().cloned().map(Into::into)) | |||
| .then(|| { | |||
| order.app_data.flashloan().map(|flashloan| FlashloanHint { | |||
There was a problem hiding this comment.
I wonder if filling in these default values makes more sense in the component that fetches the app data in the first place. 🤔
No strong opinion, though since I can see why one would want to have access to the pure unaltered appdata.
There was a problem hiding this comment.
True. For now, flashloan hints in their final shape are not needed/used in the driver domain. But we can move this logic out to driver domain when the need comes.
| @@ -684,6 +688,11 @@ fn default_simulation_bad_token_max_age() -> Duration { | |||
| Duration::from_secs(600) | |||
| } | |||
|
|
|||
| // SKY lending DAI token | |||
There was a problem hiding this comment.
Interesting. I wasn't aware that the contract @fedgiac used in the e2e test for the contract only worked with Dai. I'm fine to make this the default for the start where it's only relevant for the e2e test but we should have at least 1 supported general purpose flashloan lender.
Also this could indicate that we might need a module or component that suggests a good lending provider based on the token that needs to be borrowed. Should happen later, though (if at all).
There was a problem hiding this comment.
Agree. There are A LOT of assumptions made at the moment, but I have them in my list. Before going live, these will have to be addressed and some kind of validation for flashloan hints will be implemented to avoid weird cases.
There was a problem hiding this comment.
In principle there are wrappers for incompatible flash-loan lenders (some examples here from this EIP discussion).
But for the e2e test I wanted to avoid the extra mental overhead of the wrapper and I used a protocol that natively supports the standard (but unfortunately only for DAI).
Another assumption in the e2e test is that the flash-loan lender pulls back the token from the borrower by executing transferFrom, but this isn't necessarily the case for other protocols, some other expect the tokens to be transferred back and check that the balance at the end is as expected.
There was a problem hiding this comment.
Yes, we will have to implement a lender specific encoding once we finish this first version that works only with Sky.
| @@ -34,6 +34,7 @@ impl Auction { | |||
| fee_handler: FeeHandler, | |||
| solver_native_token: ManageNativeToken, | |||
| flashloans_enabled: bool, | |||
| flashloan_default_lender: eth::Address, | |||
There was a problem hiding this comment.
By now this constructor has quite a few arguments that will not change during the runtime of the driver.
Might be useful to have a builder component for those pieces of information. 🤔
(follow up task if at all)
Description
Expands
Order/Solutionobjects in solvers-dto crate, needed for e2e tests for flashloans to work.Changes
crates::solvers-dtoexpanded with flashloansHow to test
Existing tests.