Skip to content

Conversation

nezz0746
Copy link
Contributor

@nezz0746 nezz0746 commented Sep 30, 2024

A scientific number breaks the cashout flow. Using BigInt to make sure the Number is being formatted correctly to be sent to the SDK

Summary by CodeRabbit

  • Bug Fixes
    • Improved precision in token amount calculations by switching to BigInt.
    • Enhanced error handling with clearer console messages for route fetching issues.

Copy link

vercel bot commented Sep 30, 2024

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

Name Status Preview Comments Updated (UTC)
peanut-ui ✅ Ready (Inspect) Visit Preview 💬 Add feedback Sep 30, 2024 7:34pm

Copy link
Contributor

coderabbitai bot commented Sep 30, 2024

📝 Walkthrough

Walkthrough

The pull request modifies the fetchRouteRaw function in the src/utils/cashout.utils.ts file. The primary change involves the calculation of the _tokenAmount variable, which now utilizes BigInt for improved precision with large numbers. Additionally, the error handling within the function has been refined, with a more specific error message related to route fetching being logged.

Changes

File Change Summary
src/utils/cashout.utils.ts Updated _tokenAmount calculation to use BigInt for precision; refined error handling in logging.

Possibly related PRs

  • Fix fetching tx fee #379: Enhancements in error handling and logic for transaction fee estimation, which may relate to the improvements in the fetchRouteRaw function's error handling.

📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 63a0d62 and e228856.

📒 Files selected for processing (1)
  • src/utils/cashout.utils.ts (1 hunks)
🔇 Additional comments (3)
src/utils/cashout.utils.ts (3)

Line range hint 518-534: Overall assessment of the changes

The modifications to fetchRouteRaw address the scientific notation issue by using BigInt. This is a step in the right direction for handling large numbers accurately. However, there are opportunities for further improvements:

  1. Refine the BigInt conversion process for better precision and error handling.
  2. Parameterize the hardcoded toAddress for increased flexibility.
  3. Enhance error handling to provide more meaningful feedback to the calling code.

These suggested changes will make the function more robust, flexible, and maintainable. Please consider implementing these improvements to further enhance the reliability of the cashout process.

To ensure that these changes don't have unintended consequences, please run the following verification script:

#!/bin/bash
# Description: Comprehensive check for related functionality

echo "Checking for other BigInt usage in the codebase:"
rg --type typescript 'BigInt\('

echo "Searching for other functions that might need similar updates:"
rg --type typescript 'Math\.floor.*Math\.pow'

echo "Checking for other occurrences of error handling in utility functions:"
rg --type typescript 'catch \(error\)' 'src/utils'

echo "Searching for other hardcoded addresses in the codebase:"
rg --type typescript '0x[a-fA-F0-9]{40}'

This comprehensive check will help identify any other areas of the codebase that might benefit from similar updates or need to be adjusted based on these changes.


518-518: 🛠️ Refactor suggestion

Improve BigInt conversion for better precision and error handling

The change to use BigInt addresses the issue with scientific notation. However, there are a few improvements we can make:

  1. Remove Math.floor as it's unnecessary when using BigInt.
  2. Avoid converting to Number before multiplication to prevent potential precision loss.
  3. Add error handling for the BigInt conversion.

Consider refactoring the _tokenAmount calculation as follows:

-const _tokenAmount = BigInt(Math.floor(Number(tokenAmount) * Math.pow(10, tokenDecimals))).toString()
+const _tokenAmount = (() => {
+  try {
+    return (BigInt(tokenAmount) * BigInt(10) ** BigInt(tokenDecimals)).toString()
+  } catch (error) {
+    console.error('Error converting tokenAmount to BigInt:', error)
+    throw new Error('Invalid token amount')
+  }
+})()

This approach:

  • Maintains precision by using BigInt throughout the calculation.
  • Adds error handling to catch any issues with the input tokenAmount.
  • Uses the ** operator for exponentiation, which is more readable than Math.pow.

To ensure this change doesn't introduce any regressions, please run the following verification script:

