Magic number

[Mati0x]

#406

[vercel]

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

Name	Status	Preview	Comments	Updated (UTC)
gardens-v2	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Sep 30, 2024 8:54pm

[github-actions]

Review of Changes

This PR introduces a potential issue in how spending limits are calculated. It appears to be attempting to use a different formula, but the implementation is incomplete and may lead to incorrect results.

File: apps/web/components/Forms/PoolForm.tsx

Issue:

• A new variable maxRatioNum2 is introduced but never used in the subsequent calculation of weightNum. This suggests either an incomplete implementation or dead code.

Suggested Fix:

• Clarify the intended use of maxRatioNum2.
• If the intention is to use this new calculation for weightNum, update the formula accordingly. If not, remove the unused variable.

// apps/web/components/Forms/PoolForm.tsx
const maxRatioNum = spendingLimit / MAX_RATIO_CONSTANT;
// Remove or clarify the purpose of this calculation
// const maxRatioNum2 = spendingLimit / (1 - Math.sqrt(minimumConviction)); 

const weightNum = minimumConviction * maxRatioNum ** 2; 

File: apps/web/components/PoolHeader.tsx

Issue:

• The original calculation of spendingLimit is commented out and replaced with a new formula. However, it's unclear if this new formula is intended to be a direct replacement or serves a different purpose.

Suggested Fix:

• Provide context for the change in spendingLimit calculation.
• Explain the reasoning behind the new formula and its relation to the original.
• If the new calculation is intended to replace the old one, ensure it produces the expected results and doesn't introduce logic errors.

// apps/web/components/PoolHeader.tsx
// Explain why this is commented out and the reasoning behind the new calculation
// const spendingLimit = spendingLimitPct * MAX_RATIO_CONSTANT; 
const spendingLimit = 
  (strategy.config.maxRatio / CV_SCALE_PRECISION) * 
  (1 - Math.sqrt(minimumConviction / 100)) * 
  100; 

Review Summary

This PR needs clarification on the intended changes to the spending limit calculation. The addition of unused variables and commented-out code without explanation makes the intent unclear and raises concerns about potential errors.

Please address the issues outlined above to ensure the changes are correctly implemented and the code remains maintainable.

[Corantin]

GG

[Corantin]

!ai

[github-actions]

The reviews highlight several important points:

Critical Issues:

• Missing calculateMaxRatioNum Implementation: Both reviews emphasize that without the function's code, a thorough review is impossible. This is a blocker. The logic of this function is crucial to assess potential errors and security vulnerabilities.
• Security Risks: The reviews raise concerns about potential security issues within calculateMaxRatioNum, particularly:
  • Division by Zero: The code might introduce division by zero errors depending on the inputs and calculations within the function.
  • Integer Overflow/Underflow: If the function deals with user-provided inputs or large numbers, it could be vulnerable to integer overflow or underflow, potentially leading to unexpected behavior or security exploits.

Required Actions:

1. Provide calculateMaxRatioNum Implementation: Include the complete code for this function in the pull request so reviewers can understand its logic.
2. Document calculateMaxRatioNum: Add a comprehensive docstring to the function explaining its purpose, inputs, outputs, formula, and potential edge cases. This will make the code easier to understand and review.
3. Address Security Concerns: Analyze calculateMaxRatioNum for the identified security risks (division by zero, integer overflow/underflow). Implement appropriate checks and mitigations within the function to prevent these issues.
4. Add Unit Tests: Write unit tests, especially for calculateMaxRatioNum, to cover various input scenarios, including edge cases, and ensure the function behaves correctly and securely.

Additional Suggestions:

• Explain blockTime Update: Add a comment explaining why the blockTime for Arbitrum Sepolia was changed and the source of the new value (14 seconds).
• Consider Decimal Consistency: Review the use of toFixed(4) for convictionGrowthSec and consider keeping it consistent with other fields using toFixed(2) unless there's a specific reason for the difference.

Overall:

The pull request introduces potentially valuable improvements, but it's not ready to be merged until the critical issues, especially the missing implementation and potential security vulnerabilities, are addressed. Provide the missing code and address the reviewer's comments to move forward.

## Pull Request Review: Logic, Security, and Improvements

Summary:

This pull request refactors the calculation of maxRatioNum and removes the dependency on a hardcoded constant (MAX_RATIO_CONSTANT). It also introduces a new function calculateMaxRatioNum and refines the handling of spendingLimit. This makes the code more readable, maintainable, and potentially more accurate in calculating pool parameters.

File Specific Comments:

apps/web/components/Forms/PoolEditForm.tsx and apps/web/components/Forms/PoolForm.tsx:

• Positive: Extracting the calculateMaxRatioNum function improves code readability and avoids code duplication.
• Potential Improvement: The logic inside the calculateMaxRatioNum function is not provided. Please include its implementation for a complete review. Without it, potential issues within the function cannot be assessed.
• Consider: Adding unit tests to verify the correctness of the calculateMaxRatioNum function is highly recommended to prevent regressions in the future.

apps/web/components/PoolHeader.tsx:

• Positive: Recalculating spendingLimit based on strategy.config.maxRatio, minimumConviction, and CV_SCALE_PRECISION is a good practice as it keeps the value consistent and reflects the actual configuration.
• Minor: Using toFixed(4) for convictionGrowthSec might be visually inconsistent with other fields using toFixed(2). Consider keeping the decimal places consistent unless there's a specific reason for the difference.

Security Considerations:

• Without seeing the implementation of calculateMaxRatioNum, it's impossible to assess potential vulnerabilities like division by zero or integer overflow issues. A thorough review of that function's implementation is crucial.

Overall:

This PR shows good progress in improving code quality and potentially accuracy. However, a complete review of the calculateMaxRatioNum function is necessary to ensure its logic is sound and doesn't introduce new vulnerabilities. Adding unit tests for this function is also highly recommended.

Review of Pull Request: Update blockTime for Arbitrum Sepolia and add calculateMaxRatioNum function

This PR makes two main changes:

1. Updates the blockTime for Arbitrum Sepolia in apps/web/configs/chains.tsx.
2. Adds a new function calculateMaxRatioNum in apps/web/utils/numbers.ts.

File: apps/web/configs/chains.tsx

• Positive: Updating the blockTime for Arbitrum Sepolia to 14 seconds is crucial for accurate calculations on the network. The previous value of 0.23 seconds is significantly off and would lead to incorrect estimations.
• Suggestion: Consider adding a comment explaining the reason for this change and the source of the new blockTime value.

File: apps/web/utils/numbers.ts

• Needs more information: The purpose and usage of the calculateMaxRatioNum function are unclear. Without understanding its context within the codebase, it's difficult to assess its logic, potential security implications, or suggest improvements.
• Suggestion: Add a detailed docstring explaining:
  • The purpose of the function and its expected inputs and outputs.
  • The meaning of spendingLimit and minimumConviction.
  • The formula used and its derivation.
  • Any potential edge cases or limitations.
• Security: Depending on the function's context and how it's used, there might be security implications related to integer overflow or underflow, especially when dealing with user-provided inputs. These risks should be analyzed and mitigated.

Review Summary

This PR addresses a crucial issue by updating the blockTime for Arbitrum Sepolia. However, the newly added calculateMaxRatioNum function needs further clarification and security analysis.

Overall Score: Needs Work

Next Steps:

• Add a comment explaining the blockTime update.

• Provide comprehensive documentation for the calculateMaxRatioNum function.

• Analyze the calculateMaxRatioNum function for potential security risks, particularly related to integer overflow and underflow.

[Corantin]

Wait

[Corantin]

LGTM