-
Notifications
You must be signed in to change notification settings - Fork 7
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
Conversation
WalkthroughWalkthroughThe pull request introduces modifications to TypeScript interfaces and functions related to cross-chain transaction handling. It adds a Changes
Possibly related PRs
Recent review detailsConfiguration used: CodeRabbit UI Files selected for processing (3)
Files skipped from review as they are similar to previous changes (2)
Additional comments not posted (3)
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? TipsChatThere are 3 ways to chat with CodeRabbit:
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)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this 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 theISquidRoute
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:
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 likenumber
orstring
.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
orestimatedCost
. This will make the code more self-explanatory and maintainable.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
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 thefeeCosts
andgasCosts
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
andfeeEstimation
properties. It also correctly converts thefeeEstimation
value to a string usingtoString()
before returning it.
Line range hint
247-314
: Overall assessmentThe code changes introduce a new feature for estimating transaction fees by summing up the
amountUsd
values from thefeeCosts
andgasCosts
arrays. The code changes also modify the return object of theprepareXchainRequestFulfillmentTransaction
function to include the calculatedfeeEstimation
.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 theIPrepareXchainRequestFulfillmentTransactionProps
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 newestimate
property enhances thegetSquidRoute
function.The addition of the optional
estimate
property, derived fromdata.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.
There was a problem hiding this 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.
?
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
Bug Fixes