MR 1775 review fixes.

- Separate `invite` machinery from `group_manip` into it's own thing (includes renaming `group_invite` to `invite` where applicable).
- Move some invite/trade machinery to `ControlEvent`.
- Make `TradePhase` a proper enum instead of a bunch of bools.
- Make `TradeId` a proper newtype.
- Remove trades from `Trades` on accept (previously was only on decline).
- Typo fixes/misc cleanup.
- Add bullet point for trading to the changelog.

CHANGELOG.md

@@ -30,6 +30,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - Snow particles
 - Basic NPC interaction
 - Lights in dungeons
+- Trading system (bound to the `R` key by default, currently only works with players)
 
 ### Changed
 

client/src/lib.rs

@@ -20,7 +20,8 @@ use common::{
     comp::{
         self,
         chat::{KillSource, KillType},
-        group::{self, InviteKind},
+        group,
+        invite::{InviteKind, InviteResponse},
         skills::Skill,
         slot::Slot,
         ChatMode, ControlAction, ControlEvent, Controller, ControllerInputs, GroupManip,
@@ -32,7 +33,7 @@ use common::{
     recipe::RecipeBook,
     span,
     terrain::{block::Block, neighbors, BiomeKind, SitesKind, TerrainChunk, TerrainChunkSize},
-    trade::{PendingTrade, TradeActionMsg, TradeResult},
+    trade::{PendingTrade, TradeAction, TradeId, TradeResult},
     uid::{Uid, UidAllocator},
     vol::RectVolSize,
 };
@@ -140,14 +141,14 @@ pub struct Client {
 
     max_group_size: u32,
     // Client has received an invite (inviter uid, time out instant)
-    group_invite: Option<(Uid, std::time::Instant, std::time::Duration, InviteKind)>,
+    invite: Option<(Uid, std::time::Instant, std::time::Duration, InviteKind)>,
     group_leader: Option<Uid>,
     // Note: potentially representable as a client only component
     group_members: HashMap<Uid, group::Role>,
     // Pending invites that this client has sent out
     pending_invites: HashSet<Uid>,
     // The pending trade the client is involved in, and it's id
-    pending_trade: Option<(usize, PendingTrade)>,
+    pending_trade: Option<(TradeId, PendingTrade)>,
 
     _network: Network,
     participant: Option<Participant>,
@@ -432,7 +433,7 @@ impl Client {
             chat_mode: ChatMode::default(),
 
             max_group_size,
-            group_invite: None,
+            invite: None,
             group_leader: None,
             group_members: HashMap::new(),
             pending_invites: HashSet::new(),
@@ -546,8 +547,7 @@ impl Client {
                     | ClientGeneral::TerrainChunkRequest { .. }
                     | ClientGeneral::UnlockSkill(_)
                     | ClientGeneral::RefundSkill(_)
-                    | ClientGeneral::UnlockSkillGroup(_)
+                    | ClientGeneral::UnlockSkillGroup(_) => &mut self.in_game_stream,
-                    | ClientGeneral::UpdatePendingTrade(_, _) => &mut self.in_game_stream,
                     //Always possible
                     ClientGeneral::ChatMsg(_) | ClientGeneral::Terminate => {
                         &mut self.general_stream
@@ -650,12 +650,14 @@ impl Client {
         }
     }
 
-    pub fn trade_action(&mut self, msg: TradeActionMsg) {
+    pub fn perform_trade_action(&mut self, action: TradeAction) {
         if let Some((id, _)) = self.pending_trade {
-            if let TradeActionMsg::Decline = msg {
+            if let TradeAction::Decline = action {
                 self.pending_trade.take();
             }
-            self.send_msg(ClientGeneral::UpdatePendingTrade(id, msg));
+            self.send_msg(ClientGeneral::ControlEvent(
+                ControlEvent::PerformTradeAction(id, action),
+            ));
         }
     }
 
@@ -687,19 +689,6 @@ impl Client {
         }
     }
 
-    pub fn initiate_trade(&mut self, counterparty: EcsEntity) {
-        // If we're dead, exit before sending message
-        if self.is_dead() {
-            return;
-        }
-
-        if let Some(uid) = self.state.read_component_copied(counterparty) {
-            self.send_msg(ClientGeneral::ControlEvent(ControlEvent::InitiateTrade(
-                uid,
-            )));
-        }
-    }
-
     pub fn player_list(&self) -> &HashMap<Uid, PlayerInfo> { &self.player_list }
 
     pub fn character_list(&self) -> &CharacterList { &self.character_list }
@@ -763,10 +752,8 @@ impl Client {
 
     pub fn max_group_size(&self) -> u32 { self.max_group_size }
 
-    pub fn group_invite(
+    pub fn invite(&self) -> Option<(Uid, std::time::Instant, std::time::Duration, InviteKind)> {
-        &self,
+        self.invite
-    ) -> Option<(Uid, std::time::Instant, std::time::Duration, InviteKind)> {
-        self.group_invite
     }
 
     pub fn group_info(&self) -> Option<(String, Uid)> {
@@ -777,27 +764,27 @@ impl Client {
 
     pub fn pending_invites(&self) -> &HashSet<Uid> { &self.pending_invites }
 
-    pub fn pending_trade(&self) -> &Option<(usize, PendingTrade)> { &self.pending_trade }
+    pub fn pending_trade(&self) -> &Option<(TradeId, PendingTrade)> { &self.pending_trade }
 
-    pub fn send_group_invite(&mut self, invitee: Uid) {
+    pub fn send_invite(&mut self, invitee: Uid, kind: InviteKind) {
-        self.send_msg(ClientGeneral::ControlEvent(ControlEvent::GroupManip(
+        self.send_msg(ClientGeneral::ControlEvent(ControlEvent::InitiateInvite(
-            GroupManip::Invite(invitee),
+            invitee, kind,
         )))
     }
 
-    pub fn accept_group_invite(&mut self) {
+    pub fn accept_invite(&mut self) {
         // Clear invite
-        self.group_invite.take();
+        self.invite.take();
-        self.send_msg(ClientGeneral::ControlEvent(ControlEvent::GroupManip(
+        self.send_msg(ClientGeneral::ControlEvent(ControlEvent::InviteResponse(
-            GroupManip::Accept,
+            InviteResponse::Accept,
         )));
     }
 
-    pub fn decline_group_invite(&mut self) {
+    pub fn decline_invite(&mut self) {
         // Clear invite
-        self.group_invite.take();
+        self.invite.take();
-        self.send_msg(ClientGeneral::ControlEvent(ControlEvent::GroupManip(
+        self.send_msg(ClientGeneral::ControlEvent(ControlEvent::InviteResponse(
-            GroupManip::Decline,
+            InviteResponse::Decline,
         )));
     }
 
@@ -1121,12 +1108,12 @@ impl Client {
         frontend_events.append(&mut self.handle_new_messages()?);
 
         // 3) Update client local data
-        // Check if the group invite has timed out and remove if so
+        // Check if the invite has timed out and remove if so
         if self
-            .group_invite
+            .invite
             .map_or(false, |(_, timeout, dur, _)| timeout.elapsed() > dur)
         {
-            self.group_invite = None;
+            self.invite = None;
         }
 
         // 4) Tick the client's LocalState
@@ -1503,7 +1490,7 @@ impl Client {
                 timeout,
                 kind,
             } => {
-                self.group_invite = Some((inviter, std::time::Instant::now(), timeout, kind));
+                self.invite = Some((inviter, std::time::Instant::now(), timeout, kind));
             },
             ServerGeneral::InvitePending(uid) => {
                 if !self.pending_invites.insert(uid) {
@@ -1573,7 +1560,7 @@ impl Client {
                     });
             },
             ServerGeneral::UpdatePendingTrade(id, trade) => {
-                tracing::info!("UpdatePendingTrade {:?} {:?}", id, trade);
+                tracing::trace!("UpdatePendingTrade {:?} {:?}", id, trade);
                 self.pending_trade = Some((id, trade));
             },
             ServerGeneral::FinishedTrade(result) => {

common/net/src/msg/client.rs

@@ -4,7 +4,6 @@ use common::{
     comp,
     comp::{Skill, SkillGroupKind},
     terrain::block::Block,
-    trade::TradeActionMsg,
 };
 use serde::{Deserialize, Serialize};
 use vek::*;
@@ -80,7 +79,6 @@ pub enum ClientGeneral {
     //Always possible
     ChatMsg(String),
     Terminate,
-    UpdatePendingTrade(usize, TradeActionMsg),
 }
 
 impl ClientMsg {
@@ -116,8 +114,7 @@ impl ClientMsg {
                         | ClientGeneral::TerrainChunkRequest { .. }
                         | ClientGeneral::UnlockSkill(_)
                         | ClientGeneral::RefundSkill(_)
-                        | ClientGeneral::UnlockSkillGroup(_)
+                        | ClientGeneral::UnlockSkillGroup(_) => {
-                        | ClientGeneral::UpdatePendingTrade(_, _) => {
                             c_type == ClientType::Game && presence.is_some()
                         },
                         //Always possible

common/net/src/msg/server.rs

@@ -3,12 +3,12 @@ use crate::sync;
 use authc::AuthClientError;
 use common::{
     character::{self, CharacterItem},
-    comp::{self, group::InviteKind},
+    comp::{self, invite::InviteKind},
     outcome::Outcome,
     recipe::RecipeBook,
     resources::TimeOfDay,
     terrain::{Block, TerrainChunk},
-    trade::{PendingTrade, TradeResult},
+    trade::{PendingTrade, TradeId, TradeResult},
     uid::Uid,
 };
 use hashbrown::HashMap;
@@ -123,7 +123,7 @@ pub enum ServerGeneral {
     Disconnect(DisconnectReason),
     /// Send a popup notification such as "Waypoint Saved"
     Notification(Notification),
-    UpdatePendingTrade(usize, PendingTrade),
+    UpdatePendingTrade(TradeId, PendingTrade),
     FinishedTrade(TradeResult),
 }
 

common/src/comp/controller.rs

@@ -1,8 +1,10 @@
 use crate::{
     comp::{
         inventory::slot::{EquipSlot, InvSlotId, Slot},
+        invite::{InviteKind, InviteResponse},
         BuffKind,
     },
+    trade::{TradeAction, TradeId},
     uid::Uid,
     util::Dir,
 };
@@ -69,9 +71,6 @@ impl From<InventoryManip> for SlotManip {
 
 #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
 pub enum GroupManip {
-    Invite(Uid),
-    Accept,
-    Decline,
     Leave,
     Kick(Uid),
     AssignLeader(Uid),
@@ -83,7 +82,9 @@ pub enum ControlEvent {
     EnableLantern,
     DisableLantern,
     Interact(Uid),
-    InitiateTrade(Uid),
+    InitiateInvite(Uid, InviteKind),
+    InviteResponse(InviteResponse),
+    PerformTradeAction(TradeId, TradeAction),
     Mount(Uid),
     Unmount,
     InventoryManip(InventoryManip),

common/src/comp/group.rs

@@ -28,28 +28,6 @@ impl Component for Group {
     type Storage = DerefFlaggedStorage<Self, IdvStorage<Self>>;
 }
 
-#[derive(Copy, Clone, Debug, Serialize, Deserialize)]
-pub enum InviteKind {
-    Group,
-    Trade,
-}
-
-pub struct Invite {
-    pub inviter: specs::Entity,
-    pub kind: InviteKind,
-}
-
-impl Component for Invite {
-    type Storage = IdvStorage<Self>;
-}
-
-// Pending invites that an entity currently has sent out
-// (invited entity, instant when invite times out)
-pub struct PendingInvites(pub Vec<(specs::Entity, InviteKind, std::time::Instant)>);
-impl Component for PendingInvites {
-    type Storage = IdvStorage<Self>;
-}
-
 #[derive(Clone, Debug)]
 pub struct GroupInfo {
     // TODO: what about enemy groups, either the leader will constantly change because they have to

common/src/comp/invite.rs

@@ -0,0 +1,31 @@
+use serde::{Deserialize, Serialize};
+use specs::Component;
+use specs_idvs::IdvStorage;
+
+#[derive(Copy, Clone, Debug, PartialEq, Serialize, Deserialize)]
+pub enum InviteKind {
+    Group,
+    Trade,
+}
+
+#[derive(Copy, Clone, Debug, PartialEq, Serialize, Deserialize)]
+pub enum InviteResponse {
+    Accept,
+    Decline,
+}
+
+pub struct Invite {
+    pub inviter: specs::Entity,
+    pub kind: InviteKind,
+}
+
+impl Component for Invite {
+    type Storage = IdvStorage<Self>;
+}
+
+/// Pending invites that an entity currently has sent out
+/// (invited entity, instant when invite times out)
+pub struct PendingInvites(pub Vec<(specs::Entity, InviteKind, std::time::Instant)>);
+impl Component for PendingInvites {
+    type Storage = IdvStorage<Self>;
+}

common/src/comp/mod.rs

@@ -14,6 +14,7 @@ mod health;
 pub mod home_chunk;
 mod inputs;
 pub mod inventory;
+pub mod invite;
 mod last;
 mod location;
 mod misc;

common/src/event.rs

@@ -1,8 +1,14 @@
 use crate::{
-    character::CharacterId, comp, rtsim::RtSimEntity, trade::TradeActionMsg, uid::Uid, util::Dir,
+    character::CharacterId,
+    comp,
+    rtsim::RtSimEntity,
+    trade::{TradeAction, TradeId},
+    uid::Uid,
+    util::Dir,
     Explosion,
 };
 use comp::{
+    invite::{InviteKind, InviteResponse},
     item::{Item, Reagent},
     Ori, Pos,
 };
@@ -82,8 +88,9 @@ pub enum ServerEvent {
     EnableLantern(EcsEntity),
     DisableLantern(EcsEntity),
     NpcInteract(EcsEntity, EcsEntity),
-    InitiateTrade(EcsEntity, EcsEntity),
+    InviteResponse(EcsEntity, InviteResponse),
-    ProcessTradeAction(EcsEntity, usize, TradeActionMsg),
+    InitiateInvite(EcsEntity, Uid, InviteKind),
+    ProcessTradeAction(EcsEntity, TradeId, TradeAction),
     Mount(EcsEntity, EcsEntity),
     Unmount(EcsEntity),
     Possess(Uid, Uid),

common/src/trade.rs

@@ -6,16 +6,30 @@ use hashbrown::HashMap;
 use serde::{Deserialize, Serialize};
 use tracing::{trace, warn};
 
-/// Clients submit `TradeActionMsg` to the server, which adds the Uid of the
+#[derive(Copy, Clone, Debug, PartialEq, Serialize, Deserialize)]
+pub enum TradePhase {
+    Mutate,
+    Review,
+    Complete,
+}
+
+/// Clients submit `TradeAction` to the server, which adds the Uid of the
 /// player out-of-band (i.e. without trusting the client to say who it's
 /// accepting on behalf of)
 #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)]
-pub enum TradeActionMsg {
+pub enum TradeAction {
-    AddItem { item: InvSlotId, quantity: u32 },
+    AddItem {
-    RemoveItem { item: InvSlotId, quantity: u32 },
+        item: InvSlotId,
-    Phase1Accept,
+        quantity: u32,
-    Phase2Accept,
+    },
-
+    RemoveItem {
+        item: InvSlotId,
+        quantity: u32,
+    },
+    /// Accept needs the phase indicator to avoid progressing too far in the
+    /// trade if there's latency and a player presses the accept button
+    /// multiple times
+    Accept(TradePhase),
     Decline,
 }
 
@@ -36,7 +50,7 @@ pub enum TradeResult {
 /// 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,
+/// swap 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
@@ -59,11 +73,21 @@ pub struct PendingTrade {
     /// `offers[i]` represents the items and quantities of the party i's items
     /// being offered
     pub offers: [HashMap<InvSlotId, u32>; 2],
-    /// phase1_accepts indicate that the parties wish to proceed to review
+    /// The current phase of the trade
-    pub phase1_accepts: [bool; 2],
+    pub phase: TradePhase,
-    /// phase2_accepts indicate that the parties have reviewed the trade and
+    /// `accept_flags` indicate that which parties wish to proceed to the next
-    /// wish to commit it
+    /// phase of the trade
-    pub phase2_accepts: [bool; 2],
+    pub accept_flags: [bool; 2],
+}
+
+impl TradePhase {
+    fn next(self) -> TradePhase {
+        match self {
+            TradePhase::Mutate => TradePhase::Review,
+            TradePhase::Review => TradePhase::Complete,
+            TradePhase::Complete => TradePhase::Complete,
+        }
+    }
 }
 
 impl PendingTrade {
@@ -71,24 +95,14 @@ impl PendingTrade {
         PendingTrade {
             parties: [party, counterparty],
             offers: [HashMap::new(), HashMap::new()],
-            phase1_accepts: [false, false],
+            phase: TradePhase::Mutate,
-            phase2_accepts: [false, false],
+            accept_flags: [false, false],
         }
     }
 
-    pub fn in_phase1(&self) -> bool { !self.phase1_accepts[0] || !self.phase1_accepts[1] }
+    pub fn phase(&self) -> TradePhase { self.phase }
 
-    pub fn in_phase2(&self) -> bool {
+    pub fn should_commit(&self) -> bool { matches!(self.phase, TradePhase::Complete) }
-        (self.phase1_accepts[0] && self.phase1_accepts[1])
-            && (!self.phase2_accepts[0] || !self.phase2_accepts[1])
-    }
-
-    pub fn should_commit(&self) -> bool {
-        self.phase1_accepts[0]
-            && self.phase1_accepts[1]
-            && self.phase2_accepts[0]
-            && self.phase2_accepts[1]
-    }
 
     pub fn which_party(&self, party: Uid) -> Option<usize> {
         self.parties
@@ -104,42 +118,41 @@ impl PendingTrade {
     /// - Modifications can only happen in phase 1
     /// - 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) {
+    pub fn process_trade_action(&mut self, who: usize, action: TradeAction, inventory: &Inventory) {
-        use TradeActionMsg::*;
+        use TradeAction::*;
-        match msg {
+        match action {
             AddItem {
                 item,
                 quantity: delta,
             } => {
-                if self.in_phase1() && delta > 0 {
+                if self.phase() == TradePhase::Mutate && 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];
+                    self.accept_flags = [false, false];
                 }
             },
             RemoveItem {
                 item,
                 quantity: delta,
             } => {
-                if self.in_phase1() {
+                if self.phase() == TradePhase::Mutate {
                     self.offers[who]
                         .entry(item)
                         .and_replace_entry_with(|_, mut total| {
                             total = total.saturating_sub(delta);
                             if total > 0 { Some(total) } else { None }
                         });
-                    self.phase1_accepts = [false, false];
+                    self.accept_flags = [false, false];
                 }
             },
-            Phase1Accept => {
+            Accept(phase) => {
-                if self.in_phase1() {
+                if self.phase == phase {
-                    self.phase1_accepts[who] = true;
+                    self.accept_flags[who] = true;
                 }
-            },
+                if self.accept_flags[0] && self.accept_flags[1] {
-            Phase2Accept => {
+                    self.phase = self.phase.next();
-                if self.in_phase2() {
+                    self.accept_flags = [false, false];
-                    self.phase2_accepts[who] = true;
                 }
             },
             Decline => {},
@@ -147,16 +160,19 @@ impl PendingTrade {
     }
 }
 
+#[derive(Clone, Copy, Debug, Serialize, Deserialize, PartialEq, Eq, Hash)]
+pub struct TradeId(usize);
+
 pub struct Trades {
-    pub next_id: usize,
+    pub next_id: TradeId,
-    pub trades: HashMap<usize, PendingTrade>,
+    pub trades: HashMap<TradeId, PendingTrade>,
-    pub entity_trades: HashMap<Uid, usize>,
+    pub entity_trades: HashMap<Uid, TradeId>,
 }
 
 impl Trades {
-    pub fn begin_trade(&mut self, party: Uid, counterparty: Uid) -> usize {
+    pub fn begin_trade(&mut self, party: Uid, counterparty: Uid) -> TradeId {
         let id = self.next_id;
-        self.next_id = id.wrapping_add(1);
+        self.next_id = TradeId(id.0.wrapping_add(1));
         self.trades
             .insert(id, PendingTrade::new(party, counterparty));
         self.entity_trades.insert(party, id);
@@ -166,27 +182,27 @@ impl Trades {
 
     pub fn process_trade_action(
         &mut self,
-        id: usize,
+        id: TradeId,
         who: Uid,
-        msg: TradeActionMsg,
+        action: TradeAction,
         inventory: &Inventory,
     ) {
-        trace!("for trade id {}, message {:?}", id, msg);
+        trace!("for trade id {:?}, message {:?}", id, action);
         if let Some(trade) = self.trades.get_mut(&id) {
             if let Some(party) = trade.which_party(who) {
-                trade.process_msg(party, msg, inventory);
+                trade.process_trade_action(party, action, inventory);
             } else {
                 warn!(
-                    "An entity who is not a party to trade {} tried to modify it",
+                    "An entity who is not a party to trade {:?} tried to modify it",
                     id
                 );
             }
         } else {
-            warn!("Attempt to modify nonexistent trade id {}", id);
+            warn!("Attempt to modify nonexistent trade id {:?}", id);
         }
     }
 
-    pub fn decline_trade(&mut self, id: usize, who: Uid) -> Option<Uid> {
+    pub fn decline_trade(&mut self, id: TradeId, who: Uid) -> Option<Uid> {
         let mut to_notify = None;
         if let Some(trade) = self.trades.remove(&id) {
             match trade.which_party(who) {
@@ -198,7 +214,7 @@ impl Trades {
                 },
                 None => {
                     warn!(
-                        "An entity who is not a party to trade {} tried to decline it",
+                        "An entity who is not a party to trade {:?} tried to decline it",
                         id
                     );
                     // put it back
@@ -206,7 +222,7 @@ impl Trades {
                 },
             }
         } else {
-            warn!("Attempt to decline nonexistent trade id {}", id);
+            warn!("Attempt to decline nonexistent trade id {:?}", id);
         }
         to_notify
     }
@@ -227,18 +243,18 @@ impl Trades {
     }
 
     pub fn in_immutable_trade(&self, uid: &Uid) -> bool {
-        self.in_trade_with_property(uid, |trade| !trade.in_phase1())
+        self.in_trade_with_property(uid, |trade| trade.phase() != TradePhase::Mutate)
     }
 
     pub fn in_mutable_trade(&self, uid: &Uid) -> bool {
-        self.in_trade_with_property(uid, |trade| trade.in_phase1())
+        self.in_trade_with_property(uid, |trade| trade.phase() == TradePhase::Mutate)
     }
 
     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]);
+                .map(|trade| trade.accept_flags = [false, false]);
         }
     }
 }
@@ -246,7 +262,7 @@ impl Trades {
 impl Default for Trades {
     fn default() -> Trades {
         Trades {
-            next_id: 0,
+            next_id: TradeId(0),
             trades: HashMap::new(),
             entity_trades: HashMap::new(),
         }

common/sys/src/controller.rs

@@ -98,13 +98,15 @@ impl<'a> System<'a> for Sys {
                             server_emitter.emit(ServerEvent::NpcInteract(entity, npc_entity));
                         }
                     },
-                    ControlEvent::InitiateTrade(counterparty_uid) => {
+                    ControlEvent::InitiateInvite(inviter_uid, kind) => {
-                        if let Some(counterparty_entity) =
+                        server_emitter.emit(ServerEvent::InitiateInvite(entity, inviter_uid, kind));
-                            uid_allocator.retrieve_entity_internal(counterparty_uid.id())
+                    },
-                        {
+                    ControlEvent::InviteResponse(response) => {
+                        server_emitter.emit(ServerEvent::InviteResponse(entity, response));
+                    },
+                    ControlEvent::PerformTradeAction(trade_id, action) => {
                         server_emitter
-                                .emit(ServerEvent::InitiateTrade(entity, counterparty_entity));
+                            .emit(ServerEvent::ProcessTradeAction(entity, trade_id, action));
-                        }
                     },
                     ControlEvent::InventoryManip(manip) => {
                         server_emitter.emit(ServerEvent::InventoryManip(entity, manip.into()));

common/sys/src/state.rs

@@ -168,8 +168,8 @@ impl State {
         ecs.register::<comp::ItemDrop>();
         ecs.register::<comp::ChatMode>();
         ecs.register::<comp::Faction>();
-        ecs.register::<comp::group::Invite>();
+        ecs.register::<comp::invite::Invite>();
-        ecs.register::<comp::group::PendingInvites>();
+        ecs.register::<comp::invite::PendingInvites>();
         ecs.register::<comp::Beam>();
         ecs.register::<comp::PreviousVelDtCache>();
 

server/src/cmd.rs

@@ -13,6 +13,7 @@ use common::{
         self,
         aura::{Aura, AuraKind, AuraTarget},
         buff::{BuffCategory, BuffData, BuffKind, BuffSource},
+        invite::InviteKind,
         ChatType, Inventory, Item, LightEmitter, WaypointArea,
     },
     effect::Effect,
@@ -1618,10 +1619,7 @@ fn handle_group_invite(
                 .expect("Failed to get uid for player");
 
             ecs.read_resource::<EventBus<ServerEvent>>()
-                .emit_now(ServerEvent::GroupManip(
+                .emit_now(ServerEvent::InitiateInvite(client, uid, InviteKind::Group));
-                    client,
-                    comp::GroupManip::Invite(uid),
-                ));
 
             server.notify_client(
                 client,

server/src/events/group_manip.rs

@@ -1,63 +1,27 @@
-use crate::{client::Client, Server};
+use crate::{client::Client, Server, State};
 use common::{
     comp::{
         self,
-        group::{self, Group, GroupManager, Invite, InviteKind, PendingInvites},
+        group::{self, Group, GroupManager},
+        invite::{InviteKind, PendingInvites},
         ChatType, GroupManip,
     },
-    trade::Trades,
     uid::Uid,
 };
-use common_net::{
+use common_net::{msg::ServerGeneral, sync::WorldSyncExt};
-    msg::{InviteAnswer, ServerGeneral},
+use specs::{
-    sync::WorldSyncExt,
+    world::{Entity, WorldExt},
-};
+    ReadStorage, WriteStorage,
-use specs::world::WorldExt;
-use std::time::{Duration, Instant};
-use tracing::{error, warn};
-
-/// Time before invite times out
-const INVITE_TIMEOUT_DUR: Duration = Duration::from_secs(31);
-/// Reduced duration shown to the client to help alleviate latency issues
-const PRESENTED_INVITE_TIMEOUT_DUR: Duration = Duration::from_secs(30);
-
-pub fn handle_invite(
-    server: &mut Server,
-    inviter: specs::Entity,
-    invitee_uid: Uid,
-    kind: InviteKind,
-) {
-    let max_group_size = server.settings().max_player_group_size;
-    let state = server.state_mut();
-    let clients = state.ecs().read_storage::<Client>();
-    let invitee = match state.ecs().entity_from_uid(invitee_uid.into()) {
-        Some(t) => t,
-        None => {
-            // Inform of failure
-            if let Some(client) = clients.get(inviter) {
-                client.send_fallible(ServerGeneral::server_msg(
-                    ChatType::Meta,
-                    "Invite failed, target does not exist.",
-                ));
-            }
-            return;
-        },
 };
 
-    let uids = state.ecs().read_storage::<Uid>();
+pub fn can_invite(
-
+    state: &State,
-    // Check if entity is trying to invite themselves
+    clients: &ReadStorage<'_, Client>,
-    if uids
+    pending_invites: &mut WriteStorage<'_, PendingInvites>,
-        .get(inviter)
+    max_group_size: u32,
-        .map_or(false, |inviter_uid| *inviter_uid == invitee_uid)
+    inviter: Entity,
-    {
+    invitee: Entity,
-        warn!("Entity tried to invite themselves into a group/trade");
+) -> bool {
-        return;
-    }
-
-    let mut pending_invites = state.ecs().write_storage::<PendingInvites>();
-
-    if let InviteKind::Group = kind {
     // Disallow inviting entity that is already in your group
     let groups = state.ecs().read_storage::<Group>();
     let group_manager = state.ecs().read_resource::<GroupManager>();
@@ -75,7 +39,7 @@ pub fn handle_invite(
                 "Invite failed, can't invite someone already in your group",
             ));
         }
-            return;
+        return false;
     }
 
     // Check if group max size is already reached
@@ -94,200 +58,31 @@ pub fn handle_invite(
                 .map(|i| i.num_members)
         })
         .unwrap_or(1) as usize
-            + pending_invites.get(inviter).map_or(0, |p| p.0.len())
+        + pending_invites.get(inviter).map_or(0, |p| {
+            p.0.iter()
+                .filter(|(_, k, _)| *k == InviteKind::Group)
+                .count()
+        })
         >= max_group_size as usize;
     if group_size_limit_reached {
         // Inform inviter that they have reached the group size limit
         if let Some(client) = clients.get(inviter) {
             client.send_fallible(ServerGeneral::server_msg(
                 ChatType::Meta,
-                    "Invite failed, pending invites plus current group size have reached the \
+                "Invite failed, pending invites plus current group size have reached the group \
-                     group size limit"
+                 size limit"
                     .to_owned(),
             ));
         }
-            return;
+        return false;
-        }
     }
 
-    let agents = state.ecs().read_storage::<comp::Agent>();
-    let mut invites = state.ecs().write_storage::<Invite>();
-
-    if invites.contains(invitee) {
-        // Inform inviter that there is already an invite
-        if let Some(client) = clients.get(inviter) {
-            client.send_fallible(ServerGeneral::server_msg(
-                ChatType::Meta,
-                "This player already has a pending invite.",
-            ));
-        }
-        return;
-    }
-
-    let mut invite_sent = false;
-    // Returns true if insertion was succesful
-    let mut send_invite = || {
-        match invites.insert(invitee, group::Invite { inviter, kind }) {
-            Err(err) => {
-                error!("Failed to insert Invite component: {:?}", err);
-                false
-            },
-            Ok(_) => {
-                match pending_invites.entry(inviter) {
-                    Ok(entry) => {
-                        entry.or_insert_with(|| PendingInvites(Vec::new())).0.push((
-                            invitee,
-                            kind,
-                            Instant::now() + INVITE_TIMEOUT_DUR,
-                        ));
-                        invite_sent = true;
     true
-                    },
-                    Err(err) => {
-                        error!(
-                            "Failed to get entry for pending invites component: {:?}",
-                            err
-                        );
-                        // Cleanup
-                        invites.remove(invitee);
-                        false
-                    },
-                }
-            },
-        }
-    };
-
-    // If client comp
-    if let (Some(client), Some(inviter)) = (clients.get(invitee), uids.get(inviter).copied()) {
-        if send_invite() {
-            client.send_fallible(ServerGeneral::Invite {
-                inviter,
-                timeout: PRESENTED_INVITE_TIMEOUT_DUR,
-                kind,
-            });
-        }
-    } else if agents.contains(invitee) {
-        send_invite();
-    } else if let Some(client) = clients.get(inviter) {
-        client.send_fallible(ServerGeneral::server_msg(
-            ChatType::Meta,
-            "Can't invite, not a player or npc",
-        ));
-    }
-
-    // Notify inviter that the invite is pending
-    if invite_sent {
-        if let Some(client) = clients.get(inviter) {
-            client.send_fallible(ServerGeneral::InvitePending(invitee_uid));
-        }
-    }
 }
 
 // TODO: turn chat messages into enums
 pub fn handle_group(server: &mut Server, entity: specs::Entity, manip: GroupManip) {
     match manip {
-        GroupManip::Invite(uid) => {
-            handle_invite(server, entity, uid, InviteKind::Group);
-        },
-        GroupManip::Accept => {
-            let state = server.state_mut();
-            let clients = state.ecs().read_storage::<Client>();
-            let uids = state.ecs().read_storage::<Uid>();
-            let mut invites = state.ecs().write_storage::<Invite>();
-            if let Some((inviter, kind)) = invites.remove(entity).and_then(|invite| {
-                let Invite { inviter, kind } = invite;
-                let mut pending_invites = state.ecs().write_storage::<PendingInvites>();
-                let pending = &mut pending_invites.get_mut(inviter)?.0;
-                // Check that inviter has a pending invite and remove it from the list
-                let invite_index = pending.iter().position(|p| p.0 == entity)?;
-                pending.swap_remove(invite_index);
-                // If no pending invites remain remove the component
-                if pending.is_empty() {
-                    pending_invites.remove(inviter);
-                }
-
-                Some((inviter, kind))
-            }) {
-                if let (Some(client), Some(target)) =
-                    (clients.get(inviter), uids.get(entity).copied())
-                {
-                    client.send_fallible(ServerGeneral::InviteComplete {
-                        target,
-                        answer: InviteAnswer::Accepted,
-                        kind,
-                    });
-                }
-                match kind {
-                    InviteKind::Group => {
-                        let mut group_manager = state.ecs().write_resource::<GroupManager>();
-                        group_manager.add_group_member(
-                            inviter,
-                            entity,
-                            &state.ecs().entities(),
-                            &mut state.ecs().write_storage(),
-                            &state.ecs().read_storage(),
-                            &uids,
-                            |entity, group_change| {
-                                clients
-                                    .get(entity)
-                                    .and_then(|c| {
-                                        group_change
-                                            .try_map(|e| uids.get(e).copied())
-                                            .map(|g| (g, c))
-                                    })
-                                    .map(|(g, c)| c.send(ServerGeneral::GroupUpdate(g)));
-                            },
-                        );
-                    },
-                    InviteKind::Trade => {
-                        if let (Some(inviter_uid), Some(invitee_uid)) =
-                            (uids.get(inviter).copied(), uids.get(entity).copied())
-                        {
-                            let mut trades = state.ecs().write_resource::<Trades>();
-                            let id = trades.begin_trade(inviter_uid, invitee_uid);
-                            let trade = trades.trades[&id].clone();
-                            clients.get(inviter).map(|c| {
-                                c.send(ServerGeneral::UpdatePendingTrade(id, trade.clone()))
-                            });
-                            clients
-                                .get(entity)
-                                .map(|c| c.send(ServerGeneral::UpdatePendingTrade(id, trade)));
-                        }
-                    },
-                }
-            }
-        },
-        GroupManip::Decline => {
-            let state = server.state_mut();
-            let clients = state.ecs().read_storage::<Client>();
-            let uids = state.ecs().read_storage::<Uid>();
-            let mut invites = state.ecs().write_storage::<Invite>();
-            if let Some((inviter, kind)) = invites.remove(entity).and_then(|invite| {
-                let Invite { inviter, kind } = invite;
-                let mut pending_invites = state.ecs().write_storage::<PendingInvites>();
-                let pending = &mut pending_invites.get_mut(inviter)?.0;
-                // Check that inviter has a pending invite and remove it from the list
-                let invite_index = pending.iter().position(|p| p.0 == entity)?;
-                pending.swap_remove(invite_index);
-                // If no pending invites remain remove the component
-                if pending.is_empty() {
-                    pending_invites.remove(inviter);
-                }
-
-                Some((inviter, kind))
-            }) {
-                // Inform inviter of rejection
-                if let (Some(client), Some(target)) =
-                    (clients.get(inviter), uids.get(entity).copied())
-                {
-                    client.send_fallible(ServerGeneral::InviteComplete {
-                        target,
-                        answer: InviteAnswer::Declined,
-                        kind,
-                    });
-                }
-            }
-        },
         GroupManip::Leave => {
             let state = server.state_mut();
             let clients = state.ecs().read_storage::<Client>();

server/src/events/inventory_manip.rs

@@ -41,7 +41,12 @@ pub fn snuff_lantern(storage: &mut WriteStorage<comp::LightEmitter>, entity: Ecs
 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 uid = state
+        .ecs()
+        .uid_from_entity(entity)
+        .expect("Couldn't get uid for entity");
+
+    {
         let trades = state.ecs().read_resource::<Trades>();
         if trades.in_immutable_trade(&uid) {
             // manipulating the inventory can mutate the trade
@@ -579,14 +584,12 @@ 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>>(
     entity_cylinder: Option<find_dist::Cylinder>,

server/src/events/invite.rs

@@ -0,0 +1,253 @@
+use super::group_manip;
+use crate::{client::Client, Server};
+use common::{
+    comp::{
+        self,
+        group::GroupManager,
+        invite::{Invite, InviteKind, InviteResponse, PendingInvites},
+        ChatType,
+    },
+    trade::Trades,
+    uid::Uid,
+};
+use common_net::{
+    msg::{InviteAnswer, ServerGeneral},
+    sync::WorldSyncExt,
+};
+use specs::world::WorldExt;
+use std::time::{Duration, Instant};
+use tracing::{error, warn};
+
+/// Time before invite times out
+const INVITE_TIMEOUT_DUR: Duration = Duration::from_secs(31);
+/// Reduced duration shown to the client to help alleviate latency issues
+const PRESENTED_INVITE_TIMEOUT_DUR: Duration = Duration::from_secs(30);
+
+pub fn handle_invite(
+    server: &mut Server,
+    inviter: specs::Entity,
+    invitee_uid: Uid,
+    kind: InviteKind,
+) {
+    let max_group_size = server.settings().max_player_group_size;
+    let state = server.state_mut();
+    let clients = state.ecs().read_storage::<Client>();
+    let invitee = match state.ecs().entity_from_uid(invitee_uid.into()) {
+        Some(t) => t,
+        None => {
+            // Inform of failure
+            if let Some(client) = clients.get(inviter) {
+                client.send_fallible(ServerGeneral::server_msg(
+                    ChatType::Meta,
+                    "Invite failed, target does not exist.",
+                ));
+            }
+            return;
+        },
+    };
+
+    let uids = state.ecs().read_storage::<Uid>();
+
+    // Check if entity is trying to invite themselves
+    if uids
+        .get(inviter)
+        .map_or(false, |inviter_uid| *inviter_uid == invitee_uid)
+    {
+        warn!("Entity tried to invite themselves into a group/trade");
+        return;
+    }
+
+    let mut pending_invites = state.ecs().write_storage::<PendingInvites>();
+
+    if let InviteKind::Group = kind {
+        if !group_manip::can_invite(
+            state,
+            &clients,
+            &mut pending_invites,
+            max_group_size,
+            inviter,
+            invitee,
+        ) {
+            return;
+        }
+    }
+
+    let agents = state.ecs().read_storage::<comp::Agent>();
+    let mut invites = state.ecs().write_storage::<Invite>();
+
+    if invites.contains(invitee) {
+        // Inform inviter that there is already an invite
+        if let Some(client) = clients.get(inviter) {
+            client.send_fallible(ServerGeneral::server_msg(
+                ChatType::Meta,
+                "This player already has a pending invite.",
+            ));
+        }
+        return;
+    }
+
+    let mut invite_sent = false;
+    // Returns true if insertion was succesful
+    let mut send_invite = || {
+        match invites.insert(invitee, Invite { inviter, kind }) {
+            Err(err) => {
+                error!("Failed to insert Invite component: {:?}", err);
+                false
+            },
+            Ok(_) => {
+                match pending_invites.entry(inviter) {
+                    Ok(entry) => {
+                        entry.or_insert_with(|| PendingInvites(Vec::new())).0.push((
+                            invitee,
+                            kind,
+                            Instant::now() + INVITE_TIMEOUT_DUR,
+                        ));
+                        invite_sent = true;
+                        true
+                    },
+                    Err(err) => {
+                        error!(
+                            "Failed to get entry for pending invites component: {:?}",
+                            err
+                        );
+                        // Cleanup
+                        invites.remove(invitee);
+                        false
+                    },
+                }
+            },
+        }
+    };
+
+    // If client comp
+    if let (Some(client), Some(inviter)) = (clients.get(invitee), uids.get(inviter).copied()) {
+        if send_invite() {
+            client.send_fallible(ServerGeneral::Invite {
+                inviter,
+                timeout: PRESENTED_INVITE_TIMEOUT_DUR,
+                kind,
+            });
+        }
+    } else if agents.contains(invitee) {
+        send_invite();
+    } else if let Some(client) = clients.get(inviter) {
+        client.send_fallible(ServerGeneral::server_msg(
+            ChatType::Meta,
+            "Can't invite, not a player or npc",
+        ));
+    }
+
+    // Notify inviter that the invite is pending
+    if invite_sent {
+        if let Some(client) = clients.get(inviter) {
+            client.send_fallible(ServerGeneral::InvitePending(invitee_uid));
+        }
+    }
+}
+pub fn handle_invite_response(
+    server: &mut Server,
+    entity: specs::Entity,
+    response: InviteResponse,
+) {
+    match response {
+        InviteResponse::Accept => handle_invite_accept(server, entity),
+        InviteResponse::Decline => handle_invite_decline(server, entity),
+    }
+}
+
+pub fn handle_invite_accept(server: &mut Server, entity: specs::Entity) {
+    let state = server.state_mut();
+    let clients = state.ecs().read_storage::<Client>();
+    let uids = state.ecs().read_storage::<Uid>();
+    let mut invites = state.ecs().write_storage::<Invite>();
+    if let Some((inviter, kind)) = invites.remove(entity).and_then(|invite| {
+        let Invite { inviter, kind } = invite;
+        let mut pending_invites = state.ecs().write_storage::<PendingInvites>();
+        let pending = &mut pending_invites.get_mut(inviter)?.0;
+        // Check that inviter has a pending invite and remove it from the list
+        let invite_index = pending.iter().position(|p| p.0 == entity)?;
+        pending.swap_remove(invite_index);
+        // If no pending invites remain remove the component
+        if pending.is_empty() {
+            pending_invites.remove(inviter);
+        }
+
+        Some((inviter, kind))
+    }) {
+        if let (Some(client), Some(target)) = (clients.get(inviter), uids.get(entity).copied()) {
+            client.send_fallible(ServerGeneral::InviteComplete {
+                target,
+                answer: InviteAnswer::Accepted,
+                kind,
+            });
+        }
+        match kind {
+            InviteKind::Group => {
+                let mut group_manager = state.ecs().write_resource::<GroupManager>();
+                group_manager.add_group_member(
+                    inviter,
+                    entity,
+                    &state.ecs().entities(),
+                    &mut state.ecs().write_storage(),
+                    &state.ecs().read_storage(),
+                    &uids,
+                    |entity, group_change| {
+                        clients
+                            .get(entity)
+                            .and_then(|c| {
+                                group_change
+                                    .try_map(|e| uids.get(e).copied())
+                                    .map(|g| (g, c))
+                            })
+                            .map(|(g, c)| c.send(ServerGeneral::GroupUpdate(g)));
+                    },
+                );
+            },
+            InviteKind::Trade => {
+                if let (Some(inviter_uid), Some(invitee_uid)) =
+                    (uids.get(inviter).copied(), uids.get(entity).copied())
+                {
+                    let mut trades = state.ecs().write_resource::<Trades>();
+                    let id = trades.begin_trade(inviter_uid, invitee_uid);
+                    let trade = trades.trades[&id].clone();
+                    clients
+                        .get(inviter)
+                        .map(|c| c.send(ServerGeneral::UpdatePendingTrade(id, trade.clone())));
+                    clients
+                        .get(entity)
+                        .map(|c| c.send(ServerGeneral::UpdatePendingTrade(id, trade)));
+                }
+            },
+        }
+    }
+}
+
+pub fn handle_invite_decline(server: &mut Server, entity: specs::Entity) {
+    let state = server.state_mut();
+    let clients = state.ecs().read_storage::<Client>();
+    let uids = state.ecs().read_storage::<Uid>();
+    let mut invites = state.ecs().write_storage::<Invite>();
+    if let Some((inviter, kind)) = invites.remove(entity).and_then(|invite| {
+        let Invite { inviter, kind } = invite;
+        let mut pending_invites = state.ecs().write_storage::<PendingInvites>();
+        let pending = &mut pending_invites.get_mut(inviter)?.0;
+        // Check that inviter has a pending invite and remove it from the list
+        let invite_index = pending.iter().position(|p| p.0 == entity)?;
+        pending.swap_remove(invite_index);
+        // If no pending invites remain remove the component
+        if pending.is_empty() {
+            pending_invites.remove(inviter);
+        }
+
+        Some((inviter, kind))
+    }) {
+        // Inform inviter of rejection
+        if let (Some(client), Some(target)) = (clients.get(inviter), uids.get(entity).copied()) {
+            client.send_fallible(ServerGeneral::InviteComplete {
+                target,
+                answer: InviteAnswer::Declined,
+                kind,
+            });
+        }
+    }
+}

server/src/events/mod.rs

@@ -16,15 +16,17 @@ use interaction::{
     handle_lantern, handle_mount, handle_npc_interaction, handle_possess, handle_unmount,
 };
 use inventory_manip::handle_inventory;
+use invite::{handle_invite, handle_invite_response};
 use player::{handle_client_disconnect, handle_exit_ingame};
 use specs::{Entity as EcsEntity, WorldExt};
-use trade::{handle_initiate_trade, handle_process_trade_action};
+use trade::handle_process_trade_action;
 
 mod entity_creation;
 mod entity_manipulation;
 mod group_manip;
 mod interaction;
 mod inventory_manip;
+mod invite;
 mod player;
 mod trade;
 
@@ -105,11 +107,15 @@ impl Server {
                 ServerEvent::NpcInteract(interactor, target) => {
                     handle_npc_interaction(self, interactor, target)
                 },
-                ServerEvent::InitiateTrade(interactor, target) => {
+                ServerEvent::InitiateInvite(interactor, target, kind) => {
-                    handle_initiate_trade(self, interactor, target)
+                    handle_invite(self, interactor, target, kind)
+                    //handle_initiate_trade(self, interactor, target)
                 },
-                ServerEvent::ProcessTradeAction(entity, trade_id, msg) => {
+                ServerEvent::InviteResponse(entity, response) => {
-                    handle_process_trade_action(self, entity, trade_id, msg);
+                    handle_invite_response(self, entity, response)
+                },
+                ServerEvent::ProcessTradeAction(entity, trade_id, action) => {
+                    handle_process_trade_action(self, entity, trade_id, action);
                 },
                 ServerEvent::Mount(mounter, mountee) => handle_mount(self, mounter, mountee),
                 ServerEvent::Unmount(mounter) => handle_unmount(self, mounter),

server/src/events/trade.rs

@@ -1,33 +1,24 @@
-use crate::{events::group_manip::handle_invite, Server};
+use crate::Server;
 use common::{
-    comp::{group::InviteKind, inventory::Inventory},
+    comp::inventory::Inventory,
-    trade::{PendingTrade, TradeActionMsg, TradeResult, Trades},
+    trade::{PendingTrade, TradeAction, TradeId, TradeResult, Trades},
 };
 use common_net::{msg::ServerGeneral, sync::WorldSyncExt};
+use hashbrown::hash_map::Entry;
 use specs::{world::WorldExt, Entity as EcsEntity};
 use std::cmp::Ordering;
-use tracing::{error, trace, warn};
+use tracing::{error, trace};
-
-/// 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);
-    } else {
-        warn!("Entity tried to trade with an entity that lacks an uid");
-    }
-}
 
 /// Invoked when the trade UI is up, handling item changes, accepts, etc
 pub fn handle_process_trade_action(
     server: &mut Server,
     entity: EcsEntity,
-    trade_id: usize,
+    trade_id: TradeId,
-    msg: TradeActionMsg,
+    action: TradeAction,
 ) {
     if let Some(uid) = server.state.ecs().uid_from_entity(entity) {
         let mut trades = server.state.ecs().write_resource::<Trades>();
-        if let TradeActionMsg::Decline = msg {
+        if let TradeAction::Decline = action {
             let to_notify = trades.decline_trade(trade_id, uid);
             to_notify
                 .and_then(|u| server.state.ecs().entity_from_uid(u.0))
@@ -36,16 +27,19 @@ pub fn handle_process_trade_action(
                 });
         } else {
             if let Some(inv) = server.state.ecs().read_component::<Inventory>().get(entity) {
-                trades.process_trade_action(trade_id, uid, msg, inv);
+                trades.process_trade_action(trade_id, uid, action, inv);
-            }
-            if let Some(trade) = trades.trades.get(&trade_id) {
-                let mut msg = ServerGeneral::UpdatePendingTrade(trade_id, trade.clone());
-                if trade.should_commit() {
-                    let result = commit_trade(server.state.ecs(), trade);
-                    msg = ServerGeneral::FinishedTrade(result);
             }
+            if let Entry::Occupied(entry) = trades.trades.entry(trade_id) {
+                let parties = entry.get().parties;
+                let msg = if entry.get().should_commit() {
+                    let result = commit_trade(server.state.ecs(), entry.get());
+                    entry.remove();
+                    ServerGeneral::FinishedTrade(result)
+                } else {
+                    ServerGeneral::UpdatePendingTrade(trade_id, entry.get().clone())
+                };
                 // send the updated state to both parties
-                for party in trade.parties.iter() {
+                for party in parties.iter() {
                     server
                         .state
                         .ecs()
@@ -60,16 +54,16 @@ pub fn handle_process_trade_action(
 /// 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![];
+    let mut entities = Vec::new();
-    for who in [0, 1].iter().cloned() {
+    for party in trade.parties.iter() {
-        match ecs.entity_from_uid(trade.parties[who].0) {
+        match ecs.entity_from_uid(party.0) {
             Some(entity) => entities.push(entity),
             None => return TradeResult::Declined,
         }
     }
     let mut inventories = ecs.write_component::<Inventory>();
-    for who in [0, 1].iter().cloned() {
+    for entity in entities.iter() {
-        if inventories.get_mut(entities[who]).is_none() {
+        if inventories.get_mut(*entity).is_none() {
             return TradeResult::Declined;
         }
     }
@@ -106,7 +100,7 @@ fn commit_trade(ecs: &specs::World, trade: &PendingTrade) -> TradeResult {
                     delta_slots[1 - who] += 1; // overapproximation, assumes the stack won't merge
                 },
                 Ordering::Greater => {
-                    // no change to delta_slots[who], since they have leftovers
+                    // No change to delta_slots[who], since they have leftovers
                     delta_slots[1 - who] += 1; // overapproximation, assumes the stack won't merge
                 },
             }
@@ -121,10 +115,10 @@ fn commit_trade(ecs: &specs::World, trade: &PendingTrade) -> TradeResult {
             return TradeResult::NotEnoughSpace;
         }
     }
-    let mut items = [vec![], vec![]];
+    let mut items = [Vec::new(), Vec::new()];
     for who in [0, 1].iter().cloned() {
         for (slot, quantity) in trade.offers[who].iter() {
-            // take the items one by one, to benefit from Inventory's stack handling
+            // Take the items one by one, to benefit from Inventory's stack handling
             for _ in 0..*quantity {
                 inventories
                     .get_mut(entities[who])
@@ -140,10 +134,10 @@ fn commit_trade(ecs: &specs::World, trade: &PendingTrade) -> TradeResult {
             .expect(invmsg)
             .push_all(items[who].drain(..))
         {
-            // this should only happen if the arithmetic above for delta_slots says there's
+            // This should only happen if the arithmetic above for delta_slots says there's
             // enough space and there isn't (i.e. underapproximates)
-            error!(
+            panic!(
-                "Not enough space for all the items, destroying leftovers {:?}",
+                "Not enough space for all the items, leftovers are {:?}",
                 leftovers
             );
         }

server/src/sys/agent.rs

@@ -4,16 +4,16 @@ use common::{
         self,
         agent::{Activity, AgentEvent, Tactic, DEFAULT_INTERACTION_TIME},
         group,
-        group::Invite,
         inventory::slot::EquipSlot,
+        invite::{Invite, InviteResponse},
         item::{
             tool::{ToolKind, UniqueKind},
             ItemKind,
         },
         skills::{AxeSkill, BowSkill, HammerSkill, Skill, StaffSkill, SwordSkill},
         Agent, Alignment, Body, CharacterState, ControlAction, ControlEvent, Controller, Energy,
-        GroupManip, Health, Inventory, LightEmitter, MountState, Ori, PhysicsState, Pos, Scale,
+        Health, Inventory, LightEmitter, MountState, Ori, PhysicsState, Pos, Scale, Stats,
-        Stats, UnresolvedChatMsg, Vel,
+        UnresolvedChatMsg, Vel,
     },
     event::{EventBus, ServerEvent},
     metrics::SysMetrics,
@@ -1577,7 +1577,7 @@ impl<'a> System<'a> for Sys {
                 debug_assert!(inputs.look_dir.map(|e| !e.is_nan()).reduce_and());
             });
 
-        // Process group invites
+        // Process invites
         for (_invite, /*alignment,*/ agent, controller) in
             (&invites, /*&alignments,*/ &mut agents, &mut controllers).join()
         {
@@ -1587,11 +1587,11 @@ impl<'a> System<'a> for Sys {
                 *agent = Agent::default();
                 controller
                     .events
-                    .push(ControlEvent::GroupManip(GroupManip::Accept));
+                    .push(ControlEvent::InviteResponse(InviteResponse::Accept));
             } else {
                 controller
                     .events
-                    .push(ControlEvent::GroupManip(GroupManip::Decline));
+                    .push(ControlEvent::InviteResponse(InviteResponse::Decline));
             }
         }
         sys_metrics.agent_ns.store(

server/src/sys/invite_timeout.rs

@@ -1,14 +1,14 @@
 use super::SysTimer;
 use crate::client::Client;
 use common::{
-    comp::group::{Invite, PendingInvites},
+    comp::invite::{Invite, PendingInvites},
     span,
     uid::Uid,
 };
 use common_net::msg::{InviteAnswer, ServerGeneral};
 use specs::{Entities, Join, ReadStorage, System, Write, WriteStorage};
 
-/// This system removes timed out group invites
+/// This system removes timed out invites
 pub struct Sys;
 impl<'a> System<'a> for Sys {
     #[allow(clippy::type_complexity)] // TODO: Pending review in #587

server/src/sys/msg/in_game.rs

@@ -158,10 +158,6 @@ impl Sys {
                     .get_mut(entity)
                     .map(|mut s| s.skill_set.unlock_skill_group(skill_group_kind));
             },
-            ClientGeneral::UpdatePendingTrade(trade_id, msg) => {
-                tracing::info!("UpdatePendingTrade {:?} {:?}", trade_id, msg);
-                server_emitter.emit(ServerEvent::ProcessTradeAction(entity, trade_id, msg));
-            },
             _ => unreachable!("not a client_in_game msg"),
         }
         Ok(())

voxygen/src/hud/group.rs

@@ -16,10 +16,7 @@ use crate::{
 use client::{self, Client};
 use common::{
     combat,
-    comp::{
+    comp::{group::Role, invite::InviteKind, BuffKind, Stats},
-        group::{InviteKind, Role},
-        BuffKind, Stats,
-    },
     uid::{Uid, UidAllocator},
 };
 use common_net::sync::WorldSyncExt;
@@ -208,7 +205,7 @@ impl<'a> Widget for Group<'a> {
                 .unwrap_or_else(|| format!("Npc<{}>", uid)),
         };
 
-        let open_invite = self.client.group_invite();
+        let open_invite = self.client.invite();
 
         let my_uid = self.client.uid();
 

voxygen/src/hud/mod.rs

@@ -73,7 +73,7 @@ use common::{
     outcome::Outcome,
     span,
     terrain::TerrainChunk,
-    trade::TradeActionMsg,
+    trade::TradeAction,
     uid::Uid,
     util::srgba_to_linear,
     vol::RectRasterableVol,
@@ -399,7 +399,7 @@ pub enum Event {
     },
     DropSlot(comp::slot::Slot),
     ChangeHotbarState(Box<HotbarState>),
-    TradeAction(TradeActionMsg),
+    TradeAction(TradeAction),
     Ability3(bool),
     Logout,
     Quit,
@@ -2131,8 +2131,8 @@ impl Hud {
             )
             .set(self.ids.trade, ui_widgets)
             {
-                Some(msg) => {
+                Some(action) => {
-                    if let TradeActionMsg::Decline = msg {
+                    if let TradeAction::Decline = action {
                         self.show.stats = false;
                         self.show.trade(false);
                         if !self.show.social {
@@ -2142,7 +2142,7 @@ impl Hud {
                             self.force_ungrab = true
                         };
                     }
-                    events.push(Event::TradeAction(msg));
+                    events.push(Event::TradeAction(action));
                 },
                 None => {},
             }
@@ -2634,7 +2634,7 @@ impl Hud {
                         events.push(Event::ChangeHotbarState(Box::new(self.hotbar.to_owned())));
                     } else if let (Inventory(i), Trade(_)) = (a, b) {
                         if let Some(inventory) = inventories.get(entity) {
-                            events.push(Event::TradeAction(TradeActionMsg::AddItem {
+                            events.push(Event::TradeAction(TradeAction::AddItem {
                                 item: i.0,
                                 quantity: i.amount(inventory).unwrap_or(1),
                             }));
@@ -2642,7 +2642,7 @@ impl Hud {
                     } else if let (Trade(t), Inventory(_)) = (a, b) {
                         if let Some(inventory) = inventories.get(entity) {
                             if let Some(invslot) = t.invslot {
-                                events.push(Event::TradeAction(TradeActionMsg::RemoveItem {
+                                events.push(Event::TradeAction(TradeAction::RemoveItem {
                                     item: invslot,
                                     quantity: t.amount(inventory).unwrap_or(1),
                                 }));

voxygen/src/hud/trade.rs

@@ -15,7 +15,7 @@ use crate::{
 use client::Client;
 use common::{
     comp::Inventory,
-    trade::{PendingTrade, TradeActionMsg},
+    trade::{PendingTrade, TradeAction, TradePhase},
 };
 use common_net::sync::WorldSyncExt;
 use conrod_core::{
@@ -120,12 +120,10 @@ impl<'a> Trade<'a> {
         ui: &mut UiCell<'_>,
         trade: &'a PendingTrade,
     ) {
-        let phase_text = if trade.in_phase1() {
+        let phase_text = match trade.phase() {
-            self.localized_strings.get("hud.trade.phase1_description")
+            TradePhase::Mutate => self.localized_strings.get("hud.trade.phase1_description"),
-        } else if trade.in_phase2() {
+            TradePhase::Review => self.localized_strings.get("hud.trade.phase2_description"),
-            self.localized_strings.get("hud.trade.phase2_description")
+            TradePhase::Complete => self.localized_strings.get("hud.trade.phase3_description"),
-        } else {
-            self.localized_strings.get("hud.trade.phase3_description")
         };
 
         Text::new(&phase_text)
@@ -146,6 +144,7 @@ impl<'a> Trade<'a> {
         let inventories = self.client.inventories();
         let uid = trade.parties[who];
         let entity = self.client.state().ecs().entity_from_uid(uid.0)?;
+        // TODO: update in accordence with https://gitlab.com/veloren/veloren/-/issues/960
         let inventory = inventories.get(entity)?;
 
         // Alignment for Grid
@@ -177,8 +176,7 @@ impl<'a> Trade<'a> {
             .color(Color::Rgba(1.0, 1.0, 1.0, 1.0))
             .set(state.ids.offer_headers[who], ui);
 
-        let has_accepted = (trade.in_phase1() && trade.phase1_accepts[who])
+        let has_accepted = trade.accept_flags[who];
-            || (trade.in_phase2() && trade.phase2_accepts[who]);
         let accept_indicator = self
             .localized_strings
             .get("hud.trade.has_accepted")
@@ -207,7 +205,7 @@ impl<'a> Trade<'a> {
             })
             .collect();
 
-        if trade.in_phase1() {
+        if matches!(trade.phase(), TradePhase::Mutate) {
             self.phase1_itemwidget(state, ui, inventory, who, &tradeslots);
         } else {
             self.phase2_itemwidget(state, ui, inventory, who, &tradeslots);
@@ -333,11 +331,7 @@ impl<'a> Trade<'a> {
             .set(state.ids.accept_button, ui)
             .was_clicked()
         {
-            if trade.in_phase1() {
+            event = Some(TradeAction::Accept(trade.phase()));
-                event = Some(TradeActionMsg::Phase1Accept);
-            } else if trade.in_phase2() {
-                event = Some(TradeActionMsg::Phase2Accept);
-            }
         }
 
         if Button::image(self.imgs.button)
@@ -353,7 +347,7 @@ impl<'a> Trade<'a> {
             .set(state.ids.decline_button, ui)
             .was_clicked()
         {
-            event = Some(TradeActionMsg::Decline);
+            event = Some(TradeAction::Decline);
         }
         event
     }
@@ -371,7 +365,7 @@ impl<'a> Trade<'a> {
             .set(state.ids.trade_close, ui)
             .was_clicked()
         {
-            Some(TradeActionMsg::Decline)
+            Some(TradeAction::Decline)
         } else {
             None
         }
@@ -379,7 +373,7 @@ impl<'a> Trade<'a> {
 }
 
 impl<'a> Widget for Trade<'a> {
-    type Event = Option<TradeActionMsg>;
+    type Event = Option<TradeAction>;
     type State = State;
     type Style = ();
 
@@ -397,7 +391,7 @@ impl<'a> Widget for Trade<'a> {
         let mut event = None;
         let trade = match self.client.pending_trade() {
             Some((_, trade)) => trade,
-            None => return Some(TradeActionMsg::Decline),
+            None => return Some(TradeAction::Decline),
         };
 
         if state.ids.inv_alignment.len() < 2 {

voxygen/src/key_state.rs

@@ -19,7 +19,8 @@ pub struct KeyState {
     pub auto_walk: bool,
     pub swap_loadout: bool,
     pub respawn: bool,
-    pub collect: bool,
+    pub interact: bool,
+    pub trade: bool,
     pub analog_matrix: Vec2<f32>,
 }
 
@@ -44,7 +45,8 @@ impl Default for KeyState {
             auto_walk: false,
             swap_loadout: false,
             respawn: false,
-            collect: false,
+            interact: false,
+            trade: false,
             analog_matrix: Vec2::zero(),
         }
     }

voxygen/src/session.rs

@@ -10,7 +10,8 @@ use common::{
     assets::AssetExt,
     comp,
     comp::{
-        group::InviteKind, inventory::slot::Slot, ChatMsg, ChatType, InventoryUpdateEvent, Pos, Vel,
+        inventory::slot::Slot, invite::InviteKind, ChatMsg, ChatType, InventoryUpdateEvent, Pos,
+        Vel,
     },
     consts::{MAX_MOUNT_RANGE, MAX_PICKUP_RANGE},
     outcome::Outcome,
@@ -23,7 +24,10 @@ use common::{
     },
     vol::ReadVol,
 };
-use common_net::msg::{server::InviteAnswer, PresenceKind};
+use common_net::{
+    msg::{server::InviteAnswer, PresenceKind},
+    sync::WorldSyncExt,
+};
 
 use crate::{
     audio::sfx::SfxEvent,
@@ -528,9 +532,9 @@ impl PlayState for SessionState {
                         }
                     },
                     Event::InputUpdate(GameInput::Interact, state)
-                        if state != self.key_state.collect =>
+                        if state != self.key_state.interact =>
                     {
-                        self.key_state.collect = state;
+                        self.key_state.interact = state;
 
                         if state {
                             if let Some(interactable) = self.interactable {
@@ -559,9 +563,9 @@ impl PlayState for SessionState {
                         }
                     }
                     Event::InputUpdate(GameInput::Trade, state)
-                        if state != self.key_state.collect =>
+                        if state != self.key_state.trade =>
                     {
-                        self.key_state.collect = state;
+                        self.key_state.trade = state;
 
                         if state {
                             if let Some(interactable) = self.interactable {
@@ -569,17 +573,11 @@ impl PlayState for SessionState {
                                 match interactable {
                                     Interactable::Block(_, _) => {},
                                     Interactable::Entity(entity) => {
-                                        if client
+                                        client
                                             .state()
                                             .ecs()
-                                            .read_storage::<comp::Item>()
+                                            .uid_from_entity(entity)
-                                            .get(entity)
+                                            .map(|uid| client.send_invite(uid, InviteKind::Trade));
-                                            .is_some()
-                                        {
-                                            client.pick_up(entity);
-                                        } else {
-                                            client.initiate_trade(entity);
-                                        }
                                     },
                                 }
                             }
@@ -632,14 +630,14 @@ impl PlayState for SessionState {
                     },
                     Event::InputUpdate(GameInput::AcceptGroupInvite, true) => {
                         let mut client = self.client.borrow_mut();
-                        if client.group_invite().is_some() {
+                        if client.invite().is_some() {
-                            client.accept_group_invite();
+                            client.accept_invite();
                         }
                     },
                     Event::InputUpdate(GameInput::DeclineGroupInvite, true) => {
                         let mut client = self.client.borrow_mut();
-                        if client.group_invite().is_some() {
+                        if client.invite().is_some() {
-                            client.decline_group_invite();
+                            client.decline_invite();
                         }
                     },
                     Event::AnalogGameInput(input) => match input {
@@ -1150,9 +1148,9 @@ impl PlayState for SessionState {
 
                         info!("Event! -> ChangedHotbarState")
                     },
-                    HudEvent::TradeAction(msg) => {
+                    HudEvent::TradeAction(action) => {
                         let mut client = self.client.borrow_mut();
-                        client.trade_action(msg);
+                        client.perform_trade_action(action);
                     },
                     HudEvent::Ability3(state) => self.inputs.ability3.set_state(state),
                     HudEvent::ChangeFOV(new_fov) => {
@@ -1258,13 +1256,13 @@ impl PlayState for SessionState {
                         self.client.borrow_mut().craft_recipe(&r);
                     },
                     HudEvent::InviteMember(uid) => {
-                        self.client.borrow_mut().send_group_invite(uid);
+                        self.client.borrow_mut().send_invite(uid, InviteKind::Group);
                     },
                     HudEvent::AcceptInvite => {
-                        self.client.borrow_mut().accept_group_invite();
+                        self.client.borrow_mut().accept_invite();
                     },
                     HudEvent::DeclineInvite => {
-                        self.client.borrow_mut().decline_group_invite();
+                        self.client.borrow_mut().decline_invite();
                     },
                     HudEvent::KickMember(uid) => {
                         self.client.borrow_mut().kick_from_group(uid);