From ffd2f353130681f5973636aae0f40e4f80a6c7d2 Mon Sep 17 00:00:00 2001 From: Marco Granelli Date: Fri, 6 Sep 2024 15:54:13 +0200 Subject: [PATCH] Changes some MASP types to be unsigned --- crates/core/src/token.rs | 8 +- crates/shielded_token/src/masp.rs | 22 +++--- .../src/masp/shielded_wallet.rs | 73 ++++++++++--------- 3 files changed, 50 insertions(+), 53 deletions(-) diff --git a/crates/core/src/token.rs b/crates/core/src/token.rs index ba78b58d8b..b74759f554 100644 --- a/crates/core/src/token.rs +++ b/crates/core/src/token.rs @@ -232,16 +232,14 @@ impl Amount { Self { raw: Uint(raw) } } - /// Given a i128 and [`MaspDigitPos`], construct the corresponding + /// Given a u128 and [`MaspDigitPos`], construct the corresponding /// amount. - pub fn from_masp_denominated_i128( - val: i128, + pub fn from_masp_denominated_u128( + val: u128, denom: MaspDigitPos, ) -> Option { - #[allow(clippy::cast_sign_loss)] #[allow(clippy::cast_possible_truncation)] let lo = val as u64; - #[allow(clippy::cast_sign_loss)] let hi = (val >> 64) as u64; let lo_pos = denom as usize; let hi_pos = lo_pos.checked_add(1)?; diff --git a/crates/shielded_token/src/masp.rs b/crates/shielded_token/src/masp.rs index d2cc14a60f..a00fde8d15 100644 --- a/crates/shielded_token/src/masp.rs +++ b/crates/shielded_token/src/masp.rs @@ -23,7 +23,7 @@ use masp_primitives::sapling::keys::FullViewingKey; use masp_primitives::sapling::{Diversifier, Node, ViewingKey}; use masp_primitives::transaction::builder::{self, *}; use masp_primitives::transaction::components::sapling::builder::SaplingMetadata; -use masp_primitives::transaction::components::{I128Sum, ValueSum}; +use masp_primitives::transaction::components::{I128Sum, U128Sum, ValueSum}; use masp_primitives::transaction::Transaction; use masp_primitives::zip32::{ ExtendedFullViewingKey, ExtendedSpendingKey as MaspExtendedSpendingKey, @@ -131,7 +131,7 @@ pub struct MaspTxReorderedData { /// Data about the unspent amounts for any given shielded source coming from the /// spent notes in their posses that have been added to the builder. Can be used /// to either pay fees or to return a change -pub type Changes = HashMap; +pub type Changes = HashMap; /// Shielded pool data for a token #[allow(missing_docs)] @@ -253,7 +253,7 @@ pub fn is_amount_required( dest: I128Sum, normed_delta: I128Sum, opt_delta: Option, -) -> Option { +) -> Option { let mut changes = None; let gap = dest.clone() - src; @@ -262,7 +262,7 @@ pub fn is_amount_required( let signed_change_amt = checked!(normed_delta[asset_type] - *value).unwrap_or_default(); let unsigned_change_amt = if signed_change_amt > 0 { - signed_change_amt + signed_change_amt as u128 } else { // Even if there's no change we still need to set the return // value of this function to be Some so that the caller sees @@ -270,11 +270,8 @@ pub fn is_amount_required( 0 }; - let change_amt = I128Sum::from_nonnegative( - asset_type.to_owned(), - unsigned_change_amt, - ) - .expect("Change is guaranteed to be non-negative"); + let change_amt = + U128Sum::from_pair(asset_type.to_owned(), unsigned_change_amt); changes = changes .map(|prev| prev + change_amt.clone()) .or(Some(change_amt)); @@ -289,11 +286,10 @@ pub fn is_amount_required( changes = changes.map(|mut chngs| { for (delta_asset_type, delta_amt) in delta.components() { if !dest.asset_types().contains(delta_asset_type) { - let rmng = I128Sum::from_nonnegative( + let rmng = U128Sum::from_pair( delta_asset_type.to_owned(), - *delta_amt, - ) - .expect("Change is guaranteed to be non-negative"); + *delta_amt as u128, + ); chngs += rmng; } } diff --git a/crates/shielded_token/src/masp/shielded_wallet.rs b/crates/shielded_token/src/masp/shielded_wallet.rs index b51fb652de..298651ebb7 100644 --- a/crates/shielded_token/src/masp/shielded_wallet.rs +++ b/crates/shielded_token/src/masp/shielded_wallet.rs @@ -20,7 +20,7 @@ use masp_primitives::sapling::{ use masp_primitives::transaction::builder::Builder; use masp_primitives::transaction::components::sapling::builder::RngBuildParams; use masp_primitives::transaction::components::{ - I128Sum, TxOut, U64Sum, ValueSum, + I128Sum, TxOut, U128Sum, U64Sum, ValueSum, }; use masp_primitives::transaction::fees::fixed::FeeRule; use masp_primitives::transaction::{builder, Transaction}; @@ -1502,16 +1502,10 @@ pub trait ShieldedApi: (asset_types, amount) }; - let mut fees = I128Sum::zero(); - // Convert the shortfall into a I128Sum + let mut fees = U128Sum::zero(); + // Convert the shortfall into a U128Sum for (asset_type, val) in asset_types.iter().zip(raw_amount) { - fees += I128Sum::from_nonnegative(*asset_type, val.into()) - .map_err(|()| { - TransferErr::General( - "Fee amount is expected expected to be non-negative" - .to_string(), - ) - })?; + fees += U128Sum::from_pair(*asset_type, val.into()); } // 1. Try to use the change to pay fees @@ -1526,22 +1520,24 @@ pub trait ShieldedApi: { // Get the minimum between the available change and // the due fee - let output_amt = I128Sum::from_nonnegative( + let output_amt = U128Sum::from_pair( asset_type.to_owned(), *change.min(fee_amt), - ) - .map_err(|()| { - TransferErr::General( - "Fee amount is expected to be non-negative" - .to_string(), - ) - })?; + ); let denominated_output_amt = self .convert_masp_amount_to_namada( context.client(), // Safe to unwrap denoms.get(token).unwrap().to_owned(), - output_amt.clone(), + I128Sum::try_from_sum(output_amt.clone()).map_err( + |e| { + TransferErr::General(format!( + "Fee amount is expected to be \ + non-negative: {}", + e + )) + }, + )?, ) .await .map_err(|e| TransferErr::General(e.to_string()))?; @@ -1575,16 +1571,8 @@ pub trait ShieldedApi: // Decrease the changes by the amounts used for fee payment for (sp, temp_changes) in temp_changes.iter() { for (asset_type, temp_change) in temp_changes.components() { - let output_amt = I128Sum::from_nonnegative( - asset_type.to_owned(), - *temp_change, - ) - .map_err(|()| { - TransferErr::General( - "Fee amount is expected expected to be non-negative" - .to_string(), - ) - })?; + let output_amt = + U128Sum::from_pair(asset_type.to_owned(), *temp_change); // Entry is guaranteed to be in the map changes.entry(*sp).and_modify(|amt| *amt -= &output_amt); @@ -1604,7 +1592,8 @@ pub trait ShieldedApi: for (asset_type, fee_amt) in fees.clone().components() { let input_amt = I128Sum::from_nonnegative( asset_type.to_owned(), - *fee_amt, + i128::try_from(*fee_amt) + .map_err(|e| TransferErr::General(e.to_string()))?, ) .map_err(|()| { TransferErr::General( @@ -1669,7 +1658,12 @@ pub trait ShieldedApi: .await .map_err(|e| TransferErr::General(e.to_string()))?; - fees -= &output_amt; + fees -= U128Sum::try_from_sum(output_amt).map_err(|e| { + TransferErr::General(format!( + "Output amount is expected to be non-negative: {}", + e + )) + })?; } if fees.is_zero() { @@ -1679,8 +1673,15 @@ pub trait ShieldedApi: } if !fees.is_zero() { + let signed_fees = I128Sum::try_from_sum(fees).map_err(|e| { + TransferErr::General(format!( + "Fee amount cannot be casted to a signed integer: {}", + e + )) + })?; + return Result::Err(TransferErr::Build { - error: builder::Error::InsufficientFunds(fees), + error: builder::Error::InsufficientFunds(signed_fees), data: Some(MaspDataLog { source: None, token: token.to_owned(), @@ -1806,9 +1807,11 @@ pub trait ShieldedApi: let value_sum = self.decode_sum(client, amt).await; for ((_, decoded), val) in value_sum.components() { - let positioned_amt = - Amount::from_masp_denominated_i128(*val, decoded.position) - .unwrap_or_default(); + let positioned_amt = Amount::from_masp_denominated_u128( + *val as u128, + decoded.position, + ) + .unwrap_or_default(); amount = checked!(amount + positioned_amt)?; }