Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Changing fee estimation function #150

Merged
merged 3 commits into from
Sep 17, 2024
Merged

Changing fee estimation function #150

merged 3 commits into from
Sep 17, 2024

Conversation

pawell24
Copy link
Contributor

@pawell24 pawell24 commented Sep 16, 2024

Change in the way network fee is calculated, currently we base on data returned by squid api, i.e. gas cost and fee cost. We add all values ​​from these tables - feeCosts and gasCosts. Based on this data we can estimate network fee.

Summary by CodeRabbit

  • New Features

    • Added fee estimation functionality for cross-chain transaction preparation.
    • Introduced an estimate property in route data for enhanced routing information.
  • Bug Fixes

    • Removed outdated fee calculation logic from tests, streamlining focus on transaction signing and submission.

Copy link

coderabbitai bot commented Sep 16, 2024

Walkthrough

Walkthrough

The pull request introduces modifications to TypeScript interfaces and functions related to cross-chain transaction handling. It adds a feeEstimation property to the IPrepareXchainRequestFulfillmentTransactionProps interface and an optional txEstimation property to the ISquidRoute interface. The getSquidRoute function now returns a transaction estimation, while the prepareXchainRequestFulfillmentTransaction function includes a new fee estimation calculation, replacing the previous fee calculation function. Corresponding test cases have been updated to reflect these changes.

Changes

File Change Summary
src/consts/interfaces.consts.ts - Added feeEstimation: string to IPrepareXchainRequestFulfillmentTransactionProps.
- Added optional txEstimation?: any to ISquidRoute.
src/index.ts - Added txEstimation property in the return object of getSquidRoute function.
src/request.ts - Modified prepareXchainRequestFulfillmentTransaction to include feeEstimation: feeEstimation.toString() in the return object.
- Removed calculateCrossChainTxFee function entirely.
test/basic/RequestLinkXChain.test.ts - Removed fee calculation logic from test cases related to cross-chain transaction fees.

Possibly related PRs

  • Request Links #147: Involves modifications to the src/consts/interfaces.consts.ts file, specifically adding properties to the IPrepareXchainRequestFulfillmentTransactionProps and ISquidRoute interfaces, which are directly related to the changes made in the main PR regarding the same interfaces.

Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between c92a17b and 65ea9b9.

Files selected for processing (3)
  • src/consts/interfaces.consts.ts (2 hunks)
  • src/index.ts (1 hunks)
  • src/request.ts (2 hunks)
Files skipped from review as they are similar to previous changes (2)
  • src/consts/interfaces.consts.ts
  • src/index.ts
Additional comments not posted (3)
src/request.ts (3)

247-266: Great job adding comments to explain the fee estimation process!

The code segment correctly calculates the total estimated fees by summing the USD values from feeCosts and gasCosts arrays. The added comments provide a clear explanation of the purpose of each variable and the reason for iterating over the arrays, which will be helpful for future maintainers.


321-321: LGTM!

The change to include the feeEstimation value in the return object aligns with the function's purpose and provides the caller with the estimated fees for the transaction.


247-266: Past review comments addressed.

The concerns raised in the previous review comments have been addressed by the added comments in the code segment. The comments now provide a clear explanation of the fee estimation process, the purpose of feeCosts and gasCosts, and the reason for iterating over the arrays to calculate the total estimated fees.


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 generate interesting stats about this repository and render them as a table.
    -- @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

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 0

Outside diff range and nitpick comments (1)
src/consts/interfaces.consts.ts (1)

210-210: Consider using a more specific type and a more descriptive name.

The addition of the optional estimate property to the ISquidRoute interface provides flexibility by allowing it to hold an estimate value of any type. However, there are a couple of suggestions to improve the code:

  1. Consider using a more specific type instead of any to provide better type safety and clarity. If the estimate represents a specific value, such as a cost or time estimate, consider using a more appropriate type like number or string.

  2. Consider renaming the property to be more descriptive about what the estimate represents. For example, if it represents a cost estimate, you could name it costEstimate or estimatedCost. This will make the code more self-explanatory and maintainable.

Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

Commits

Files that changed from the base of the PR and between f022cbc and c92a17b.

Files selected for processing (4)
  • src/consts/interfaces.consts.ts (2 hunks)
  • src/index.ts (1 hunks)
  • src/request.ts (2 hunks)
  • test/basic/RequestLinkXChain.test.ts (0 hunks)
Files not reviewed due to no reviewable changes (1)
  • test/basic/RequestLinkXChain.test.ts
Additional comments not posted (5)
src/request.ts (3)

247-259: LGTM!

The code segment correctly calculates the fee estimation by summing up the amountUsd values from the feeCosts and gasCosts arrays. It also correctly checks if the arrays are not empty before iterating over them to avoid unnecessary iterations.


314-314: LGTM!

The code segment correctly returns an object with unsignedTxs and feeEstimation properties. It also correctly converts the feeEstimation value to a string using toString() before returning it.


Line range hint 247-314: Overall assessment

The code changes introduce a new feature for estimating transaction fees by summing up the amountUsd values from the feeCosts and gasCosts arrays. The code changes also modify the return object of the prepareXchainRequestFulfillmentTransaction function to include the calculated feeEstimation.

The code changes are correctly implementing the new feature and do not seem to have any impact on the existing functionality. The code is well-structured and follows good practices such as checking if the arrays are not empty before iterating over them and converting the feeEstimation value to a string before returning it.

Great job on implementing this new feature! 👍

src/consts/interfaces.consts.ts (1)

172-172: LGTM!

The addition of the feeEstimation property to the IPrepareXchainRequestFulfillmentTransactionProps interface is a good enhancement. It provides more information about the estimated fees for the transaction, which can be useful for various purposes.

The property name is clear and the type string is appropriate for holding a string representation of the fee estimation.

src/index.ts (1)

Line range hint 2321-2330: Approved: The new estimate property enhances the getSquidRoute function.

The addition of the optional estimate property, derived from data.route.estimate, provides valuable extra information about the squid route to the consumers of this function. This change is backward compatible and allows downstream code to access and utilize the additional route details when available.

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.

@pawell24 you agreed in our call to explain why the changes where needed - I don't see any explanation.

?

The only summary of PR i see is done by coderabbit
image

@Hugo0 Hugo0 merged commit e27a639 into main Sep 17, 2024
1 check failed
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