From 74ddead79d641175ea858e7f6a7da49dca387e6b Mon Sep 17 00:00:00 2001 From: Tal Derei Date: Tue, 7 May 2024 13:25:14 -0700 Subject: [PATCH 1/4] reorgize planner components --- .../transaction/src}/action_list.rs | 51 ++----------------- crates/core/transaction/src/lib.rs | 5 +- crates/view/src/planner.rs | 50 ++++++++++++++++-- 3 files changed, 52 insertions(+), 54 deletions(-) rename crates/{view/src/planner => core/transaction/src}/action_list.rs (82%) diff --git a/crates/view/src/planner/action_list.rs b/crates/core/transaction/src/action_list.rs similarity index 82% rename from crates/view/src/planner/action_list.rs rename to crates/core/transaction/src/action_list.rs index d866eaa692..f61f25731c 100644 --- a/crates/view/src/planner/action_list.rs +++ b/crates/core/transaction/src/action_list.rs @@ -1,18 +1,16 @@ use anyhow::Result; use std::collections::BTreeMap; +use crate::plan::MemoPlan; +use crate::{gas::GasCost, TransactionParameters}; +use crate::{ActionPlan, TransactionPlan}; use penumbra_asset::{asset, Balance}; use penumbra_fee::{Fee, FeeTier, Gas, GasPrices}; use penumbra_keys::Address; use penumbra_num::Amount; use penumbra_shielded_pool::{fmd, OutputPlan}; -use penumbra_transaction::plan::MemoPlan; -use penumbra_transaction::{gas::GasCost, TransactionParameters}; -use penumbra_transaction::{ActionPlan, TransactionPlan}; use rand_core::{CryptoRng, RngCore}; -use crate::SpendableNoteRecord; - /// A list of planned actions to be turned into a TransactionPlan. /// /// A transaction is a bundle of actions plus auxiliary data (like a memo). A @@ -230,46 +228,3 @@ impl ActionList { .retain(|_, output| output.value.amount > Amount::zero()); } } - -/// Prioritize notes to spend to release value of a specific transaction. -/// -/// Various logic is possible for note selection. Currently, this method -/// prioritizes notes sent to a one-time address, then notes with the largest -/// value: -/// -/// - Prioritizing notes sent to one-time addresses optimizes for a future in -/// which we implement DAGSync keyed by fuzzy message detection (which will not -/// be able to detect notes sent to one-time addresses). Spending these notes -/// immediately converts them into change notes, sent to the default address for -/// the users' account, which are detectable. -/// -/// - Prioritizing notes with the largest value optimizes for gas used by the -/// transaction. -/// -/// We may want to make note prioritization configurable in the future. For -/// instance, a user might prefer a note prioritization strategy that harvested -/// capital losses when possible, using cost basis information retained by the -/// view server. -pub fn prioritize_and_filter_spendable_notes( - records: Vec, -) -> Vec { - let mut filtered = records - .into_iter() - .filter(|record| record.note.amount() > Amount::zero()) - .collect::>(); - - filtered.sort_by(|a, b| { - // Sort by whether the note was sent to an ephemeral address... - match ( - a.address_index.is_ephemeral(), - b.address_index.is_ephemeral(), - ) { - (true, false) => std::cmp::Ordering::Less, - (false, true) => std::cmp::Ordering::Greater, - // ... then by largest amount. - _ => b.note.amount().cmp(&a.note.amount()), - } - }); - - filtered -} diff --git a/crates/core/transaction/src/lib.rs b/crates/core/transaction/src/lib.rs index 9925d59132..71debd5f09 100644 --- a/crates/core/transaction/src/lib.rs +++ b/crates/core/transaction/src/lib.rs @@ -26,24 +26,25 @@ mod transaction; mod witness_data; pub mod action; +pub mod action_list; pub mod gas; pub mod memo; pub mod plan; pub mod view; pub use action::Action; +pub use action_list::ActionList; pub use auth_data::AuthorizationData; pub use detection_data::DetectionData; pub use error::Error; pub use is_action::IsAction; pub use parameters::TransactionParameters; +pub use penumbra_txhash as txhash; pub use plan::{ActionPlan, TransactionPlan}; pub use transaction::{Transaction, TransactionBody}; pub use view::{ActionView, MemoPlaintextView, MemoView, TransactionPerspective, TransactionView}; pub use witness_data::WitnessData; -pub use penumbra_txhash as txhash; - /// A compatibility wrapper for trait implementations that are temporarily duplicated /// in multiple crates as an orphan rule work around until we finish splitting crates (#2288). pub struct Compat<'a, T>(pub &'a T); diff --git a/crates/view/src/planner.rs b/crates/view/src/planner.rs index 0eb99fcb58..0c55d652e9 100644 --- a/crates/view/src/planner.rs +++ b/crates/view/src/planner.rs @@ -46,11 +46,9 @@ use penumbra_transaction::{ memo::MemoPlaintext, plan::{ActionPlan, MemoPlan, TransactionPlan}, TransactionParameters, + ActionList }; -mod action_list; -use action_list::ActionList; - /// A planner for a [`TransactionPlan`] that can fill in the required spends and change outputs upon /// finalization to make a transaction balance. pub struct Planner { @@ -455,6 +453,50 @@ impl Planner { self } + /// Prioritize notes to spend to release value of a specific transaction. + /// + /// Various logic is possible for note selection. Currently, this method + /// prioritizes notes sent to a one-time address, then notes with the largest + /// value: + /// + /// - Prioritizing notes sent to one-time addresses optimizes for a future in + /// which we implement DAGSync keyed by fuzzy message detection (which will not + /// be able to detect notes sent to one-time addresses). Spending these notes + /// immediately converts them into change notes, sent to the default address for + /// the users' account, which are detectable. + /// + /// - Prioritizing notes with the largest value optimizes for gas used by the + /// transaction. + /// + /// We may want to make note prioritization configurable in the future. For + /// instance, a user might prefer a note prioritization strategy that harvested + /// capital losses when possible, using cost basis information retained by the + /// view server. + pub fn prioritize_and_filter_spendable_notes( + &mut self, + records: Vec, + ) -> Vec { + let mut filtered = records + .into_iter() + .filter(|record| record.note.amount() > Amount::zero()) + .collect::>(); + + filtered.sort_by(|a, b| { + // Sort by whether the note was sent to an ephemeral address... + match ( + a.address_index.is_ephemeral(), + b.address_index.is_ephemeral(), + ) { + (true, false) => std::cmp::Ordering::Less, + (false, true) => std::cmp::Ordering::Greater, + // ... then by largest amount. + _ => b.note.amount().cmp(&a.note.amount()), + } + }); + + filtered + } + /// Add spends and change outputs as required to balance the transaction, using the view service /// provided to supply the notes and other information. pub async fn plan( @@ -507,7 +549,7 @@ impl Planner { required.asset_id, // TODO: reorganize later, the important thing here is that this // is separate from the Planner logic (will never be commonized) - action_list::prioritize_and_filter_spendable_notes(records), + self.prioritize_and_filter_spendable_notes(records), ); } From b482d643dd3f522e47b8cbdfa3ab73fe7441aed6 Mon Sep 17 00:00:00 2001 From: Tal Derei Date: Tue, 7 May 2024 13:29:53 -0700 Subject: [PATCH 2/4] filter for largest notes --- crates/view/src/planner.rs | 17 +++-------------- 1 file changed, 3 insertions(+), 14 deletions(-) diff --git a/crates/view/src/planner.rs b/crates/view/src/planner.rs index 0c55d652e9..b5480fe84d 100644 --- a/crates/view/src/planner.rs +++ b/crates/view/src/planner.rs @@ -472,27 +472,16 @@ impl Planner { /// instance, a user might prefer a note prioritization strategy that harvested /// capital losses when possible, using cost basis information retained by the /// view server. - pub fn prioritize_and_filter_spendable_notes( - &mut self, + fn prioritize_and_filter_spendable_notes( records: Vec, ) -> Vec { + // Filter out zero valued notes. let mut filtered = records .into_iter() .filter(|record| record.note.amount() > Amount::zero()) .collect::>(); - filtered.sort_by(|a, b| { - // Sort by whether the note was sent to an ephemeral address... - match ( - a.address_index.is_ephemeral(), - b.address_index.is_ephemeral(), - ) { - (true, false) => std::cmp::Ordering::Less, - (false, true) => std::cmp::Ordering::Greater, - // ... then by largest amount. - _ => b.note.amount().cmp(&a.note.amount()), - } - }); + filtered.sort_by(|a, b| b.note.amount().cmp(&a.note.amount())); filtered } From 695cf943966aebacc7924eed2a8cfb37173e9d53 Mon Sep 17 00:00:00 2001 From: Tal Derei Date: Tue, 7 May 2024 13:37:30 -0700 Subject: [PATCH 3/4] fix --- crates/view/src/planner.rs | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/crates/view/src/planner.rs b/crates/view/src/planner.rs index b5480fe84d..8094dbee36 100644 --- a/crates/view/src/planner.rs +++ b/crates/view/src/planner.rs @@ -45,8 +45,7 @@ use penumbra_tct as tct; use penumbra_transaction::{ memo::MemoPlaintext, plan::{ActionPlan, MemoPlan, TransactionPlan}, - TransactionParameters, - ActionList + ActionList, TransactionParameters, }; /// A planner for a [`TransactionPlan`] that can fill in the required spends and change outputs upon @@ -473,6 +472,7 @@ impl Planner { /// capital losses when possible, using cost basis information retained by the /// view server. fn prioritize_and_filter_spendable_notes( + &mut self, records: Vec, ) -> Vec { // Filter out zero valued notes. @@ -536,8 +536,6 @@ impl Planner { .await?; notes_by_asset_id.insert( required.asset_id, - // TODO: reorganize later, the important thing here is that this - // is separate from the Planner logic (will never be commonized) self.prioritize_and_filter_spendable_notes(records), ); } From 32942222f856b8699d86fb819d1c215d2ebaa6b5 Mon Sep 17 00:00:00 2001 From: Tal Derei Date: Tue, 7 May 2024 14:14:21 -0700 Subject: [PATCH 4/4] restore note prioritization logic --- crates/view/src/planner.rs | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/crates/view/src/planner.rs b/crates/view/src/planner.rs index 8094dbee36..dc50a9f600 100644 --- a/crates/view/src/planner.rs +++ b/crates/view/src/planner.rs @@ -471,18 +471,26 @@ impl Planner { /// instance, a user might prefer a note prioritization strategy that harvested /// capital losses when possible, using cost basis information retained by the /// view server. - fn prioritize_and_filter_spendable_notes( + pub fn prioritize_and_filter_spendable_notes( &mut self, records: Vec, ) -> Vec { - // Filter out zero valued notes. let mut filtered = records .into_iter() .filter(|record| record.note.amount() > Amount::zero()) .collect::>(); - - filtered.sort_by(|a, b| b.note.amount().cmp(&a.note.amount())); - + filtered.sort_by(|a, b| { + // Sort by whether the note was sent to an ephemeral address... + match ( + a.address_index.is_ephemeral(), + b.address_index.is_ephemeral(), + ) { + (true, false) => std::cmp::Ordering::Less, + (false, true) => std::cmp::Ordering::Greater, + // ... then by largest amount. + _ => b.note.amount().cmp(&a.note.amount()), + } + }); filtered }