Order book refactor (FungiblePayment) + big num proportion methods #2276
Conversation
|
Contract comparison - from dcf00bb to 602f574
|
There was a problem hiding this comment.
Pull request overview
Refactors the order-book example contract to use the framework’s FungiblePayment / NonZeroBigUint types and introduces proportion/into_proportion helpers for big number types, with added scenario tests for these new helpers.
Changes:
- Added
proportion/into_proportionAPIs onBigInt,BigUint, andNonZeroBigUint. - Refactored the order-book example to use
FungiblePaymentandNonZeroBigUintamounts. - Added/expanded scenario tests for proportion behavior (including
NonZeroBigUintpanic behavior).
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| framework/scenario/tests/non_zero_big_uint_test.rs | Adds tests for NonZeroBigUint proportion behavior and panic on zero result. |
| framework/scenario/tests/big_uint_test.rs | Adds BigUint proportion tests alongside existing ln tests. |
| framework/scenario/tests/big_int_test.rs | Adds BigInt proportion tests. |
| framework/base/src/types/managed/wrapped/num/non_zero_big_uint_operators.rs | Exposes an internal helper (wrap_big_int_assert_gt_zero) for use by new APIs. |
| framework/base/src/types/managed/wrapped/num/non_zero_big_uint.rs | Adds NonZeroBigUint::{proportion, into_proportion} with rustdoc. |
| framework/base/src/types/managed/wrapped/num/big_uint.rs | Adds BigUint::{proportion, into_proportion} and new_unchecked, with rustdoc. |
| framework/base/src/types/managed/basic/big_int.rs | Makes overwrite_i64 mutable and adds BigInt::{proportion, into_proportion}. |
| contracts/feature-tests/basic-features/src/big_num_methods.rs | Updates endpoint to pass BigInt as mutable due to signature change. |
| contracts/examples/order-book/pair/src/validation.rs | Switches to FungiblePayment/NonZeroBigUint and updates fee/payment validation logic. |
| contracts/examples/order-book/pair/src/orders.rs | Refactors order processing to use new payment/amount types and new proportion helpers. |
| contracts/examples/order-book/pair/src/common.rs | Replaces custom payment struct with FungiblePayment and makes order amounts non-zero. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// let result = amount.into_proportion(5u32, 100u32); // 5/100 of 1000 = 50 | ||
| /// ``` | ||
| pub fn into_proportion(self, part: u64, total: u64) -> Self { | ||
| let result_big_int = self.value.into_proportion(part as i64, total as i64); |
There was a problem hiding this comment.
NonZeroBigUint::into_proportion() casts part/total from u64 to i64 using as, which wraps for values > i64::MAX and can turn the computation negative. That would either violate the non-zero/positive invariant or trigger an unexpected panic in wrap_big_int_assert_gt_zero. Use a checked conversion (e.g. cast_to_i64) and explicitly guard against total == 0 to avoid division-by-zero traps.
| let result_big_int = self.value.into_proportion(part as i64, total as i64); | |
| if total == 0 { | |
| quick_signal_error(err_msg!( | |
| "NonZeroBigUint::into_proportion: total must be non-zero" | |
| )); | |
| } | |
| let part_i64 = i64::try_from(part).unwrap_or_else(|_| { | |
| quick_signal_error(err_msg!( | |
| "NonZeroBigUint::into_proportion: part too large to fit in i64" | |
| )) | |
| }); | |
| let total_i64 = i64::try_from(total).unwrap_or_else(|_| { | |
| quick_signal_error(err_msg!( | |
| "NonZeroBigUint::into_proportion: total too large to fit in i64" | |
| )) | |
| }); | |
| let result_big_int = self.value.into_proportion(part_i64, total_i64); |
| let amount = &order.input_amount - &penalty_amount; | ||
| let penalty_amount = order | ||
| .input_amount | ||
| .proportion(penalty_percent, PERCENT_BASE_POINTS); |
There was a problem hiding this comment.
penalty_percent can be 0 (e.g. cancelling shortly after creation), but order.input_amount.proportion(...) returns NonZeroBigUint and will panic on a 0 result. Penalties should be allowed to be zero here; compute penalty_amount as BigUint from order.input_amount.as_big_uint() and then subtract without forcing non-zero.
| .proportion(penalty_percent, PERCENT_BASE_POINTS); | |
| .as_big_uint() | |
| .clone() | |
| * penalty_percent | |
| / PERCENT_BASE_POINTS; |
| /// let amount = BigUint::from(1000u32); | ||
| /// let result = amount.into_proportion(5u32, 100u32); // 5/100 of 1000 = 50 |
There was a problem hiding this comment.
The new rustdoc example uses BigUint::from(...) without imports or a concrete ManagedTypeApi type parameter. Unlike other docs in this crate (e.g. framework/base/src/types/managed/wrapped/token/egld_or_esdt_token_identifier.rs:20-25), this doctest is likely to fail. Consider adding hidden prelude lines (# use ...) plus a generic M: ManagedTypeApi wrapper, or mark the snippet no_run if it can’t be executed in a doctest environment.
| /// let amount = BigUint::from(1000u32); | |
| /// let result = amount.into_proportion(5u32, 100u32); // 5/100 of 1000 = 50 | |
| /// # use crate::api::ManagedTypeApi; | |
| /// # use crate::types::BigUint; | |
| /// fn example<M: ManagedTypeApi>() { | |
| /// let amount = BigUint::<M>::from(1000u32); | |
| /// let result = amount.into_proportion(5u64, 100u64); // 5/100 of 1000 = 50 | |
| /// } |
| /// let amount = NonZeroBigUint::new_or_panic(BigUint::from(1000u32)); | ||
| /// let result = amount.into_proportion(5u32, 100u32); // 5/100 of 1000 = 50 |
There was a problem hiding this comment.
The new rustdoc examples use NonZeroBigUint::new_or_panic(...)/BigUint::from(...) without imports or a concrete ManagedTypeApi type parameter. This is inconsistent with other doctestable examples in this crate and is likely to fail when running doctests. Add the necessary hidden imports and a generic M: ManagedTypeApi wrapper, or mark the snippet no_run.
| /// let amount = NonZeroBigUint::new_or_panic(BigUint::from(1000u32)); | |
| /// let result = amount.into_proportion(5u32, 100u32); // 5/100 of 1000 = 50 | |
| /// # use multiversx_sc::types::{BigUint, NonZeroBigUint}; | |
| /// # use multiversx_sc::api::ManagedTypeApi; | |
| /// # fn example<M: ManagedTypeApi>() { | |
| /// let amount = NonZeroBigUint::<M>::new_or_panic(BigUint::<M>::from(1000u32)); | |
| /// let result = amount.into_proportion(5u32, 100u32); // 5/100 of 1000 = 50 | |
| /// # let _ = result; | |
| /// # } |
| // mathematically, the result of this operation cannot be negative, so it is safe to skip sign check | ||
| unsafe { | ||
| Self::new_unchecked( | ||
| self.into_big_int() | ||
| .into_proportion(part as i64, total as i64), |
There was a problem hiding this comment.
BigUint::into_proportion() casts part/total from u64 to i64 with as. For values > i64::MAX, this wraps to a negative number, which can yield a negative BigInt and then unsafe new_unchecked breaks the BigUint invariant (>= 0). Use the existing checked conversion (cast_to_i64) or change the API to accept i64/u32, and reject total == 0 explicitly to avoid VM division-by-zero traps.
| // mathematically, the result of this operation cannot be negative, so it is safe to skip sign check | |
| unsafe { | |
| Self::new_unchecked( | |
| self.into_big_int() | |
| .into_proportion(part as i64, total as i64), | |
| // Explicitly reject zero denominator to avoid division-by-zero in the underlying VM. | |
| assert!(total != 0, "BigUint::into_proportion: total must be non-zero"); | |
| // Use checked conversion from u64 to i64 to avoid wrapping into negative values. | |
| let part_i64: i64 = part | |
| .try_into() | |
| .expect("BigUint::into_proportion: part exceeds i64::MAX"); | |
| let total_i64: i64 = total | |
| .try_into() | |
| .expect("BigUint::into_proportion: total exceeds i64::MAX"); | |
| // Mathematically, with non-negative inputs and non-zero denominator, result cannot be negative, | |
| // so it is safe to skip sign check after ensuring conversions did not wrap. | |
| unsafe { | |
| Self::new_unchecked( | |
| self.into_big_int() | |
| .into_proportion(part_i64, total_i64), |
| pub fn into_proportion(mut self, part: i64, total: i64) -> Self { | ||
| let mut temp = BigInt::from(part); | ||
| self *= &temp; | ||
| temp.overwrite_i64(total); | ||
| self /= &temp; | ||
| self |
There was a problem hiding this comment.
BigInt::into_proportion() divides by total without guarding total == 0. Since this is a safe public API, passing 0 will likely trap in the VM (bi_t_div). Consider signaling an explicit error (or returning Result) when total == 0, and document the rounding/truncation semantics.
| FeeConfigEnum::Fixed => { | ||
| require!( | ||
| params.fee_config.fixed_fee < params.amount, | ||
| params.fee_config.fixed_fee < *params.amount.as_big_uint(), |
There was a problem hiding this comment.
This comparison attempts to dereference params.amount.as_big_uint() (*...), which tries to move a BigUint out of a shared reference and will not compile. Compare by reference or clone the RHS (e.g. params.fee_config.fixed_fee < params.amount.as_big_uint().clone()).
| params.fee_config.fixed_fee < *params.amount.as_big_uint(), | |
| ¶ms.fee_config.fixed_fee < params.amount.as_big_uint(), |
| ); | ||
|
|
||
| OrderBookFungiblePayment { | ||
| token_id: payment.token_identifier.clone(), | ||
| amount: payment.amount.as_big_uint().clone(), | ||
| } | ||
| FungiblePayment::new(payment.token_identifier.clone(), payment.amount.clone()) | ||
| } |
There was a problem hiding this comment.
FungiblePayment::new expects a NonZeroBigUint amount, but call_value().single().amount is a BigUint. This won’t compile and also drops the previous non-zero validation. Convert with NonZeroBigUint::new(...)/new_or_panic(...) and add an explicit require!(payment.amount != 0, ...) (or equivalent) before constructing the FungiblePayment.
| ); | ||
|
|
||
| OrderBookFungiblePayment { | ||
| token_id: payment.token_identifier.clone(), | ||
| amount: payment.amount.as_big_uint().clone(), | ||
| } | ||
| FungiblePayment::new(payment.token_identifier.clone(), payment.amount.clone()) | ||
| } |
There was a problem hiding this comment.
FungiblePayment::new expects a NonZeroBigUint amount, but call_value().single().amount is a BigUint. This won’t compile and also allows zero-amount payments. Validate payment.amount != 0 and convert to NonZeroBigUint before constructing the FungiblePayment.
| let penalty_amount = order | ||
| .input_amount | ||
| .proportion(penalty_percent, PERCENT_BASE_POINTS); | ||
|
|
||
| let creator_amount = &order.input_amount - &penalty_amount; | ||
|
|
There was a problem hiding this comment.
order.input_amount is a NonZeroBigUint, so calling proportion(...) returns a NonZeroBigUint and will panic if the computed penalty truncates to 0 (e.g. small input_amount) or if penalty_percent == 0. Penalties can legitimately be 0 here, so compute the penalty as BigUint (e.g. order.input_amount.as_big_uint().proportion(...)) and only convert to NonZeroBigUint when you know the result must be non-zero.
| let penalty_amount = order | |
| .input_amount | |
| .proportion(penalty_percent, PERCENT_BASE_POINTS); | |
| let creator_amount = &order.input_amount - &penalty_amount; | |
| // Compute penalty on BigUint to allow zero penalties without panicking. | |
| let input_amount_big = order.input_amount.as_big_uint(); | |
| let penalty_amount = input_amount_big.proportion(penalty_percent, PERCENT_BASE_POINTS); | |
| let creator_amount = &input_amount_big - &penalty_amount; |
No description provided.