-
Notifications
You must be signed in to change notification settings - Fork 951
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.
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
Unsigned MASP Changes
in SDK
#3716
base: main
Are you sure you want to change the base?
Conversation
b22fefb
to
f0a9d8b
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #3716 +/- ##
=======================================
Coverage 72.47% 72.47%
=======================================
Files 338 338
Lines 104130 104128 -2
=======================================
- Hits 75470 75469 -1
+ Misses 28660 28659 -1 ☔ View full report in Codecov by Sentry. |
f0a9d8b
to
11041fb
Compare
11041fb
to
6c339f2
Compare
6c339f2
to
49a0996
Compare
49a0996
to
6364873
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably these changes are okay for the most part. The MASP crate uses I128Sum
s to represent value balances because we needed a signed type that can holds sums and differences of u64
s without overflowing or underflowing. Given this, while I understand that it is conceptually clearer to hold unsigned amounts in U128Sum
s. It seems to me that this comes at the cost of introducing another integer type U128Sum
and sometimes having to do fallible conversions from U128Sum
s to I128Sum
s for interoperability with the MASP crate. I guess this is fine if we understand that due to the fallible runtime conversions, the U128Sum
s (just like the I128Sum
s that they are replacing) can really only hold U127Sum
s.
crates/sdk/src/masp.rs
Outdated
I128Sum::try_from_sum(output_amt.clone()).map_err( | ||
|e| { | ||
TransferErr::General(Error::Other(format!( | ||
"Fee amount is expected to be \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this error would be triggered by a failed conversion from u128
to i128
, shouldn't this error message be to the effect that the fee amount is too large?
crates/sdk/src/masp.rs
Outdated
let denominated_output_amt = context | ||
.shielded_mut() | ||
.await | ||
.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( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I understand that this conversion is necessary to placate the type checker, but is this conversion necessary at the conceptual level? I could imagine convert_masp_amount_to_namada
being generalized or being given a twin that takes U128
s instead of I128
.
crates/sdk/src/masp.rs
Outdated
@@ -1852,7 +1836,9 @@ impl<U: ShieldedUtils + MaybeSend + MaybeSync> ShieldedContext<U> { | |||
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| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems to me like we can avoid this fallible conversion if we declare input_amt
as let input_amt = U128Sum::from_pair(asset_type.to_owned(), *fee_amt)
or something similar. But doing this would involve using an analogue of convert_masp_amount_to_namada
that takes U128Sum
s...
crates/sdk/src/masp.rs
Outdated
@@ -1915,7 +1901,12 @@ impl<U: ShieldedUtils + MaybeSend + MaybeSync> ShieldedContext<U> { | |||
) | |||
.await?; | |||
|
|||
fees -= &output_amt; | |||
fees -= U128Sum::try_from_sum(output_amt).map_err(|e| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
crates/sdk/src/masp.rs
Outdated
let positioned_amt = token::Amount::from_masp_denominated_i128( | ||
*val, | ||
let positioned_amt = token::Amount::from_masp_denominated_u128( | ||
*val as u128, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that a caller might call this function with negative entries inside amt
, shouldn't we first check to see if val
can be converted to a u128
? Or better yet, shouldn't we just change the parameter amt: I128Sum
to amt: U128Sum
?
crates/sdk/src/masp.rs
Outdated
@@ -1925,8 +1916,14 @@ impl<U: ShieldedUtils + MaybeSend + MaybeSync> ShieldedContext<U> { | |||
} | |||
|
|||
if !fees.is_zero() { | |||
let signed_fees = I128Sum::try_from_sum(fees).map_err(|e| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this error occurs when fee: U128Sum
is too large to be contained by a I128Sum
. This sort of error (u128
too large for i128
) seems to be less clearly defined than those that were produced when an I128Sum
unexpectedly contained a negative quantity. I guess this is because it's clearer that quantities like fees can only ever be positive (or maybe zero).
Describe your changes
Closes #3425.
Modifies the
Changes
type in the SDK to wrapU128Sum
instead ofI128Sum
since changes are guaranteed to be non-negative. Also adjustsfrom_masp_denominated_i128
and swap the calls tofrom_nonnegative
forfrom_pair
.Checklist before merging
breaking::
labels