Skip to content

Commit

Permalink
feat: minimum_balance for placing orders set to 1 atom for FOK (#2558)
Browse files Browse the repository at this point in the history
# Description

The frontend wants to enable placement of LIMIT orders without requiring
the full balance.
It can be tested here cowprotocol/cowswap#3742

It is already possible for Partially fillable orders due to the partial
fill nature, but Fill or kill orders still require the full balance.

The arguments for the feature is:
1. allow users setting up a limit order in advance of receiving funds,
for example in a newly created Safe. Once funds are received, order
becomes valid and trade-able

The argument to remove the Fill or kill limitation is:
1. allow users a seamless experience when placing LIMIT orders across
both sub-types, without requiring a different balance constraint

Due to spam protection, 1 atom of the sell token balance is still
required, and that is not being removed.


# Changes

- [ ] Remove full balance limitation for both order types (Fill or kill
and Partially fillable)
- [ ] Turn `minimum_balance` function into a constant of value `1`.

## How to test
This change relies on existing unit tests

## Notes
Please do let me know where best to put the constant, whether there is
need to add any specific unit test and so on

---------

Co-authored-by: Martin Beckmann <martin.beckmann@protonmail.com>
  • Loading branch information
alfetopito and MartinquaXD authored Mar 25, 2024
1 parent 48ad4c9 commit 5950d92
Show file tree
Hide file tree
Showing 3 changed files with 91 additions and 84 deletions.
1 change: 1 addition & 0 deletions crates/e2e/tests/e2e/main.rs
Original file line number Diff line number Diff line change
Expand Up @@ -27,5 +27,6 @@ mod smart_contract_orders;
mod solver_competition;
mod submission;
mod tracking_insufficient_funds;
mod uncovered_order;
mod univ2;
mod vault_balances;
84 changes: 84 additions & 0 deletions crates/e2e/tests/e2e/uncovered_order.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
use {
e2e::{setup::*, tx, tx_value},
ethcontract::U256,
model::{
order::{OrderCreation, OrderKind},
signature::EcdsaSigningScheme,
},
secp256k1::SecretKey,
shared::ethrpc::Web3,
web3::signing::SecretKeyRef,
};

#[tokio::test]
#[ignore]
async fn local_node_uncovered_order() {
run_test(test).await;
}

/// Tests that a user can already create an order if they only have
/// 1 wei of the sell token and later fund their account to get the
/// order executed.
async fn test(web3: Web3) {
tracing::info!("Setting up chain state.");
let mut onchain = OnchainComponents::deploy(web3).await;

let [solver] = onchain.make_solvers(to_wei(10)).await;
let [trader] = onchain.make_accounts(to_wei(10)).await;
let [token] = onchain
.deploy_tokens_with_weth_uni_v2_pools(to_wei(1_000), to_wei(1_000))
.await;
let weth = &onchain.contracts().weth;

tx!(
trader.account(),
weth.approve(onchain.contracts().allowance, to_wei(3))
);

tracing::info!("Starting services.");
let services = Services::new(onchain.contracts()).await;
services.start_protocol(solver).await;

tracing::info!("Placing order with 0 sell tokens");
let order = OrderCreation {
sell_token: weth.address(),
sell_amount: to_wei(2),
fee_amount: 0.into(),
buy_token: token.address(),
buy_amount: to_wei(1),
valid_to: model::time::now_in_epoch_seconds() + 300,
kind: OrderKind::Sell,
partially_fillable: false,
..Default::default()
}
.sign(
EcdsaSigningScheme::Eip712,
&onchain.contracts().domain_separator,
SecretKeyRef::from(&SecretKey::from_slice(trader.private_key()).unwrap()),
);
// This order can't be created because we require the trader
// to have at least 1 wei of sell tokens.
services.create_order(&order).await.unwrap_err();

tracing::info!("Placing order with 1 wei of sell_tokens");
tx_value!(trader.account(), 1.into(), weth.deposit());
// Now that the trader has some funds they are able to create
// an order (even if it exceeds their current balance).
services.create_order(&order).await.unwrap();

tracing::info!("Deposit ETH to make order executable");
tx_value!(trader.account(), to_wei(2), weth.deposit());

tracing::info!("Waiting for order to show up in auction");
wait_for_condition(TIMEOUT, || async { services.solvable_orders().await == 1 })
.await
.unwrap();

// Drive solution
tracing::info!("Waiting for trade.");
wait_for_condition(TIMEOUT, || async { services.solvable_orders().await == 0 })
.await
.unwrap();
let balance_after = weth.balance_of(trader.address()).call().await.unwrap();
assert_eq!(U256::one(), balance_after);
}
90 changes: 6 additions & 84 deletions crates/shared/src/order_validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -591,8 +591,6 @@ impl OrderValidating for OrderValidator {
verification,
};

let min_balance = minimum_balance(&data).ok_or(ValidationError::SellAmountOverflow)?;

// Fast path to check if transfer is possible with a single node query.
// If not, run extra queries for additional information.
match self
Expand All @@ -604,7 +602,7 @@ impl OrderValidating for OrderValidator {
source: data.sell_token_balance,
interactions: app_data.interactions.pre.clone(),
},
min_balance,
MINIMUM_BALANCE,
)
.await
{
Expand Down Expand Up @@ -826,17 +824,11 @@ fn has_same_buy_and_sell_token(order: &PreOrderData, native_token: &WETH9) -> bo
}

