fix amm liquidity provision

credence0x commented 3 months ago

User description

lp fees should work accurately now

PR Type

Bug fix, Tests

Description

Fixed a bug in the AMM liquidity provision by removing the liquidity function and updating the mint_shares function.
Updated tests to reflect changes in liquidity calculation by using total_shares instead of liquidity.
Updated block_number and class_hash values in the manifest files.

Changes walkthrough 📝

Relevant files

Configuration changes

manifest.json `Update block number and class hashes in manifest` contracts/manifests/dev/manifest.json Updated `block_number` from 19 to 20. Updated `class_hash` and `original_class_hash` for a `DojoContract`.	+3/-3
manifest.toml `Update block number and class hashes in manifest` contracts/manifests/dev/manifest.toml Updated `block_number` from 19 to 20. Updated `class_hash` and `original_class_hash` for a `DojoContract`.	+3/-3

Bug fix

market.cairo

Fix liquidity provision and update related tests

contracts/src/models/bank/market.cairo

Removed liquidity function.

Updated mint_shares function to remove liquidity calculation.

Modified tests to use total_shares instead of liquidity.

+7/-31

💡 PR-Agent usage: Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

vercel[bot] commented 3 months ago

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
eternum	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Jul 5, 2024 4:32pm

github-actions[bot] commented 3 months ago

PR Reviewer Guide 🔍

⏱️ Estimated effort to review [1-5]	3
🧪 Relevant tests	Yes
🔒 Security concerns	No
⚡ Key issues to review	Possible Bug: The removal of the `liquidity` function and its replacement with `total_shares` in the `mint_shares` function may not correctly calculate the shares to mint based on the new logic. This change could affect the market dynamics and should be thoroughly tested, especially under edge cases.
⚡ Key issues to review	Refactoring Concern: The removal of the `liquidity` function simplifies the code but requires careful consideration of all places where this function was used to ensure that the new logic with `total_shares` maintains the intended behavior.

github-actions[bot] commented 3 months ago

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Add a conditional check to prevent division by zero ___ Replace the direct assignment of `amount` to `FixedTrait::new_unscaled` with a conditional check to ensure `amount` is non-zero to prevent potential division by zero errors in subsequent calculations. [contracts/src/models/bank/market.cairo [183]](https://github.com/BibliothecaDAO/eternum/pull/1058/files#diff-0ed7dbca92a513e5ffe5f67ef1f914b92b611abe3bc3972cb39b6254cc1bedf8R183-R183) ```diff -FixedTrait::new_unscaled(amount, false) +FixedTrait::new_unscaled(amount, false) if amount != 0 else FixedTrait::new_unscaled(1, false) ``` Suggestion importance[1-10]: 8 Why: This suggestion addresses a potential division by zero error, which is a critical issue that can cause runtime exceptions. The conditional check ensures robustness in the code.	8
Possible bug	Add a check to ensure non-zero value before division ___ Ensure that `quantity_optimal` is not zero before proceeding with the division to avoid division by zero errors. [contracts/src/models/bank/market.cairo [148]](https://github.com/BibliothecaDAO/eternum/pull/1058/files#diff-0ed7dbca92a513e5ffe5f67ef1f914b92b611abe3bc3972cb39b6254cc1bedf8R148-R148) ```diff -if quantity_optimal <= quantity +if quantity_optimal <= quantity and quantity_optimal != 0 ``` Suggestion importance[1-10]: 8 Why: This suggestion prevents division by zero errors, which are critical issues that can cause runtime exceptions. Adding this check ensures the code handles edge cases more gracefully.	8
Best practice	Use safe math operations to prevent overflow and underflow ___ Replace the direct multiplication and division for share calculation with a function that handles potential overflow or underflow scenarios. [contracts/src/models/bank/market.cairo [196]](https://github.com/BibliothecaDAO/eternum/pull/1058/files#diff-0ed7dbca92a513e5ffe5f67ef1f914b92b611abe3bc3972cb39b6254cc1bedf8R196-R196) ```diff -(amount * self.total_shares) / reserve_amount +safe_div(safe_mul(amount, self.total_shares), reserve_amount) ``` Suggestion importance[1-10]: 7 Why: Using safe math operations is a good practice to prevent overflow and underflow, which can lead to incorrect calculations and potential vulnerabilities. This improves the reliability of the code.	7
Enhancement	Restore liquidity computation with a new method ___ Replace the removed liquidity calculation with a new method that correctly computes liquidity based on the current state of the market. [contracts/src/models/bank/market.cairo [336]](https://github.com/BibliothecaDAO/eternum/pull/1058/files#diff-0ed7dbca92a513e5ffe5f67ef1f914b92b611abe3bc3972cb39b6254cc1bedf8R336-R336) ```diff -let initial_liquidity = market.total_shares; +let initial_liquidity = market.compute_liquidity(); ``` Suggestion importance[1-10]: 6 Why: Reintroducing a method to compute liquidity can enhance the accuracy of liquidity calculations. However, the necessity of this change depends on the context and usage of liquidity in the code.	6

BibliothecaDAO / eternum