-
Notifications
You must be signed in to change notification settings - Fork 512
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?
Changes from 6 commits
56723e1
d96931b
bb3156d
80a6d1f
d907ce5
a36a8f0
a2a9a91
309e3c2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
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, | ||
|
@@ -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) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can omit the 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, | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -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 | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
@@ -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 = | ||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hello, It looks like the 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 commentThe reason will be displayed to describe this comment to others. Learn more. It is because the first payment transaction in this test Hence we either have to make this first payment to be at least the account_reserve or setup a destination account before sending payments |
||||||||||||||||||||||||||||||
(await fetchAccountReserveFee(testContext.client)) ?? DEFAULT_AMOUNT | ||||||||||||||||||||||||||||||
Comment on lines
+41
to
+43
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
|
||||||||||||||||||||||||||||||
}) | ||||||||||||||||||||||||||||||
afterAll(async () => teardownClient(testContext)) | ||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||
|
@@ -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, | ||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||
|
@@ -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, | ||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||
|
@@ -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, | ||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -9,6 +9,7 @@ import { | |||||||||||||||||||||
XChainCreateClaimID, | ||||||||||||||||||||||
xrpToDrops, | ||||||||||||||||||||||
} from '../../../src' | ||||||||||||||||||||||
import getFeeXrp from '../../../src/sugar/getFeeXrp' | ||||||||||||||||||||||
import serverUrl from '../serverUrl' | ||||||||||||||||||||||
import { | ||||||||||||||||||||||
setupBridge, | ||||||||||||||||||||||
|
@@ -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', | ||||||||||||||||||||||
|
@@ -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
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: 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
Suggested change
|
||||||||||||||||||||||
"The destination's balance should not change yet", | ||||||||||||||||||||||
) | ||||||||||||||||||||||
}, | ||||||||||||||||||||||
|
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' | ||
|
@@ -444,3 +445,16 @@ export async function createAMMPool(client: Client): Promise<{ | |
asset2, | ||
} | ||
} | ||
|
||
export async function fetchAccountReserveFee( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 commentThe 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 |
||
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) | ||
} |
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.
Do you know why we are dividing the
tx.Fulfillment.length
by2
? I know that you didn't write this code, but I didn't understand it.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 is from the docs:
The division by 2 is due to the fact that each byte is represented by 2 characters in hex