From 72fece0be8255ccf5020e6d5b5586a68ce9ef73d Mon Sep 17 00:00:00 2001 From: Chris Czub Date: Fri, 8 Sep 2023 16:57:01 -0400 Subject: [PATCH] Remove usage of "unwrap" in the dex component --- .../dex/src/batch_swap_output_data.rs | 54 +++++++++++++---- .../core/component/dex/src/component/dex.rs | 16 ++--- .../dex/src/component/router/fill_route.rs | 28 ++++++--- .../dex/src/component/router/params.rs | 14 ++--- .../dex/src/component/router/path.rs | 2 +- crates/core/component/dex/src/lib.rs | 1 + .../component/dex/src/lp/trading_function.rs | 7 ++- crates/core/component/dex/src/swap/proof.rs | 30 ++++++++-- .../component/dex/src/swap_claim/proof.rs | 60 +++++++++++++++---- crates/core/component/dex/src/trading_pair.rs | 12 +++- 10 files changed, 168 insertions(+), 56 deletions(-) diff --git a/crates/core/component/dex/src/batch_swap_output_data.rs b/crates/core/component/dex/src/batch_swap_output_data.rs index c6e7b440ee..21af60a66c 100644 --- a/crates/core/component/dex/src/batch_swap_output_data.rs +++ b/crates/core/component/dex/src/batch_swap_output_data.rs @@ -88,18 +88,50 @@ impl ToConstraintField for BatchSwapOutputData { fn to_field_elements(&self) -> Option> { let mut public_inputs = Vec::new(); let delta_1 = U128x128::from(self.delta_1); - public_inputs.extend(delta_1.to_field_elements().unwrap()); - public_inputs.extend(U128x128::from(self.delta_2).to_field_elements().unwrap()); - public_inputs.extend(U128x128::from(self.lambda_1).to_field_elements().unwrap()); - public_inputs.extend(U128x128::from(self.lambda_2).to_field_elements().unwrap()); - public_inputs.extend(U128x128::from(self.unfilled_1).to_field_elements().unwrap()); - public_inputs.extend(U128x128::from(self.unfilled_2).to_field_elements().unwrap()); - public_inputs.extend(Fq::from(self.height).to_field_elements().unwrap()); - public_inputs.extend(self.trading_pair.to_field_elements().unwrap()); + public_inputs.extend( + delta_1 + .to_field_elements() + .expect("delta_1 is a Bls12-377 field member"), + ); + public_inputs.extend( + U128x128::from(self.delta_2) + .to_field_elements() + .expect("U128x128 types are Bls12-377 field members"), + ); + public_inputs.extend( + U128x128::from(self.lambda_1) + .to_field_elements() + .expect("U128x128 types are Bls12-377 field members"), + ); + public_inputs.extend( + U128x128::from(self.lambda_2) + .to_field_elements() + .expect("U128x128 types are Bls12-377 field members"), + ); + public_inputs.extend( + U128x128::from(self.unfilled_1) + .to_field_elements() + .expect("U128x128 types are Bls12-377 field members"), + ); + public_inputs.extend( + U128x128::from(self.unfilled_2) + .to_field_elements() + .expect("U128x128 types are Bls12-377 field members"), + ); + public_inputs.extend( + Fq::from(self.height) + .to_field_elements() + .expect("Fq types are Bls12-377 field members"), + ); + public_inputs.extend( + self.trading_pair + .to_field_elements() + .expect("trading_pair is a Bls12-377 field member"), + ); public_inputs.extend( Fq::from(self.epoch_starting_height) .to_field_elements() - .unwrap(), + .expect("Fq types are Bls12-377 field members"), ); Some(public_inputs) } @@ -376,11 +408,11 @@ mod tests { let trading_pair = TradingPair { asset_1: asset::Cache::with_known_assets() .get_unit("upenumbra") - .unwrap() + .expect("upenumbra denom should always be known by the asset registry") .id(), asset_2: asset::Cache::with_known_assets() .get_unit("nala") - .unwrap() + .expect("nala denom should always be known by the asset registry") .id(), }; Self { diff --git a/crates/core/component/dex/src/component/dex.rs b/crates/core/component/dex/src/component/dex.rs index 43294e825e..6d79cf8554 100644 --- a/crates/core/component/dex/src/component/dex.rs +++ b/crates/core/component/dex/src/component/dex.rs @@ -41,7 +41,7 @@ impl Component for Dex { state: &mut Arc, end_block: &abci::request::EndBlock, ) { - let current_epoch = state.epoch().await.unwrap(); + let current_epoch = state.epoch().await.expect("epoch is set"); // For each batch swap during the block, calculate clearing prices and set in the JMT. for (trading_pair, swap_flows) in state.swap_flows() { @@ -77,27 +77,27 @@ impl Component for Dex { *STAKING_TOKEN_ASSET_ID, asset::Cache::with_known_assets() .get_unit("gm") - .unwrap() + .expect("gm is a known asset") .id(), asset::Cache::with_known_assets() .get_unit("gn") - .unwrap() + .expect("gn is a known asset") .id(), asset::Cache::with_known_assets() .get_unit("test_usd") - .unwrap() + .expect("test_usd is a known asset") .id(), asset::Cache::with_known_assets() .get_unit("test_btc") - .unwrap() + .expect("test_btc is a known asset") .id(), asset::Cache::with_known_assets() .get_unit("test_atom") - .unwrap() + .expect("test_atom is a known asset") .id(), asset::Cache::with_known_assets() .get_unit("test_osmo") - .unwrap() + .expect("test_osmo is a known asset") .id(), ], ) @@ -108,7 +108,7 @@ impl Component for Dex { // TODO: hack to avoid needing an asset cache for nice debug output let unit = asset::Cache::with_known_assets() .get_unit("penumbra") - .unwrap(); + .expect("penumbra is a known asset"); let burn = format!("{}{}", unit.format_value(arb_burn.amount), unit); tracing::info!(%burn, "executed arbitrage opportunity"); } diff --git a/crates/core/component/dex/src/component/router/fill_route.rs b/crates/core/component/dex/src/component/router/fill_route.rs index 3e1337dd10..259becd78f 100644 --- a/crates/core/component/dex/src/component/router/fill_route.rs +++ b/crates/core/component/dex/src/component/router/fill_route.rs @@ -320,13 +320,13 @@ impl FrontierTx { let input: U128x128 = self .trace .first() - .unwrap() + .expect("input amount is set in a complete trace") .expect("input amount is set in a complete trace") .into(); let output: U128x128 = self .trace .last() - .unwrap() + .expect("output amount is set in a complete trace") .expect("output amount is set in a complete trace") .into(); @@ -364,7 +364,7 @@ impl Frontier { 'next_position: loop { let id = positions_by_price .get_mut(pair) - .unwrap() + .expect("positions_by_price should have an entry for each pair") .as_mut() .next() .await @@ -435,8 +435,16 @@ impl Frontier { self.trace.push(trace); ( - changes.trace.first().unwrap().unwrap(), - changes.trace.last().unwrap().unwrap(), + changes + .trace + .first() + .expect("first should be set for a trace") + .expect("input amount should be set for a trace"), + changes + .trace + .last() + .expect("last should be set for a trace") + .expect("output amount should be set for a trace"), ) } @@ -481,7 +489,7 @@ impl Frontier { let next_position_id = match self .positions_by_price .get_mut(pair) - .unwrap() + .expect("positions_by_price should have an entry for each pair") .as_mut() .next() .await @@ -585,7 +593,13 @@ impl Frontier { #[instrument(skip(self, input), fields(input = ?input.amount))] fn fill_unconstrained(&self, input: Value) -> FrontierTx { - assert_eq!(input.asset_id, self.pairs.first().unwrap().start); + assert_eq!( + input.asset_id, + self.pairs + .first() + .expect("first should be set for a trace") + .start + ); let mut tx = FrontierTx::new(&self); // We have to manually update the trace here, because fill_forward diff --git a/crates/core/component/dex/src/component/router/params.rs b/crates/core/component/dex/src/component/router/params.rs index 2aa81eb3e4..c9d02e89a0 100644 --- a/crates/core/component/dex/src/component/router/params.rs +++ b/crates/core/component/dex/src/component/router/params.rs @@ -17,31 +17,31 @@ impl Default for RoutingParams { fixed_candidates: Arc::new(vec![ asset::Cache::with_known_assets() .get_unit("test_usd") - .unwrap() + .expect("hardcoded \"test_usd\" denom should be known") .id(), asset::Cache::with_known_assets() .get_unit("penumbra") - .unwrap() + .expect("hardcoded \"penumbra\" denom should be known") .id(), asset::Cache::with_known_assets() .get_unit("gm") - .unwrap() + .expect("hardcoded \"gm\" denom should be known") .id(), asset::Cache::with_known_assets() .get_unit("gn") - .unwrap() + .expect("hardcoded \"gn\" denom should be known") .id(), asset::Cache::with_known_assets() .get_unit("test_atom") - .unwrap() + .expect("hardcoded \"test_atom\" denom should be known") .id(), asset::Cache::with_known_assets() .get_unit("test_osmo") - .unwrap() + .expect("hardcoded \"test_osmo\" denom should be known") .id(), asset::Cache::with_known_assets() .get_unit("test_btc") - .unwrap() + .expect("hardcoded \"test_btc\" denom should be known") .id(), ]), max_hops: 4, diff --git a/crates/core/component/dex/src/component/router/path.rs b/crates/core/component/dex/src/component/router/path.rs index 39c18224b6..a699b750d7 100644 --- a/crates/core/component/dex/src/component/router/path.rs +++ b/crates/core/component/dex/src/component/router/path.rs @@ -84,7 +84,7 @@ impl Path { let hop_price = best_price_position .phi .orient_end(new_end) - .unwrap() + .expect("position should be contain the end asset") .effective_price(); match self.price * hop_price { diff --git a/crates/core/component/dex/src/lib.rs b/crates/core/component/dex/src/lib.rs index 707604c0c9..6abe9b3d4e 100644 --- a/crates/core/component/dex/src/lib.rs +++ b/crates/core/component/dex/src/lib.rs @@ -1,3 +1,4 @@ +#![deny(clippy::unwrap_used)] #![cfg_attr(docsrs, feature(doc_cfg))] #[cfg_attr(docsrs, doc(cfg(feature = "component")))] diff --git a/crates/core/component/dex/src/lp/trading_function.rs b/crates/core/component/dex/src/lp/trading_function.rs index 5a3c914be9..2584c139a7 100644 --- a/crates/core/component/dex/src/lp/trading_function.rs +++ b/crates/core/component/dex/src/lp/trading_function.rs @@ -329,7 +329,10 @@ impl BareTradingFunction { // Observe that for the case when `tentative_lambda_2` equals // `reserves.r1`, rounding it down does not change anything since // `reserves.r1` is integral. Therefore `reserves.r1 - lambda_2 >= 0`. - let lambda_2: Amount = tentative_lambda_2.round_down().try_into().unwrap(); + let lambda_2: Amount = tentative_lambda_2 + .round_down() + .try_into() + .expect("lambda_2 fits in an Amount"); let new_reserves = Reserves { r1: reserves.r1 + delta_1, r2: reserves.r2 - lambda_2, @@ -362,7 +365,7 @@ impl BareTradingFunction { .round_up() .expect("no overflow") .try_into() - .unwrap(); + .expect("fillable_delta_1 fits in an Amount"); // How to show that: `unfilled_amount >= 0`: // In this branch, we have: diff --git a/crates/core/component/dex/src/swap/proof.rs b/crates/core/component/dex/src/swap/proof.rs index 52598bc727..bfebcc65ed 100644 --- a/crates/core/component/dex/src/swap/proof.rs +++ b/crates/core/component/dex/src/swap/proof.rs @@ -1,3 +1,4 @@ +use anyhow::Context; use ark_ff::ToConstraintField; use ark_groth16::{ r1cs_to_qap::LibsnarkReduction, Groth16, PreparedVerifyingKey, Proof, ProvingKey, @@ -105,8 +106,10 @@ impl DummyWitness for SwapCircuit { fn with_dummy_witness() -> Self { let a = asset::Cache::with_known_assets() .get_unit("upenumbra") - .unwrap(); - let b = asset::Cache::with_known_assets().get_unit("nala").unwrap(); + .expect("upenumbra asset exists"); + let b = asset::Cache::with_known_assets() + .get_unit("nala") + .expect("nala asset exists"); let trading_pair = TradingPair::new(a.id(), b.id()); let diversifier_bytes = [1u8; 16]; let pk_d_bytes = decaf377::basepoint().vartime_compress().0; @@ -126,7 +129,7 @@ impl DummyWitness for SwapCircuit { amount: 3u64.into(), asset_id: asset::Cache::with_known_assets() .get_unit("upenumbra") - .unwrap() + .expect("upenumbra asset exists") .id(), }), claim_address: address, @@ -196,9 +199,24 @@ impl SwapProof { Proof::deserialize_compressed_unchecked(&self.0[..]).map_err(|e| anyhow::anyhow!(e))?; let mut public_inputs = Vec::new(); - public_inputs.extend(balance_commitment.0.to_field_elements().unwrap()); - public_inputs.extend(swap_commitment.0.to_field_elements().unwrap()); - public_inputs.extend(fee_commitment.0.to_field_elements().unwrap()); + public_inputs.extend( + balance_commitment + .0 + .to_field_elements() + .context("balance_commitment should be a Bls12-377 field member")?, + ); + public_inputs.extend( + swap_commitment + .0 + .to_field_elements() + .context("swap_commitment should be a Bls12-377 field member")?, + ); + public_inputs.extend( + fee_commitment + .0 + .to_field_elements() + .context("fee_commitment should be a Bls12-377 field member")?, + ); tracing::trace!(?public_inputs); let start = std::time::Instant::now(); diff --git a/crates/core/component/dex/src/swap_claim/proof.rs b/crates/core/component/dex/src/swap_claim/proof.rs index 12cd673d60..07130f3680 100644 --- a/crates/core/component/dex/src/swap_claim/proof.rs +++ b/crates/core/component/dex/src/swap_claim/proof.rs @@ -208,11 +208,11 @@ impl DummyWitness for SwapClaimCircuit { let trading_pair = TradingPair { asset_1: asset::Cache::with_known_assets() .get_unit("upenumbra") - .unwrap() + .expect("upenumbra denom is known") .id(), asset_2: asset::Cache::with_known_assets() .get_unit("nala") - .unwrap() + .expect("nala denom is known") .id(), }; @@ -233,7 +233,7 @@ impl DummyWitness for SwapClaimCircuit { amount: 3u64.into(), asset_id: asset::Cache::with_known_assets() .get_unit("upenumbra") - .unwrap() + .expect("upenumbra denom is known") .id(), }), claim_address: address, @@ -241,9 +241,12 @@ impl DummyWitness for SwapClaimCircuit { }; let mut sct = tct::Tree::new(); let swap_commitment = swap_plaintext.swap_commitment(); - sct.insert(tct::Witness::Keep, swap_commitment).unwrap(); + sct.insert(tct::Witness::Keep, swap_commitment) + .expect("insertion of the swap commitment into the SCT should succeed"); let anchor = sct.root(); - let state_commitment_proof = sct.witness(swap_commitment).unwrap(); + let state_commitment_proof = sct + .witness(swap_commitment) + .expect("the SCT should be able to witness the just-inserted swap commitment"); let nullifier = Nullifier(Fq::from(1)); let claim_fee = Fee::default(); let output_data = BatchSwapOutputData { @@ -348,13 +351,46 @@ impl SwapClaimProof { Proof::deserialize_compressed_unchecked(&self.0[..]).map_err(|e| anyhow::anyhow!(e))?; let mut public_inputs = Vec::new(); - public_inputs.extend(Fq::from(anchor.0).to_field_elements().unwrap()); - public_inputs.extend(nullifier.0.to_field_elements().unwrap()); - public_inputs.extend(Fq::from(fee.0.amount).to_field_elements().unwrap()); - public_inputs.extend(fee.0.asset_id.0.to_field_elements().unwrap()); - public_inputs.extend(output_data.to_field_elements().unwrap()); - public_inputs.extend(note_commitment_1.0.to_field_elements().unwrap()); - public_inputs.extend(note_commitment_2.0.to_field_elements().unwrap()); + public_inputs.extend( + Fq::from(anchor.0) + .to_field_elements() + .expect("Fq types are Bls12-377 field members"), + ); + public_inputs.extend( + nullifier + .0 + .to_field_elements() + .expect("nullifier is a Bls12-377 field member"), + ); + public_inputs.extend( + Fq::from(fee.0.amount) + .to_field_elements() + .expect("Fq types are Bls12-377 field members"), + ); + public_inputs.extend( + fee.0 + .asset_id + .0 + .to_field_elements() + .expect("asset_id is a Bls12-377 field member"), + ); + public_inputs.extend( + output_data + .to_field_elements() + .expect("output_data is a Bls12-377 field member"), + ); + public_inputs.extend( + note_commitment_1 + .0 + .to_field_elements() + .expect("note_commitment_1 is a Bls12-377 field member"), + ); + public_inputs.extend( + note_commitment_2 + .0 + .to_field_elements() + .expect("note_commitment_2 is a Bls12-377 field member"), + ); tracing::trace!(?public_inputs); let start = std::time::Instant::now(); diff --git a/crates/core/component/dex/src/trading_pair.rs b/crates/core/component/dex/src/trading_pair.rs index 09a85dc4f2..132d925477 100644 --- a/crates/core/component/dex/src/trading_pair.rs +++ b/crates/core/component/dex/src/trading_pair.rs @@ -166,8 +166,16 @@ impl TradingPairVar { impl ToConstraintField for TradingPair { fn to_field_elements(&self) -> Option> { let mut public_inputs = Vec::new(); - public_inputs.extend(Fq::from(self.asset_1().0).to_field_elements().unwrap()); - public_inputs.extend(Fq::from(self.asset_2().0).to_field_elements().unwrap()); + public_inputs.extend( + Fq::from(self.asset_1().0) + .to_field_elements() + .expect("Fq types are Bls12-377 field members"), + ); + public_inputs.extend( + Fq::from(self.asset_2().0) + .to_field_elements() + .expect("Fq types are Bls12-377 field members"), + ); Some(public_inputs) } }