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 all 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
[voting]
reference_fee = 200 # 200 drops
account_reserve = 20000000 # 20 XRP
owner_reserve = 5000000 # 5 XRP
2 changes: 1 addition & 1 deletion CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ From the top-level xrpl.js folder (one level above `packages`), run the followin
```bash
npm install
# sets up the rippled standalone Docker container - you can skip this step if you already have it set up
docker run -p 6006:6006 --interactive -t --volume $PWD/.ci-config:/opt/ripple/etc/ --platform linux/amd64 rippleci/rippled:2.0.0-b4 /opt/ripple/bin/rippled -a --conf /opt/ripple/etc/rippled.cfg
docker run -p 6006:6006 --interactive -t --volume $PWD/.ci-config:/opt/ripple/etc/ --platform linux/amd64 rippleci/rippled:2.2.0-b3 /opt/ripple/bin/rippled -a --conf /opt/ripple/etc/rippled.cfg
npm run build
npm run test:integration
```
Expand Down
1 change: 1 addition & 0 deletions packages/xrpl/HISTORY.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ Subscribe to [the **xrpl-announce** mailing list](https://groups.google.com/g/xr
* `TransactionStream` model supports APIv2
* `TransactionStream` model includes `close_time_iso` field
* `Ledger` model includes `close_time_iso` field
* 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)

Expand Down
36 changes: 17 additions & 19 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.
* @throws {Error} Throws an error if the account deletion fee cannot be fetched.
* @returns A Promise that resolves to the owner reserve fee as a BigNumber.
* @throws {Error} Throws an error if the owner reserve 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,45 +260,43 @@ 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))
const product = new BigNumber(
// BaseFee × (33 + (Fulfillment size in bytes / 16))
baseFee = 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)
}

if (
tx.TransactionType === 'AccountDelete' ||
tx.TransactionType === 'AMMCreate'
) {
baseFee = await fetchAccountDeleteFee(client)
const isSpecialTxCost = ['AccountDelete', 'AMMCreate'].includes(
tx.TransactionType,
)

if (isSpecialTxCost) {
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 = isSpecialTxCost
? baseFee
: BigNumber.min(baseFee, maxFeeDrops)

// 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
// eslint-disable-next-line no-param-reassign, @typescript-eslint/no-magic-numbers, require-atomic-updates -- safe reassignment.
tx.Fee = totalFee.dp(0, BigNumber.ROUND_CEIL).toString(10)
}

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_base

if (fee == null) {
return null
}

return new BigNumber(fee).dp(0, BigNumber.ROUND_CEIL).toString(10)
}
Loading