Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Unsigned MASP Changes in SDK #3716

Closed
wants to merge 2 commits into from
Closed

Unsigned MASP Changes in SDK #3716

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ffd2f35
Changes some MASP types to be unsigned
grarco Sep 6, 2024
6364873
Changelog #3716
grarco Sep 6, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .changelog/unreleased/improvements/3716-unsigned-change-in-sdk.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- Modified the `Changes` type to use unsigned integers.
([\#3716](https://github.com/anoma/namada/pull/3716))
8 changes: 3 additions & 5 deletions crates/core/src/token.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<Self> {
#[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)?;
Expand Down
22 changes: 9 additions & 13 deletions crates/shielded_token/src/masp.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -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<namada_core::masp::ExtendedSpendingKey, I128Sum>;
pub type Changes = HashMap<namada_core::masp::ExtendedSpendingKey, U128Sum>;

/// Shielded pool data for a token
#[allow(missing_docs)]
Expand Down Expand Up @@ -253,7 +253,7 @@ pub fn is_amount_required(
dest: I128Sum,
normed_delta: I128Sum,
opt_delta: Option<I128Sum>,
) -> Option<I128Sum> {
) -> Option<U128Sum> {
let mut changes = None;
let gap = dest.clone() - src;

Expand All @@ -262,19 +262,16 @@ 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
// that this note should be used
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));
Expand All @@ -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;
}
}
Expand Down
73 changes: 38 additions & 35 deletions crates/shielded_token/src/masp/shielded_wallet.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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};
Expand Down Expand Up @@ -1502,16 +1502,10 @@ pub trait ShieldedApi<U: ShieldedUtils + MaybeSend + MaybeSync>:
(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
Expand All @@ -1526,22 +1520,24 @@ pub trait ShieldedApi<U: ShieldedUtils + MaybeSend + MaybeSync>:
{
// 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()))?;
Expand Down Expand Up @@ -1575,16 +1571,8 @@ pub trait ShieldedApi<U: ShieldedUtils + MaybeSend + MaybeSync>:
// 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);
Expand All @@ -1604,7 +1592,8 @@ pub trait ShieldedApi<U: ShieldedUtils + MaybeSend + MaybeSync>:
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(
Expand Down Expand Up @@ -1669,7 +1658,12 @@ pub trait ShieldedApi<U: ShieldedUtils + MaybeSend + MaybeSync>:
.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() {
Expand All @@ -1679,8 +1673,15 @@ pub trait ShieldedApi<U: ShieldedUtils + MaybeSend + MaybeSync>:
}

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(),
Expand Down Expand Up @@ -1806,9 +1807,11 @@ pub trait ShieldedApi<U: ShieldedUtils + MaybeSend + MaybeSync>:
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)?;
}

Expand Down
Loading