diff --git a/.changelog/unreleased/bug-fixes/4287-consolidate-dust.md b/.changelog/unreleased/bug-fixes/4287-consolidate-dust.md new file mode 100644 index 0000000000..397ee574c4 --- /dev/null +++ b/.changelog/unreleased/bug-fixes/4287-consolidate-dust.md @@ -0,0 +1,2 @@ +- Fix underestimation of MASP balances + ([\#4287](https://github.com/anoma/namada/pull/4287)) \ No newline at end of file diff --git a/.changelog/unreleased/improvements/4288-reverse-conversion-at-source.md b/.changelog/unreleased/improvements/4288-reverse-conversion-at-source.md new file mode 100644 index 0000000000..48d7298fdd --- /dev/null +++ b/.changelog/unreleased/improvements/4288-reverse-conversion-at-source.md @@ -0,0 +1,2 @@ +- Simplify exchanging balance amounts to older epochs + ([\#4288](https://github.com/anoma/namada/pull/4288)) \ No newline at end of file diff --git a/crates/apps_lib/src/client/rpc.rs b/crates/apps_lib/src/client/rpc.rs index 22e1cafd62..53dd1b29d1 100644 --- a/crates/apps_lib/src/client/rpc.rs +++ b/crates/apps_lib/src/client/rpc.rs @@ -524,7 +524,6 @@ async fn query_shielded_balance( context.client(), context.io(), &viewing_key, - masp_epoch, ) .await .unwrap() diff --git a/crates/core/src/masp.rs b/crates/core/src/masp.rs index caac4ccde8..9bfa48f9da 100644 --- a/crates/core/src/masp.rs +++ b/crates/core/src/masp.rs @@ -201,24 +201,35 @@ impl AssetData { /// Give this pre-asset type the given epoch if already has an epoch. Return /// the replaced value. - pub fn redate(&mut self, to: MaspEpoch) { + pub fn redate(self, to: MaspEpoch) -> Self { if self.epoch.is_some() { - self.epoch = Some(to); + Self { + epoch: Some(to), + ..self + } + } else { + self } } /// Update the MaspEpoch to the next one - pub fn redate_to_next_epoch(&mut self) { - if let Some(ep) = self.epoch.as_mut() { - if let Some(next) = ep.next() { - *ep = next; + pub fn redate_to_next_epoch(self) -> Self { + if let Some(next) = self.epoch.as_ref().and_then(MaspEpoch::next) { + Self { + epoch: Some(next), + ..self } + } else { + self } } /// Remove the epoch associated with this pre-asset type - pub fn undate(&mut self) { - self.epoch = None; + pub fn undate(self) -> Self { + Self { + epoch: None, + ..self + } } } @@ -1128,23 +1139,23 @@ mod test { #[test] fn test_masp_asset_data_basics() { - let mut data = AssetData { + let data = AssetData { token: address::testing::nam(), denom: Denomination(6), position: MaspDigitPos::One, epoch: None, }; - data.undate(); + let data = data.undate(); assert!(data.epoch.is_none()); let epoch_0 = MaspEpoch::new(3); - data.redate(epoch_0); + let mut data = data.redate(epoch_0); assert!(data.epoch.is_none()); data.epoch = Some(epoch_0); let epoch_1 = MaspEpoch::new(5); - data.redate(epoch_1); + let data = data.redate(epoch_1); assert_eq!(data.epoch, Some(epoch_1)); } diff --git a/crates/shielded_token/src/masp.rs b/crates/shielded_token/src/masp.rs index d1203fe02d..b8a708126e 100644 --- a/crates/shielded_token/src/masp.rs +++ b/crates/shielded_token/src/masp.rs @@ -297,11 +297,6 @@ pub type MaspAmount = ValueSum<(Option, Address), token::Change>; /// transaction pub type SpentNotesTracker = HashMap>; -/// An extension of Option's cloned method for pair types -fn cloned_pair((a, b): (&T, &U)) -> (T, U) { - (a.clone(), b.clone()) -} - /// Represents the amount used of different conversions pub type Conversions = BTreeMap, i128)>; diff --git a/crates/shielded_token/src/masp/shielded_wallet.rs b/crates/shielded_token/src/masp/shielded_wallet.rs index 65ce440d85..5c77b2ef0b 100644 --- a/crates/shielded_token/src/masp/shielded_wallet.rs +++ b/crates/shielded_token/src/masp/shielded_wallet.rs @@ -49,15 +49,15 @@ use rand::prelude::StdRng; use rand_core::{OsRng, SeedableRng}; use crate::masp::utils::MaspClient; -use crate::masp::{ - cloned_pair, ContextSyncStatus, Conversions, MaspAmount, MaspDataLogEntry, - MaspFeeData, MaspSourceTransferData, MaspTargetTransferData, - MaspTransferData, MaspTxReorderedData, NoteIndex, ShieldedSyncConfig, - ShieldedTransfer, ShieldedUtils, SpentNotesTracker, TransferErr, WalletMap, - WitnessMap, NETWORK, -}; #[cfg(any(test, feature = "testing"))] use crate::masp::{testing, ENV_VAR_MASP_TEST_SEED}; +use crate::masp::{ + ContextSyncStatus, Conversions, MaspAmount, MaspDataLogEntry, MaspFeeData, + MaspSourceTransferData, MaspTargetTransferData, MaspTransferData, + MaspTxReorderedData, NoteIndex, ShieldedSyncConfig, ShieldedTransfer, + ShieldedUtils, SpentNotesTracker, TransferErr, WalletMap, WitnessMap, + NETWORK, +}; /// Represents the current state of the shielded pool from the perspective of /// the chosen viewing keys. @@ -499,36 +499,64 @@ pub trait ShieldedApi: } /// Query the ledger for the conversion that is allowed for the given asset - /// type and cache it. + /// type and cache it. The target epoch must be greater than or equal to the + /// asset's epoch. If the target epoch is None, then convert to the latest + /// epoch. #[allow(async_fn_in_trait)] async fn query_allowed_conversion<'a, C: Client + Sync>( &'a mut self, client: &C, asset_type: AssetType, + target_epoch: Option, conversions: &'a mut Conversions, - ) { - if let btree_map::Entry::Vacant(conv_entry) = + ) -> Result<(), eyre::Error> { + let btree_map::Entry::Vacant(conv_entry) = conversions.entry(asset_type) - { - let Some((token, denom, position, ep, conv, path)) = - Self::query_conversion(client, asset_type).await - else { - return; - }; - self.asset_types.insert( - asset_type, - AssetData { - token, - denom, - position, - epoch: Some(ep), - }, - ); - // If the conversion is 0, then we just have a pure decoding - if !conv.is_zero() { - conv_entry.insert((conv.into(), path, 0)); + else { + return Ok(()); + }; + // Get the conversion for the given asset type, otherwise fail + let Some((token, denom, position, ep, mut conv, path)) = + Self::query_conversion(client, asset_type).await + else { + return Ok(()); + }; + let asset_data = AssetData { + token, + denom, + position, + epoch: Some(ep), + }; + self.asset_types.insert(asset_type, asset_data.clone()); + // If no target epoch is specified, then we are converting to the latest + // epoch and can short circuit the inverse conversion and the queries + // involved in that + if let Some(target_epoch) = target_epoch { + // Get the equivalent to the original asset in the target epoch + let asset_data = asset_data.redate(target_epoch); + let target_asset_type = asset_data + .encode() + .map_err(|_| eyre!("unable to create asset type",))?; + // Get the conversion for the target asset type. If this query + // returns None for a target_epoch that is less than or equal to the + // latest, then the token stopped participating in the rewards + // program before target_epoch. Therefore a conversion to + // target_epoch is the same as one to the latest epoch. + if let Some((_token, _denom, _position, _ep, nconv, _path)) = + Self::query_conversion(client, target_asset_type).await + { + self.asset_types.insert(target_asset_type, asset_data); + // Subtract (conversion from target to latest) from (conversion + // from original to latest) to get (conversion from original to + // target). + conv -= nconv; } } + // If the conversion is 0, then we just have a pure decoding + if !conv.is_zero() { + conv_entry.insert((conv.into(), path, 0)); + } + Ok(()) } /// Convert the given amount into the latest asset types whilst making a @@ -541,41 +569,27 @@ pub trait ShieldedApi: client: &(impl Client + Sync), io: &impl Io, mut input: I128Sum, - target_epoch: MaspEpoch, + target_epoch: Option, mut conversions: Conversions, ) -> Result<(I128Sum, Conversions), eyre::Error> { // Where we will store our exchanged value let mut output = I128Sum::zero(); // Repeatedly exchange assets until it is no longer possible - while let Some((asset_type, value)) = - input.components().next().map(cloned_pair) - { - // Get the equivalent to the current asset in the target epoch and - // note whether this equivalent chronologically comes after the - // current asset - let target_asset_type = self - .decode_asset_type(client, asset_type) - .await - .map(|mut pre_asset_type| { - pre_asset_type.redate(target_epoch); - pre_asset_type - .encode() - .map_err(|_| eyre!("unable to create asset type",)) - }) - .transpose()? - .unwrap_or(asset_type); - let at_target_asset_type = target_asset_type == asset_type; - // Fetch and store the required conversions + while let Some(asset_type) = input.asset_types().next().cloned() { self.query_allowed_conversion( client, - target_asset_type, + asset_type, + target_epoch, &mut conversions, ) - .await; - self.query_allowed_conversion(client, asset_type, &mut conversions) - .await; - if let (Some((conv, _wit, usage)), false) = - (conversions.get_mut(&asset_type), at_target_asset_type) + .await?; + // Consolidate the current amount with any dust from output. + // Whatever is not used is moved back to output anyway. + let dust = output.project(asset_type); + input += dust.clone(); + output -= dust; + // Now attempt to apply conversions + if let Some((conv, _wit, usage)) = conversions.get_mut(&asset_type) { display_line!( io, @@ -588,28 +602,7 @@ pub trait ShieldedApi: io, conv.clone(), asset_type, - value, - usage, - &mut input, - &mut output, - ) - .await?; - } else if let (Some((conv, _wit, usage)), false) = ( - conversions.get_mut(&target_asset_type), - at_target_asset_type, - ) { - display_line!( - io, - "converting latest asset type to target asset type..." - ); - // Not at the target asset type, yet at the latest asset type. - // Apply inverse conversion to get from latest asset type to the - // target asset type. - self.apply_conversion( - io, - conv.clone(), - asset_type, - value, + input[&asset_type], usage, &mut input, &mut output, @@ -635,7 +628,6 @@ pub trait ShieldedApi: client: &(impl Client + Sync), io: &impl Io, vk: &ViewingKey, - target_epoch: MaspEpoch, ) -> Result, eyre::Error> { // First get the unexchanged balance if let Some(balance) = self.compute_shielded_balance(vk).await? { @@ -644,7 +636,7 @@ pub trait ShieldedApi: client, io, balance, - target_epoch, + None, BTreeMap::new(), ) .await? @@ -748,7 +740,7 @@ pub trait ShieldedApi: context.client(), context.io(), raw_balance.to_owned(), - current_epoch, + None, Conversions::new(), ) .await? @@ -760,7 +752,7 @@ pub trait ShieldedApi: // exchanged balance. For these assets there are no conversions yet so // we need to see if there are conversions for the previous epoch for (asset_type, _) in current_exchanged_balance.components() { - let mut asset = match self + let asset = match self .decode_asset_type(context.client(), *asset_type) .await { @@ -771,21 +763,21 @@ pub trait ShieldedApi: ) if ep == current_epoch => data, _ => continue, }; - asset.redate(previous_epoch); - let redated_asset_type = asset.encode()?; + let redated_asset_type = asset.redate(previous_epoch).encode()?; self.query_allowed_conversion( context.client(), redated_asset_type, + None, &mut latest_conversions, ) - .await; + .await?; } let mut estimated_next_epoch_conversions = Conversions::new(); // re-date the all the latest conversions up one epoch for (asset_type, (conv, wit, _)) in latest_conversions { - let mut asset = self + let asset = self .decode_asset_type(context.client(), asset_type) .await .ok_or_else(|| { @@ -810,14 +802,13 @@ pub trait ShieldedApi: if asset.token == native_token || asset_data.token != native_token { - asset_data.redate_to_next_epoch(); + asset_data = asset_data.redate_to_next_epoch(); } est_conv += ValueSum::from_pair(asset_data.encode()?, val) } - asset.redate_to_next_epoch(); estimated_next_epoch_conversions.insert( - asset.encode().unwrap(), + asset.redate_to_next_epoch().encode().unwrap(), (AllowedConversion::from(est_conv), wit.clone(), 0), ); } @@ -828,7 +819,7 @@ pub trait ShieldedApi: context.client(), context.io(), current_exchanged_balance.clone(), - next_epoch, + Some(next_epoch), estimated_next_epoch_conversions, ) .await? @@ -948,7 +939,6 @@ pub trait ShieldedApi: spent_notes: &mut SpentNotesTracker, sk: PseudoExtendedKey, target: ValueSum<(MaspDigitPos, Address), i128>, - target_epoch: MaspEpoch, ) -> Result< ( I128Sum, @@ -1000,7 +990,7 @@ pub trait ShieldedApi: context.client(), context.io(), pre_contr, - target_epoch, + None, conversions.clone(), ) .await?; @@ -1543,7 +1533,8 @@ pub trait ShieldedApi: Ok(change) } - /// Add the necessary transaction inputs to the builder. + /// Add the necessary transaction inputs to the builder. The supplied epoch + /// must be the current epoch. #[allow(async_fn_in_trait)] #[allow(clippy::too_many_arguments)] async fn add_inputs( @@ -1616,7 +1607,6 @@ pub trait ShieldedApi: notes_tracker, sk, required_amt.clone(), - epoch, ) .await .map_err(|e| TransferErr::General(e.to_string()))?; @@ -1834,7 +1824,7 @@ pub trait ShieldedApi: "Failed to decode epoched asset type, undating it: {:#?}", decoded ); - decoded.undate(); + *decoded = decoded.clone().undate(); asset_type = decoded .encode() .map_err(|_| eyre!("unable to create asset type"))?; @@ -2035,18 +2025,18 @@ mod test_shielded_wallet { } .encode() .unwrap(), - -1, + -2i128.pow(epoch as u32), ); conv += I128Sum::from_pair( AssetData { token: native_token.clone(), denom: native_token_denom, position: MaspDigitPos::Zero, - epoch: Some(MaspEpoch::new(epoch + 1)), + epoch: Some(MaspEpoch::new(5)), } .encode() .unwrap(), - 2, + 2i128.pow(5), ); context.add_conversions( AssetData { @@ -2073,7 +2063,7 @@ mod test_shielded_wallet { token: native_token.clone(), denom: native_token_denom, position: MaspDigitPos::Zero, - epoch: Some(MaspEpoch::new(5)), + epoch: Some(MaspEpoch::new(1)), }; wallet.add_asset_type(asset_data.clone()); wallet.add_note(create_note(asset_data.clone(), 10, pa), vk); @@ -2092,7 +2082,7 @@ mod test_shielded_wallet { context.client(), context.io(), balance, - MaspEpoch::new(4), + Some(MaspEpoch::new(4)), Conversions::new(), ) .await @@ -2109,7 +2099,7 @@ mod test_shielded_wallet { } .encode() .unwrap(), - 5 + 80, ) ); }