-
Notifications
You must be signed in to change notification settings - Fork 579
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
Estimate fee RPC endpoint #2428
Conversation
throw new ValidationError(`No account found with name ${request.data.fromAccountName}`) | ||
} | ||
|
||
const priority = request.data.priority || 'medium' |
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.
If there's no priority passed in I'm wondering if it's better to default to medium or to fetch estimates for all levels?
When this is used in the CLI pay
and deposit
commands, will we make separate requests for each priority level?
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.
updated the interface
await node.chain.addBlock(block) | ||
await node.wallet.updateHead() | ||
}) | ||
|
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.
It doesn't look like there is a test in here for the different priority levels, do they exist already elsewhere?
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.
Yeah, the test with different priority level is in https://github.com/iron-fish/ironfish/ironfish/src/memPool/feeEstimator.test.ts
const low = await feeEstimator.estimateFee('low', account, receives) | ||
const medium = await feeEstimator.estimateFee('medium', account, receives) | ||
const high = await feeEstimator.estimateFee('high', account, receives) |
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.
For a followup PR - this will result in iterating over the notes for a transaction three times.
Maybe when TransactionBuilder lands we should explore caching a built transaction between calls to estimateFee and sendTransaction?
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.
sounds good!
Summary
RPC for estimate fee. This will return the suggested fee for a proposed transaction.
Testing Plan
unit test
Breaking Change
Is this a breaking change? If yes, add notes below on why this is breaking and
what additional work is required, if any.
@hughy @NullSoldier