dex: replace PositionRewardClaim by repeated PositionWithdrawal

crates/bin/pcli/src/command/view/balance.rs

@@ -44,11 +44,14 @@ impl BalanceCmd {
                         })
                     })
                 })
+                /* Don't exclude withdrawn LPNFTs in by_note, which is a more precise view.
                 // Exclude withdrawn LPNFTs.
                 .filter(|(_, value, _, _)| match asset_cache.get(&value.asset_id) {
                     None => true,
                     Some(denom) => !denom.is_withdrawn_position_nft(),
                 });
+                 */
+                ;
 
             for (index, value, source, return_address) in rows {
                 table.add_row(vec![

crates/bin/pcli/src/transaction_view_ext.rs

@@ -340,9 +340,6 @@ impl TransactionViewExt for TransactionView {
                 penumbra_transaction::ActionView::PositionWithdraw(_) => {
                     ["Withdraw Liquitity Position", ""]
                 }
-                penumbra_transaction::ActionView::PositionRewardClaim(_) => {
-                    ["Claim Liquitity Position Reward", ""]
-                }
                 penumbra_transaction::ActionView::ProposalDepositClaim(proposal_deposit_claim) => {
                     action = format!(
                         "Claim Deposit for Governance Proposal #{}",

crates/core/app/src/action_handler/actions.rs

@@ -33,7 +33,6 @@ impl ActionHandler for Action {
             Action::ValidatorVote(action) => action.check_stateless(()).await,
             Action::PositionClose(action) => action.check_stateless(()).await,
             Action::PositionOpen(action) => action.check_stateless(()).await,
-            Action::PositionRewardClaim(action) => action.check_stateless(()).await,
             Action::PositionWithdraw(action) => action.check_stateless(()).await,
             Action::ProposalSubmit(action) => action.check_stateless(()).await,
             Action::ProposalWithdraw(action) => action.check_stateless(()).await,
@@ -64,7 +63,6 @@ impl ActionHandler for Action {
             Action::ValidatorVote(action) => action.check_stateful(state).await,
             Action::PositionClose(action) => action.check_stateful(state).await,
             Action::PositionOpen(action) => action.check_stateful(state).await,
-            Action::PositionRewardClaim(action) => action.check_stateful(state).await,
             Action::PositionWithdraw(action) => action.check_stateful(state).await,
             Action::ProposalSubmit(action) => action.check_stateful(state).await,
             Action::ProposalWithdraw(action) => action.check_stateful(state).await,
@@ -101,7 +99,6 @@ impl ActionHandler for Action {
             Action::ValidatorVote(action) => action.execute(state).await,
             Action::PositionClose(action) => action.execute(state).await,
             Action::PositionOpen(action) => action.execute(state).await,
-            Action::PositionRewardClaim(action) => action.execute(state).await,
             Action::PositionWithdraw(action) => action.execute(state).await,
             Action::ProposalSubmit(action) => action.execute(state).await,
             Action::ProposalWithdraw(action) => action.execute(state).await,

crates/core/app/src/action_handler/actions/submit.rs

@@ -123,7 +123,6 @@ impl ActionHandler for ProposalSubmit {
                         | PositionOpen(_)
                         | PositionClose(_)
                         | PositionWithdraw(_)
-                        | PositionRewardClaim(_)
                         | CommunityPoolSpend(_)
                         | CommunityPoolOutput(_)
                         | Ics20Withdrawal(_)

crates/core/asset/src/asset/registry.rs

@@ -382,7 +382,7 @@ pub static REGISTRY: Lazy<Registry> = Lazy::new(|| {
             // Note: this regex must be in sync with LpNft::try_from
             // and the bech32 prefix for LP IDs defined in the proto crate.
             // TODO: this doesn't restrict the length of the bech32 encoding
-            "^lpnft_(?P<data>[a-z]+_plpid1[a-zA-HJ-NP-Z0-9]+)$",
+            "^lpnft_(?P<data>[a-z_0-9]+_plpid1[a-zA-HJ-NP-Z0-9]+)$",
             &[ /* no display units - nft, unit 1 */ ],
             (|data: &str| {
                 assert!(!data.is_empty());

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

@@ -1,4 +1,3 @@
 mod close;
 mod open;
-mod reward_claim;
 mod withdraw;

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

@@ -11,7 +11,7 @@ use penumbra_proto::StateWriteProto;
 use crate::{
     component::{PositionManager, PositionRead},
     event,
-    lp::{action::PositionWithdraw, position},
+    lp::{action::PositionWithdraw, position, Reserves},
 };
 
 #[async_trait]
@@ -24,16 +24,35 @@ impl ActionHandler for PositionWithdraw {
         Ok(())
     }
 
-    async fn check_stateful<S: StateRead + 'static>(&self, state: Arc<S>) -> Result<()> {
-        // Check that the committed reserves in the action match the state.
-        let position = state
+    async fn check_stateful<S: StateRead + 'static>(&self, _state: Arc<S>) -> Result<()> {
+        // Nothing to do here: we defer consistency checks on the reserves to
+        // execution, to avoid having to reason about parallellism in checks.
+        Ok(())
+    }
+
+    async fn execute<S: StateWrite>(&self, mut state: S) -> Result<()> {
+        // See comment in check_stateful for why we check the position state here:
+        // 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))?;
 
-        let expected_reserves_commitment = position
+        // First, 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(&position.phi.pair)
+            .balance(&metadata.phi.pair)
             .commit(Fr::zero());
 
         if self.reserves_commitment != expected_reserves_commitment {
@@ -44,33 +63,50 @@ impl ActionHandler for PositionWithdraw {
             );
         }
 
-        // We don't check that the position state is Closed here, because all
-        // stateful checks run in parallel, and we don't want to prevent someone
-        // from closing and withdrawing a position in one transaction (though
-        // they'll only be able to do so if the reserves don't shift before
-        // submission).
-
-        Ok(())
-    }
-
-    async fn execute<S: StateWrite>(&self, mut state: S) -> Result<()> {
-        // See comment in check_stateful for why we check the position state here.
-        let mut metadata = state
-            .position_by_id(&self.position_id)
-            .await?
-            .ok_or_else(|| anyhow!("withdrew from unknown position {}", self.position_id))?;
-
-        if metadata.state != position::State::Closed {
-            anyhow::bail!(
-                "attempted to withdraw position {} with state {}, expected Closed",
-                self.position_id,
-                metadata.state
-            );
+        // 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));
 
-        metadata.state = position::State::Withdrawn;
+        // 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/position_manager.rs

@@ -412,7 +412,7 @@ pub(crate) trait Inner: StateWrite {
 
                 (new_a_from_b, current_a_from_b)
             }
-            (State::Withdrawn, _) | (State::Claimed, _) | (State::Closed, None) => {
+            (State::Withdrawn { .. }, _) | (State::Closed, None) => {
                 // The position already went through the `Closed` state or was opened in the `Closed` state, so its contribution has already been subtracted.
                 return Ok(());
             }
@@ -554,7 +554,7 @@ pub(crate) trait Inner: StateWrite {
                 // The position is closed, so the change is the negative of the previous reserves.
                 (-old_a, -old_b)
             }
-            (State::Withdrawn, _) | (State::Claimed, _) | (State::Closed, None) => {
+            (State::Withdrawn { .. }, _) | (State::Closed, None) => {
                 // The position already went through the `Closed` state or was opened in the `Closed` state, so its contribution has already been subtracted.
                 return Ok(());
             }

crates/core/component/dex/src/event.rs

@@ -1,7 +1,7 @@
 use crate::{
     lp::{
         action::{PositionClose, PositionOpen, PositionWithdraw},
-        position::Position,
+        position::{self, Position},
     },
     swap::Swap,
     swap_claim::SwapClaim,
@@ -50,11 +50,17 @@ pub fn position_withdraw(
     position_withdraw: &PositionWithdraw,
     final_position_state: &Position,
 ) -> pb::EventPositionWithdraw {
+    let sequence = if let position::State::Withdrawn { sequence, .. } = final_position_state.state {
+        sequence + 1
+    } else {
+        0
+    };
     pb::EventPositionWithdraw {
         position_id: Some(position_withdraw.position_id.into()),
         trading_pair: Some(final_position_state.phi.pair.into()),
         reserves_1: Some(final_position_state.reserves.r1.into()),
         reserves_2: Some(final_position_state.reserves.r2.into()),
+        sequence,
     }
 }
 

crates/core/component/dex/src/lib.rs

@@ -20,6 +20,6 @@ pub mod lp;
 pub mod swap;
 pub mod swap_claim;
 
-pub use lp::action::{PositionClose, PositionOpen, PositionRewardClaim, PositionWithdraw};
+pub use lp::action::{PositionClose, PositionOpen, PositionWithdraw};
 pub use swap::Swap;
 pub use swap_claim::SwapClaim;

crates/core/component/dex/src/lp/action.rs

@@ -95,6 +95,8 @@ pub struct PositionWithdraw {
     ///
     /// The chain will check this commitment by recomputing it with the on-chain state.
     pub reserves_commitment: balance::Commitment,
+    /// The sequence number of the withdrawal, allowing multiple withdrawals from the same position.
+    pub sequence: u64,
 }
 
 impl EffectingData for PositionWithdraw {
@@ -103,27 +105,6 @@ impl EffectingData for PositionWithdraw {
     }
 }
 
-/// A transaction action that claims retroactive rewards for a historical
-/// position.
-///
-/// This action's contribution to the transaction's value balance is to consume a
-/// withdrawn position NFT and contribute its reward balance.
-#[derive(Debug, Clone, Serialize, Deserialize)]
-#[serde(try_from = "pb::PositionRewardClaim", into = "pb::PositionRewardClaim")]
-pub struct PositionRewardClaim {
-    pub position_id: position::Id,
-    /// A transparent (zero blinding factor) commitment to the position's accumulated rewards.
-    ///
-    /// The chain will check this commitment by recomputing it with the on-chain state.
-    pub rewards_commitment: balance::Commitment,
-}
-
-impl EffectingData for PositionRewardClaim {
-    fn effect_hash(&self) -> EffectHash {
-        EffectHash::from_proto_effecting_data(&self.to_proto())
-    }
-}
-
 impl DomainType for PositionOpen {
     type Proto = pb::PositionOpen;
 }
@@ -183,6 +164,7 @@ impl From<PositionWithdraw> for pb::PositionWithdraw {
         Self {
             position_id: Some(value.position_id.into()),
             reserves_commitment: Some(value.reserves_commitment.into()),
+            sequence: value.sequence,
         }
     }
 }
@@ -200,36 +182,7 @@ impl TryFrom<pb::PositionWithdraw> for PositionWithdraw {
                 .reserves_commitment
                 .ok_or_else(|| anyhow::anyhow!("missing balance_commitment"))?
                 .try_into()?,
-        })
-    }
-}
-
-impl DomainType for PositionRewardClaim {
-    type Proto = pb::PositionRewardClaim;
-}
-
-impl From<PositionRewardClaim> for pb::PositionRewardClaim {
-    fn from(value: PositionRewardClaim) -> Self {
-        Self {
-            position_id: Some(value.position_id.into()),
-            rewards_commitment: Some(value.rewards_commitment.into()),
-        }
-    }
-}
-
-impl TryFrom<pb::PositionRewardClaim> for PositionRewardClaim {
-    type Error = anyhow::Error;
-
-    fn try_from(value: pb::PositionRewardClaim) -> Result<Self, Self::Error> {
-        Ok(Self {
-            position_id: value
-                .position_id
-                .ok_or_else(|| anyhow::anyhow!("missing position_id"))?
-                .try_into()?,
-            rewards_commitment: value
-                .rewards_commitment
-                .ok_or_else(|| anyhow::anyhow!("missing balance_commitment"))?
-                .try_into()?,
+            sequence: value.sequence,
         })
     }
 }