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