dex: streamline PositionManager

crates/core/component/dex/src/component/action_handler/position/open.rs

@@ -2,11 +2,9 @@ use anyhow::Result;
 use async_trait::async_trait;
 use cnidarium::StateWrite;
 use cnidarium_component::ActionHandler;
-use penumbra_proto::StateWriteProto as _;
 
 use crate::{
-    component::{PositionManager, PositionRead, ValueCircuitBreaker},
-    event,
+    component::PositionManager,
     lp::{action::PositionOpen, position},
 };
 
@@ -31,17 +29,7 @@ impl ActionHandler for PositionOpen {
     }
 
     async fn check_and_execute<S: StateWrite>(&self, mut state: S) -> Result<()> {
-        // Validate that the position ID doesn't collide
-        state.check_position_id_unused(&self.position.id()).await?;
-
-        // Credit the DEX for the inflows from this position.
-        // TODO: in a future PR, split current PositionManager to PositionManagerInner
-        // and fold this into a position open method
-        state.vcb_credit(self.position.reserves_1()).await?;
-        state.vcb_credit(self.position.reserves_2()).await?;
-
-        state.put_position(self.position.clone()).await?;
-        state.record_proto(event::position_open(self));
+        state.open_position(self.position.clone()).await?;
         Ok(())
     }
 }

crates/core/component/dex/src/component/action_handler/position/withdraw.rs

@@ -1,16 +1,11 @@
-use anyhow::{anyhow, Result};
+use anyhow::Result;
 use ark_ff::Zero;
 use async_trait::async_trait;
 use cnidarium::StateWrite;
 use cnidarium_component::ActionHandler;
 use decaf377::Fr;
-use penumbra_proto::StateWriteProto;
 
-use crate::{
-    component::{PositionManager, PositionRead, ValueCircuitBreaker},
-    event,
-    lp::{action::PositionWithdraw, position, Reserves},
-};
+use crate::{component::PositionManager, lp::action::PositionWithdraw};
 
 #[async_trait]
 /// Debits a closed position NFT and credits a withdrawn position NFT and the final reserves.
@@ -27,25 +22,22 @@ impl ActionHandler for PositionWithdraw {
         // we need to ensure that we're checking the reserves at the moment we execute
         // the withdrawal, to prevent any possibility of TOCTOU attacks.
 
-        let mut metadata = state
-            .position_by_id(&self.position_id)
-            .await?
-            .ok_or_else(|| anyhow!("withdrew from unknown position {}", self.position_id))?;
+        // Execute the withdrawal, extracting the reserves from the position.
+        let actual_reserves = state
+            .withdraw_position(self.position_id, self.sequence)
+            .await?;
 
-        // First, check that the commitment to the amount the user is
+        // Next, and CRITICALLY, check that the commitment to the amount the user is
         // withdrawing is correct.
         //
         // Unlike other actions, where a balance commitment is used for
         // shielding a value, this commitment is used for compression, giving a
         // single commitment rather than a list of token amounts.
-
+        //
         // Note: since this is forming a commitment only to the reserves,
         // we are implicitly setting the reward amount to 0. However, we can
         // add support for rewards in the future without client changes.
-        let expected_reserves_commitment = metadata
-            .reserves
-            .balance(&metadata.phi.pair)
-            .commit(Fr::zero());
+        let expected_reserves_commitment = actual_reserves.commit(Fr::zero());
 
         if self.reserves_commitment != expected_reserves_commitment {
             anyhow::bail!(
@@ -55,58 +47,6 @@ impl ActionHandler for PositionWithdraw {
             );
         }
 
-        // Next, check that the withdrawal is consistent with the position state.
-        // This should be redundant with the value balance mechanism (clients should
-        // only be able to get the required input LPNFTs if the state transitions are
-        // consistent), but we check it here for defense in depth.
-        if self.sequence == 0 {
-            if metadata.state != position::State::Closed {
-                anyhow::bail!(
-                    "attempted to withdraw position {} with state {}, expected Closed",
-                    self.position_id,
-                    metadata.state
-                );
-            }
-        } else {
-            if let position::State::Withdrawn { sequence } = metadata.state {
-                if sequence + 1 != self.sequence {
-                    anyhow::bail!(
-                        "attempted to withdraw position {} with sequence {}, expected {}",
-                        self.position_id,
-                        self.sequence,
-                        sequence + 1
-                    );
-                }
-            } else {
-                anyhow::bail!(
-                    "attempted to withdraw position {} with state {}, expected Withdrawn",
-                    self.position_id,
-                    metadata.state
-                );
-            }
-        }
-
-        // Record an event prior to updating the position state, so we have access to
-        // the current reserves.
-        state.record_proto(event::position_withdraw(self, &metadata));
-
-        // Debit the DEX for the outflows from this position.
-        // TODO: in a future PR, split current PositionManager to PositionManagerInner
-        // and fold this into a position open method
-        state.vcb_debit(metadata.reserves_1()).await?;
-        state.vcb_debit(metadata.reserves_2()).await?;
-
-        // Finally, update the position. This has two steps:
-        // - update the state with the correct sequence number;
-        // - zero out the reserves, to prevent double-withdrawals.
-        metadata.state = position::State::Withdrawn {
-            // We just checked that the supplied sequence number is incremented by 1 from prev.
-            sequence: self.sequence,
-        };
-        metadata.reserves = Reserves::zero();
-
-        state.put_position(metadata).await?;
-
         Ok(())
     }
 }

crates/core/component/dex/src/component/circuit_breaker/value.rs

@@ -52,7 +52,7 @@ mod tests {
     use crate::component::{StateReadExt as _, StateWriteExt as _};
     use crate::lp::plan::PositionWithdrawPlan;
     use crate::{
-        component::{router::limit_buy, tests::TempStorageExt, PositionManager as _},
+        component::{router::limit_buy, tests::TempStorageExt},
         state_key, DirectedUnitPair,
     };
     use crate::{BatchSwapOutputData, PositionOpen};
@@ -224,7 +224,7 @@ mod tests {
 
         let id = buy_1.id();
 
-        let position = state_tx.handle_limit_order(&None, buy_1);
+        let position = buy_1;
         state_tx.index_position_by_price(&position);
         state_tx
             .update_available_liquidity(&position, &None)