Fixes for gas estimation when performing SC transactions - #672
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR ensures gas estimation is triggered when creating smart contract and multisig transactions by defaulting gasLimit to 0n and adds integration-style tests to validate estimation flows.
- Default gasLimit to 0n in SmartContractController and MultisigController methods to enable estimator usage.
- Update tests to cast caught errors to Error for message assertions.
- Add tests that exercise gas estimation using ProxyNetworkProvider and GasLimitEstimator.
Reviewed Changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/transfers/transferTransactionsFactory.spec.ts | Adjusts error assertion to cast err to Error for type-safe message checks. |
| src/smartContracts/smartContractTransactionsFactory.spec.ts | Same error assertion casting for smart contract factory tests. |
| src/smartContracts/smartContractController.ts | Defaults options.gasLimit to 0n before building deploy/upgrade/execute transactions. |
| src/smartContracts/smartContractController.spec.ts | Adds a test that estimates gas on devnet via ProxyNetworkProvider and GasLimitEstimator. |
| src/multisig/multisigController.ts | Defaults options.gasLimit to 0n across multiple multisig flows. |
| src/multisig/multisigController.spec.ts | Adds a test for multisig deploy with gas estimation using live devnet network. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| options: resources.ContractDeployInput & BaseControllerInput, | ||
| ): Promise<Transaction> { | ||
| const transaction = await this.factory.createTransactionForDeploy(sender.address, options); | ||
| options.gasLimit = options.gasLimit ? options.gasLimit : 0n; |
There was a problem hiding this comment.
Use the nullish coalescing assignment operator for clarity and correct semantics on optional values. Replace with options.gasLimit ??= 0n; and apply the same change to similar occurrences in this file.
| options.gasLimit = options.gasLimit ? options.gasLimit : 0n; | |
| options.gasLimit ??= 0n; |
| nonce: bigint, | ||
| options: resources.DeployMultisigContractInput & BaseControllerInput, | ||
| ): Promise<Transaction> { | ||
| options.gasLimit = options.gasLimit ? options.gasLimit : 0n; |
There was a problem hiding this comment.
This gasLimit defaulting logic is duplicated across many methods in this controller. Consider centralizing it (e.g., a small helper like ensureGasLimit(options) or handling it once in setupAndSignTransaction), and use the nullish coalescing assignment form options.gasLimit ??= 0n; for clearer intent.
| const networkProvider = new ProxyNetworkProvider("https://devnet-gateway.multiversx.com"); | ||
|
|
||
| const gasLimitEstimator = new GasLimitEstimator({ networkProvider: networkProvider }); | ||
| const controller = new SmartContractController({ | ||
| chainID: "D", | ||
| networkProvider: networkProvider, | ||
| gasLimitEstimator: gasLimitEstimator, |
There was a problem hiding this comment.
This test depends on a live devnet endpoint, which can introduce flakiness and external dependencies in unit tests. Prefer a mock/fake provider (extend MockNetworkProvider to support the estimation path) or mark/move this as an integration test gated behind an environment flag.
| const networkProvider = new ProxyNetworkProvider("https://devnet-gateway.multiversx.com"); | ||
| const gasLimitEstimator = new GasLimitEstimator({ networkProvider: networkProvider }); | ||
|
|
||
| const controller = new MultisigController({ | ||
| chainID: "D", | ||
| networkProvider: networkProvider, | ||
| abi: abi, | ||
| gasLimitEstimator: gasLimitEstimator, | ||
| }); |
There was a problem hiding this comment.
The test uses a live network provider, making the spec non-deterministic and potentially flaky (also fetching account nonce from network). Use a mock provider that simulates gas estimation and account queries, or convert this into an integration test that only runs when an env flag is set.
No description provided.