From 2a778f5644b9613ea40d7995c490c21ca7f2d4b6 Mon Sep 17 00:00:00 2001 From: Franck Royer Date: Wed, 23 Dec 2020 15:06:43 +1100 Subject: [PATCH] Remove overzealous clippy overrides We have overridden a number of clippy warnings such as "large enum variant". Considering that we have a number of issues with the stack size in CI, it is more prudent to follow clippy's advice and box larger items so that the enum does not take larger space. Do note that an instance of the enum always takes as much space as its largest variant. --- swap/src/alice.rs | 13 +++-- swap/src/alice/event_loop.rs | 4 +- swap/src/alice/message0.rs | 4 +- swap/src/alice/message1.rs | 2 +- swap/src/alice/swap.rs | 79 +++++++++++++++------------- swap/src/bob.rs | 9 ++-- swap/src/bob/event_loop.rs | 4 +- swap/src/bob/message0.rs | 4 +- swap/src/bob/message1.rs | 2 +- swap/src/network/request_response.rs | 8 ++- swap/src/state.rs | 5 +- xmr-btc/src/alice.rs | 9 ++-- xmr-btc/src/bitcoin/transactions.rs | 2 + xmr-btc/src/bob.rs | 1 - xmr-btc/src/lib.rs | 28 ++++++++++ 15 files changed, 103 insertions(+), 71 deletions(-) diff --git a/swap/src/alice.rs b/swap/src/alice.rs index 05d0ba81..903d22da 100644 --- a/swap/src/alice.rs +++ b/swap/src/alice.rs @@ -53,15 +53,15 @@ pub fn new_swarm( Ok(swarm) } -#[allow(clippy::large_enum_variant)] #[derive(Debug)] pub enum OutEvent { ConnectionEstablished(PeerId), // TODO (Franck): Change this to get both amounts so parties can verify the amounts are // expected early on. - Request(amounts::OutEvent), // Not-uniform with Bob on purpose, ready for adding Xmr event. + Request(Box), /* Not-uniform with Bob on purpose, ready for adding Xmr + * event. */ Message0 { - msg: bob::Message0, + msg: Box, channel: ResponseChannel, }, Message1 { @@ -87,14 +87,17 @@ impl From for OutEvent { impl From for OutEvent { fn from(event: amounts::OutEvent) -> Self { - OutEvent::Request(event) + OutEvent::Request(Box::new(event)) } } impl From for OutEvent { fn from(event: message0::OutEvent) -> Self { match event { - message0::OutEvent::Msg { channel, msg } => OutEvent::Message0 { msg, channel }, + message0::OutEvent::Msg { channel, msg } => OutEvent::Message0 { + msg: Box::new(msg), + channel, + }, } } } diff --git a/swap/src/alice/event_loop.rs b/swap/src/alice/event_loop.rs index 52a056c1..f650932d 100644 --- a/swap/src/alice/event_loop.rs +++ b/swap/src/alice/event_loop.rs @@ -206,7 +206,7 @@ impl EventLoop { let _ = self.conn_established.send(alice).await; } OutEvent::Message0 { msg, channel } => { - let _ = self.msg0.send((msg, channel)).await; + let _ = self.msg0.send((*msg, channel)).await; } OutEvent::Message1 { msg, channel } => { let _ = self.msg1.send((msg, channel)).await; @@ -218,7 +218,7 @@ impl EventLoop { let _ = self.msg3.send(msg).await; } OutEvent::Request(event) => { - let _ = self.request.send(event).await; + let _ = self.request.send(*event).await; } } }, diff --git a/swap/src/alice/message0.rs b/swap/src/alice/message0.rs index 2fb8753b..92ca7a43 100644 --- a/swap/src/alice/message0.rs +++ b/swap/src/alice/message0.rs @@ -37,7 +37,7 @@ pub struct Message0 { impl Message0 { pub fn send(&mut self, channel: ResponseChannel, msg: xmr_btc::alice::Message0) { - let msg = AliceToBob::Message0(msg); + let msg = AliceToBob::Message0(Box::new(msg)); self.rr.send_response(channel, msg); } fn poll( @@ -82,7 +82,7 @@ impl NetworkBehaviourEventProcess> } => { if let BobToAlice::Message0(msg) = request { debug!("Received Message0"); - self.events.push_back(OutEvent::Msg { msg, channel }); + self.events.push_back(OutEvent::Msg { msg: *msg, channel }); } } RequestResponseEvent::Message { diff --git a/swap/src/alice/message1.rs b/swap/src/alice/message1.rs index 4d44b0a2..cf6d2bd2 100644 --- a/swap/src/alice/message1.rs +++ b/swap/src/alice/message1.rs @@ -38,7 +38,7 @@ pub struct Message1 { impl Message1 { pub fn send(&mut self, channel: ResponseChannel, msg: xmr_btc::alice::Message1) { - let msg = AliceToBob::Message1(msg); + let msg = AliceToBob::Message1(Box::new(msg)); self.rr.send_response(channel, msg); } diff --git a/swap/src/alice/swap.rs b/swap/src/alice/swap.rs index 64114511..0e114ed3 100644 --- a/swap/src/alice/swap.rs +++ b/swap/src/alice/swap.rs @@ -42,7 +42,6 @@ trait Rng: RngCore + CryptoRng + Send {} impl Rng for T where T: RngCore + CryptoRng + Send {} #[derive(Debug)] -#[allow(clippy::large_enum_variant)] pub enum AliceState { Started { amounts: SwapAmounts, @@ -51,36 +50,36 @@ pub enum AliceState { Negotiated { channel: Option>, amounts: SwapAmounts, - state3: State3, + state3: Box, }, BtcLocked { channel: Option>, amounts: SwapAmounts, - state3: State3, + state3: Box, }, XmrLocked { - state3: State3, + state3: Box, }, EncSigLearned { - state3: State3, encrypted_signature: EncryptedSignature, + state3: Box, }, BtcRedeemed, BtcCancelled { - state3: State3, tx_cancel: TxCancel, + state3: Box, }, BtcRefunded { spend_key: monero::PrivateKey, - state3: State3, + state3: Box, }, BtcPunishable { tx_refund: TxRefund, - state3: State3, + state3: Box, }, XmrRefunded, CancelTimelockExpired { - state3: State3, + state3: Box, }, BtcPunished, SafelyAborted, @@ -109,26 +108,28 @@ impl fmt::Display for AliceState { impl From<&AliceState> for state::Alice { fn from(alice_state: &AliceState) -> Self { match alice_state { - AliceState::Negotiated { state3, .. } => Alice::Negotiated(state3.clone()), - AliceState::BtcLocked { state3, .. } => Alice::BtcLocked(state3.clone()), - AliceState::XmrLocked { state3 } => Alice::XmrLocked(state3.clone()), + AliceState::Negotiated { state3, .. } => Alice::Negotiated(state3.as_ref().clone()), + AliceState::BtcLocked { state3, .. } => Alice::BtcLocked(state3.as_ref().clone()), + AliceState::XmrLocked { state3 } => Alice::XmrLocked(state3.as_ref().clone()), AliceState::EncSigLearned { state3, encrypted_signature, } => Alice::EncSigLearned { - state: state3.clone(), + state3: state3.as_ref().clone(), encrypted_signature: encrypted_signature.clone(), }, AliceState::BtcRedeemed => Alice::Done(AliceEndState::BtcRedeemed), - AliceState::BtcCancelled { state3, .. } => Alice::BtcCancelled(state3.clone()), + AliceState::BtcCancelled { state3, .. } => Alice::BtcCancelled(state3.as_ref().clone()), AliceState::BtcRefunded { spend_key, state3 } => Alice::BtcRefunded { spend_key: *spend_key, - state3: state3.clone(), + state3: state3.as_ref().clone(), }, - AliceState::BtcPunishable { state3, .. } => Alice::BtcPunishable(state3.clone()), + AliceState::BtcPunishable { state3, .. } => { + Alice::BtcPunishable(state3.as_ref().clone()) + } AliceState::XmrRefunded => Alice::Done(AliceEndState::XmrRefunded), AliceState::CancelTimelockExpired { state3 } => { - Alice::CancelTimelockExpired(state3.clone()) + Alice::CancelTimelockExpired(state3.as_ref().clone()) } AliceState::BtcPunished => Alice::Done(AliceEndState::BtcPunished), AliceState::SafelyAborted => Alice::Done(AliceEndState::SafelyAborted), @@ -151,7 +152,7 @@ impl From for AliceState { btc: state3.btc, xmr: state3.xmr, }, - state3, + state3: Box::new(state3), }, Alice::BtcLocked(state3) => BtcLocked { channel: None, @@ -159,17 +160,21 @@ impl From for AliceState { btc: state3.btc, xmr: state3.xmr, }, - state3, + state3: Box::new(state3), + }, + Alice::XmrLocked(state3) => XmrLocked { + state3: Box::new(state3), }, - Alice::XmrLocked(state3) => XmrLocked { state3 }, Alice::EncSigLearned { - state, - encrypted_signature, - } => EncSigLearned { state3: state, encrypted_signature, + } => EncSigLearned { + state3: Box::new(state), + encrypted_signature, + }, + Alice::CancelTimelockExpired(state3) => AliceState::CancelTimelockExpired { + state3: Box::new(state3), }, - Alice::CancelTimelockExpired(state3) => AliceState::CancelTimelockExpired { state3 }, Alice::BtcCancelled(state) => { let tx_cancel = bitcoin::TxCancel::new( &state.tx_lock, @@ -179,30 +184,28 @@ impl From for AliceState { ); BtcCancelled { - state3: state, + state3: Box::new(state), tx_cancel, } } - Alice::BtcPunishable(state) => { + Alice::BtcPunishable(state3) => { let tx_cancel = bitcoin::TxCancel::new( - &state.tx_lock, - state.cancel_timelock, - state.a.public(), - state.B, + &state3.tx_lock, + state3.cancel_timelock, + state3.a.public(), + state3.B, ); - let tx_refund = bitcoin::TxRefund::new(&tx_cancel, &state.refund_address); + let tx_refund = bitcoin::TxRefund::new(&tx_cancel, &state3.refund_address); BtcPunishable { tx_refund, - state3: state, + state3: Box::new(state3), } } Alice::BtcRefunded { - state3: state, - spend_key, - .. + state3, spend_key, .. } => BtcRefunded { spend_key, - state3: state, + state3: Box::new(state3), }, Alice::Done(end_state) => match end_state { AliceEndState::SafelyAborted => SafelyAborted, @@ -286,7 +289,7 @@ pub async fn run_until( let state = AliceState::Negotiated { channel: Some(channel), amounts, - state3, + state3: Box::new(state3), }; let db_state = (&state).into(); @@ -357,7 +360,7 @@ pub async fn run_until( lock_xmr( channel, amounts, - state3.clone(), + *state3.clone(), &mut event_loop_handle, monero_wallet.clone(), ) diff --git a/swap/src/bob.rs b/swap/src/bob.rs index 852e0f0f..33917b57 100644 --- a/swap/src/bob.rs +++ b/swap/src/bob.rs @@ -45,13 +45,12 @@ pub fn new_swarm(transport: SwapTransport, behaviour: Behaviour) -> Result), + Message1(Box), Message2(alice::Message2), Message3, } @@ -77,7 +76,7 @@ impl From for OutEvent { impl From for OutEvent { fn from(event: message0::OutEvent) -> Self { match event { - message0::OutEvent::Msg(msg) => OutEvent::Message0(msg), + message0::OutEvent::Msg(msg) => OutEvent::Message0(Box::new(msg)), } } } @@ -85,7 +84,7 @@ impl From for OutEvent { impl From for OutEvent { fn from(event: message1::OutEvent) -> Self { match event { - message1::OutEvent::Msg(msg) => OutEvent::Message1(msg), + message1::OutEvent::Msg(msg) => OutEvent::Message1(Box::new(msg)), } } } diff --git a/swap/src/bob/event_loop.rs b/swap/src/bob/event_loop.rs index 16cc19c2..80a3fac9 100644 --- a/swap/src/bob/event_loop.rs +++ b/swap/src/bob/event_loop.rs @@ -192,10 +192,10 @@ impl EventLoop { } OutEvent::Amounts(_amounts) => info!("Amounts received from Alice"), OutEvent::Message0(msg) => { - let _ = self.msg0.send(msg).await; + let _ = self.msg0.send(*msg).await; } OutEvent::Message1(msg) => { - let _ = self.msg1.send(msg).await; + let _ = self.msg1.send(*msg).await; } OutEvent::Message2(msg) => { let _ = self.msg2.send(msg).await; diff --git a/swap/src/bob/message0.rs b/swap/src/bob/message0.rs index 89504fbb..16ae9588 100644 --- a/swap/src/bob/message0.rs +++ b/swap/src/bob/message0.rs @@ -33,7 +33,7 @@ pub struct Message0 { impl Message0 { pub fn send(&mut self, alice: PeerId, msg: bob::Message0) { - let msg = BobToAlice::Message0(msg); + let msg = BobToAlice::Message0(Box::new(msg)); let _id = self.rr.send_request(&alice, msg); } @@ -80,7 +80,7 @@ impl NetworkBehaviourEventProcess> } => { if let AliceToBob::Message0(msg) = response { debug!("Received Message0"); - self.events.push_back(OutEvent::Msg(msg)); + self.events.push_back(OutEvent::Msg(*msg)); } } RequestResponseEvent::InboundFailure { error, .. } => { diff --git a/swap/src/bob/message1.rs b/swap/src/bob/message1.rs index 32fd6c52..62696180 100644 --- a/swap/src/bob/message1.rs +++ b/swap/src/bob/message1.rs @@ -80,7 +80,7 @@ impl NetworkBehaviourEventProcess> } => { if let AliceToBob::Message1(msg) = response { debug!("Received Message1"); - self.events.push_back(OutEvent::Msg(msg)); + self.events.push_back(OutEvent::Msg(*msg)); } } RequestResponseEvent::InboundFailure { error, .. } => { diff --git a/swap/src/network/request_response.rs b/swap/src/network/request_response.rs index a042debc..e58d81ad 100644 --- a/swap/src/network/request_response.rs +++ b/swap/src/network/request_response.rs @@ -22,12 +22,11 @@ const BUF_SIZE: usize = 1024 * 1024; /// Messages Bob sends to Alice. #[derive(Clone, Debug, Serialize, Deserialize)] -#[allow(clippy::large_enum_variant)] pub enum BobToAlice { #[serde(with = "::bitcoin::util::amount::serde::as_sat")] AmountsFromBtc(::bitcoin::Amount), AmountsFromXmr(monero::Amount), - Message0(bob::Message0), + Message0(Box), Message1(bob::Message1), Message2(bob::Message2), Message3(bob::Message3), @@ -35,11 +34,10 @@ pub enum BobToAlice { /// Messages Alice sends to Bob. #[derive(Clone, Debug, Serialize, Deserialize)] -#[allow(clippy::large_enum_variant)] pub enum AliceToBob { Amounts(SwapAmounts), - Message0(alice::Message0), - Message1(alice::Message1), + Message0(Box), + Message1(Box), Message2(alice::Message2), Message3, // empty response } diff --git a/swap/src/state.rs b/swap/src/state.rs index 82fb901e..667943e2 100644 --- a/swap/src/state.rs +++ b/swap/src/state.rs @@ -3,13 +3,14 @@ use serde::{Deserialize, Serialize}; use std::fmt::Display; use xmr_btc::{alice, bitcoin::EncryptedSignature, bob, monero, serde::monero_private_key}; -#[allow(clippy::large_enum_variant)] #[derive(Clone, Debug, Deserialize, Serialize, PartialEq)] pub enum Swap { Alice(Alice), Bob(Bob), } +// Large enum variant is fine because this is only used for storage +// and is dropped once written in DB. #[allow(clippy::large_enum_variant)] #[derive(Clone, Debug, Deserialize, Serialize, PartialEq)] pub enum Alice { @@ -21,8 +22,8 @@ pub enum Alice { BtcLocked(alice::State3), XmrLocked(alice::State3), EncSigLearned { - state: alice::State3, encrypted_signature: EncryptedSignature, + state3: alice::State3, }, CancelTimelockExpired(alice::State3), BtcCancelled(alice::State3), diff --git a/xmr-btc/src/alice.rs b/xmr-btc/src/alice.rs index 52418c51..7297bded 100644 --- a/xmr-btc/src/alice.rs +++ b/xmr-btc/src/alice.rs @@ -375,11 +375,10 @@ pub async fn next_state< state6.redeem_btc(bitcoin_wallet).await?; Ok(state6.into()) } - State::State6(state6) => Ok(state6.into()), + State::State6(state6) => Ok((*state6).into()), } } -#[allow(clippy::large_enum_variant)] #[derive(Debug, Deserialize, Serialize)] pub enum State { State0(State0), @@ -388,7 +387,7 @@ pub enum State { State3(State3), State4(State4), State5(State5), - State6(State6), + State6(Box), } impl_try_from_parent_enum!(State0, State); @@ -397,7 +396,7 @@ impl_try_from_parent_enum!(State2, State); impl_try_from_parent_enum!(State3, State); impl_try_from_parent_enum!(State4, State); impl_try_from_parent_enum!(State5, State); -impl_try_from_parent_enum!(State6, State); +impl_try_from_parent_enum_for_boxed!(State6, State); impl_from_child_enum!(State0, State); impl_from_child_enum!(State1, State); @@ -405,7 +404,7 @@ impl_from_child_enum!(State2, State); impl_from_child_enum!(State3, State); impl_from_child_enum!(State4, State); impl_from_child_enum!(State5, State); -impl_from_child_enum!(State6, State); +impl_from_child_enum_for_boxed!(State6, State); impl State { pub fn new( diff --git a/xmr-btc/src/bitcoin/transactions.rs b/xmr-btc/src/bitcoin/transactions.rs index e9b789ad..9ebafb38 100644 --- a/xmr-btc/src/bitcoin/transactions.rs +++ b/xmr-btc/src/bitcoin/transactions.rs @@ -50,6 +50,8 @@ impl TxLock { } pub fn as_outpoint(&self) -> OutPoint { + // This is fine because a transaction that has that many outputs is not + // realistic #[allow(clippy::cast_possible_truncation)] OutPoint::new(self.inner.txid(), self.lock_output_vout() as u32) } diff --git a/xmr-btc/src/bob.rs b/xmr-btc/src/bob.rs index f879de28..42fdd6df 100644 --- a/xmr-btc/src/bob.rs +++ b/xmr-btc/src/bob.rs @@ -43,7 +43,6 @@ use crate::{ use ::bitcoin::{Transaction, Txid}; pub use message::{Message, Message0, Message1, Message2, Message3}; -#[allow(clippy::large_enum_variant)] #[derive(Debug)] pub enum Action { LockBtc(bitcoin::TxLock), diff --git a/xmr-btc/src/lib.rs b/xmr-btc/src/lib.rs index 3cfe0c50..22fc8807 100644 --- a/xmr-btc/src/lib.rs +++ b/xmr-btc/src/lib.rs @@ -41,6 +41,24 @@ mod utils { }; } + macro_rules! impl_try_from_parent_enum_for_boxed { + ($type:ident, $parent:ident) => { + impl TryFrom<$parent> for $type { + type Error = anyhow::Error; + + fn try_from(from: $parent) -> Result { + if let $parent::$type(inner) = from { + Ok(*inner) + } else { + Err(anyhow::anyhow!( + "Failed to convert parent state to child state" + )) + } + } + } + }; + } + macro_rules! impl_from_child_enum { ($type:ident, $parent:ident) => { impl From<$type> for $parent { @@ -50,6 +68,16 @@ mod utils { } }; } + + macro_rules! impl_from_child_enum_for_boxed { + ($type:ident, $parent:ident) => { + impl From<$type> for $parent { + fn from(from: $type) -> Self { + $parent::$type(Box::new(from)) + } + } + }; + } } pub mod alice;