refactor(transaction): Implement canonical ordering in TransactionPlan

crates/bin/pcli/tests/network_integration.rs

@@ -170,15 +170,11 @@ fn transaction_send_from_addr_0_to_addr_1() {
 
     let tvp: ProtoTransactionView = serde_json::value::from_value(view_json).unwrap();
     let tv: TransactionView = tvp.try_into().unwrap();
-    // TODO: the first may no longer be a spend because of ordering changes.
-    // Let's not try to fix this at the moment. Later we can put a "canonical ordering" into the planner.
-    /*
-    // There will be a lot of ActionViews in the body... let's just check that one is a Spend.
+
     assert!(matches!(
         &tv.body_view.action_views[0],
         penumbra_transaction::ActionView::Spend(_)
     ));
-     */
 
     // Inspect the TransactionView and ensure that we can read the memo text.
     let mv = tv

crates/core/transaction/src/action.rs

@@ -127,6 +127,36 @@ impl Action {
             }
         }
     }
+
+    /// Canonical action ordering according to protobuf definitions
+    pub fn variant_index(&self) -> usize {
+        match self {
+            Action::Spend(_) => 1,
+            Action::Output(_) => 2,
+            Action::Swap(_) => 3,
+            Action::SwapClaim(_) => 4,
+            Action::ValidatorDefinition(_) => 16,
+            Action::IbcRelay(_) => 17,
+            Action::ProposalSubmit(_) => 18,
+            Action::ProposalWithdraw(_) => 19,
+            Action::ValidatorVote(_) => 20,
+            Action::DelegatorVote(_) => 21,
+            Action::ProposalDepositClaim(_) => 22,
+            Action::PositionOpen(_) => 30,
+            Action::PositionClose(_) => 31,
+            Action::PositionWithdraw(_) => 32,
+            Action::Delegate(_) => 40,
+            Action::Undelegate(_) => 41,
+            Action::UndelegateClaim(_) => 42,
+            Action::CommunityPoolSpend(_) => 50,
+            Action::CommunityPoolOutput(_) => 51,
+            Action::CommunityPoolDeposit(_) => 52,
+            Action::Ics20Withdrawal(_) => 200,
+            Action::ActionDutchAuctionSchedule(_) => 53,
+            Action::ActionDutchAuctionEnd(_) => 54,
+            Action::ActionDutchAuctionWithdraw(_) => 55,
+        }
+    }
 }
 
 impl IsAction for Action {

crates/core/transaction/src/plan.rs

@@ -1,6 +1,7 @@
 //! Declarative transaction plans, used for transaction authorization and
 //! creation.
 
+use crate::action::Action;
 use anyhow::Result;
 use penumbra_community_pool::{CommunityPoolDeposit, CommunityPoolOutput, CommunityPoolSpend};
 use penumbra_dex::{
@@ -48,6 +49,20 @@ pub struct TransactionPlan {
 }
 
 impl TransactionPlan {
+    /// Sort the actions by type, using the protobuf field number in the [`Action`].
+    pub fn sort_actions(&self, mut actions: Vec<Action>) -> Vec<Action> {
+        actions.sort_by_key(|action: &Action| action.variant_index());
+
+        actions
+    }
+
+    /// Sort the action plans by type, using the protobuf field number in the [`ActionPlan`].
+    pub fn sort_action_plans(&self, mut actions_plans: Vec<ActionPlan>) -> Vec<ActionPlan> {
+        actions_plans.sort_by_key(|action: &ActionPlan| action.variant_index());
+
+        actions_plans
+    }
+
     /// Computes the [`EffectHash`] for the [`Transaction`] described by this
     /// [`TransactionPlan`].
     ///
@@ -433,7 +448,7 @@ mod tests {
     use crate::{
         memo::MemoPlaintext,
         plan::{CluePlan, DetectionDataPlan, MemoPlan, TransactionPlan},
-        TransactionParameters, WitnessData,
+        ActionPlan, TransactionParameters, WitnessData,
     };
 
     /// This isn't an exhaustive test, but we don't currently have a
@@ -500,7 +515,7 @@ mod tests {
         let mut rng = OsRng;
 
         let memo_plaintext = MemoPlaintext::new(Address::dummy(&mut rng), "".to_string()).unwrap();
-        let plan = TransactionPlan {
+        let mut plan: TransactionPlan = TransactionPlan {
             // Put outputs first to check that the auth hash
             // computation is not affected by plan ordering.
             actions: vec![
@@ -528,6 +543,10 @@ mod tests {
             memo: Some(MemoPlan::new(&mut OsRng, memo_plaintext.clone()).unwrap()),
         };
 
+        // Implement canonical ordering to the action plans to reduce client distinguishability.
+        plan.actions
+            .sort_by_key(|action: &ActionPlan| action.variant_index());
+
         println!("{}", serde_json::to_string_pretty(&plan).unwrap());
 
         let plan_effect_hash = plan.effect_hash(fvk).unwrap();

crates/core/transaction/src/plan/action.rs

@@ -168,6 +168,36 @@ impl ActionPlan {
         })
     }
 
+    /// Canonical action plan ordering according to protobuf definitions
+    pub fn variant_index(&self) -> usize {
+        match self {
+            ActionPlan::Spend(_) => 1,
+            ActionPlan::Output(_) => 2,
+            ActionPlan::Swap(_) => 3,
+            ActionPlan::SwapClaim(_) => 4,
+            ActionPlan::ValidatorDefinition(_) => 16,
+            ActionPlan::IbcAction(_) => 17,
+            ActionPlan::ProposalSubmit(_) => 18,
+            ActionPlan::ProposalWithdraw(_) => 19,
+            ActionPlan::ValidatorVote(_) => 20,
+            ActionPlan::DelegatorVote(_) => 21,
+            ActionPlan::ProposalDepositClaim(_) => 22,
+            ActionPlan::PositionOpen(_) => 30,
+            ActionPlan::PositionClose(_) => 31,
+            ActionPlan::PositionWithdraw(_) => 32,
+            ActionPlan::Delegate(_) => 40,
+            ActionPlan::Undelegate(_) => 41,
+            ActionPlan::UndelegateClaim(_) => 42,
+            ActionPlan::CommunityPoolSpend(_) => 50,
+            ActionPlan::CommunityPoolOutput(_) => 51,
+            ActionPlan::CommunityPoolDeposit(_) => 52,
+            ActionPlan::Ics20Withdrawal(_) => 200,
+            ActionPlan::ActionDutchAuctionSchedule(_) => 53,
+            ActionPlan::ActionDutchAuctionEnd(_) => 54,
+            ActionPlan::ActionDutchAuctionWithdraw(_) => 55,
+        }
+    }
+
     pub fn balance(&self) -> Balance {
         use ActionPlan::*;
 

crates/core/transaction/src/plan/build.rs

@@ -15,7 +15,7 @@ impl TransactionPlan {
     /// [`ActionPlan`]s in the TransactionPlan.
     pub fn build_unauth_with_actions(
         self,
-        actions: Vec<Action>,
+        mut actions: Vec<Action>,
         witness_data: &WitnessData,
     ) -> Result<Transaction> {
         // Add the memo if it is planned.
@@ -27,6 +27,9 @@ impl TransactionPlan {
 
         let detection_data = self.detection_data.as_ref().map(|x| x.detection_data());
 
+        // Implement canonical ordering to the actions to reduce client distinguishability.
+        actions = self.sort_actions(actions);
+
         let transaction_body = TransactionBody {
             actions,
             transaction_parameters: self.transaction_parameters,