Delegation refactoring#542
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the delegation functionality by removing the custom DelegationOperations wrapper class and integrating the DelegationController from the updated MultiversX SDK.
- Upgrades MultiversX SDK from version 2.0.1 to 2.2.0
- Removes the custom delegation wrapper and uses the SDK's native
DelegationController - Updates all delegation operations to use the new controller API patterns
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| pyproject.toml | Updates MultiversX SDK dependency to version 2.2.0 |
| multiversx_sdk_cli/delegation.py | Removes the entire DelegationOperations wrapper class |
| multiversx_sdk_cli/cli_delegation.py | Refactors to use DelegationController and ValidatorsController from SDK |
| multiversx_sdk_cli/tests/test_cli_staking_provider.py | Updates gas limit test value to match new SDK behavior |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| total_delegation_cap=int(args.total_delegation_cap), | ||
| service_fee=int(args.service_fee), | ||
| guardian=guardian_and_relayer_data.guardian.address if guardian_and_relayer_data.guardian else None, | ||
| relayer=guardian_and_relayer_data.relayer.address if guardian_and_relayer_data.relayer else None, |
There was a problem hiding this comment.
[nitpick] The pattern guardian_and_relayer_data.guardian.address if guardian_and_relayer_data.guardian else None is repeated throughout the file. Consider extracting this logic into a helper function to reduce duplication and improve maintainability.
| if args.options: | ||
| transaction.options = args.options | ||
| altered = True | ||
|
|
||
| if args.version != DEFAULT_TX_VERSION: | ||
| transaction.version = args.version |
There was a problem hiding this comment.
We can check if different from the fields of transaction (flow optimization).
| return cast(list[str], parsed) | ||
|
|
||
|
|
||
| def _options_set_for_hash_signing(args: Any) -> bool: |
There was a problem hiding this comment.
should prefix, since it returns bool?
| if _options_set_for_hash_signing(args): | ||
| acc.use_hash_signing = True |
There was a problem hiding this comment.
Can this be refactored to be written only once?
Oh, it's not on all flows, sorry (not on LedgerAccount).
|
|
||
| def initialize_gas_limit_estimator(args: Any) -> Union[GasLimitEstimator, None]: | ||
| # if gas limit is provided, we don't need GasLimitEstimator | ||
| if hasattr(args, "gas_limit") and args.gas_limit: |
Removed the Delegation Wrapper, now using the DelegationController within sdk-py.