Polish trading implementation and address review comments.

- Fix item swapping edge case
- Document more assumptions/edge cases
- fmt and clippy
- s/ServerGeneral::GroupInvite/ServerGeneral::Invite/
- Use `Client::current` in `Client::is_dead`

client/src/lib.rs

@@ -659,13 +659,7 @@ impl Client {
         }
     }
 
-    pub fn is_dead(&self) -> bool {
-        self.state
-            .ecs()
-            .read_storage::<comp::Health>()
-            .get(self.entity)
-            .map_or(false, |h| h.is_dead)
-    }
+    pub fn is_dead(&self) -> bool { self.current::<comp::Health>().map_or(false, |h| h.is_dead) }
 
     pub fn pick_up(&mut self, entity: EcsEntity) {
         // Get the health component from the entity
@@ -1504,7 +1498,7 @@ impl Client {
                     },
                 }
             },
-            ServerGeneral::GroupInvite {
+            ServerGeneral::Invite {
                 inviter,
                 timeout,
                 kind,

common/net/src/msg/server.rs

@@ -78,7 +78,7 @@ pub enum ServerGeneral {
     //Ingame related
     GroupUpdate(comp::group::ChangeNotification<sync::Uid>),
     /// Indicate to the client that they are invited to join a group
-    GroupInvite {
+    Invite {
         inviter: sync::Uid,
         timeout: std::time::Duration,
         kind: InviteKind,
@@ -221,7 +221,7 @@ impl ServerMsg {
                         },
                         //Ingame related
                         ServerGeneral::GroupUpdate(_)
-                        | ServerGeneral::GroupInvite { .. }
+                        | ServerGeneral::Invite { .. }
                         | ServerGeneral::InvitePending(_)
                         | ServerGeneral::InviteComplete { .. }
                         | ServerGeneral::ExitInGameSuccess

common/src/trade.rs

@@ -4,7 +4,7 @@ use crate::{
 };
 use hashbrown::HashMap;
 use serde::{Deserialize, Serialize};
-use tracing::{warn, trace};
+use tracing::{trace, warn};
 
 /// Clients submit `TradeActionMsg` to the server, which adds the Uid of the
 /// player out-of-band (i.e. without trusting the client to say who it's
@@ -15,6 +15,7 @@ pub enum TradeActionMsg {
     RemoveItem { item: InvSlotId, quantity: u32 },
     Phase1Accept,
     Phase2Accept,
+
     Decline,
 }
 
@@ -28,6 +29,28 @@ pub enum TradeResult {
 /// Items are not removed from the inventory during a PendingTrade: all the
 /// items are moved atomically (if there's space and both parties agree) upon
 /// completion
+///
+/// Since this stores `InvSlotId`s (i.e. references into inventories) instead of
+/// items themselves, there aren't any duplication/loss risks from things like
+/// dropped connections or declines, since the server doesn't have to move items
+/// from a trade back into a player's inventory.
+///
+/// On the flip side, since they are references to *slots*, if a player could
+/// swaps items in their inventory during a trade, they could mutate the trade,
+/// enabling them to remove an item from the trade even after receiving the
+/// counterparty's phase2 accept. To prevent this, we disallow all
+/// forms of inventory manipulation in `server::events::inventory_manip` if
+/// there's a pending trade that's past phase1 (in phase1, the trade should be
+/// mutable anyway).
+///
+/// Inventory manipulation in phase1 may be beneficial to trade (e.g. splitting
+/// a stack of items, once that's implemented), but should reset both phase1
+/// accept flags to make the changes more visible.
+///
+/// Another edge case prevented by using `InvSlotId`s is that it disallows
+/// trading currently-equipped items (since `EquipSlot`s are disjoint from
+/// `InvSlotId`s), which avoids the issues associated with trading equipped bags
+/// that may still have contents.
 #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
 pub struct PendingTrade {
     /// `parties[0]` is the entity that initiated the trade, parties[1] is the
@@ -79,8 +102,7 @@ impl PendingTrade {
     /// - A party is never shown as offering more of an item than they own
     /// - Offers with a quantity of zero get removed from the trade
     /// - Modifications can only happen in phase 1
-    /// - Whenever a trade is modified, both accept flags get reset (TODO: detect or prevent
-    /// inventory swaps)
+    /// - Whenever a trade is modified, both accept flags get reset
     /// - Accept flags only get set for the current phase
     pub fn process_msg(&mut self, who: usize, msg: TradeActionMsg, inventory: &Inventory) {
         use TradeActionMsg::*;
@@ -89,13 +111,11 @@ impl PendingTrade {
                 item,
                 quantity: delta,
             } => {
-                if self.in_phase1() {
-                    if delta > 0 {
-                        let total = self.offers[who].entry(item).or_insert(0);
-                        let owned_quantity = inventory.get(item).map(|i| i.amount()).unwrap_or(0);
-                        *total = total.saturating_add(delta).min(owned_quantity);
-                        self.phase1_accepts = [false, false];
-                    }
+                if self.in_phase1() && delta > 0 {
+                    let total = self.offers[who].entry(item).or_insert(0);
+                    let owned_quantity = inventory.get(item).map(|i| i.amount()).unwrap_or(0);
+                    *total = total.saturating_add(delta).min(owned_quantity);
+                    self.phase1_accepts = [false, false];
                 }
             },
             RemoveItem {
@@ -130,6 +150,7 @@ impl PendingTrade {
 pub struct Trades {
     pub next_id: usize,
     pub trades: HashMap<usize, PendingTrade>,
+    pub entity_trades: HashMap<Uid, usize>,
 }
 
 impl Trades {
@@ -138,6 +159,8 @@ impl Trades {
         self.next_id = id.wrapping_add(1);
         self.trades
             .insert(id, PendingTrade::new(party, counterparty));
+        self.entity_trades.insert(party, id);
+        self.entity_trades.insert(counterparty, id);
         id
     }
 
@@ -168,6 +191,8 @@ impl Trades {
         if let Some(trade) = self.trades.remove(&id) {
             match trade.which_party(who) {
                 Some(i) => {
+                    self.entity_trades.remove(&trade.parties[0]);
+                    self.entity_trades.remove(&trade.parties[1]);
                     // let the other person know the trade was declined
                     to_notify = Some(trade.parties[1 - i])
                 },
@@ -185,6 +210,37 @@ impl Trades {
         }
         to_notify
     }
+
+    /// See the doc comment on `common::trade::PendingTrade` for the
+    /// significance of these checks
+    pub fn in_trade_with_property<F: FnOnce(&PendingTrade) -> bool>(
+        &self,
+        uid: &Uid,
+        f: F,
+    ) -> bool {
+        self.entity_trades
+            .get(uid)
+            .and_then(|trade_id| self.trades.get(trade_id))
+            .map(f)
+            // if any of the option lookups failed, we're not in any trade
+            .unwrap_or(false)
+    }
+
+    pub fn in_immutable_trade(&self, uid: &Uid) -> bool {
+        self.in_trade_with_property(uid, |trade| !trade.in_phase1())
+    }
+
+    pub fn in_mutable_trade(&self, uid: &Uid) -> bool {
+        self.in_trade_with_property(uid, |trade| trade.in_phase1())
+    }
+
+    pub fn implicit_mutation_occurred(&mut self, uid: &Uid) {
+        if let Some(trade_id) = self.entity_trades.get(uid) {
+            self.trades
+                .get_mut(trade_id)
+                .map(|trade| trade.phase1_accepts = [false, false]);
+        }
+    }
 }
 
 impl Default for Trades {
@@ -192,6 +248,7 @@ impl Default for Trades {
         Trades {
             next_id: 0,
             trades: HashMap::new(),
+            entity_trades: HashMap::new(),
         }
     }
 }

server/src/client.rs

@@ -79,7 +79,7 @@ impl Client {
                     },
                     //Ingame related
                     ServerGeneral::GroupUpdate(_)
-                    | ServerGeneral::GroupInvite { .. }
+                    | ServerGeneral::Invite { .. }
                     | ServerGeneral::InvitePending(_)
                     | ServerGeneral::InviteComplete { .. }
                     | ServerGeneral::ExitInGameSuccess
@@ -159,7 +159,7 @@ impl Client {
                     },
                     //Ingame related
                     ServerGeneral::GroupUpdate(_)
-                    | ServerGeneral::GroupInvite { .. }
+                    | ServerGeneral::Invite { .. }
                     | ServerGeneral::InvitePending(_)
                     | ServerGeneral::InviteComplete { .. }
                     | ServerGeneral::ExitInGameSuccess
@@ -170,7 +170,9 @@ impl Client {
                     | ServerGeneral::Outcomes(_)
                     | ServerGeneral::Knockback(_)
                     | ServerGeneral::UpdatePendingTrade(_, _)
-                    | ServerGeneral::FinishedTrade(_) => PreparedMsg::new(2, &g, &self.in_game_stream),
+                    | ServerGeneral::FinishedTrade(_) => {
+                        PreparedMsg::new(2, &g, &self.in_game_stream)
+                    },
                     // Always possible
                     ServerGeneral::PlayerListUpdate(_)
                     | ServerGeneral::ChatMsg(_)

server/src/events/group_manip.rs

@@ -160,7 +160,7 @@ pub fn handle_invite(
     // If client comp
     if let (Some(client), Some(inviter)) = (clients.get(invitee), uids.get(inviter).copied()) {
         if send_invite() {
-            client.send_fallible(ServerGeneral::GroupInvite {
+            client.send_fallible(ServerGeneral::Invite {
                 inviter,
                 timeout: PRESENTED_INVITE_TIMEOUT_DUR,
                 kind,

server/src/events/inventory_manip.rs

@@ -10,6 +10,7 @@ use common::{
     },
     consts::MAX_PICKUP_RANGE,
     recipe::default_recipe_book,
+    trade::Trades,
     uid::Uid,
     util::find_dist::{self, FindDist},
     vol::ReadVol,
@@ -39,6 +40,15 @@ pub fn snuff_lantern(storage: &mut WriteStorage<comp::LightEmitter>, entity: Ecs
 #[allow(clippy::same_item_push)] // TODO: Pending review in #587
 pub fn handle_inventory(server: &mut Server, entity: EcsEntity, manip: comp::SlotManip) {
     let state = server.state_mut();
+
+    if let Some(uid) = state.ecs().uid_from_entity(entity) {
+        let trades = state.ecs().read_resource::<Trades>();
+        if trades.in_immutable_trade(&uid) {
+            // manipulating the inventory can mutate the trade
+            return;
+        }
+    }
+
     let mut dropped_items = Vec::new();
     let mut thrown_items = Vec::new();
 
@@ -568,6 +578,14 @@ pub fn handle_inventory(server: &mut Server, entity: EcsEntity, manip: comp::Slo
 
         new_entity.build();
     }
+
+    if let Some(uid) = state.ecs().uid_from_entity(entity) {
+        let mut trades = state.ecs().write_resource::<Trades>();
+        if trades.in_mutable_trade(&uid) {
+            // manipulating the inventory mutated the trade, so reset the accept flags
+            trades.implicit_mutation_occurred(&uid);
+        }
+    }
 }
 
 fn within_pickup_range<S: FindDist<find_dist::Cylinder>>(

server/src/events/trade.rs

@@ -1,17 +1,15 @@
 use crate::{events::group_manip::handle_invite, Server};
 use common::{
-    comp::{
-        group::InviteKind,
-        inventory::Inventory,
-    },
-    trade::{PendingTrade, TradeActionMsg, Trades, TradeResult},
+    comp::{group::InviteKind, inventory::Inventory},
+    trade::{PendingTrade, TradeActionMsg, TradeResult, Trades},
 };
 use common_net::{msg::ServerGeneral, sync::WorldSyncExt};
 use specs::{world::WorldExt, Entity as EcsEntity};
 use std::cmp::Ordering;
-use tracing::{error, warn, trace};
+use tracing::{error, trace, warn};
 
-/// Invoked when pressing the trade button near an entity, triggering the invite UI flow
+/// Invoked when pressing the trade button near an entity, triggering the invite
+/// UI flow
 pub fn handle_initiate_trade(server: &mut Server, interactor: EcsEntity, counterparty: EcsEntity) {
     if let Some(uid) = server.state_mut().ecs().uid_from_entity(counterparty) {
         handle_invite(server, interactor, uid, InviteKind::Trade);
@@ -33,7 +31,9 @@ pub fn handle_process_trade_action(
             let to_notify = trades.decline_trade(trade_id, uid);
             to_notify
                 .and_then(|u| server.state.ecs().entity_from_uid(u.0))
-                .map(|e| server.notify_client(e, ServerGeneral::FinishedTrade(TradeResult::Declined)));
+                .map(|e| {
+                    server.notify_client(e, ServerGeneral::FinishedTrade(TradeResult::Declined))
+                });
         } else {
             if let Some(inv) = server.state.ecs().read_component::<Inventory>().get(entity) {
                 trades.process_trade_action(trade_id, uid, msg, inv);
@@ -46,19 +46,19 @@ pub fn handle_process_trade_action(
                 }
                 // send the updated state to both parties
                 for party in trade.parties.iter() {
-                    server.state.ecs().entity_from_uid(party.0).map(|e| {
-                        server.notify_client(
-                            e,
-                            msg.clone(),
-                        )
-                    });
+                    server
+                        .state
+                        .ecs()
+                        .entity_from_uid(party.0)
+                        .map(|e| server.notify_client(e, msg.clone()));
                 }
             }
         }
     }
 }
 
-/// Commit a trade that both parties have agreed to, modifying their respective inventories
+/// Commit a trade that both parties have agreed to, modifying their respective
+/// inventories
 fn commit_trade(ecs: &specs::World, trade: &PendingTrade) -> TradeResult {
     let mut entities = vec![];
     for who in [0, 1].iter().cloned() {
@@ -75,8 +75,8 @@ fn commit_trade(ecs: &specs::World, trade: &PendingTrade) -> TradeResult {
     }
     let invmsg = "inventories.get_mut(entities[who]).is_none() should have returned already";
     trace!("committing trade: {:?}", trade);
-    // Compute the net change in slots of each player during the trade, to detect out-of-space-ness
-    // before transferring any items
+    // Compute the net change in slots of each player during the trade, to detect
+    // out-of-space-ness before transferring any items
     let mut delta_slots: [isize; 2] = [0, 0];
     for who in [0, 1].iter().cloned() {
         for (slot, quantity) in trade.offers[who].iter() {
@@ -84,30 +84,38 @@ fn commit_trade(ecs: &specs::World, trade: &PendingTrade) -> TradeResult {
             let item = match inventory.get(*slot) {
                 Some(item) => item,
                 None => {
-                    error!("PendingTrade invariant violation in trade {:?}: slots offered in a trade should be non-empty", trade);
+                    error!(
+                        "PendingTrade invariant violation in trade {:?}: slots offered in a trade \
+                         should be non-empty",
+                        trade
+                    );
                     return TradeResult::Declined;
                 },
             };
             match item.amount().cmp(&quantity) {
                 Ordering::Less => {
-                    error!("PendingTrade invariant violation in trade {:?}: party {} offered more of an item than they have", trade, who);
+                    error!(
+                        "PendingTrade invariant violation in trade {:?}: party {} offered more of \
+                         an item than they have",
+                        trade, who
+                    );
                     return TradeResult::Declined;
                 },
                 Ordering::Equal => {
                     delta_slots[who] -= 1; // exact, removes the whole stack
-                    delta_slots[1-who] += 1; // overapproximation, assumes the stack won't merge
+                    delta_slots[1 - who] += 1; // overapproximation, assumes the stack won't merge
                 },
                 Ordering::Greater => {
                     // no change to delta_slots[who], since they have leftovers
-                    delta_slots[1-who] += 1; // overapproximation, assumes the stack won't merge
+                    delta_slots[1 - who] += 1; // overapproximation, assumes the stack won't merge
                 },
             }
         }
     }
     trace!("delta_slots: {:?}", delta_slots);
     for who in [0, 1].iter().cloned() {
-        // Inventories should never exceed 2^{63} slots, so the usize -> isize conversions here
-        // should be safe
+        // Inventories should never exceed 2^{63} slots, so the usize -> isize
+        // conversions here should be safe
         let inv = inventories.get_mut(entities[who]).expect(invmsg);
         if inv.populated_slots() as isize + delta_slots[who] > inv.capacity() as isize {
             return TradeResult::NotEnoughSpace;
@@ -118,15 +126,26 @@ fn commit_trade(ecs: &specs::World, trade: &PendingTrade) -> TradeResult {
         for (slot, quantity) in trade.offers[who].iter() {
             // take the items one by one, to benefit from Inventory's stack handling
             for _ in 0..*quantity {
-                inventories.get_mut(entities[who]).expect(invmsg).take(*slot).map(|item| items[who].push(item));
+                inventories
+                    .get_mut(entities[who])
+                    .expect(invmsg)
+                    .take(*slot)
+                    .map(|item| items[who].push(item));
             }
         }
     }
     for who in [0, 1].iter().cloned() {
-        if let Err(leftovers) = inventories.get_mut(entities[1-who]).expect(invmsg).push_all(items[who].drain(..)) {
-            // this should only happen if the arithmetic above for delta_slots says there's enough
-            // space and there isn't (i.e. underapproximates)
-            error!("Not enough space for all the items, destroying leftovers {:?}", leftovers);
+        if let Err(leftovers) = inventories
+            .get_mut(entities[1 - who])
+            .expect(invmsg)
+            .push_all(items[who].drain(..))
+        {
+            // this should only happen if the arithmetic above for delta_slots says there's
+            // enough space and there isn't (i.e. underapproximates)
+            error!(
+                "Not enough space for all the items, destroying leftovers {:?}",
+                leftovers
+            );
         }
     }
     TradeResult::Completed

voxygen/src/hud/trade.rs

@@ -151,8 +151,7 @@ impl<'a> Trade<'a> {
         // Alignment for Grid
         let mut alignment = Rectangle::fill_with([200.0, 340.0], color::TRANSPARENT);
         if who % 2 == 0 {
-            alignment =
-                alignment.top_left_with_margins_on(state.ids.bg, 180.0, 46.5);
+            alignment = alignment.top_left_with_margins_on(state.ids.bg, 180.0, 46.5);
         } else {
             alignment = alignment.right_from(state.ids.inv_alignment[0], 0.0);
         }
@@ -188,14 +187,15 @@ impl<'a> Trade<'a> {
             .down_from(state.ids.inv_alignment[who], 20.0)
             .font_id(self.fonts.cyri.conrod_id)
             .font_size(self.fonts.cyri.scale(20))
-            .color(Color::Rgba(1.0, 1.0, 1.0, if has_accepted { 1.0 } else { 0.0 }))
+            .color(Color::Rgba(
+                1.0,
+                1.0,
+                1.0,
+                if has_accepted { 1.0 } else { 0.0 },
+            ))
             .set(state.ids.accept_indicators[who], ui);
 
-
-        let mut invslots: Vec<_> = trade.offers[who]
-            .iter()
-            .map(|(k, v)| (k.clone(), v.clone()))
-            .collect();
+        let mut invslots: Vec<_> = trade.offers[who].iter().map(|(k, v)| (*k, *v)).collect();
         invslots.sort();
         let tradeslots: Vec<_> = invslots
             .into_iter()
@@ -222,7 +222,7 @@ impl<'a> Trade<'a> {
         ui: &mut UiCell<'_>,
         inventory: &Inventory,
         who: usize,
-        tradeslots: &Vec<TradeSlot>,
+        tradeslots: &[TradeSlot],
     ) {
         let mut slot_maker = SlotMaker {
             empty_slot: self.imgs.inv_slot,
@@ -262,7 +262,7 @@ impl<'a> Trade<'a> {
             });
             // Slot
             let slot_widget = slot_maker
-                .fabricate(slot.clone(), [40.0; 2])
+                .fabricate(slot, [40.0; 2])
                 .top_left_with_margins_on(
                     state.ids.inv_alignment[who],
                     0.0 + y as f64 * (40.0),
@@ -271,13 +271,14 @@ impl<'a> Trade<'a> {
             slot_widget.set(state.ids.inv_slots[i + who * MAX_TRADE_SLOTS], ui);
         }
     }
+
     fn phase2_itemwidget(
         &mut self,
         state: &mut ConrodState<'_, State>,
         ui: &mut UiCell<'_>,
         inventory: &Inventory,
         who: usize,
-        tradeslots: &Vec<TradeSlot>,
+        tradeslots: &[TradeSlot],
     ) {
         if state.ids.inv_textslots.len() < 2 * MAX_TRADE_SLOTS {
             state.update(|s| {
@@ -302,7 +303,12 @@ impl<'a> Trade<'a> {
                 .top_left_with_margins_on(state.ids.inv_alignment[who], 10.0 + i as f64 * 30.0, 0.0)
                 .font_id(self.fonts.cyri.conrod_id)
                 .font_size(self.fonts.cyri.scale(20))
-                .color(Color::Rgba(1.0, 1.0, 1.0, if is_present { 1.0 } else { 0.0 }))
+                .color(Color::Rgba(
+                    1.0,
+                    1.0,
+                    1.0,
+                    if is_present { 1.0 } else { 0.0 },
+                ))
                 .set(state.ids.inv_textslots[i + who * MAX_TRADE_SLOTS], ui);
         }
     }
@@ -406,7 +412,9 @@ impl<'a> Widget for Trade<'a> {
         }
         if state.ids.accept_indicators.len() < 2 {
             state.update(|s| {
-                s.ids.accept_indicators.resize(2, &mut ui.widget_id_generator());
+                s.ids
+                    .accept_indicators
+                    .resize(2, &mut ui.widget_id_generator());
             });
         }