This script will help identify if similar conversions exist elsewhere in the codebase that might need similar updates.

✅ Verification successful

BigInt conversion verified: No additional occurrences found

The changes to _tokenAmount in src/utils/cashout.utils.ts are correctly implemented, and no other similar conversions were found in the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for other occurrences of tokenAmount conversion in the codebase

echo "Searching for other tokenAmount conversions:"
rg --type typescript 'tokenAmount.*Math\.pow' -g '!src/utils/cashout.utils.ts'

echo "Searching for other BigInt conversions:"
rg --type typescript 'BigInt\(' -g '!src/utils/cashout.utils.ts'

Length of output: 405


Script:

#!/bin/bash
# Description: Check for other occurrences of tokenAmount conversion in the codebase

echo "Searching for other tokenAmount conversions:"
rg -g '*.ts' 'tokenAmount.*Math\.pow' -g '!src/utils/cashout.utils.ts'

echo "Searching for other BigInt conversions:"
rg -g '*.ts' 'BigInt\(' -g '!src/utils/cashout.utils.ts'

Length of output: 319


Line range hint 520-534: Improve error handling and consider parameterizing the toAddress

While the main change addresses the scientific notation issue, there are two additional points to consider:

  1. The toAddress is hardcoded. This might cause issues if the address needs to change or if it's environment-specific.
  2. The error handling could be more robust to provide better feedback to the calling code.

Consider the following improvements:

  1. Parameterize the toAddress:
-export const fetchRouteRaw = async (
+export const fetchRouteRaw = async (
   fromToken: string,
   fromChain: string,
   toToken: string,
   toChain: string,
   tokenDecimals: number,
   tokenAmount: string,
-  senderAddress: string
+  senderAddress: string,
+  toAddress: string
 ) => {
   // ...
-  toAddress: '0x04B5f21facD2ef7c7dbdEe7EbCFBC68616adC45C',
+  toAddress: toAddress,
   // ...
 }
  1. Enhance error handling:
 } catch (error) {
-  console.error('Error fetching route:', error)
-  return undefined
+  console.error('Error fetching route:', error)
+  if (error instanceof Error) {
+    throw new Error(`Failed to fetch route: ${error.message}`)
+  } else {
+    throw new Error('An unknown error occurred while fetching the route')
+  }
 }

These changes will make the function more flexible and provide better error information to the calling code.

To ensure these changes don't introduce any regressions, please run the following verification script:

This script will help identify if there are other places in the codebase that might need to be updated based on these changes.

✅ Verification successful

Hardcoded toAddress confirmed in src/utils/cashout.utils.ts.

The toAddress is indeed hardcoded in src/utils/cashout.utils.ts and should be parameterized as suggested. No other usages of fetchRouteRaw or unintended hardcoded addresses were found in the codebase.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for other occurrences of fetchRouteRaw usage and hardcoded addresses

echo "Searching for fetchRouteRaw usage:"
rg --type typescript 'fetchRouteRaw\('

echo "Searching for hardcoded addresses similar to the one in fetchRouteRaw:"
rg --type typescript '0x[a-fA-F0-9]{40}'

Length of output: 831


Script:

#!/bin/bash
# Description: Check for other occurrences of fetchRouteRaw usage and hardcoded addresses

echo "Searching for fetchRouteRaw usage in TypeScript files:"
rg 'fetchRouteRaw\(' --glob '*.ts' -A 2

echo "Searching for hardcoded addresses in TypeScript files:"
rg '0x[a-fA-F0-9]{40}' --glob '*.ts' -A 2

Length of output: 3726


Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments.

CodeRabbit Commands (Invoked using PR comments)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@Hugo0 Hugo0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah always use Bigint math where possible. If you find other places in the code where we're not doing that, pls update too

merging

@Hugo0 Hugo0 merged commit 9c38bfe into develop Oct 1, 2024
2 checks passed
@Hugo0 Hugo0 deleted the fix/large-token-amount-format branch July 3, 2025 18:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants