Fix/unsafe arithmetics #42
Conversation
… and improved code formatting. - Inserted a placeholder unit for WeightInfo in the Afloat pallet's RBAC mock configuration. This satisfies the expected configuration for the pallet, allowing tests to compile and run correctly. - Implemented a new test case `replicate_overflow_for_start_take_sell_order` in the Afloat pallet. This test mimics a situation where there's a potential overflow in the `start_take_sell_order` function. By setting a highly valued price per credit and low valued tax credit amount, the test ensures that our pallet correctly handles such situations, thus improving the error handling and robustness of the platform.
…float pallet. This function checks for possible arithmetic overflow when multiplying two balances - a critical action in the process of starting a sell order. Ensuring the safety of this operation will increase the reliability and robustness of our pallet. - Added the `ArithmeticOverflow` error to the Afloat pallet error definitions. This error is used within the implementation of the safe multiplication function to catch the scenario of multiplication operations exceeding the maximum balance limit. - Updated the `replicate_overflow_for_start_take_sell_order` test case in the Afloat pallet to confirm the correct operation of the `safe_multiply_offer` function and the proper detection of arithmetic overflows. It ensures that when a multiplication operation would result in an overflow, the `ArithmeticOverflow` error is correctly returned.
…n Pallet to mitigate risks from arithmetic overflow. This function checks if the addition of two numbers results in an overflow and, if so, throws the `ArithmeticOverflow` error. This check has been incorporated into the `do_calculate_drawdown_total_amount` and `do_calculate_revenue_total_amount` functions to ensure safe addition operation and provide user feedback. - Defined a new `ArithmeticOverflow` error in the Fund Admin Pallet to provide feedback for operations that can potentially cause arithmetic overflow. This helps increase the robustness of our numeric operations. - Introduced two new test cases `replicate_overflow_for_a_drawdown_submission` and `replicate_overflow_for_a_revenue_submission` in the Fund Admin Pallet test suite to ensure accurate arithmetic overflow detection. These tests validate that the system correctly rejects drawdown and revenue submissions which would result in arithmetic overflow conditions. This expands the coverage of our testing and ensures the safe operation of financial transactions.
|
GPT summary of fde90b4: Error: couldn't generate summary |
|
GPT summary of 0e49d6c: Error: couldn't generate summary |
|
GPT summary of b901f22: Error: couldn't generate summary |
|
GPT summary of 9954101: Error: couldn't generate summary |
|
GPT summary of 198569a: Not generating summary for merge commits |
|
GPT summary of efb1177: Error: couldn't generate summary |
|
GPT summary of 65cf25f: Error: couldn't generate summary |
|
GPT summary of 3e35e06: Error: couldn't generate summary |
|
GPT summary of b1434e8: Error: couldn't generate summary |
|
GPT summary of 7ef8f8a: Error: couldn't generate summary |
|
GPT summary of 7c1a04a: Error: couldn't generate summary |
|
GPT summary of f90a8a9: Error: couldn't generate summary |
|
GPT summary of da3ce92: Error: couldn't generate summary |
|
GPT summary of 69418f7: Error: couldn't generate summary |
|
GPT summary of e8244e8: Error: couldn't generate summary PR summary so far: Error: couldn't generate summary |
| (revenue_id, job_eligible_id, project_id, timestamp).using_encoded(blake2_256); | ||
|
|
||
| // Ensure revenue transaction id doesn't exist | ||
| ensure!( |
There was a problem hiding this comment.
Por que se elimino este check?
There was a problem hiding this comment.
Estaba repetido, m{as abajo se vuelve a validar.
There was a problem hiding this comment.
Ok, movamos el ensure antes de crear el RevenueTransactionData por favor
There was a problem hiding this comment.
Falta este cambio
…product of two numbers after checked multiplication. This helps ensure arithmetic safety by throwing an `ArithmeticOverflow` error when an overflow occurs. Adjusted the `start_take_sell_order` function to incorporate these changes. It now uses the return value from `safe_multiply_offer` in calculations which increases the safety and reliability of our operations. - Made adjustments in the test case `replicate_overflow_for_start_take_sell_order` omitting the unused variable `offer` for cleaner and more efficient code. - Revised the `safe_add` function to return the sum of two numbers after checked addition. This helps ensure arithmetic safety by throwing an `ArithmeticOverflow` error when an overflow occurs. The `do_calculate_drawdown_total_amount` and `do_calculate_revenue_total_amount` functions have been updated to use the returned value from `safe_add` which increases arithmetic safety in these functions.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
GPT summary of 0936cea: Error: couldn't generate summary PR summary so far: Error: couldn't generate summary |
pallets/afloat/src/functions.rs
Outdated
| let creation_date: u64 = T::Timestamp::now().into(); | ||
| let price_per_credit: T::Balance = offer.price_per_credit.into(); | ||
| let total_price: T::Balance = price_per_credit * tax_credit_amount; | ||
| let total_price: T::Balance = Self::safe_multiply_offer(price_per_credit, tax_credit_amount)?; |
There was a problem hiding this comment.
mover esta sentencia arriba del ensure en la linea 478, para poder utilizar el valor de la variable en el ensure
There was a problem hiding this comment.
Falta este cambio
| (revenue_id, job_eligible_id, project_id, timestamp).using_encoded(blake2_256); | ||
|
|
||
| // Ensure revenue transaction id doesn't exist | ||
| ensure!( |
There was a problem hiding this comment.
Ok, movamos el ensure antes de crear el RevenueTransactionData por favor
…n into step-by-step processes in the Pallet for Afloat. I changed the order of operations by calculating the 'price_per_credit' and 'total_price' before checking user's afloat balance. This streamlines the computation and the ensures the growth of variables `price_per_credit` and `total_price`. - I added the check for existing Revenue Transaction ID earlier in the code to prevent duplicate entries in the Fund Admin. We ensure the transaction ID is unique before creating the revenue transaction data now. This helps in reducing the instances of `RevenueTransactionIdAlreadyExists` error. Removal of duplicate checks after the data creation provides smoother execution and cleaner code.
sebastianmontero
left a comment
sebastianmontero
left a comment
There was a problem hiding this comment.
No veo los cambios que habia solicitados
| use super::*; | ||
| use crate::types::*; | ||
| use frame_support::pallet_prelude::*; | ||
| use frame_system::{pallet_prelude::*, RawOrigin}; |
|
|
||
| // Get timestamp | ||
| let timestamp = Self::get_timestamp_in_milliseconds().ok_or(Error::<T>::TimestampError)?; | ||
|
|
|
GPT summary of 240f3f8: Error: couldn't generate summary PR summary so far: Error: couldn't generate summary |
Title of changes made: [Fix] Avoiding arithmetic overflows in key scaling operations
Overview
The primary goal of the changes is to mitigate potential arithmetic overflows while performing some vital scaling operations in the "Afloat" and "Fund-Admin" pallets. The purpose is to ensure the safety of mathematical computations and maintain data integrity across the systems.
This PR adds some "safe" arithmetic functions to add, multiply amounts or perform other arithmetic operations without risking any overflow. Whenever such functions are called, they check if the given operation would result in an overflow. If true, it returns an error before executing the operation.
The changes were mainly targeting the "create_tax_credit()" in the "Afloat" Pallet and "calculate_drawdown_total_amount()" and "calculate_revenue_total_amount()" in the "Fund-Admin" Pallet.
Tickets
Fix #31
Implementation notes
The implementation involves two significant changes:
Addition of the safe arithmetic functions, "safe_add()" and "safe_multiply_offer()". These functions perform the mathematical operation and check for a potential overflow. If the operation would result in an overflow, they short-circuit and return an
ArithmeticOverflowerror.Integration of the newly
created safe arithmetic functions into the existing logic. The safe functions are called before performing mathematical calculations in "create_tax_credit()" in "Afloat" Pallet and "calculate_drawdown_total_amount()" and "calculate_revenue_total_amount()" in the "Fund-Admin" Pallet. If an overflow is detected, the functions will return an error, stopping the operation and preserving the integrity of the system.
Interesting/controversial decisions
The changes performed might have some tradeoffs in terms of performance since we are checking for potential arithmetic overflow for each operation. However, these checks are essential to maintain the integrity of the system and avoid any data corruption. Additionally, the checks for overflow should have minimum performance impact since they are straightforward and fast to compute.
Test coverage
The changes include testing of the newly added functionality. In the "Afloat" pallets, we tested the potential overflow using the "replicate_overflow_for_start_take_sell_order()" function, and in the "Fund-Admin" pallets, we used the "replicate_overflow_for_a_drawdown_submission()" and "replicate_overflow_for_a_revenue_submission()". These tests can effectively detect and prevent any arithmetic overflow during the operations.
Loose ends
This PR doesn't cover every potential overflow scenario. Other parts of the codebase may also benefit from the addition of similar safe arithmetic operations.