-
Notifications
You must be signed in to change notification settings - Fork 102
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
Better fee calculation #370
Conversation
If this PR should be merged should I add the changes as 'Added' or 'Fixed' in the |
I am a little confused how the integration tests work. Every rippled api request you make is basically made via localhost, right? Why does the Thank you :) You guys already taught me so much 🙏 |
My main worry with that is if updating xrpl.js version changes how much money people spend on fees without any warning. While I do think the dynamic calculation is better, I don't want to increase how much people are paying without their explicit opt-in. So my thought was that it'd be backwards compatible to keep the calculations the same unless folks opt-in to the better calculations. That being said, I haven't done the math to see if the new calculations are higher than the previous calculations. If they're always lower when the queue is empty, then I think it makes sense to just use the better fee calculation as the default. |
Ah I did not think about that. I agree to you, the "dynamic" option should not be the default to prevent unwanted higher fees :) |
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.
LGTM :)
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.
Left a couple notes on where to leave some additional comments for better code readability, looks good otherwise
High Level Overview of Change
Use of better
fee calculation formula
but keep supporting the current. The current way is marked as deprecated.Add info to the docstring that
get_fee
is returning drops.Context of Change
As proposed in #330 and #352
Type of Change
Test Plan
None