Skip to content

Commit

Permalink
Change quote request construction for fee and quote (#234)
Browse files Browse the repository at this point in the history
We recently added a regression to the "legacy" `api/v1/feeAndQuote` API endpoint.

Under the hood, this request uses the same logic as for `api/v1/quote` which recently introduced a cap on the `valid_to` field. This broke the aforementioned fee and quote route which was internally converting the request into a quote with `valid_to` set to `u32::MAX`.

This PR fixes this by changing the order quote construction to use a "sane" `valid_to` value: the default validity that is used in the CowSwap UI - which should be super unlikely to ever become "invalid" and break the legacy API.

### Test Plan

`api/v1/feeAndQuote` should start working again!
```
% curl -s 'http://localhost:8080/api/v1/feeAndQuote/sell?sellToken=0xc778417E063141139Fce010982780140Aa0cD5Ab&buyToken=0xa7D1C04fAF998F9161fC9F800a99A809b84cfc9D&sellAmountBeforeFee=100000000000000000' | jq
{
  "fee": {
    "amount": "220417002424587",
    "expirationDate": "2022-05-30T15:16:45.916306Z"
  },
  "buyAmountAfterFee": "110754541550422939367"
}
```
  • Loading branch information
Nicholas Rodrigues Lordello authored May 31, 2022
1 parent 77d267f commit 519f957
Show file tree
Hide file tree
Showing 12 changed files with 26 additions and 23 deletions.
4 changes: 2 additions & 2 deletions crates/e2e/tests/e2e/eth_integration.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ async fn eth_integration(web3: Web3) {
.with_fee_amount(to_wei(1))
.with_buy_token(BUY_ETH_ADDRESS)
.with_buy_amount(to_wei(49))
.with_valid_to(shared::time::now_in_epoch_seconds() + 300)
.with_valid_to(model::time::now_in_epoch_seconds() + 300)
.sign_with(
EcdsaSigningScheme::Eip712,
&contracts.domain_separator,
Expand All @@ -169,7 +169,7 @@ async fn eth_integration(web3: Web3) {
.with_fee_amount(to_wei(1))
.with_buy_token(BUY_ETH_ADDRESS)
.with_buy_amount(to_wei(49))
.with_valid_to(shared::time::now_in_epoch_seconds() + 300)
.with_valid_to(model::time::now_in_epoch_seconds() + 300)
.sign_with(
EcdsaSigningScheme::Eip712,
&contracts.domain_separator,
Expand Down
4 changes: 2 additions & 2 deletions crates/e2e/tests/e2e/onchain_settlement.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ async fn onchain_settlement(web3: Web3) {
.with_fee_amount(to_wei(1))
.with_buy_token(token_b.address())
.with_buy_amount(to_wei(80))
.with_valid_to(shared::time::now_in_epoch_seconds() + 300)
.with_valid_to(model::time::now_in_epoch_seconds() + 300)
.with_kind(OrderKind::Sell)
.sign_with(
EcdsaSigningScheme::Eip712,
Expand All @@ -174,7 +174,7 @@ async fn onchain_settlement(web3: Web3) {
.with_fee_amount(to_wei(1))
.with_buy_token(token_a.address())
.with_buy_amount(to_wei(40))
.with_valid_to(shared::time::now_in_epoch_seconds() + 300)
.with_valid_to(model::time::now_in_epoch_seconds() + 300)
.with_kind(OrderKind::Sell)
.sign_with(
EcdsaSigningScheme::EthSign,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -154,7 +154,7 @@ async fn onchain_settlement_without_liquidity(web3: Web3) {
.with_sell_amount(to_wei(100))
.with_buy_token(token_b.address())
.with_buy_amount(to_wei(90))
.with_valid_to(shared::time::now_in_epoch_seconds() + 300)
.with_valid_to(model::time::now_in_epoch_seconds() + 300)
.with_kind(OrderKind::Sell)
.sign_with(
EcdsaSigningScheme::Eip712,
Expand Down
2 changes: 1 addition & 1 deletion crates/e2e/tests/e2e/smart_contract_orders.rs
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,7 @@ async fn smart_contract_orders(web3: Web3) {
.with_fee_amount(to_wei(1))
.with_buy_token(contracts.weth.address())
.with_buy_amount(to_wei(8))
.with_valid_to(shared::time::now_in_epoch_seconds() + 300)
.with_valid_to(model::time::now_in_epoch_seconds() + 300)
.with_presign(trader.address())
.build()
.creation;
Expand Down
2 changes: 1 addition & 1 deletion crates/e2e/tests/e2e/vault_balances.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,7 +113,7 @@ async fn vault_balances(web3: Web3) {
.with_fee_amount(to_wei(1))
.with_buy_token(contracts.weth.address())
.with_buy_amount(to_wei(8))
.with_valid_to(shared::time::now_in_epoch_seconds() + 300)
.with_valid_to(model::time::now_in_epoch_seconds() + 300)
.sign_with(
EcdsaSigningScheme::Eip712,
&contracts.domain_separator,
Expand Down
1 change: 1 addition & 0 deletions crates/model/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ pub mod quote;
pub mod ratio_as_decimal;
pub mod signature;
pub mod solver_competition;
pub mod time;
pub mod trade;
pub mod u256_decimal;

Expand Down
7 changes: 5 additions & 2 deletions crates/model/src/quote.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@ use crate::{
app_id::AppId,
order::{BuyTokenDestination, OrderKind, SellTokenSource},
signature::SigningScheme,
u256_decimal,
time, u256_decimal,
};
use chrono::{DateTime, Utc};
use primitive_types::{H160, U256};
Expand Down Expand Up @@ -119,7 +119,10 @@ impl OrderQuoteRequest {
sell_token,
buy_token,
side,
valid_to: u32::MAX,
// Use the default validity from the CowSwap UI of 20 minutes. This
// prevents any weird issues that we may encounter in some of our
// legacy routes if we futher restrict "good" values.
valid_to: time::now_in_epoch_seconds() + 20 * 60,
..Default::default()
}
}
Expand Down
File renamed without changes.
2 changes: 1 addition & 1 deletion crates/orderbook/src/api/get_orders.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,9 @@ use crate::{
{database::orders::OrderFilter, orderbook::Orderbook},
};
use anyhow::{Context, Result};
use model::time::now_in_epoch_seconds;
use primitive_types::H160;
use serde::Deserialize;
use shared::time::now_in_epoch_seconds;
use std::{convert::Infallible, sync::Arc};
use warp::{hyper::StatusCode, Filter, Rejection};

Expand Down
20 changes: 10 additions & 10 deletions crates/orderbook/src/api/order_validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -322,7 +322,7 @@ impl OrderValidating for OrderValidator {
));
}

let now = shared::time::now_in_epoch_seconds();
let now = model::time::now_in_epoch_seconds();
if order.valid_to < now + self.min_order_validity_period.as_secs() as u32 {
return Err(PartialValidationError::InsufficientValidTo);
}
Expand Down Expand Up @@ -584,7 +584,7 @@ mod tests {
let max_order_validity_period = Duration::from_secs(100);
let banned_users = hashset![H160::from_low_u64_be(1)];
let legit_valid_to =
shared::time::now_in_epoch_seconds() + min_order_validity_period.as_secs() as u32 + 2;
model::time::now_in_epoch_seconds() + min_order_validity_period.as_secs() as u32 + 2;
code_fetcher
.expect_code_size()
.times(1)
Expand Down Expand Up @@ -737,7 +737,7 @@ mod tests {
Arc::new(MockBalanceFetching::new()),
);
let order = || PreOrderData {
valid_to: shared::time::now_in_epoch_seconds()
valid_to: model::time::now_in_epoch_seconds()
+ min_order_validity_period.as_secs() as u32
+ 2,
sell_token: H160::from_low_u64_be(1),
Expand Down Expand Up @@ -792,7 +792,7 @@ mod tests {
Arc::new(balance_fetcher),
);
let order = OrderCreation {
valid_to: shared::time::now_in_epoch_seconds() + 2,
valid_to: model::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),
Expand Down Expand Up @@ -832,7 +832,7 @@ mod tests {
Arc::new(balance_fetcher),
);
let order = OrderCreation {
valid_to: shared::time::now_in_epoch_seconds() + 2,
valid_to: model::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(0),
Expand Down Expand Up @@ -872,7 +872,7 @@ mod tests {
Arc::new(balance_fetcher),
);
let order = OrderCreation {
valid_to: shared::time::now_in_epoch_seconds() + 2,
valid_to: model::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),
Expand Down Expand Up @@ -916,7 +916,7 @@ mod tests {
Arc::new(balance_fetcher),
);
let order = OrderCreation {
valid_to: shared::time::now_in_epoch_seconds() + 2,
valid_to: model::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),
Expand Down Expand Up @@ -958,7 +958,7 @@ mod tests {
Arc::new(balance_fetcher),
);
let order = OrderCreation {
valid_to: shared::time::now_in_epoch_seconds() + 2,
valid_to: model::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),
Expand Down Expand Up @@ -998,7 +998,7 @@ mod tests {
Arc::new(balance_fetcher),
);
let order = OrderCreation {
valid_to: shared::time::now_in_epoch_seconds() + 2,
valid_to: model::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),
Expand Down Expand Up @@ -1039,7 +1039,7 @@ mod tests {
Arc::new(balance_fetcher),
);
let order = OrderCreation {
valid_to: shared::time::now_in_epoch_seconds() + 2,
valid_to: model::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),
Expand Down
4 changes: 2 additions & 2 deletions crates/orderbook/src/solvable_orders.rs
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@ use crate::{
};
use anyhow::{Context as _, Result};
use futures::StreamExt;
use model::{auction::Auction, order::Order};
use model::{auction::Auction, order::Order, time::now_in_epoch_seconds};
use primitive_types::{H160, U256};
use shared::{
bad_token::BadTokenDetecting, current_block::CurrentBlockStream, maintenance::Maintaining,
price_estimation::native::NativePriceEstimating, time::now_in_epoch_seconds,
price_estimation::native::NativePriceEstimating,
};
use std::{
collections::{BTreeMap, HashMap, HashSet},
Expand Down
1 change: 0 additions & 1 deletion crates/shared/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ pub mod request_sharing;
pub mod solver_utils;
pub mod sources;
pub mod subgraph;
pub mod time;
pub mod token_info;
pub mod token_list;
pub mod trace_many;
Expand Down

0 comments on commit 519f957

Please sign in to comment.