refactor(transaction): Implement canonical ordering in TransactionPlan

crates/bin/pcli/tests/network_integration.rs

@@ -172,15 +172,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

@@ -48,6 +48,12 @@ pub struct TransactionPlan {
 }
 
 impl TransactionPlan {
+    /// Sort the actions in [`TransactionPlan`] by type, using the protobuf field number in the [`ActionPlan`].
+    pub fn sort_actions(&mut self) {
+        self.actions
+            .sort_by_key(|action: &ActionPlan| action.variant_index());
+    }
+
     /// Computes the [`EffectHash`] for the [`Transaction`] described by this
     /// [`TransactionPlan`].
     ///
@@ -500,7 +506,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 +534,9 @@ mod tests {
             memo: Some(MemoPlan::new(&mut OsRng, memo_plaintext.clone())),
         };
 
+        // Sort actions within the transaction plan.
+        plan.sort_actions();
+
         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

@@ -119,7 +119,6 @@ impl TransactionPlan {
         witness_data: &WitnessData,
         auth_data: &AuthorizationData,
     ) -> Result<Transaction> {
-        // TODO: stream progress updates
         // 1. Build each action.
         let actions = self
             .actions

crates/view/src/planner/action_list.rs

@@ -64,6 +64,10 @@ impl ActionList {
         };
         plan.populate_detection_data(rng, fmd_params.precision_bits.into());
 
+        // Implement a canonical ordering to the actions within the transaction
+        // plan to reduce client distinguishability.
+        plan.sort_actions();
+
         Ok(plan)
     }