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 13 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.
Jump to
Jump to file
Failed to load files.
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
2 changes: 1 addition & 1 deletion .github/workflows/pr-release.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ jobs:
name: "Release PR to npm"
runs-on: ubuntu-latest
# comment out if:false to enable release PR to npm
if: false
# if: false
Torres-ssf marked this conversation as resolved.
Show resolved Hide resolved
permissions: write-all
steps:
- name: Checkout
Expand Down
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
81 changes: 46 additions & 35 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' | '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.
* @template T - The type of the TransactionRequest.
* @param T - request - The transaction request to fund.
arboleya marked this conversation as resolved.
Show resolved Hide resolved
* @param EstimatedTxParams - The estimated transaction parameters.
arboleya marked this conversation as resolved.
Show resolved Hide resolved
* @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;
}
}
12 changes: 6 additions & 6 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;
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},
maschad marked this conversation as resolved.
Show resolved Hide resolved
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
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"
}
}