Skip to content

Commit

Permalink
Always enable all signature types (#2307)
Browse files Browse the repository at this point in the history
# Description
Whenever we implement a new feature we make usually enable it by setting
a CLI flag.
However, at some point the flags are thoroughly tested and do not get
disabled anymore.
IMO there is no longer a need to temporarily disable eip-1271 or presign
signatures so I disabled their feature flags.

# Changes
Delete dead code

## How to test
Test should still pass
  • Loading branch information
MartinquaXD authored Jan 22, 2024
1 parent 22a2c04 commit 34fa87f
Show file tree
Hide file tree
Showing 6 changed files with 21 additions and 113 deletions.
2 changes: 0 additions & 2 deletions crates/e2e/src/setup/services.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,8 +113,6 @@ impl<'a> Services<'a> {
pub async fn start_api(&self, extra_args: Vec<String>) {
let args = [
"orderbook".to_string(),
"--enable-presign-orders=true".to_string(),
"--enable-eip1271-orders=true".to_string(),
"--enable-custom-interactions=true".to_string(),
"--allow-placing-partially-fillable-limit-orders=true".to_string(),
format!(
Expand Down
2 changes: 0 additions & 2 deletions crates/orderbook/openapi.yml
Original file line number Diff line number Diff line change
Expand Up @@ -1205,7 +1205,6 @@ components:
TransferEthToContract,
InvalidNativeSellToken,
SameBuyAndSellToken,
UnsupportedSignature,
UnsupportedToken,
UnsupportedCustomInteraction
InvalidAppData,
Expand Down Expand Up @@ -1264,7 +1263,6 @@ components:
UnsupportedBuyTokenDestination,
UnsupportedSellTokenSource,
UnsupportedOrderType,
UnsupportedSignature,
]
description:
type: string
Expand Down
4 changes: 0 additions & 4 deletions crates/orderbook/src/api/post_order.rs
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,6 @@ impl IntoWarpReply for PartialValidationErrorWrapper {
),
StatusCode::BAD_REQUEST,
),
PartialValidationError::UnsupportedSignature => with_status(
error("UnsupportedSignature", "signing scheme is not supported"),
StatusCode::BAD_REQUEST,
),
PartialValidationError::UnsupportedToken { token, reason } => with_status(
error(
"UnsupportedToken",
Expand Down
16 changes: 0 additions & 16 deletions crates/orderbook/src/arguments.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,22 +112,10 @@ pub struct Arguments {
#[clap(long, env, default_value = "200")]
pub pool_cache_lru_size: NonZeroUsize,

/// Enable EIP-1271 orders.
#[clap(long, env, action = clap::ArgAction::Set, default_value = "false")]
pub enable_eip1271_orders: bool,

/// Skip EIP-1271 order signature validation on creation.
#[clap(long, env, action = clap::ArgAction::Set, default_value = "false")]
pub eip1271_skip_creation_validation: bool,

/// Enable pre-sign orders. Pre-sign orders are accepted into the database
/// without a valid signature, so this flag allows this feature to be
/// turned off if malicious users are abusing the database by inserting
/// a bunch of order rows that won't ever be valid. This flag can be
/// removed once DDoS protection is implemented.
#[clap(long, env, action = clap::ArgAction::Set, default_value = "false")]
pub enable_presign_orders: bool,

/// If solvable orders haven't been successfully updated in this many blocks
/// attempting to get them errors and our liveness check fails.
#[clap(long, env, default_value = "24")]
Expand Down Expand Up @@ -191,9 +179,7 @@ impl std::fmt::Display for Arguments {
banned_users,
allowed_tokens,
pool_cache_lru_size,
enable_eip1271_orders,
eip1271_skip_creation_validation,
enable_presign_orders,
solvable_orders_max_update_age_blocks,
native_price_estimators,
fast_price_estimation_results_required,
Expand Down Expand Up @@ -242,13 +228,11 @@ impl std::fmt::Display for Arguments {
writeln!(f, "banned_users: {:?}", banned_users)?;
writeln!(f, "allowed_tokens: {:?}", allowed_tokens)?;
writeln!(f, "pool_cache_lru_size: {}", pool_cache_lru_size)?;
writeln!(f, "enable_eip1271_orders: {}", enable_eip1271_orders)?;
writeln!(
f,
"eip1271_skip_creation_validation: {}",
eip1271_skip_creation_validation
)?;
writeln!(f, "enable_presign_orders: {}", enable_presign_orders)?;
writeln!(
f,
"solvable_orders_max_update_age_blocks: {}",
Expand Down
9 changes: 2 additions & 7 deletions crates/orderbook/src/run.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ use {
metrics::{serve_metrics, DEFAULT_METRICS_PORT},
network::network_name,
order_quoting::{self, OrderQuoter},
order_validation::{OrderValidPeriodConfiguration, OrderValidator, SignatureConfiguration},
order_validation::{OrderValidPeriodConfiguration, OrderValidator},
price_estimation::{
factory::{self, PriceEstimatorFactory, PriceEstimatorSource},
native::NativePriceEstimating,
Expand Down Expand Up @@ -425,11 +425,6 @@ pub async fn run(args: Arguments) {
max_market: args.max_order_validity_period,
max_limit: args.max_limit_order_validity_period,
};
let signature_configuration = SignatureConfiguration {
eip1271: args.enable_eip1271_orders,
eip1271_skip_creation_validation: args.eip1271_skip_creation_validation,
presign: args.enable_presign_orders,
};

let create_quoter = |price_estimator: Arc<dyn PriceEstimating>| {
Arc::new(OrderQuoter::new(
Expand Down Expand Up @@ -468,7 +463,7 @@ pub async fn run(args: Arguments) {
.copied()
.collect(),
validity_configuration,
signature_configuration,
args.eip1271_skip_creation_validation,
bad_token_detector.clone(),
hooks_contract,
optimal_quoter.clone(),
Expand Down
101 changes: 19 additions & 82 deletions crates/shared/src/order_validation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,6 @@ pub enum PartialValidationError {
UnsupportedBuyTokenDestination(BuyTokenDestination),
UnsupportedSellTokenSource(SellTokenSource),
UnsupportedOrderType,
UnsupportedSignature,
UnsupportedToken { token: H160, reason: String },
Other(anyhow::Error),
}
Expand Down Expand Up @@ -243,7 +242,7 @@ pub struct OrderValidator {
banned_users: HashSet<H160>,
liquidity_order_owners: HashSet<H160>,
validity_configuration: OrderValidPeriodConfiguration,
signature_configuration: SignatureConfiguration,
eip1271_skip_creation_validation: bool,
bad_token_detector: Arc<dyn BadTokenDetecting>,
hooks: HooksTrampoline,
/// For Full-Validation: performed time of order placement
Expand Down Expand Up @@ -322,7 +321,7 @@ impl OrderValidator {
banned_users: HashSet<H160>,
liquidity_order_owners: HashSet<H160>,
validity_configuration: OrderValidPeriodConfiguration,
signature_configuration: SignatureConfiguration,
eip1271_skip_creation_validation: bool,
bad_token_detector: Arc<dyn BadTokenDetecting>,
hooks: HooksTrampoline,
quoter: Arc<dyn OrderQuoting>,
Expand All @@ -338,7 +337,7 @@ impl OrderValidator {
banned_users,
liquidity_order_owners,
validity_configuration,
signature_configuration,
eip1271_skip_creation_validation,
bad_token_detector,
hooks,
quoter,
Expand Down Expand Up @@ -476,14 +475,6 @@ impl OrderValidating for OrderValidator {

self.validity_configuration.validate_period(&order)?;

// Eventually we will support all Signature types and can remove this.
if !self
.signature_configuration
.is_signing_scheme_supported(order.signing_scheme)
{
return Err(PartialValidationError::UnsupportedSignature);
}

if has_same_buy_and_sell_token(&order, &self.native_token) {
return Err(PartialValidationError::SameBuyAndSellToken);
}
Expand Down Expand Up @@ -592,10 +583,7 @@ impl OrderValidating for OrderValidator {
let uid = data.uid(domain_separator, &owner);

let verification_gas_limit = if let Signature::Eip1271(signature) = &order.signature {
if self
.signature_configuration
.eip1271_skip_creation_validation
{
if self.eip1271_skip_creation_validation {
tracing::debug!(?signature, "skipping EIP-1271 signature validation");
// We don't care! Because we are skipping validation anyway
0u64
Expand Down Expand Up @@ -834,44 +822,6 @@ pub enum OrderValidToError {
Excessive,
}

/// Signature configuration that is accepted by the orderbook.
#[derive(Clone, Debug, Eq, PartialEq)]
pub struct SignatureConfiguration {
pub eip1271: bool,
pub eip1271_skip_creation_validation: bool,
pub presign: bool,
}

impl SignatureConfiguration {
/// Returns a configuration where only off-chain signing schemes are
/// supported.
pub fn off_chain() -> Self {
Self {
eip1271: false,
eip1271_skip_creation_validation: false,
presign: false,
}
}

/// Returns a configuration where all signing schemes are enabled.
pub fn all() -> Self {
Self {
eip1271: true,
eip1271_skip_creation_validation: false,
presign: true,
}
}

/// returns whether the supplied signature scheme is supported.
pub fn is_signing_scheme_supported(&self, signing_scheme: SigningScheme) -> bool {
match signing_scheme {
SigningScheme::Eip712 | SigningScheme::EthSign => true,
SigningScheme::Eip1271 => self.eip1271,
SigningScheme::PreSign => self.presign,
}
}
}

/// Returns true if the orders have same buy and sell tokens.
///
/// This also checks for orders selling wrapped native token for native token.
Expand Down Expand Up @@ -1109,7 +1059,7 @@ mod tests {
banned_users,
hashset!(),
validity_configuration,
SignatureConfiguration::off_chain(),
false,
Arc::new(MockBadTokenDetecting::new()),
dummy_contract!(HooksTrampoline, [0xcf; 20]),
Arc::new(MockOrderQuoting::new()),
Expand Down Expand Up @@ -1232,16 +1182,6 @@ mod tests {
.await,
Err(PartialValidationError::InvalidNativeSellToken)
));
assert!(matches!(
validator
.partial_validate(PreOrderData {
valid_to: legit_valid_to,
signing_scheme: SigningScheme::PreSign,
..Default::default()
})
.await,
Err(PartialValidationError::UnsupportedSignature)
));
}

#[tokio::test]
Expand Down Expand Up @@ -1270,7 +1210,7 @@ mod tests {
hashset!(),
hashset!(liquidity_order_owner),
validity_configuration,
SignatureConfiguration::all(),
false,
Arc::new(bad_token_detector),
dummy_contract!(HooksTrampoline, [0xcf; 20]),
Arc::new(MockOrderQuoting::new()),
Expand Down Expand Up @@ -1360,7 +1300,7 @@ mod tests {
max_market: Duration::from_secs(100),
max_limit: Duration::from_secs(200),
},
SignatureConfiguration::all(),
false,
Arc::new(bad_token_detector),
hooks.clone(),
Arc::new(order_quoter),
Expand Down Expand Up @@ -1479,10 +1419,7 @@ mod tests {
let validator = OrderValidator {
enable_custom_interactions: true,
signature_validator: Arc::new(signature_validator),
signature_configuration: SignatureConfiguration {
eip1271_skip_creation_validation: true,
..SignatureConfiguration::all()
},
eip1271_skip_creation_validation: true,
..validator
};

Expand Down Expand Up @@ -1556,7 +1493,7 @@ mod tests {
hashset!(),
hashset!(),
OrderValidPeriodConfiguration::any(),
SignatureConfiguration::all(),
false,
Arc::new(bad_token_detector),
dummy_contract!(HooksTrampoline, [0xcf; 20]),
Arc::new(order_quoter),
Expand Down Expand Up @@ -1613,7 +1550,7 @@ mod tests {
hashset!(),
hashset!(),
OrderValidPeriodConfiguration::any(),
SignatureConfiguration::all(),
false,
Arc::new(bad_token_detector),
dummy_contract!(HooksTrampoline, [0xcf; 20]),
Arc::new(order_quoter),
Expand Down Expand Up @@ -1664,7 +1601,7 @@ mod tests {
hashset!(),
hashset!(),
OrderValidPeriodConfiguration::any(),
SignatureConfiguration::all(),
false,
Arc::new(bad_token_detector),
dummy_contract!(HooksTrampoline, [0xcf; 20]),
Arc::new(order_quoter),
Expand Down Expand Up @@ -1727,7 +1664,7 @@ mod tests {
hashset!(),
hashset!(),
OrderValidPeriodConfiguration::any(),
SignatureConfiguration::all(),
false,
Arc::new(bad_token_detector),
dummy_contract!(HooksTrampoline, [0xcf; 20]),
Arc::new(order_quoter),
Expand Down Expand Up @@ -1781,7 +1718,7 @@ mod tests {
hashset!(),
hashset!(),
OrderValidPeriodConfiguration::any(),
SignatureConfiguration::all(),
false,
Arc::new(bad_token_detector),
dummy_contract!(HooksTrampoline, [0xcf; 20]),
Arc::new(order_quoter),
Expand Down Expand Up @@ -1830,7 +1767,7 @@ mod tests {
hashset!(),
hashset!(),
OrderValidPeriodConfiguration::any(),
SignatureConfiguration::all(),
false,
Arc::new(bad_token_detector),
dummy_contract!(HooksTrampoline, [0xcf; 20]),
Arc::new(order_quoter),
Expand Down Expand Up @@ -1881,7 +1818,7 @@ mod tests {
hashset!(),
hashset!(),
OrderValidPeriodConfiguration::any(),
SignatureConfiguration::all(),
false,
Arc::new(bad_token_detector),
dummy_contract!(HooksTrampoline, [0xcf; 20]),
Arc::new(order_quoter),
Expand Down Expand Up @@ -1936,7 +1873,7 @@ mod tests {
hashset!(),
hashset!(),
OrderValidPeriodConfiguration::any(),
SignatureConfiguration::all(),
false,
Arc::new(bad_token_detector),
dummy_contract!(HooksTrampoline, [0xcf; 20]),
Arc::new(order_quoter),
Expand Down Expand Up @@ -1985,7 +1922,7 @@ mod tests {
hashset!(),
hashset!(),
OrderValidPeriodConfiguration::any(),
SignatureConfiguration::all(),
false,
Arc::new(bad_token_detector),
dummy_contract!(HooksTrampoline, [0xcf; 20]),
Arc::new(order_quoter),
Expand Down Expand Up @@ -2038,7 +1975,7 @@ mod tests {
hashset!(),
hashset!(),
OrderValidPeriodConfiguration::any(),
SignatureConfiguration::all(),
false,
Arc::new(bad_token_detector),
dummy_contract!(HooksTrampoline, [0xcf; 20]),
Arc::new(order_quoter),
Expand Down Expand Up @@ -2098,7 +2035,7 @@ mod tests {
hashset!(),
hashset!(),
OrderValidPeriodConfiguration::any(),
SignatureConfiguration::all(),
false,
Arc::new(bad_token_detector),
dummy_contract!(HooksTrampoline, [0xcf; 20]),
Arc::new(order_quoter),
Expand Down

0 comments on commit 34fa87f

Please sign in to comment.