Fix amm ux

[aymericdelab]

only client work

=> fixes #1063 #1062 #1061 #1050

[vercel[bot]]

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 3:24pm

[github-actions[bot]]

PR Reviewer Guide 🔍

⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Key issues to review **Possible Bug:** In the `MarketManager.ts` file, the methods `getOutputAmount` and `getInputPrice` now include a `feeRateNum` parameter, which is passed down to other methods like `buyResource` and `sellResource`. However, the `slippage` method uses hardcoded `0` for `feeRateNum` when calling these methods. This inconsistency could lead to incorrect calculations and should be addressed. **Data Integrity:** The removal of `Math.floor` in the `quoteResource` and `quoteLords` methods in `MarketManager.ts` changes the behavior from rounding down to returning floating-point numbers. This change could potentially introduce precision-related bugs or unexpected behavior, especially if the consuming code expects integer values. **Error Handling:** The change in error conditions from `<= 0` to `< 0` for reserves and input amounts in `MarketManager.ts` might not correctly handle the case where the value is exactly `0`, which is typically an edge case in financial calculations.

[github-actions[bot]]

PR Code Suggestions ✨

Category	Suggestion	Score
Possible bug	Add a check for zero denominator to prevent division by zero ___ **Add a check to ensure that feeRateDenom is not zero before performing division to prevent a potential division by zero error.** [client/src/dojo/modelManager/MarketManager.ts [94-95]](https://github.com/BibliothecaDAO/eternum/pull/1087/files#diff-5ad042e0aeddfa934a06675eef3ba3a7365fc2811f8f76afc4475c972cebc977R94-R95) ```diff const feeRateDenom = EternumGlobalConfig.banks.lpFeesDenominator; +if (feeRateDenom === 0) { + throw new Error("Fee denominator cannot be zero"); +} const inputAmountWithFee = BigInt(inputAmount) * BigInt(feeRateDenom - feeRateNum); ``` Suggestion importance[1-10]: 10 Why: This suggestion addresses a potential division by zero error, which is a critical issue. Adding a check for zero denominator enhances the robustness of the code.	10
	Add checks to ensure amounts are not zero before calculations ___ **Ensure that the lordsAmount and resourceAmount are not zero before performing calculations to avoid division by zero errors.** [client/src/dojo/modelManager/MarketManager.ts [136-137]](https://github.com/BibliothecaDAO/eternum/pull/1087/files#diff-5ad042e0aeddfa934a06675eef3ba3a7365fc2811f8f76afc4475c972cebc977R136-R137) ```diff +if (lordsAmount === 0 || market.lords_amount === 0 || market.resource_amount === 0) { + throw new Error("Amounts must be greater than zero"); +} let outputAmount = this.getOutputAmount(lordsAmount, market.lords_amount, market.resource_amount, feeRateNum); ``` Suggestion importance[1-10]: 8 Why: This suggestion prevents potential division by zero errors by ensuring that the amounts are not zero before performing calculations. It is a significant improvement for code safety.	8
	Ensure balance calculations do not result in negative values ___ **Replace the direct subtraction of lordsFee from selectedResourceBalance with a safer calculation that ensures the result is not negative. This prevents displaying a negative balance in the UI.** [client/src/ui/components/bank/ResourceBar.tsx [48]](https://github.com/BibliothecaDAO/eternum/pull/1087/files#diff-8528de8fd31390c2b1c12196f09c87bbf40c655f25271b47761c398b1b7de60bR48-R48) ```diff -const finalResourceBalance = hasLordsFees ? selectedResourceBalance - lordsFee : selectedResourceBalance; +const finalResourceBalance = hasLordsFees ? Math.max(0, selectedResourceBalance - lordsFee) : selectedResourceBalance; ``` Suggestion importance[1-10]: 8 Why: This suggestion addresses a potential bug by ensuring that the balance displayed is never negative, which improves the user experience and prevents misleading information.	8
	Make travelTime a required prop to avoid potential runtime errors ___ **Ensure that the travelTime prop is always provided to avoid runtime errors due to its use in calculations without null checks. Make travelTime a required prop or provide a default value.** [client/src/ui/components/resources/ResourceWeight.tsx [16]](https://github.com/BibliothecaDAO/eternum/pull/1087/files#diff-cb1944e8fb6258c30583665a6237d4196296b5fe8e4fd798889097796753e0cfR16-R16) ```diff -travelTime?: number; +travelTime: number; ``` Suggestion importance[1-10]: 7 Why: Ensuring `travelTime` is always provided helps avoid runtime errors, which is important for the reliability of the component. However, it could be handled with default values or null checks as well.	7
Possible issue	Add checks for undefined IDs before using them in component props ___ **Consider adding a check for bank_entity_id and entity_id to ensure they are not undefined before using them as props in LiquidityResourceRow. This will prevent potential runtime errors if these values are not provided.** [client/src/ui/components/bank/LiquidityTable.tsx [38-42]](https://github.com/BibliothecaDAO/eternum/pull/1087/files#diff-6f4a8ce4165053b92e4b52e0c34b97f34d6231dd1e562c8297e43ad6c68304b4R38-R42) ```diff - +{bank_entity_id && entity_id && ( + +)} ``` Suggestion importance[1-10]: 9 Why: This suggestion prevents potential runtime errors by ensuring `bank_entity_id` and `entity_id` are defined before using them, which is crucial for the stability of the component.	9
	Verify the intended increase in fee percentages ___ **Ensure that the new fee structure (15% for both lpFees and ownerFees) is intended, as it significantly increases from the previous 1%. This could have a substantial impact on the financial model.** [sdk/packages/eternum/src/constants/global.ts [25-28]](https://github.com/BibliothecaDAO/eternum/pull/1087/files#diff-679a37db0e7e68c37d60fbd39ba068183a0560b3443b67d5043758a317dc2049R25-R28) ```diff -lpFeesNumerator: 15, +lpFeesNumerator: 15, // Confirm this increase lpFeesDenominator: 100, // % -ownerFeesNumerator: 15, +ownerFeesNumerator: 15, // Confirm this increase ownerFeesDenominator: 100, // % ``` Suggestion importance[1-10]: 9 Why: This suggestion addresses a significant change in the fee structure, which could have a substantial impact on the financial model. Ensuring the change is intentional is crucial.	9
Documentation	Add explanations for significant numerical changes in fee structure ___ **Consider adding a comment or documentation to explain the rationale behind the specific values of lpFeesNumerator and ownerFeesNumerator, especially since they represent a significant change from the previous values.** [sdk/packages/eternum/src/constants/global.ts [25-27]](https://github.com/BibliothecaDAO/eternum/pull/1087/files#diff-679a37db0e7e68c37d60fbd39ba068183a0560b3443b67d5043758a317dc2049R25-R27) ```diff -lpFeesNumerator: 15, -ownerFeesNumerator: 15, +lpFeesNumerator: 15, // Increased to enhance liquidity pool stability +ownerFeesNumerator: 15, // Adjusted to maintain competitive owner fees ``` Suggestion importance[1-10]: 8 Why: Adding comments to explain significant changes in the fee structure improves code documentation and helps future developers understand the rationale behind these values.	8
Enhancement	Improve user experience by handling undefined or loading states for travel time ___ **To enhance the user experience, consider adding a loading indicator or a placeholder when travelTime is undefined or being fetched, instead of defaulting to 0 which might be misleading.** [client/src/ui/components/trading/TransferBetweenEntities.tsx [127]](https://github.com/BibliothecaDAO/eternum/pull/1087/files#diff-5ce2f1e45fead506c5de11a56a5f1927ff1efbe1645641f5cf0fcb88094d64baR127-R127) ```diff -Travel Time: {Math.floor((travelTime || 0) / 60)} hrs {(travelTime || 0) % 60} mins +Travel Time: {travelTime ? `${Math.floor(travelTime / 60)} hrs ${travelTime % 60} mins` : } ``` Suggestion importance[1-10]: 8 Why: Adding a loading indicator or placeholder for undefined travel time enhances the user experience by providing clear feedback, rather than defaulting to 0 which might be misleading.	8
Maintainability	Improve readability by using a utility function for time formatting ___ **Consider using a utility function to format the travel time into hours and minutes. This will improve code readability and maintainability, especially if similar time formatting is needed elsewhere in the application.** [client/src/ui/components/trading/TransferBetweenEntities.tsx [127]](https://github.com/BibliothecaDAO/eternum/pull/1087/files#diff-5ce2f1e45fead506c5de11a56a5f1927ff1efbe1645641f5cf0fcb88094d64baR127-R127) ```diff -Travel Time: {Math.floor((travelTime || 0) / 60)} hrs {(travelTime || 0) % 60} mins +Travel Time: {formatTravelTime(travelTime)} ``` Suggestion importance[1-10]: 7 Why: Using a utility function for time formatting enhances code readability and maintainability, especially if the same formatting logic is used in multiple places. However, it is not a critical change.	7
	Improve variable naming for clarity ___ **Use a more descriptive variable name than canCarry to indicate its purpose clearly, such as canProceedWithSwap, to improve code readability and maintainability.** [client/src/ui/components/bank/Swap.tsx [36]](https://github.com/BibliothecaDAO/eternum/pull/1087/files#diff-6fd1358a929cec1bf83122c321d7ff36a8a272f06787bf8f7c0c912864a04ae7R36-R36) ```diff -const [canCarry, setCanCarry] = useState(false); +const [canProceedWithSwap, setCanProceedWithSwap] = useState(false); ``` Suggestion importance[1-10]: 6 Why: While this suggestion improves code readability and maintainability, it is not crucial for functionality. Therefore, it receives a moderate score.	6