Adjustments for fee voting #2812
Conversation
WalkthroughThe pull request introduces several updates across multiple files, focusing on the configuration and functionality of the XRPL ecosystem. A new Changes
Suggested reviewers
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
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
packages/xrpl/test/integration/requests/fee.test.ts (2)
40-45: LGTM: Robust fee calculations with precision handling.The dynamic fee calculations are well-implemented with proper precision handling using BigNumber. However, consider adding test cases for edge scenarios:
- Maximum possible fee values
- Minimum acceptable fee values
- Fee calculations with different network load conditions
Example test case structure:
it('handles maximum network load fees', async () => { // Test with high current_ledger_size and current_queue_size }) it('handles minimum acceptable fees', async () => { // Test with minimum network conditions })
Line range hint
1-70: Consider expanding test coverage for fee calculations.While the current test covers the basic fee calculation scenario, consider adding a comprehensive test suite that validates:
- Fee calculations across different transaction types
- Fee adjustments based on network load
- Integration with the new Fee Voting mechanism
- Edge cases around reserve requirements
This would ensure the library remains reliable as the XRPL fee structure evolves.
.ci-config/rippled.cfg (1)
182-183: Consider enhancing the documentation.While the comment indicates this is for simulating FeeSettings scenarios, it would be helpful to add more context about:
- The relationship to current mainnet values
- The purpose of these specific values in CI/testing scenarios
-# # This section can be used to simulate various FeeSettings scenarios for rippled node in standalone mode +# This section configures FeeSettings for standalone mode to match expected mainnet values +# These values reflect ongoing fee voting by mainnet UNL validators: +# - reference_fee: approaching 200 drops on mainnet +# - reserves: configured to test with realistic mainnet valuespackages/xrpl/HISTORY.md (2)
Line range hint
1-24: Consider adding SHA checksums for recent releases.Recent releases are missing the SHA-256 checksums that were historically included for browser builds. For consistency and security verification, consider adding these checksums back for the latest releases.
Add a section at the end of each recent release with SHA-256 checksums, following the established format:
The SHA-256 checksums for the browser version of this release can be found below. % shasum -a 256 * <checksum> ripple-<version>-debug.js <checksum> ripple-<version>-min.js <checksum> ripple-<version>.js
Line range hint
1-24: Consider enhancing documentation with more consistent issue/PR linking.While the changelog is well-documented, consider enhancing it by:
- Consistently linking to related GitHub issues/PRs using the format
(#<number>)- Adding links to migration guides when breaking changes are introduced
- Including references to relevant rippled version requirements
For example, the latest changes could include:
### Fixed * Remove hard-coded reference to 10 drops as the transaction cost (#2812) * Fix `calculateFeePerTransaction` for `AMMCreate` operations (#2812)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (5)
- .ci-config/rippled.cfg (1 hunks)
- packages/xrpl/HISTORY.md (1 hunks)
- packages/xrpl/src/sugar/autofill.ts (3 hunks)
- packages/xrpl/test/integration/requests/fee.test.ts (2 hunks)
- packages/xrpl/test/integration/transactions/xchainClaim.test.ts (3 hunks)
🔇 Additional comments (8)
packages/xrpl/test/integration/requests/fee.test.ts (2)
5-6: LGTM: Appropriate imports for dynamic fee handling.The addition of
xrpToDropsandgetFeeXrpimports aligns well with the PR's objective to move away from hard-coded fee values.
31-32: Document and verify the MEDIAN_FEE_MULTIPLIER value.While the dynamic fee calculation is good, the
MEDIAN_FEE_MULTIPLIERvalue of 500 needs documentation explaining its significance and how it was determined. Given that mainnet UNL validators are approaching 200 drops for base_fee, we should verify if this multiplier aligns with expected fee calculations.packages/xrpl/test/integration/transactions/xchainClaim.test.ts (2)
12-12: LGTM: Import of dynamic fee calculation utilityThe addition of
getFeeXrpimport aligns with the PR's objective to move away from hardcoded fee values.
43-43: LGTM: Dynamic fee calculation implementationThe implementation correctly retrieves and converts the network fee to drops, replacing the hardcoded value.
Let's verify consistency across other test files:
✅ Verification successful
Let me analyze the specific usage of fees by examining the content of these files.
Based on the analysis of the test files, I can now generate the final response:
Dynamic fee calculation is correctly implemented and consistent
The implementation in
xchainClaim.test.tsfollows the established pattern in the codebase where:
getFeeXrpis used to fetch dynamic network fees- The result is converted to drops using
xrpToDrops- Test files properly handle both normal and high load factor scenarios
- The fee calculation includes proper cushioning and maximum fee handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for any remaining hardcoded fee values in test files # and verify consistent usage of getFeeXrp # Check for hardcoded fee values in test files rg -l "(?i)(fee.*?[0-9]+|[0-9]+.*?fee)" "packages/xrpl/test" # Verify consistent usage of getFeeXrp rg -l "getFeeXrp" "packages/xrpl/test"Length of output: 5321
Script:
#!/bin/bash # Check the content of fee-related test files rg -A 5 -B 5 "getFeeXrp" packages/xrpl/test/integration/requests/fee.test.ts packages/xrpl/test/client/getFeeXrp.test.ts # Check for fee handling in integration tests rg "fee.*=|=.*fee" "packages/xrpl/test/integration/transactions/"Length of output: 8934
.ci-config/rippled.cfg (1)
182-186: LGTM! Values align with mainnet fee voting trends.The new
[voting]section correctly implements the fee settings to match the expected mainnet values, particularly thereference_feeof 200 drops which aligns with the current UNL validator voting trends.packages/xrpl/src/sugar/autofill.ts (2)
Line range hint
233-248: LGTM! Function rename improves clarity.The renaming from
fetchAccountDeleteFeetofetchOwnerReserveFeebetter reflects its purpose of fetching the owner reserve increment from the server state. This aligns well with the PR's objective to accommodate fee voting changes.
Line range hint
270-282: LGTM! Fee calculation logic updated correctly.The changes correctly implement the fee calculation for special transaction types (AccountDelete and AMMCreate) using the owner reserve fee, which aligns with the PR objectives to handle fee voting changes.
packages/xrpl/HISTORY.md (1)
10-11: Verify that the changelog entry matches the PR objectives.The changelog entry correctly documents the bug fix for removing the hard-coded reference to 10 drops as the transaction cost. This aligns with the PR objectives to accommodate changes in FeeSettings values from Fee Voting.
However, to better reflect all the changes mentioned in the PR objectives, consider expanding the changelog entry to also mention:
- The modifications to
calculateFeePerTransactionforAMMCreateoperations- The context about mainnet UNL validators' fee voting approaching 200 drops
| initialBalance + | ||
| Number(amount) - | ||
| Number(signatureReward) - | ||
| Number(netFee), |
There was a problem hiding this comment.
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:
finalBalance,
initialBalance +
Number(amount) -
Number(signatureReward) -
Number(netFee),
- "The destination's balance should not change yet",
+ "The destination's final balance should reflect the claim amount minus fees",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| initialBalance + | |
| Number(amount) - | |
| Number(signatureReward) - | |
| Number(netFee), | |
| finalBalance, | |
| initialBalance + | |
| Number(amount) - | |
| Number(signatureReward) - | |
| Number(netFee), | |
| "The destination's final balance should reflect the claim amount minus fees", |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
packages/xrpl/test/integration/transactions/payment.test.ts (1)
22-23: Add documentation for DEFAULT_AMOUNT constant.Consider adding a comment explaining why this specific value was chosen as the default amount and its significance in the context of account reserves and payments.
packages/xrpl/test/integration/utils.ts (1)
449-460: Add JSDoc documentation to improve maintainability.The function implementation aligns well with the PR objectives for handling dynamic FeeSettings. Consider adding documentation to improve maintainability.
Add JSDoc documentation:
+/** + * Fetches the account reserve fee from the server state. + * This fee is subject to change through Fee Voting by validators. + * + * @param client - The XRPL client instance + * @returns A promise that resolves to: + * - The reserve fee as a string (rounded up to the nearest integer) + * - null if the fee is not available in the validated ledger + * @throws Error if the request fails or if the fee is negative + */ export async function fetchAccountReserveFee( client: Client, ): Promise<string | null> {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
- packages/xrpl/test/integration/transactions/payment.test.ts (5 hunks)
- packages/xrpl/test/integration/utils.ts (2 hunks)
🔇 Additional comments (5)
packages/xrpl/test/integration/transactions/payment.test.ts (4)
10-14: LGTM! Import changes align with dynamic fee calculation requirements.
33-33: LGTM! Successfully implements dynamic amount in payment transaction.
Line range hint
1-112: Verify integration with fee voting changes.While the changes successfully implement dynamic amounts based on account reserves, we should verify that these tests adequately cover scenarios related to the fee voting changes mentioned in the PR objectives.
71-71: Verify edge cases in amount validation.While the assertions are correctly updated to use the dynamic amount, consider adding test cases for edge cases such as:
- Minimum possible amount
- Maximum possible amount
- Amount just above/below reserve requirements
Also applies to: 91-91, 109-109
packages/xrpl/test/integration/utils.ts (1)
449-460: Verify the integration with payment tests.Let's ensure the new function is properly integrated with the payment tests and doesn't have unintended effects on other test utilities.
✅ Verification successful
Let me check the actual usage in payment.test.ts to ensure proper integration.
Integration verified successfully
The
fetchAccountReserveFeefunction is properly integrated:
- It's correctly exported from utils.ts and imported in payment.test.ts
- Used appropriately in payment tests to determine minimum reserve requirement
- No conflicts with other fee-related functions in the test utilities
- Implementation follows the codebase's patterns for fee handling
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check integration of fetchAccountReserveFee with payment tests and other utilities # Test 1: Check if the function is used in payment tests echo "Checking usage in payment tests..." rg -l "fetchAccountReserveFee" "packages/xrpl/test/integration/transactions/" # Test 2: Check for potential conflicts with other fee-related functions echo "Checking for related fee functions..." rg -l "Fee|fee" "packages/xrpl/test/integration/" # Test 3: Check if the function is properly exported and imported echo "Checking exports and imports..." rg -l "export.*fetchAccountReserveFee|import.*fetchAccountReserveFee" "packages/xrpl/test/"Length of output: 1296
Script:
#!/bin/bash # Check the actual usage of fetchAccountReserveFee in payment.test.ts echo "Checking usage context in payment.test.ts..." rg -B 2 -A 2 "fetchAccountReserveFee" "packages/xrpl/test/integration/transactions/payment.test.ts" # Check the implementation of related fee functions in utils.ts echo -e "\nChecking related fee functions in utils.ts..." rg "Fee|fee" "packages/xrpl/test/integration/utils.ts"Length of output: 915
| // Make sure the amount sent satisfies minimum reserve requirement to fund an account. | ||
| amount = | ||
| (await fetchAccountReserveFee(testContext.client)) ?? DEFAULT_AMOUNT |
There was a problem hiding this comment.
🛠️ 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.
// Make sure the amount sent satisfies minimum reserve requirement to fund an account.
- amount =
- (await fetchAccountReserveFee(testContext.client)) ?? DEFAULT_AMOUNT
+ try {
+ amount = await fetchAccountReserveFee(testContext.client)
+ if (!amount) {
+ console.warn('Failed to fetch account reserve fee, using default amount')
+ amount = DEFAULT_AMOUNT
+ }
+ } catch (error) {
+ console.warn(`Error fetching account reserve fee: ${error.message}`)
+ amount = DEFAULT_AMOUNT
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // Make sure the amount sent satisfies minimum reserve requirement to fund an account. | |
| amount = | |
| (await fetchAccountReserveFee(testContext.client)) ?? DEFAULT_AMOUNT | |
| // Make sure the amount sent satisfies minimum reserve requirement to fund an account. | |
| try { | |
| amount = await fetchAccountReserveFee(testContext.client) | |
| if (!amount) { | |
| console.warn('Failed to fetch account reserve fee, using default amount') | |
| amount = DEFAULT_AMOUNT | |
| } | |
| } catch (error) { | |
| console.warn(`Error fetching account reserve fee: ${error.message}`) | |
| amount = DEFAULT_AMOUNT | |
| } |
There was a problem hiding this comment.
@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:
// Remove the DEFAULT_AMOUNT constant
// const DEFAULT_AMOUNT = '10000000';
// Update the code to throw an error if fetching the amount fails
beforeAll(async () => {
testContext = await setupClient(serverUrl)
senderWallet = await generateFundedWallet(testContext.client)
// Fetch the account reserve fee
amount = await fetchAccountReserveFee(testContext.client)
if (!amount) {
throw new Error('Failed to fetch account reserve fee from the latest validated ledger.')
}
})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
Learnt from: ckeshava
PR: XRPLF/xrpl.js#2812
File: packages/xrpl/test/integration/transactions/payment.test.ts:41-43
Timestamp: 2024-10-30T01:05:33.417Z
Learning: In tests involving fee calculations for XRPL, avoid using default fee amounts. If unable to retrieve fee values from the latest validated ledger, throw an error instead of defaulting to a specific amount.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| export async function fetchAccountReserveFee( | ||
| client: Client, | ||
| ): Promise<string | null> { | ||
| const response = await client.request({ command: 'server_state' }) | ||
| const fee = response.result.state.validated_ledger?.reserve_inc | ||
|
|
||
| if (fee == null) { | ||
| return null | ||
| } | ||
|
|
||
| return new BigNumber(fee).dp(0, BigNumber.ROUND_CEIL).toString(10) | ||
| } |
There was a problem hiding this comment.
🛠️ 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:
- Add error handling for failed requests
- Add type safety for the response structure
- Add validation for negative values
Here's a suggested implementation with these improvements:
+interface ServerState {
+ result: {
+ state: {
+ validated_ledger?: {
+ reserve_inc?: string
+ }
+ }
+ }
+}
export async function fetchAccountReserveFee(
client: Client,
): Promise<string | null> {
- const response = await client.request({ command: 'server_state' })
- const fee = response.result.state.validated_ledger?.reserve_inc
+ try {
+ const response = await client.request({ command: 'server_state' }) as ServerState
+ const fee = response.result.state.validated_ledger?.reserve_inc
+ if (fee == null) {
+ return null
+ }
+ const feeBN = new BigNumber(fee)
+ if (feeBN.isNegative()) {
+ throw new Error('Reserve fee cannot be negative')
+ }
+ return feeBN.dp(0, BigNumber.ROUND_CEIL).toString(10)
+ } catch (error) {
+ if (error instanceof Error) {
+ throw new Error(`Failed to fetch reserve fee: ${error.message}`)
+ }
+ throw new Error('Failed to fetch reserve fee')
+ }
- if (fee == null) {
- return null
- }
-
- return new BigNumber(fee).dp(0, BigNumber.ROUND_CEIL).toString(10)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export async function fetchAccountReserveFee( | |
| client: Client, | |
| ): Promise<string | null> { | |
| const response = await client.request({ command: 'server_state' }) | |
| const fee = response.result.state.validated_ledger?.reserve_inc | |
| if (fee == null) { | |
| return null | |
| } | |
| return new BigNumber(fee).dp(0, BigNumber.ROUND_CEIL).toString(10) | |
| } | |
| interface ServerState { | |
| result: { | |
| state: { | |
| validated_ledger?: { | |
| reserve_inc?: string | |
| } | |
| } | |
| } | |
| } | |
| export async function fetchAccountReserveFee( | |
| client: Client, | |
| ): Promise<string | null> { | |
| try { | |
| const response = await client.request({ command: 'server_state' }) as ServerState | |
| const fee = response.result.state.validated_ledger?.reserve_inc | |
| if (fee == null) { | |
| return null | |
| } | |
| const feeBN = new BigNumber(fee) | |
| if (feeBN.isNegative()) { | |
| throw new Error('Reserve fee cannot be negative') | |
| } | |
| return feeBN.dp(0, BigNumber.ROUND_CEIL).toString(10) | |
| } catch (error) { | |
| if (error instanceof Error) { | |
| throw new Error(`Failed to fetch reserve fee: ${error.message}`) | |
| } | |
| throw new Error('Failed to fetch reserve fee') | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/xrpl/test/integration/utils.ts (1)
449-460: Consider moving this utility to the main library.Given that this function provides essential fee-related functionality aligned with the PR objectives, consider moving it from the test utilities to the main library (e.g.,
packages/xrpl/src/utils/). This would make it available for production use and maintain consistency with the library's fee calculation capabilities.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- packages/xrpl/test/integration/utils.ts (2 hunks)
🔇 Additional comments (2)
packages/xrpl/test/integration/utils.ts (2)
449-460: LGTM with suggestions for improvement.The implementation aligns with the PR objectives to support dynamic fee calculations by fetching the reserve fee from the server state. The previous review comment about enhancing error handling and type safety is still applicable.
449-460: Verify integration with fee-related changes.Let's verify how this function is integrated with other fee-related changes in the codebase.
✅ Verification successful
Let me gather more context about the payment test integration to understand how the function is being used.
Based on the gathered information, I can now provide the final response:
Function is properly integrated and used in payment tests
The
fetchAccountReserveFeefunction is:
- Correctly implemented using the
server_statecommand- Integrated into payment tests for reserve fee calculations
- Part of the broader fee-related changes across the codebase (as evidenced by related fee calculation files)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check usage of fetchAccountReserveFee and related fee calculations # Check for direct usage of fetchAccountReserveFee echo "Checking direct usage of fetchAccountReserveFee:" rg "fetchAccountReserveFee" # Check for related fee calculation changes echo -e "\nChecking related fee calculation changes:" rg -l "calculateFeePerTransaction|FeeSettings|base_fee" # Check for test coverage echo -e "\nChecking test coverage:" rg -l "fetchAccountReserveFee.*test"Length of output: 3744
Script:
#!/bin/bash # Check the payment test implementation echo "Checking payment test implementation:" rg -A 10 "fetchAccountReserveFee" packages/xrpl/test/integration/transactions/payment.test.ts # Check server_state implementation echo -e "\nChecking server_state implementation:" ast-grep --pattern 'command: "server_state"'Length of output: 914
|
nit: this PR name should be changed - it has nothing to do with the |
packages/xrpl/src/sugar/autofill.ts
Outdated
| tx.TransactionType === 'AccountDelete' | ||
| ? baseFee | ||
| : BigNumber.min(baseFee, maxFeeDrops) | ||
| const totalFee = ['AccountDelete', 'AMMCreate'].includes(tx.TransactionType) |
There was a problem hiding this comment.
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 any cushion value in the getFeeXrp() function call, hence there is no possibility of the library over-estimating the fees.
There was a problem hiding this comment.
| minimum_fee: '10', | ||
| open_ledger_fee: '10', | ||
| base_fee: referenceFee, | ||
| median_fee: new BigNumber(referenceFee) |
There was a problem hiding this comment.
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
| Destination: 'rfkE1aSy9G8Upk4JssnwBxhEv5p4mn2KTy', | ||
| } | ||
| }) | ||
|
|
||
| beforeAll(async () => { | ||
| testContext = await setupClient(serverUrl) | ||
| senderWallet = await generateFundedWallet(testContext.client) | ||
| // Make sure the amount sent satisfies minimum reserve requirement to fund an account. | ||
| amount = |
There was a problem hiding this comment.
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 to DEFAULT_AMOUNT. So, I don't see a need for another amount 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.
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 account rfkE1aSy9G8Upk4JssnwBxhEv5p4mn2KTy, which does not exist beforehand. So if we increase account_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.
ah okay, thanks for the clarification
There was a problem hiding this comment.
Changing only line 18 to const AMOUNT = '10000000', could have been sufficient. But this change is also fine.
| @@ -444,3 +445,16 @@ export async function createAMMPool(client: Client): Promise<{ | |||
| asset2, | |||
| } | |||
| } | |||
|
|
|||
| export async function fetchAccountReserveFee( | |||
There was a problem hiding this comment.
Instead of creating a new function, can we re-use the one from packages/xrpl/src/sugar/autofill.ts ? Its titled fetchOwnerReserve
This will improve the test-coverage of the function which is already in the main codebase.
There was a problem hiding this comment.
Hi Chenna, OwnerReserve and AccountReserve refers to 2 different fees. One would be reserve_inc and the other reserve_base.
Please refer to: https://xrpl.org/docs/references/protocol/ledger-data/ledger-entry-types/feesettings
There was a problem hiding this comment.
okay, thanks for the clarification. Personally, I'd have preferred the names reserve_base and reserve_increment because its easier to understand their intent. But its not a strong opinion.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
.ci-config/rippled.cfg (1)
183-186: Consider adding parameter validation rangesTo prevent configuration errors, consider adding comments documenting the valid ranges for each parameter. This would help prevent issues when testing different fee scenarios.
Example:
[voting] -reference_fee = 200 # 200 drops +reference_fee = 200 # 200 drops (valid range: 10-1000) -account_reserve = 20000000 # 20 XRP +account_reserve = 20000000 # 20 XRP (valid range: 10-50 XRP) -owner_reserve = 5000000 # 5 XRP +owner_reserve = 5000000 # 5 XRP (valid range: 1-10 XRP)packages/xrpl/src/sugar/autofill.ts (1)
Line range hint
233-248: LGTM! Consider improving error message.The function renaming and implementation look good. The error handling is appropriate, but the error message could be more descriptive.
Consider updating the error message to be more specific:
- return Promise.reject(new Error('Could not fetch Owner Reserve.')) + return Promise.reject(new Error('Could not fetch Owner Reserve from server state.'))
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
.ci-config/rippled.cfg(1 hunks)packages/xrpl/src/sugar/autofill.ts(2 hunks)
🔇 Additional comments (3)
.ci-config/rippled.cfg (1)
182-186: LGTM! The new [voting] section aligns with fee voting objectives.
The configuration values match the expected changes:
- reference_fee of 200 drops aligns with mainnet UNL validators' voting
- Clear documentation of values in both drops and XRP
- Enables testing of different fee scenarios
packages/xrpl/src/sugar/autofill.ts (2)
278-283: LGTM! Good refactoring.
The introduction of isSpecialTxCost variable eliminates code duplication and improves readability.
295-301: Verify maxFeeDrops comparison necessity and optimize rounding.
The comparison with maxFeeDrops for non-special transactions needs verification. Additionally, the rounding operation could be optimized.
Let's verify the necessity of maxFeeDrops comparison:
Consider optimizing the rounding operation:
- const totalFee = isSpecialTxCost
- ? baseFee
- : BigNumber.min(baseFee, maxFeeDrops)
- tx.Fee = totalFee.dp(0, BigNumber.ROUND_CEIL).toString(10)
+ tx.Fee = (isSpecialTxCost ? baseFee : BigNumber.min(baseFee, maxFeeDrops))
+ .integerValue(BigNumber.ROUND_CEIL)
+ .toString(10)
packages/xrpl/src/sugar/autofill.ts
Outdated
| // BaseFee × (33 + (Fulfillment size in bytes / 16)) | ||
| const product = new BigNumber( | ||
| // eslint-disable-next-line @typescript-eslint/no-magic-numbers -- expected use of magic numbers | ||
| scaleValue(netFeeDrops, 33 + fulfillmentBytesSize / 16), | ||
| ) | ||
| baseFee = product.dp(0, BigNumber.ROUND_CEIL) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix premature rounding and document Fulfillment size calculation.
Two issues need attention:
- The rounding operation
product.dp(0, BigNumber.ROUND_CEIL)is premature and could lead to precision loss. - The Fulfillment length division needs documentation.
const fulfillmentBytesSize: number = Math.ceil(tx.Fulfillment.length / 2)
+ // Divide by 2 because Fulfillment is hex-encoded (2 characters per byte)
// BaseFee × (33 + (Fulfillment size in bytes / 16))
const product = new BigNumber(
scaleValue(netFeeDrops, 33 + fulfillmentBytesSize / 16),
)
- baseFee = product.dp(0, BigNumber.ROUND_CEIL)
+ baseFee = productCommittable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/xrpl/src/sugar/autofill.ts (2)
277-279: Consider a more descriptive variable name.The variable name
isSpecialTxCostcould be more specific about what makes these transactions special. ConsiderrequiresOwnerReserveFeeorusesReserveAsFeeto better reflect its purpose.- const isSpecialTxCost = ['AccountDelete', 'AMMCreate'].includes( + const requiresOwnerReserveFee = ['AccountDelete', 'AMMCreate'].includes( tx.TransactionType, )
Line range hint
239-248: Consider validating fee values against voting outcomes.Given that this PR is related to fee voting changes, consider adding validation to ensure the fetched owner reserve fee aligns with expected voting outcomes. This could help catch unexpected fee values early.
Consider adding validation like:
async function fetchOwnerReserveFee(client: Client): Promise<BigNumber> { const response = await client.request({ command: 'server_state' }) const fee = response.result.state.validated_ledger?.reserve_inc if (fee == null) { return Promise.reject(new Error('Could not fetch Owner Reserve.')) } + // Validate fee against expected range from fee voting + const feeValue = new BigNumber(fee) + if (feeValue.isLessThan(0)) { + return Promise.reject(new Error('Invalid negative owner reserve fee')) + } return new BigNumber(fee) }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
packages/xrpl/src/sugar/autofill.ts(2 hunks)
🔇 Additional comments (3)
packages/xrpl/src/sugar/autofill.ts (3)
233-239: LGTM! Clear and accurate documentation.
The function renaming from fetchAccountDeleteFee to fetchOwnerReserveFee better reflects its purpose, as it's used for both AccountDelete and AMMCreate transactions.
299-299: LGTM! Appropriate placement of rounding operation.
The rounding operation is correctly placed after all fee calculations are complete, which preserves precision throughout the calculations.
294-296: Verify the necessity of maxFeeDrops comparison.
Based on previous discussions, the comparison with maxFeeDrops might be unnecessary since baseFee already computes the final fee with multi-signers accounted for.
Let's analyze the codebase for the rationale behind this comparison:
pdp2121
force-pushed
the
check-fee-updates
branch
from
ec590b3 to
a36a8f0
Compare
There was a problem hiding this comment.
@pdp2121 I'm getting the following error message on many integration tests. I suspect the tests need to be updated to use a generic fee value. Did you observe this in your local run?
AMMWithdraw › withdraw with Amount and LPTokenIn
xrpl: assert.strictEqual(received, expected)
xrpl: Expected value to strictly be equal to:
xrpl: "tesSUCCESS"
xrpl: Received:
xrpl: "telINSUF_FEE_P"
xrpl: Message:
xrpl: Fee insufficient.: expected 'telINSUF_FEE_P' to equal 'tesSUCCESS'
|
For additional context, I'm using the fee-voting parameters as specified in this PR's |
High Level Overview of Change
Ensure the library works for changes FeeSettings values resulted by Fee Voting:
calculateFeePerTransactionto get accurate tx fee forAMMCreatein caseowner_reserveincreases.Context of Change
Fee voting by mainnet UNL validators are approaching 200 drops for
base_fee. Bothaccount_reserveandreserve_incare also getting proposed changes.Type of Change
Did you update HISTORY.md?
Test Plan
All integration tests passed.