/// Min balance user must have in sell token for order to be accepted.
///
/// None when addition overflows.
fn minimum_balance(order: &OrderData) -> Option<U256> {
// TODO: We might even want to allow 0 balance for partially fillable but we
// require balance for fok limit orders too so this make some sense and protects
// against accidentally creating order for token without balance.
if order.partially_fillable {
return Some(1.into());
}
order.sell_amount.checked_add(order.fee_amount)
}
// All orders can be placed without having the full sell balance.
// A minimum, of 1 atom is still required as a spam protection measure.
// TODO: ideally, we should keep the full balance enforcement for SWAPs,
// but given all orders are LIMIT now, this is harder to do.
const MINIMUM_BALANCE: U256 = U256::one(); // 1 atom of a token

/// Retrieves the quote for an order that is being created and verify that its
/// fee is sufficient.
Expand Down Expand Up @@ -988,22 +980,6 @@ mod tests {
std::str::FromStr,
};

#[test]
fn minimum_balance_() {
let order = OrderData {
sell_amount: U256::MAX,
fee_amount: U256::from(1),
..Default::default()
};
assert_eq!(minimum_balance(&order), None);
let order = OrderData {
sell_amount: U256::from(1),
fee_amount: U256::from(1),
..Default::default()
};
assert_eq!(minimum_balance(&order), Some(U256::from(2)));
}

#[test]
fn detects_orders_with_same_buy_and_sell_token() {
let native_token = dummy_contract!(WETH9, [0xef; 20]);
Expand Down Expand Up @@ -1895,60 +1871,6 @@ mod tests {
));
}

#[tokio::test]
async fn post_validate_err_sell_amount_overflow() {
let mut order_quoter = MockOrderQuoting::new();
let mut bad_token_detector = MockBadTokenDetecting::new();
let mut balance_fetcher = MockBalanceFetching::new();
order_quoter
.expect_find_quote()
.returning(|_, _| Ok(Default::default()));
order_quoter.expect_store_quote().returning(Ok);
bad_token_detector
.expect_detect()
.returning(|_| Ok(TokenQuality::Good));
balance_fetcher
.expect_can_transfer()
.returning(|_, _| Ok(()));
let mut limit_order_counter = MockLimitOrderCounting::new();
limit_order_counter.expect_count().returning(|_| Ok(0u64));
let validator = OrderValidator::new(
dummy_contract!(WETH9, [0xef; 20]),
Arc::new(order_validation::banned::Users::none()),
OrderValidPeriodConfiguration::any(),
false,
Arc::new(bad_token_detector),
dummy_contract!(HooksTrampoline, [0xcf; 20]),
Arc::new(order_quoter),
Arc::new(balance_fetcher),
Arc::new(MockSignatureValidating::new()),
Arc::new(limit_order_counter),
0,
Arc::new(MockCodeFetching::new()),
Default::default(),
None,
u64::MAX,
);
let order = OrderCreation {
valid_to: time::now_in_epoch_seconds() + 2,
sell_token: H160::from_low_u64_be(1),
buy_token: H160::from_low_u64_be(2),
buy_amount: U256::from(1),
sell_amount: U256::MAX,
fee_amount: U256::from(1),
signature: Signature::Eip712(EcdsaSignature::non_zero()),
app_data: OrderCreationAppData::Full {
full: "{}".to_string(),
},
..Default::default()
};
let result = validator
.validate_and_construct_order(order, &Default::default(), Default::default(), None)
.await;
dbg!(&result);
assert!(matches!(result, Err(ValidationError::SellAmountOverflow)));
}

#[tokio::test]
async fn post_validate_err_insufficient_balance() {
let mut order_quoter = MockOrderQuoting::new();
Expand Down

0 comments on commit 5950d92

Please sign in to comment.