Conversation
Merge main in feat/next
Gas limit estimator & remove interaction and smartContract
Switch to options for entrypoint's constructor
…utation Update error message for gas computation
…de-for-v-15 Remove deprecated code
Merge main in next branch
…-for-load-abi-and-contract-code Cookbook: Add actual implementation for load abi and load bytecode
Patch estimate gas endpoints
There was a problem hiding this comment.
Pull Request Overview
This PR introduces major architectural changes to support gas limit estimation throughout the MultiversX SDK, making it a key feature in v15. The PR adds automated gas estimation capabilities and modernizes the transaction building patterns by moving from the old TransactionBuilder pattern to direct Transaction creation with asynchronous factory methods.
- Addition of
GasLimitEstimatorclass for automated gas limit calculation - Conversion of all transaction factory methods from synchronous to asynchronous
- Migration from
TransactionBuilderto directTransactioncreation withBaseFactory
Reviewed Changes
Copilot reviewed 72 out of 73 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/gasEstimator/gasLimitEstimator.ts | New gas limit estimation implementation with network provider integration |
| src/transfers/transferTransactionsFactory.ts | Converted to async methods and BaseFactory pattern |
| src/smartContracts/smartContractTransactionsFactory.ts | Updated to async pattern with gas estimation support |
| src/tokenManagement/tokenManagementTransactionsFactory.ts | Migrated to new BaseFactory async pattern |
| src/multisig/multisigTransactionsFactory.ts | Updated factory methods to async with BaseFactory |
| src/governance/governanceTransactionsFactory.ts | Converted to async factory pattern |
| src/entrypoints/entrypoints.ts | Added gas estimator configuration to all entrypoints |
| src/networkProviders/proxyNetworkProvider.ts | Enhanced transaction cost estimation with proper field handling |
| gasLimit: bigint; | ||
| /** | ||
| * The gas limit for the operation. If not provided, a gas limit estimator must be used. | ||
| * Failure to provide this parameter without an estimator may lead to runtime errors. |
There was a problem hiding this comment.
[nitpick] The comment mentions 'Failure to provide this parameter without an estimator may lead to runtime errors' but this is vague. Consider documenting the specific error type or condition that would occur.
| * Failure to provide this parameter without an estimator may lead to runtime errors. | |
| * Failure to provide this parameter without an estimator may result in a transaction failure | |
| * or an out-of-gas error during execution. |
| isPayableBySmartContract?: boolean; | ||
| gasLimit: bigint; | ||
| gasLimit?: bigint; | ||
| }; | ||
|
|
||
| export type MultisigContractInput = { | ||
| multisigContract: Address; |
There was a problem hiding this comment.
Missing documentation comment explaining when gasLimit is optional and what happens when it's not provided, unlike the smart contracts resources file which includes this information.
| constructor(options?: { | ||
| url?: string; | ||
| kind?: string; | ||
| clientName?: string; | ||
| withGasLimitEstimator?: boolean; | ||
| gasLimitMultiplier?: number; | ||
| }) { | ||
| const entrypointConfig = new TestnetEntrypointConfig(); | ||
| options = options || {}; |
There was a problem hiding this comment.
The constructor parameter change from individual parameters (url?, kind?, clientName?) to an options object is a breaking change. Consider maintaining backward compatibility or clearly documenting this as a breaking change.
| constructor(options?: { | |
| url?: string; | |
| kind?: string; | |
| clientName?: string; | |
| withGasLimitEstimator?: boolean; | |
| gasLimitMultiplier?: number; | |
| }) { | |
| const entrypointConfig = new TestnetEntrypointConfig(); | |
| options = options || {}; | |
| constructor( | |
| urlOrOptions?: string | { | |
| url?: string; | |
| kind?: string; | |
| clientName?: string; | |
| withGasLimitEstimator?: boolean; | |
| gasLimitMultiplier?: number; | |
| }, | |
| kind?: string, | |
| clientName?: string, | |
| withGasLimitEstimator?: boolean, | |
| gasLimitMultiplier?: number | |
| ) { | |
| const entrypointConfig = new TestnetEntrypointConfig(); | |
| let options: { | |
| url?: string; | |
| kind?: string; | |
| clientName?: string; | |
| withGasLimitEstimator?: boolean; | |
| gasLimitMultiplier?: number; | |
| }; | |
| if (typeof urlOrOptions === "string") { | |
| options = { | |
| url: urlOrOptions, | |
| kind, | |
| clientName, | |
| withGasLimitEstimator, | |
| gasLimitMultiplier, | |
| }; | |
| } else { | |
| options = urlOrOptions || {}; | |
| } |
| // Load from file. | ||
| let buffer: Buffer = await fs.promises.readFile(path); | ||
| return Code.fromBuffer(buffer); | ||
| return buffer; |
There was a problem hiding this comment.
The function return type changed from Code to Uint8Array, but the function name 'loadContractCode' still suggests it returns a Code object. Consider renaming to 'loadContractBytecode' or similar to reflect the actual return type.
| }); | ||
|
|
||
| let serialized = transaction.serializeForSigning(); | ||
| let serialized = transactionComputer.computeBytesForSigning(transaction); |
There was a problem hiding this comment.
[nitpick] The TransactionComputer instance is created multiple times throughout the test file. Consider creating it once at the test suite level to reduce duplication and improve maintainability.
3 participants
No description provided.