From 2d40dde61ded0be5ffaed3a0ef970a9ddf664884 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?S=C3=A9rgio=20Torres?= <30977845+Torres-ssf@users.noreply.github.com> Date: Fri, 15 Dec 2023 13:17:54 -0300 Subject: [PATCH] fix: ensure `getTransactionCost` not returns fee values as 0 when node `minGasPrice` is 0 (#1523) --- .changeset/six-shoes-impress.md | 7 +++++++ packages/math/src/bn.test.ts | 6 ++++++ packages/math/src/bn.ts | 4 ++++ packages/providers/src/coin-quantity.ts | 4 +++- packages/providers/src/provider.ts | 14 +++++++++++--- packages/providers/test/provider.test.ts | 24 ++++++++++++++++++++++++ packages/wallet/src/account.ts | 12 +++++++----- 7 files changed, 62 insertions(+), 9 deletions(-) create mode 100644 .changeset/six-shoes-impress.md diff --git a/.changeset/six-shoes-impress.md b/.changeset/six-shoes-impress.md new file mode 100644 index 00000000000..0a6d3d31d77 --- /dev/null +++ b/.changeset/six-shoes-impress.md @@ -0,0 +1,7 @@ +--- +"@fuel-ts/math": minor +"@fuel-ts/providers": minor +"@fuel-ts/wallet": minor +--- + +ensure estimated fee values returned by getTransactionCost are never 0 diff --git a/packages/math/src/bn.test.ts b/packages/math/src/bn.test.ts index 8cbfeae639c..b92b442e83f 100644 --- a/packages/math/src/bn.test.ts +++ b/packages/math/src/bn.test.ts @@ -477,6 +477,12 @@ describe('Math - BN', () => { }).toThrow("Decimal can't have more than 5 digits."); }); + it('should normalize zero to one', () => { + expect(bn(0).normalizeZeroToOne().eq(1)).toBeTruthy(); + + expect(bn(2).normalizeZeroToOne().eq(1)).not.toBeTruthy(); + }); + it('should match valueOf to toString with no base arguments', () => { expect(bn('1000000000').valueOf()).toEqual('1000000000'); expect(bn('2').valueOf()).toEqual('2'); diff --git a/packages/math/src/bn.ts b/packages/math/src/bn.ts index 11f637aff9d..65e7f796664 100644 --- a/packages/math/src/bn.ts +++ b/packages/math/src/bn.ts @@ -264,6 +264,10 @@ export class BN extends BnJs implements BNInputOverrides, BNHiddenTypes, BNHelpe maxU64(): BN { return this.gte(this.MAX_U64) ? new BN(this.MAX_U64) : this; } + + normalizeZeroToOne(): BN { + return this.isZero() ? new BN(1) : this; + } // END ANCHOR: OVERRIDES to avoid losing references } diff --git a/packages/providers/src/coin-quantity.ts b/packages/providers/src/coin-quantity.ts index 241e5eeedbc..49489d2e33f 100644 --- a/packages/providers/src/coin-quantity.ts +++ b/packages/providers/src/coin-quantity.ts @@ -38,7 +38,9 @@ export interface IAddAmountToAssetParams { } export const addAmountToAsset = (params: IAddAmountToAssetParams): CoinQuantity[] => { - const { amount, assetId, coinQuantities } = params; + const { amount, assetId } = params; + + const coinQuantities = [...params.coinQuantities]; const assetIdx = coinQuantities.findIndex((coinQuantity) => coinQuantity.assetId === assetId); diff --git a/packages/providers/src/provider.ts b/packages/providers/src/provider.ts index 5ed43568eee..eb0ac793da7 100644 --- a/packages/providers/src/provider.ts +++ b/packages/providers/src/provider.ts @@ -771,6 +771,14 @@ export default class Provider { gasUsed = minGas; } + const usedFee = calculatePriceWithFactor( + gasUsed, + gasPrice, + gasPriceFactor + ).normalizeZeroToOne(); + const minFee = calculatePriceWithFactor(minGas, gasPrice, gasPriceFactor).normalizeZeroToOne(); + const maxFee = calculatePriceWithFactor(maxGas, gasPrice, gasPriceFactor).normalizeZeroToOne(); + return { requiredQuantities: allQuantities, receipts, @@ -779,9 +787,9 @@ export default class Provider { gasPrice, minGas, maxGas, - usedFee: calculatePriceWithFactor(gasUsed, gasPrice, gasPriceFactor), - minFee: calculatePriceWithFactor(minGas, gasPrice, gasPriceFactor), - maxFee: calculatePriceWithFactor(maxGas, gasPrice, gasPriceFactor), + usedFee, + minFee, + maxFee, }; } diff --git a/packages/providers/test/provider.test.ts b/packages/providers/test/provider.test.ts index 0998b35f809..24e3ac66c04 100644 --- a/packages/providers/test/provider.test.ts +++ b/packages/providers/test/provider.test.ts @@ -994,4 +994,28 @@ describe('Provider', () => { witnessLimit: transactionRequest.witnessLimit, }); }); + + it('should ensure estimated fee values on getTransactionCost are never 0', async () => { + const provider = await Provider.create(FUEL_NETWORK_URL); + + const request = new ScriptTransactionRequest(); + + // forcing calculatePriceWithFactor to return 0 + const calculatePriceWithFactorMock = jest + .spyOn(gasMod, 'calculatePriceWithFactor') + .mockReturnValue(bn(0)); + + const normalizeZeroToOneSpy = jest.spyOn(BN.prototype, 'normalizeZeroToOne'); + + const { minFee, maxFee, usedFee } = await provider.getTransactionCost(request); + + expect(calculatePriceWithFactorMock).toHaveBeenCalledTimes(3); + + expect(normalizeZeroToOneSpy).toHaveBeenCalledTimes(3); + expect(normalizeZeroToOneSpy).toHaveReturnedWith(bn(1)); + + expect(maxFee.eq(0)).not.toBeTruthy(); + expect(usedFee.eq(0)).not.toBeTruthy(); + expect(minFee.eq(0)).not.toBeTruthy(); + }); }); diff --git a/packages/wallet/src/account.ts b/packages/wallet/src/account.ts index a0c719b6633..5f008351ab9 100644 --- a/packages/wallet/src/account.ts +++ b/packages/wallet/src/account.ts @@ -207,20 +207,22 @@ export class Account extends AbstractAccount { * Adds resources to the transaction enough to fund it. * * @param request - The transaction request. + * @param coinQuantities - The coin quantities required to execute the transaction. + * @param fee - The estimated transaction fee. * @returns A promise that resolves when the resources are added to the transaction. */ async fund( request: T, - quantities: CoinQuantity[], + coinQuantities: CoinQuantity[], fee: BN ): Promise { - // TODO: Rollback to fee value after fix fee calculation - addAmountToAsset({ + const updatedQuantities = addAmountToAsset({ amount: bn(fee), assetId: BaseAssetId, - coinQuantities: quantities, + coinQuantities, }); - const resources = await this.getResourcesToSpend(quantities); + + const resources = await this.getResourcesToSpend(updatedQuantities); request.addResources(resources); }