Skip to content

Commit

Permalink
fix: ensure getTransactionCost not returns fee values as 0 when nod…
Browse files Browse the repository at this point in the history
…e `minGasPrice` is 0 (#1523)
  • Loading branch information
Torres-ssf authored Dec 15, 2023
1 parent 2a7ad09 commit 2d40dde
Show file tree
Hide file tree
Showing 7 changed files with 62 additions and 9 deletions.
7 changes: 7 additions & 0 deletions .changeset/six-shoes-impress.md
Original file line number Diff line number Diff line change
@@ -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
6 changes: 6 additions & 0 deletions packages/math/src/bn.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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');
Expand Down
4 changes: 4 additions & 0 deletions packages/math/src/bn.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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
}

Expand Down
4 changes: 3 additions & 1 deletion packages/providers/src/coin-quantity.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand Down
14 changes: 11 additions & 3 deletions packages/providers/src/provider.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
};
}

Expand Down
24 changes: 24 additions & 0 deletions packages/providers/test/provider.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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();
});
});
12 changes: 7 additions & 5 deletions packages/wallet/src/account.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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<T extends TransactionRequest>(
request: T,
quantities: CoinQuantity[],
coinQuantities: CoinQuantity[],
fee: BN
): Promise<void> {
// 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);
}

Expand Down

0 comments on commit 2d40dde

Please sign in to comment.