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

fix: avoid overriding user gasLimit and maxFee inputs #2262

Merged
merged 25 commits into from
May 10, 2024
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
584f0f2
add ramda to program package
Torres-ssf May 8, 2024
262ab41
refact setDefaultTxParams
Torres-ssf May 8, 2024
6c9b8fc
add setMaxFee to getTransactionCost return
Torres-ssf May 8, 2024
0afb188
ajust fund method
Torres-ssf May 8, 2024
d619235
refact method validateGasLimitAndMaxFee
Torres-ssf May 8, 2024
392f79b
remove unused flag from BaseInvocationScope TxParams
Torres-ssf May 8, 2024
f108faf
avoid mutationg transaction request on fundWithRequiredCoins
Torres-ssf May 8, 2024
302d29c
ajusting some tests
Torres-ssf May 8, 2024
7f4419e
refact code
Torres-ssf May 8, 2024
6d02711
add changeset
Torres-ssf May 8, 2024
61e6818
Merge branch 'master' into st/fix/avoid-overriding-user-gaslimit-maxfee
Torres-ssf May 8, 2024
0452091
rename setMaxFee to updateMaxFee
Torres-ssf May 8, 2024
c9478cb
release PR
Torres-ssf May 8, 2024
02147be
formating annoying messages
Torres-ssf May 8, 2024
76a9375
fix TS DOC
Torres-ssf May 8, 2024
e49d0de
Merge branch 'master' into st/fix/avoid-overriding-user-gaslimit-maxfee
Torres-ssf May 8, 2024
63758fe
unrelease PR
Torres-ssf May 8, 2024
ea0e2b7
Merge branch 'master' into st/fix/avoid-overriding-user-gaslimit-maxfee
arboleya May 8, 2024
fb8c255
Merge branch 'master' into st/fix/avoid-overriding-user-gaslimit-maxfee
Torres-ssf May 8, 2024
b2b9b25
Merge branch 'master' into st/fix/avoid-overriding-user-gaslimit-maxfee
petertonysmith94 May 9, 2024
1b2a3d9
Merge branch 'master' into st/fix/avoid-overriding-user-gaslimit-maxfee
arboleya May 9, 2024
f8bed39
adding tests
Torres-ssf May 9, 2024
b2768ec
fix test
Torres-ssf May 9, 2024
3d1748e
Merge branch 'master' into st/fix/avoid-overriding-user-gaslimit-maxfee
danielbate May 10, 2024
d814fb6
Merge branch 'master' into st/fix/avoid-overriding-user-gaslimit-maxfee
Torres-ssf May 10, 2024
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 .changeset/hot-vans-battle.md
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
3 changes: 2 additions & 1 deletion packages/account/src/account.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -251,6 +251,8 @@ describe('Account', () => {

const request = new ScriptTransactionRequest();

request.maxFee = fee;

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

await account.fund(request, {
requiredQuantities: quantities,
maxFee: fee,
estimatedPredicates: [],
addedSignatures: 0,
});
Expand Down
110 changes: 61 additions & 49 deletions packages/account/src/account.ts
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' | 'setMaxFee'
>;
const MAX_FUNDING_ATTEMPTS = 2;

Expand Down Expand Up @@ -252,13 +252,13 @@ export class Account extends AbstractAccount {
* @returns A promise that resolves when the resources are added to the transaction.
*/
async fund<T extends TransactionRequest>(request: T, params: EstimatedTxParams): Promise<T> {
const { addedSignatures, estimatedPredicates, maxFee: fee, requiredQuantities } = params;
const { addedSignatures, estimatedPredicates, requiredQuantities, setMaxFee } = 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,57 +301,65 @@ 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()
);
}

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

const totalBaseAssetOnInputs = getAssetAmountInRequestInputs(
request.inputs,
baseAssetId,
baseAssetId
);

const totalBaseAssetRequiredWithFee = requiredInBaseAsset.add(newFee);

if (totalBaseAssetOnInputs.gt(totalBaseAssetRequiredWithFee)) {
if (setMaxFee) {
needsToBeFunded = false;
} else {
missingQuantities = [
{
amount: totalBaseAssetRequiredWithFee.sub(totalBaseAssetOnInputs),
assetId: baseAssetId,
},
];
const { maxFee: newFee } = await this.provider.estimateTxGasAndFee({
transactionRequest: requestToReestimate,
});

const totalBaseAssetOnInputs = getAssetAmountInRequestInputs(
request.inputs,
baseAssetId,
baseAssetId
);

const totalBaseAssetRequiredWithFee = requiredInBaseAsset.add(newFee);

if (totalBaseAssetOnInputs.gt(totalBaseAssetRequiredWithFee)) {
needsToBeFunded = false;
} else {
missingQuantities = [
{
amount: totalBaseAssetRequiredWithFee.sub(totalBaseAssetOnInputs),
assetId: baseAssetId,
},
];
}
}

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 (setMaxFee) {
return request;
}

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

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

return txRequest;
return request;
}

/**
Expand All @@ -373,23 +381,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 +465,7 @@ export class Account extends AbstractAccount {
assetId: assetIdToTransfer,
});

const request = new ScriptTransactionRequest({
let request = new ScriptTransactionRequest({
...txParams,
script,
scriptData,
Expand All @@ -472,15 +478,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 +523,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 +606,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.ts
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;
setMaxFee?: boolean;
Torres-ssf marked this conversation as resolved.
Show resolved Hide resolved
};
// #endregion cost-estimation-1

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 setMaxFee = !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,
setMaxFee,
};
}

Expand Down
6 changes: 1 addition & 5 deletions packages/fuel-gauge/src/contract.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -478,7 +478,6 @@ describe('Contract', () => {
])
.txParams({
gasLimit: 4_000_000,
optimizeGas: false,
})
.call<[BN, BN]>();

Expand Down Expand Up @@ -510,7 +509,6 @@ describe('Contract', () => {
const { value } = await invocationScope
.txParams({
gasLimit: transactionCost.gasUsed,
optimizeGas: false,
})
.call<[string, string]>();

Expand Down Expand Up @@ -646,9 +644,7 @@ describe('Contract', () => {
const struct = { a: true, b: 1337 };
const invocationScopes = [contract.functions.foo(num), contract.functions.boo(struct)];
const multiCallScope = contract.multiCall(invocationScopes);
await multiCallScope.fundWithRequiredCoins();

const transactionRequest = await multiCallScope.getTransactionRequest();
const transactionRequest = await multiCallScope.fundWithRequiredCoins();

const txRequest = JSON.stringify(transactionRequest);
const txRequestParsed = JSON.parse(txRequest);
Expand Down
4 changes: 4 additions & 0 deletions packages/program/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
},
"license": "Apache-2.0",
"dependencies": {
"ramda": "^0.29.0",
"@fuel-ts/abi-coder": "workspace:*",
"@fuel-ts/account": "workspace:*",
"@fuel-ts/address": "workspace:*",
Expand All @@ -34,5 +35,8 @@
"@fuel-ts/transactions": "workspace:*",
"@fuel-ts/utils": "workspace:*",
"@fuels/vm-asm": "0.49.0"
},
"devDependencies": {
"@types/ramda": "^0.29.3"
}
}
Loading
Loading