Skip balance check for flashloan orders#4278
Skip balance check for flashloan orders#4278felixjff wants to merge 1 commit intocowprotocol:mainfrom
Conversation
Flashloan orders get their sell tokens from the flashloan at settlement time, so they should not be filtered out due to insufficient user balance. Previously, flashloan orders were also required to have the settlement contract as receiver, which is unnecessarily restrictive.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request modifies the handling of flashloan orders to bypass user balance checks and removes the restriction on the order's receiver. The implementation is correct for cases where application data is available. However, I've identified a high-severity logic error where flashloan orders may be incorrectly dropped if their application data fails to be fetched, which undermines the goal of this change.
|
What is the motivation for this PR? |
|
@jmg-duarte currently the Driver is filtering flashloan orders that don't have the settlement contract as receiver. I've noticed several flashloan orders have different receiver addresses (see example). The constraint to only allow orders that have the settlement contract as receiver address seems outdated. Could you explain what the logic behind that is? Why is the Driver allowing only flashloan orders through that have the settlement contract as receiver? |
jmg-duarte
left a comment
jmg-duarte
left a comment
There was a problem hiding this comment.
About the motivation, I miscommunicated.
Please update the PR description to include the reasoning behind the change, thanks in advance!
| app_data: Arc<HashMap<order::app_data::AppDataHash, Arc<app_data::ValidatedAppData>>>, | ||
| cow_amm_orders: Arc<Vec<Order>>, | ||
| settlement_contract: ð::Address, | ||
| _settlement_contract: ð::Address, |
There was a problem hiding this comment.
Remove this parameter and update other call sites
|
This pull request has been marked as stale because it has been inactive a while. Please update this pull request or it will be automatically closed. |
2 participants
Summary