diff --git a/client/src/lib.rs b/client/src/lib.rs
index 256bc5c155..074a8b6f77 100644
--- a/client/src/lib.rs
+++ b/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,
diff --git a/common/net/src/msg/server.rs b/common/net/src/msg/server.rs
index b9b9e1e76b..08fea64d8a 100644
--- a/common/net/src/msg/server.rs
+++ b/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
diff --git a/common/src/trade.rs b/common/src/trade.rs
index 9b1283ad16..68861e8b64 100644
--- a/common/src/trade.rs
+++ b/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(),
}
}
}
diff --git a/server/src/client.rs b/server/src/client.rs
index 4255808cec..c4b1e68057 100644
--- a/server/src/client.rs
+++ b/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(_)
diff --git a/server/src/events/group_manip.rs b/server/src/events/group_manip.rs
index d7ff25792c..61275c31b1 100644
--- a/server/src/events/group_manip.rs
+++ b/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,
diff --git a/server/src/events/inventory_manip.rs b/server/src/events/inventory_manip.rs
index 38103ad025..f6419a6b85 100644
--- a/server/src/events/inventory_manip.rs
+++ b/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>>(
diff --git a/server/src/events/trade.rs b/server/src/events/trade.rs
index 144e739ed8..422e080f7a 100644
--- a/server/src/events/trade.rs
+++ b/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
diff --git a/voxygen/src/hud/trade.rs b/voxygen/src/hud/trade.rs
index 9794e8b395..fef8ef5ef5 100644
--- a/voxygen/src/hud/trade.rs
+++ b/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());
});
}