Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR integrates a gas limit estimator feature into the MultiversX SDK CLI, upgrading the SDK dependency from v1.6.2 to v2.0.0. The main changes enable automatic gas limit estimation for transactions when no explicit gas limit is provided.
Key changes include:
- Added gas limit estimator integration across all transaction controllers
- Updated parameter types from
inttoUnion[int, None]for gas limits - Added gas limit multiplier configuration and CLI argument support
- Updated SDK method calls to use newer API patterns
Reviewed Changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| pyproject.toml | Updated CLI version to 11.1.0 and SDK dependency to 2.0.0 |
| multiversx_sdk_cli/validators.py | Added GasLimitEstimator support to ValidatorsController |
| multiversx_sdk_cli/transactions.py | Integrated gas estimator into TransactionsController |
| multiversx_sdk_cli/tests/test_*.py | Updated tests for gas estimation and method name changes |
| multiversx_sdk_cli//cli_.py | Added gas estimator initialization across CLI modules |
| multiversx_sdk_cli/config.py | Added gas_limit_multiplier configuration option |
| multiversx_sdk_cli/cli_shared.py | Added gas estimator initialization helper function |
Comments suppressed due to low confidence (1)
| def test_contract_verification_create_request_signature(): | ||
| account = Account.new_from_pem(file_path=testdata_folder / "walletKey.pem") | ||
| contract_address = Address.from_bech32("erd1qqqqqqqqqqqqqpgqeyj9g344pqguukajpcfqz9p0rfqgyg4l396qespdck") | ||
| contract_address = Address.new_from_bech32("erd1qqqqqqqqqqqqqpgqeyj9g344pqguukajpcfqz9p0rfqgyg4l396qespdck") |
There was a problem hiding this comment.
The method name change from Address.from_bech32 to Address.new_from_bech32 is inconsistent with the pattern used elsewhere in the codebase. Consider using a consistent naming pattern across all similar changes.
| contract_address = Address.new_from_bech32("erd1qqqqqqqqqqqqqpgqeyj9g344pqguukajpcfqz9p0rfqgyg4l396qespdck") | |
| contract_address = Address.from_bech32("erd1qqqqqqqqqqqqqpgqeyj9g344pqguukajpcfqz9p0rfqgyg4l396qespdck") |
|
|
||
| delegation_config["owner"] = owner.address.bech32() | ||
| dns_config["owner"] = owner.address.bech32() | ||
| delegation_config["owner"] = owner.address.to_bech32() |
There was a problem hiding this comment.
The method name change from bech32() to to_bech32() should be consistent with the pattern used elsewhere. Verify this aligns with the new SDK API conventions.
| "signature": self.signature.hex(), | ||
| "payload": { | ||
| "contract": self.contract.bech32(), | ||
| "contract": self.contract.to_bech32(), |
There was a problem hiding this comment.
Similar to other files, ensure the method name change from bech32() to to_bech32() is consistent with the new SDK API.
| def initialize_gas_limit_estimator(args: Any) -> Union[GasLimitEstimator, None]: | ||
| # if proxy is not provided, we can't use GasLimitEstimator | ||
| if hasattr(args, "proxy") and not args.proxy: |
There was a problem hiding this comment.
The function returns None when proxy is not provided, but the return type annotation indicates Union[GasLimitEstimator, None]. Consider adding a check to ensure this behavior is intentional and properly handled by callers.
| def initialize_gas_limit_estimator(args: Any) -> Union[GasLimitEstimator, None]: | |
| # if proxy is not provided, we can't use GasLimitEstimator | |
| if hasattr(args, "proxy") and not args.proxy: | |
| def initialize_gas_limit_estimator(args: Any) -> Union[GasLimitEstimator, None]: | |
| """ | |
| Initializes and returns a GasLimitEstimator object. | |
| Returns: | |
| GasLimitEstimator: If a valid proxy is provided. | |
| None: If the proxy is not provided or is falsy. | |
| Note: | |
| If `None` is returned, callers must handle this case appropriately. | |
| """ | |
| # if proxy is not provided, we can't use GasLimitEstimator | |
| if hasattr(args, "proxy") and not args.proxy: | |
| logger.warning("Proxy is not provided. GasLimitEstimator cannot be initialized.") |
| validate_chain_id_args(args) | ||
|
|
||
|
|
||
| def _initialize_multisig_wrapper(args: Any) -> MultisigWrapper: |
There was a problem hiding this comment.
Can be _create_multisig_wrapper.
| chain_id = proxy.get_network_config().chain_id | ||
| multisig = MultisigController(chain_id, proxy, abi) | ||
|
|
||
| multisig = _initialize_multisig_controller(args) |
There was a problem hiding this comment.
Also here, maybe _create... instead of _initialize....
| "--gas-limit-multiplier", | ||
| required=False, | ||
| type=float, | ||
| help="if `--gas-limit` is not provided, the estimated value will be multiplied by this multiplier (e.g 1.1)", |
There was a problem hiding this comment.
Can be ... is not explicitly provided ... (nitpicking).
| args.proxy = env.proxy_url | ||
|
|
||
|
|
||
| def initialize_gas_limit_estimator(args: Any) -> Union[GasLimitEstimator, None]: |
No description provided.