Skip to content

Commit

Permalink
fix: avoid overriding user gasLimit and maxFee inputs (#2262)
Browse files Browse the repository at this point in the history
  • Loading branch information
@Torres-ssf
Torres-ssf authored May 10, 2024
1 parent 7d6c744 commit 9bc893b
Show file tree
Hide file tree
Showing 10 changed files with 220 additions and 73 deletions.
6 changes: 6 additions & 0 deletions .changeset/hot-vans-battle.md
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@fuel-ts/account": patch
"@fuel-ts/program": patch
---

fix: avoid overriding user `gasLimit` and `maxFee` inputs
49 changes: 48 additions & 1 deletion packages/account/src/account.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import { ZeroBytes32 } from '@fuel-ts/address/configs';
import { ErrorCode, FuelError } from '@fuel-ts/errors';
import { expectToThrowFuelError } from '@fuel-ts/errors/test-utils';
import { bn } from '@fuel-ts/math';
import { PolicyType } from '@fuel-ts/transactions';
import { ASSET_A, ASSET_B } from '@fuel-ts/utils/test-utils';

import { Account } from './account';
Expand Down Expand Up @@ -251,6 +252,8 @@ describe('Account', () => {

const request = new ScriptTransactionRequest();

request.maxFee = fee;

const resourcesToSpend: Resource[] = [];
const getResourcesToSpendSpy = vi
.spyOn(Account.prototype, 'getResourcesToSpend')
Expand All @@ -267,7 +270,6 @@ describe('Account', () => {

await account.fund(request, {
requiredQuantities: quantities,
maxFee: fee,
estimatedPredicates: [],
addedSignatures: 0,
});
Expand Down Expand Up @@ -408,6 +410,28 @@ describe('Account', () => {
expect(receiverBalances).toEqual([{ assetId: baseAssetId, amount: bn(1) }]);
});

it('can set "gasLimit" and "maxFee" when transferring amounts', async () => {
const sender = await generateTestWallet(provider, [[500_000, baseAssetId]]);
const receiver = Address.fromRandom();

const gasLimit = 30_000;
const maxFee = 15_000;

const request = await sender.createTransfer(receiver, 1, baseAssetId, {
gasLimit,
maxFee,
});

const response = await sender.sendTransaction(request);
const { transaction } = await response.wait();

const { scriptGasLimit, policies } = transaction;
const maxFeePolicy = policies?.find((policy) => policy.type === PolicyType.MaxFee);

expect(scriptGasLimit?.toNumber()).toBe(gasLimit);
expect(bn(maxFeePolicy?.data).toNumber()).toBe(maxFee);
});

it('can transfer with custom TX Params', async () => {
const sender = await generateTestWallet(provider, [[50_000, baseAssetId]]);
const receiver = Wallet.generate({ provider });
Expand Down Expand Up @@ -590,6 +614,29 @@ describe('Account', () => {
expect(senderBalances).toEqual([{ assetId: baseAssetId, amount: bn(expectedRemaining) }]);
});

it('can set "gasLimit" and "maxFee" when withdrawing to base layer', async () => {
const sender = Wallet.generate({
provider,
});

await seedTestWallet(sender, [[500_000, baseAssetId]]);

const recipient = Address.fromRandom();
const amount = 110;

const gasLimit = 100_000;
const maxFee = 50_000;

const tx = await sender.withdrawToBaseLayer(recipient, amount, { gasLimit, maxFee });
const { transaction } = await tx.wait();

const { scriptGasLimit, policies } = transaction;
const maxFeePolicy = policies?.find((policy) => policy.type === PolicyType.MaxFee);

expect(scriptGasLimit?.toNumber()).toBe(gasLimit);
expect(bn(maxFeePolicy?.data).toNumber()).toBe(maxFee);
});

it('should ensure gas price and gas limit are validated when transfering amounts', async () => {
const sender = await generateTestWallet(provider, [[1000, baseAssetId]]);
const receiver = Wallet.generate({ provider });
Expand Down
81 changes: 46 additions & 35 deletions packages/account/src/account.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ export type TxParamsType = Pick<

export type EstimatedTxParams = Pick<
TransactionCost,
'maxFee' | 'estimatedPredicates' | 'addedSignatures' | 'requiredQuantities'
'estimatedPredicates' | 'addedSignatures' | 'requiredQuantities' | 'updateMaxFee'
>;
const MAX_FUNDING_ATTEMPTS = 2;

Expand Down Expand Up @@ -244,21 +244,21 @@ export class Account extends AbstractAccount {
}

/**
* Adds resources to the transaction enough to fund it.
* Funds a transaction request by adding the necessary resources.
*
* @param request - The transaction request.
* @param requiredQuantities - 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.
* @typeParam T - The type of the TransactionRequest.
* @param request - The transaction request to fund.
* @param params - The estimated transaction parameters.
* @returns The funded transaction request.
*/
async fund<T extends TransactionRequest>(request: T, params: EstimatedTxParams): Promise<T> {
const { addedSignatures, estimatedPredicates, maxFee: fee, requiredQuantities } = params;
const { addedSignatures, estimatedPredicates, requiredQuantities, updateMaxFee } = params;

const fee = request.maxFee;
const baseAssetId = this.provider.getBaseAssetId();
const requiredInBaseAsset =
requiredQuantities.find((quantity) => quantity.assetId === baseAssetId)?.amount || bn(0);

const txRequest = request as T;
const requiredQuantitiesWithFee = addAmountToCoinQuantities({
amount: bn(fee),
assetId: baseAssetId,
Expand Down Expand Up @@ -301,17 +301,19 @@ export class Account extends AbstractAccount {
);

request.addResources(resources);
request.shiftPredicateData();
request.updatePredicateGasUsed(estimatedPredicates);

txRequest.shiftPredicateData();
txRequest.updatePredicateGasUsed(estimatedPredicates);

const requestToReestimate = clone(txRequest);
const requestToReestimate = clone(request);
if (addedSignatures) {
Array.from({ length: addedSignatures }).forEach(() =>
requestToReestimate.addEmptyWitness()
);
}

if (!updateMaxFee) {
break;
}
const { maxFee: newFee } = await this.provider.estimateTxGasAndFee({
transactionRequest: requestToReestimate,
});
Expand All @@ -338,20 +340,25 @@ export class Account extends AbstractAccount {
fundingAttempts += 1;
}

txRequest.shiftPredicateData();
txRequest.updatePredicateGasUsed(estimatedPredicates);
request.shiftPredicateData();
request.updatePredicateGasUsed(estimatedPredicates);

const requestToReestimate = clone(txRequest);
const requestToReestimate = clone(request);
if (addedSignatures) {
Array.from({ length: addedSignatures }).forEach(() => requestToReestimate.addEmptyWitness());
}

if (!updateMaxFee) {
return request;
}

const { maxFee } = await this.provider.estimateTxGasAndFee({
transactionRequest: requestToReestimate,
});

txRequest.maxFee = maxFee;
request.maxFee = maxFee;

return txRequest;
return request;
}

/**
Expand All @@ -373,23 +380,21 @@ export class Account extends AbstractAccount {
/** Tx Params */
txParams: TxParamsType = {}
): Promise<TransactionRequest> {
const request = new ScriptTransactionRequest(txParams);
let request = new ScriptTransactionRequest(txParams);
const assetIdToTransfer = assetId ?? this.provider.getBaseAssetId();
request.addCoinOutput(Address.fromAddressOrString(destination), amount, assetIdToTransfer);
const txCost = await this.provider.getTransactionCost(request, {
estimateTxDependencies: true,
resourcesOwner: this,
});

this.validateGasLimitAndMaxFee({
request = this.validateGasLimitAndMaxFee({
transactionRequest: request,
gasUsed: txCost.gasUsed,
maxFee: txCost.maxFee,
txParams,
});

request.gasLimit = txCost.gasUsed;
request.maxFee = txCost.maxFee;

await this.fund(request, txCost);

return request;
Expand Down Expand Up @@ -459,7 +464,7 @@ export class Account extends AbstractAccount {
assetId: assetIdToTransfer,
});

const request = new ScriptTransactionRequest({
let request = new ScriptTransactionRequest({
...txParams,
script,
scriptData,
Expand All @@ -472,15 +477,13 @@ export class Account extends AbstractAccount {
quantitiesToContract: [{ amount: bn(amount), assetId: String(assetIdToTransfer) }],
});

this.validateGasLimitAndMaxFee({
request = this.validateGasLimitAndMaxFee({
transactionRequest: request,
gasUsed: txCost.gasUsed,
maxFee: txCost.maxFee,
txParams,
});

request.gasLimit = txCost.gasUsed;
request.maxFee = txCost.maxFee;

await this.fund(request, txCost);

return this.sendTransaction(request);
Expand Down Expand Up @@ -519,20 +522,18 @@ export class Account extends AbstractAccount {
const params: ScriptTransactionRequestLike = { script, ...txParams };

const baseAssetId = this.provider.getBaseAssetId();
const request = new ScriptTransactionRequest(params);
let request = new ScriptTransactionRequest(params);
const quantitiesToContract = [{ amount: bn(amount), assetId: baseAssetId }];

const txCost = await this.provider.getTransactionCost(request, { quantitiesToContract });

this.validateGasLimitAndMaxFee({
request = this.validateGasLimitAndMaxFee({
transactionRequest: request,
gasUsed: txCost.gasUsed,
maxFee: txCost.maxFee,
txParams,
});

request.maxFee = txCost.maxFee;
request.gasLimit = txCost.gasUsed;

await this.fund(request, txCost);

return this.sendTransaction(request);
Expand Down Expand Up @@ -604,26 +605,36 @@ export class Account extends AbstractAccount {
}

private validateGasLimitAndMaxFee({
txParams: { gasLimit: setGasLimit, maxFee: setMaxFee },
gasUsed,
maxFee,
transactionRequest,
txParams: { gasLimit: setGasLimit, maxFee: setMaxFee },
}: {
gasUsed: BN;
maxFee: BN;
transactionRequest: ScriptTransactionRequest;
txParams: Pick<TxParamsType, 'gasLimit' | 'maxFee'>;
}) {
if (isDefined(setGasLimit) && gasUsed.gt(setGasLimit)) {
const request = transactionRequestify(transactionRequest) as ScriptTransactionRequest;

if (!isDefined(setGasLimit)) {
request.gasLimit = gasUsed;
} else if (gasUsed.gt(setGasLimit)) {
throw new FuelError(
ErrorCode.GAS_LIMIT_TOO_LOW,
`Gas limit '${setGasLimit}' is lower than the required: '${gasUsed}'.`
);
}

if (isDefined(setMaxFee) && maxFee.gt(setMaxFee)) {
if (!isDefined(setMaxFee)) {
request.maxFee = maxFee;
} else if (maxFee.gt(setMaxFee)) {
throw new FuelError(
ErrorCode.MAX_FEE_TOO_LOW,
`Max fee '${setMaxFee}' is lower than the required: '${maxFee}'.`
);
}

return request;
}
}
8 changes: 4 additions & 4 deletions packages/account/src/providers/provider.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -882,8 +882,8 @@ describe('Provider', () => {

expect(consoleWarnSpy).toHaveBeenCalledOnce();
expect(consoleWarnSpy).toHaveBeenCalledWith(
`The Fuel Node that you are trying to connect to is using fuel-core version ${FUEL_CORE},
which is not supported by the version of the TS SDK that you are using.
`The Fuel Node that you are trying to connect to is using fuel-core version ${FUEL_CORE},
which is not supported by the version of the TS SDK that you are using.
Things may not work as expected.
Supported fuel-core version: ${mock.supportedVersion}.`
);
Expand Down Expand Up @@ -914,8 +914,8 @@ Supported fuel-core version: ${mock.supportedVersion}.`

expect(consoleWarnSpy).toHaveBeenCalledOnce();
expect(consoleWarnSpy).toHaveBeenCalledWith(
`The Fuel Node that you are trying to connect to is using fuel-core version ${FUEL_CORE},
which is not supported by the version of the TS SDK that you are using.
`The Fuel Node that you are trying to connect to is using fuel-core version ${FUEL_CORE},
which is not supported by the version of the TS SDK that you are using.
Things may not work as expected.
Supported fuel-core version: ${mock.supportedVersion}.`
);
Expand Down
12 changes: 6 additions & 6 deletions packages/account/src/providers/provider.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,7 @@ export type TransactionCost = {
requiredQuantities: CoinQuantity[];
addedSignatures: number;
dryRunStatus?: DryRunStatus;
updateMaxFee?: boolean;
};
// #endregion cost-estimation-1

Expand Down Expand Up @@ -483,8 +484,8 @@ export default class Provider {
if (!isMajorSupported || !isMinorSupported) {
// eslint-disable-next-line no-console
console.warn(
`The Fuel Node that you are trying to connect to is using fuel-core version ${nodeInfo.nodeVersion},
which is not supported by the version of the TS SDK that you are using.
`The Fuel Node that you are trying to connect to is using fuel-core version ${nodeInfo.nodeVersion},
which is not supported by the version of the TS SDK that you are using.
Things may not work as expected.
Supported fuel-core version: ${supportedVersion}.`
);
Expand Down Expand Up @@ -1059,7 +1060,7 @@ Supported fuel-core version: ${supportedVersion}.`
const txRequestClone = clone(transactionRequestify(transactionRequestLike));
const isScriptTransaction = txRequestClone.type === TransactionType.Script;
const baseAssetId = this.getBaseAssetId();

const updateMaxFee = txRequestClone.maxFee.eq(0);
// Fund with fake UTXOs to avoid not enough funds error
// Getting coin quantities from amounts being transferred
const coinOutputsQuantities = txRequestClone.getCoinOutputsQuantities();
Expand All @@ -1072,7 +1073,6 @@ Supported fuel-core version: ${supportedVersion}.`
* Estimate predicates gasUsed
*/
// Remove gasLimit to avoid gasLimit when estimating predicates
txRequestClone.maxFee = bn(0);
if (isScriptTransaction) {
txRequestClone.gasLimit = bn(0);
}
Expand All @@ -1097,6 +1097,7 @@ Supported fuel-core version: ${supportedVersion}.`
}

await this.estimatePredicates(signedRequest);
txRequestClone.updatePredicateGasUsed(signedRequest.inputs);

/**
* Calculate minGas and maxGas based on the real transaction
Expand All @@ -1112,8 +1113,6 @@ Supported fuel-core version: ${supportedVersion}.`
let outputVariables = 0;
let gasUsed = bn(0);

txRequestClone.updatePredicateGasUsed(signedRequest.inputs);

txRequestClone.maxFee = maxFee;
if (isScriptTransaction) {
txRequestClone.gasLimit = gasLimit;
Expand Down Expand Up @@ -1148,6 +1147,7 @@ Supported fuel-core version: ${supportedVersion}.`
addedSignatures,
estimatedPredicates: txRequestClone.inputs,
dryRunStatus,
updateMaxFee,
};
}

Expand Down
Loading

0 comments on commit 9bc893b

Please sign in to comment.