Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Adjustments for fee voting #2812
base: main
Are you sure you want to change the base?
Adjustments for fee voting #2812
Changes from 3 commits
56723e1
d96931b
bb3156d
80a6d1f
d907ce5
a36a8f0
a2a9a91
309e3c2
4da156d
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
What is the need for this comparison with
maxFeeDrops
( ==2,000,000
drops) ?the
baseFee
variable computes the final fees, accounting for the cases where multiple-signers are provided.Why are performing this arbitrary comparison with
2 XRP
? Moreover, we are not using anycushion
value in thegetFeeXrp()
function call, hence there is no possibility of the library over-estimating the 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.
This part confuses me a bit as well since baseFee computed the final fee with multi-signers accounted so not sure why we need this comparison.
@khancode could you explain why since you wrote this code a while ago in this PR
Btw, cushion is defaulted to 1.2 if not set.
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.
We can omit the
median_fee
field from the deep-equality comparison. Quoting from the docs:An approximation of the median transaction cost among transactions included in the previous validated ledger, represented in drops of XRP.
Since this value depends on the transactions-mix of the previous validated ledger, it is not informative for integration test status. Docs: https://xrpl.org/docs/references/http-websocket-apis/public-api-methods/server-info-methods/fee#response-format
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.
Hello,
The intention in these tests is not to fund an account into existence. Hence, the Amount field could be any positive value, it does not have to be above the reserve requirement.
It looks like the
amount
value is always equal toDEFAULT_AMOUNT
. So, I don't see a need for anotheramount
variable.Are changes in this file required? I don't this file is impacted by the fee-change-calculation updates
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 is because the first payment transaction in this test
Validate Payment transaction API v2: Payment Transaction: Specify Only Amount field
will create the hardcoded accountrfkE1aSy9G8Upk4JssnwBxhEv5p4mn2KTy
, which does not exist beforehand. So if we increaseaccount_reserve = 20000000
instead of 10XRP then all payment transactions in these tests will fail:xrpl: Message: xrpl: Destination does not exist. Too little XRP sent to create it.: expected 'tecNO_DST_INSUF_XRP' to equal 'tesSUCCESS'
Hence we either have to make this first payment to be at least the account_reserve or setup a destination account before sending payments
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.
ah okay, thanks for the clarification
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.
Changing only line 18 to
const AMOUNT = '10000000'
, could have been sufficient. But this change is also fine.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.
🛠️ Refactor suggestion
Consider adding error handling for fetchAccountReserveFee.
While the fallback to DEFAULT_AMOUNT is good, it would be beneficial to add error handling and logging to understand when and why the fetch fails. This helps with test debugging and maintenance.
📝 Committable suggestion
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.
@ckeshava, you make a valid point. Since the XRPL mainnet can vote for arbitrary fee values, we should not default to any specific amount. Instead, we should throw an error if we're unable to retrieve the fees from the latest validated ledger.
Here's how we can adjust the code:
This change ensures that if we are unable to retrieve the fee from the latest validated ledger, the test will fail, making us aware of the issue rather than using a potentially incorrect default amount.
(・_・)
✏️ Learnings added
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.
Fix misleading assertion message
The error message "The destination's balance should not change yet" doesn't match the context of this final balance assertion, which actually verifies the correct balance after all changes.
Apply this change:
📝 Committable suggestion
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.
Instead of creating a new function, can we re-use the one from
packages/xrpl/src/sugar/autofill.ts
? Its titledfetchOwnerReserve
This will improve the test-coverage of the function which is already in the main codebase.
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.
Hi Chenna, OwnerReserve and AccountReserve refers to 2 different fees. One would be
reserve_inc
and the otherreserve_base
.Please refer to: https://xrpl.org/docs/references/protocol/ledger-data/ledger-entry-types/feesettings
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.
okay, thanks for the clarification. Personally, I'd have preferred the names
reserve_base
andreserve_increment
because its easier to understand their intent. But its not a strong opinion.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.
🛠️ Refactor suggestion
Enhance error handling and type safety in
fetchAccountReserveFee
.While the implementation aligns with the PR objectives to support dynamic fee calculations, consider these improvements:
Here's a suggested implementation with these improvements:
📝 Committable suggestion