Skip to content
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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .ci-config/rippled.cfg
Original file line number Diff line number Diff line change
Expand Up @@ -178,3 +178,9 @@ PriceOracle
fixEmptyDID
fixXChainRewardRounding
fixPreviousTxnID

# # This section can be used to simulate various FeeSettings scenarios for rippled node in standalone mode
pdp2121 marked this conversation as resolved.
Show resolved Hide resolved
[voting]
reference_fee = 200 # 200 drops
account_reserve = 20000000 # 20 XRP
owner_reserve = 5000000 # 5 XRP
3 changes: 3 additions & 0 deletions packages/xrpl/HISTORY.md
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,9 @@ Subscribe to [the **xrpl-announce** mailing list](https://groups.google.com/g/xr
### Added
* parseTransactionFlags as a utility function in the xrpl package to streamline transactions flags-to-map conversion

### Fixed
* Remove hard-coded reference to 10 drops as the reference transaction cost. Ensure tests passed for all transaction fee scenarios and `AMMCreate` transaction fee calculation is correct in case `owner_reserve` increases.

## 4.0.0 (2024-07-15)

### BREAKING CHANGES
Expand Down
18 changes: 8 additions & 10 deletions packages/xrpl/src/sugar/autofill.ts
Original file line number Diff line number Diff line change
Expand Up @@ -230,13 +230,13 @@ export async function setNextValidSequenceNumber(
}

/**
* Fetches the account deletion fee from the server state using the provided client.
* Fetches the owner reserve fee from the server state using the provided client.
*
* @param client - The client object used to make the request.
* @returns A Promise that resolves to the account deletion fee as a BigNumber.
pdp2121 marked this conversation as resolved.
Show resolved Hide resolved
* @throws {Error} Throws an error if the account deletion fee cannot be fetched.
*/
async function fetchAccountDeleteFee(client: Client): Promise<BigNumber> {
async function fetchOwnerReserveFee(client: Client): Promise<BigNumber> {
const response = await client.request({ command: 'server_state' })
const fee = response.result.state.validated_ledger?.reserve_inc

Expand All @@ -260,15 +260,14 @@ export async function calculateFeePerTransactionType(
tx: Transaction,
signersCount = 0,
): Promise<void> {
// netFee is usually 0.00001 XRP (10 drops)
const netFeeXRP = await getFeeXrp(client)
const netFeeDrops = xrpToDrops(netFeeXRP)
let baseFee = new BigNumber(netFeeDrops)

// EscrowFinish Transaction with Fulfillment
if (tx.TransactionType === 'EscrowFinish' && tx.Fulfillment != null) {
const fulfillmentBytesSize: number = Math.ceil(tx.Fulfillment.length / 2)
ckeshava marked this conversation as resolved.
Show resolved Hide resolved
// 10 drops × (33 + (Fulfillment size in bytes / 16))
// 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),
Expand All @@ -280,22 +279,21 @@ export async function calculateFeePerTransactionType(
tx.TransactionType === 'AccountDelete' ||
pdp2121 marked this conversation as resolved.
Show resolved Hide resolved
tx.TransactionType === 'AMMCreate'
) {
baseFee = await fetchAccountDeleteFee(client)
baseFee = await fetchOwnerReserveFee(client)
}

/*
* Multi-signed Transaction
* 10 drops × (1 + Number of Signatures Provided)
* BaseFee × (1 + Number of Signatures Provided)
*/
if (signersCount > 0) {
baseFee = BigNumber.sum(baseFee, scaleValue(netFeeDrops, 1 + signersCount))
}

const maxFeeDrops = xrpToDrops(client.maxFeeXRP)
const totalFee =
tx.TransactionType === 'AccountDelete'
? baseFee
: BigNumber.min(baseFee, maxFeeDrops)
const totalFee = ['AccountDelete', 'AMMCreate'].includes(tx.TransactionType)
Copy link
Collaborator

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 any cushion value in the getFeeXrp() function call, hence there is no possibility of the library over-estimating the fees.

Copy link
Collaborator Author

@pdp2121 pdp2121 Nov 12, 2024

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.

? baseFee
: BigNumber.min(baseFee, maxFeeDrops)
pdp2121 marked this conversation as resolved.
Show resolved Hide resolved

// Round up baseFee and return it as a string
// eslint-disable-next-line no-param-reassign, @typescript-eslint/no-magic-numbers -- param reassign is safe, base 10 magic num
Expand Down
16 changes: 11 additions & 5 deletions packages/xrpl/test/integration/requests/fee.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import BigNumber from 'bignumber.js'
import { assert } from 'chai'
import omit from 'lodash/omit'

import { FeeRequest } from '../../../src'
import { FeeRequest, xrpToDrops } from '../../../src'
import getFeeXrp from '../../../src/sugar/getFeeXrp'
import serverUrl from '../serverUrl'
import {
setupClient,
Expand All @@ -26,17 +28,21 @@ describe('fee', function () {
const request: FeeRequest = {
command: 'fee',
}
const referenceFee = xrpToDrops(await getFeeXrp(testContext.client, 1))
const MEDIAN_FEE_MULTIPLIER = 500
const response = await testContext.client.request(request)
const expected = {
id: 0,
result: {
current_ledger_size: '0',
current_queue_size: '0',
drops: {
base_fee: '10',
median_fee: '5000',
minimum_fee: '10',
open_ledger_fee: '10',
base_fee: referenceFee,
median_fee: new BigNumber(referenceFee)
Copy link
Collaborator

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

.times(MEDIAN_FEE_MULTIPLIER)
.toString(),
minimum_fee: referenceFee,
open_ledger_fee: referenceFee,
},
expected_ledger_size: '1000',
ledger_current_index: 2925,
Expand Down
20 changes: 14 additions & 6 deletions packages/xrpl/test/integration/transactions/payment.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,15 +7,20 @@ import {
teardownClient,
type XrplIntegrationTestContext,
} from '../setup'
import { generateFundedWallet, testTransaction } from '../utils'
import {
fetchAccountReserveFee,
generateFundedWallet,
testTransaction,
} from '../utils'

// how long before each test case times out
const TIMEOUT = 20000

describe('Payment', function () {
let testContext: XrplIntegrationTestContext
let paymentTx: Payment
const AMOUNT = '10000000'
let amount: string
const DEFAULT_AMOUNT = '10000000'
// This wallet is used for DeliverMax related tests
let senderWallet: Wallet

Expand All @@ -25,14 +30,17 @@ describe('Payment', function () {
paymentTx = {
TransactionType: 'Payment',
Account: senderWallet.classicAddress,
Amount: AMOUNT,
Amount: amount,
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 =
Copy link
Collaborator

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 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

Copy link
Collaborator Author

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 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

Copy link
Collaborator

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

Copy link
Collaborator

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.

(await fetchAccountReserveFee(testContext.client)) ?? DEFAULT_AMOUNT
Comment on lines +41 to +43
Copy link

@coderabbitai coderabbitai bot Oct 25, 2024

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.

    // 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.

Suggested change
// 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
}

Copy link

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:

// 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.

})
afterAll(async () => teardownClient(testContext))

Expand Down Expand Up @@ -60,7 +68,7 @@ describe('Payment', function () {
)

assert.equal(result.result.engine_result_code, 0)
assert.equal((result.result.tx_json as Payment).Amount, AMOUNT)
assert.equal((result.result.tx_json as Payment).Amount, amount)
},
TIMEOUT,
)
Expand All @@ -80,7 +88,7 @@ describe('Payment', function () {
)

assert.equal(result.result.engine_result_code, 0)
assert.equal((result.result.tx_json as Payment).Amount, AMOUNT)
assert.equal((result.result.tx_json as Payment).Amount, amount)
},
TIMEOUT,
)
Expand All @@ -98,7 +106,7 @@ describe('Payment', function () {
)

assert.equal(result.result.engine_result_code, 0)
assert.equal((result.result.tx_json as Payment).Amount, AMOUNT)
assert.equal((result.result.tx_json as Payment).Amount, amount)
},
TIMEOUT,
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import {
XChainCreateClaimID,
xrpToDrops,
} from '../../../src'
import getFeeXrp from '../../../src/sugar/getFeeXrp'
import serverUrl from '../serverUrl'
import {
setupBridge,
Expand Down Expand Up @@ -39,6 +40,7 @@ describe('XChainCreateBridge', function () {
const destination = await generateFundedWallet(testContext.client)
const otherChainSource = Wallet.generate()
const amount = xrpToDrops(10)
const netFee = xrpToDrops(await getFeeXrp(testContext.client))

const claimIdTx: XChainCreateClaimID = {
TransactionType: 'XChainCreateClaimID',
Expand Down Expand Up @@ -103,7 +105,10 @@ describe('XChainCreateBridge', function () {
)
assert.equal(
finalBalance,
initialBalance + Number(amount) - Number(signatureReward) - 12,
initialBalance +
Number(amount) -
Number(signatureReward) -
Number(netFee),
Comment on lines +108 to +111
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

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.

Suggested change
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",

"The destination's balance should not change yet",
)
},
Expand Down
14 changes: 14 additions & 0 deletions packages/xrpl/test/integration/utils.ts
Original file line number Diff line number Diff line change
@@ -1,3 +1,4 @@
import BigNumber from 'bignumber.js'
import { assert } from 'chai'
import omit from 'lodash/omit'
import throttle from 'lodash/throttle'
Expand Down Expand Up @@ -444,3 +445,16 @@ export async function createAMMPool(client: Client): Promise<{
asset2,
}
}

export async function fetchAccountReserveFee(
Copy link
Collaborator

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 titled fetchOwnerReserve

This will improve the test-coverage of the function which is already in the main codebase.

Copy link
Collaborator Author

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 other reserve_base.
Please refer to: https://xrpl.org/docs/references/protocol/ledger-data/ledger-entry-types/feesettings

Copy link
Collaborator

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 and reserve_increment because its easier to understand their intent. But its not a strong opinion.

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)
}
Copy link

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:

  1. Add error handling for failed requests
  2. Add type safety for the response structure
  3. 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.

Suggested change
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')
}
}

Loading