Refactoring: Remove validators wrapper#547
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the validators functionality by removing the local ValidatorsController wrapper class and replacing it with direct usage of the ValidatorsController from the multiversx_sdk package. The refactoring simplifies the codebase by eliminating redundant wrapper code and standardizes the interface to match the SDK's API.
- Removes the entire
validators.pyfile containing the localValidatorsControllerwrapper - Updates import statements to use
ValidatorsControllerdirectly frommultiversx_sdk - Refactors all validator transaction creation calls to use the SDK's direct API instead of the wrapper methods
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| multiversx_sdk_cli/validators.py | Complete removal of the local ValidatorsController wrapper class |
| multiversx_sdk_cli/cli_validators.py | Updated imports and refactored all function calls to use SDK's ValidatorsController directly |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| guardian=guardian_and_relayer_data.guardian_address, | ||
| relayer=guardian_and_relayer_data.relayer_address, | ||
| gas_limit=args.gas_limit, | ||
| gas_price=args.gas_price, | ||
| nonce=sender.nonce, | ||
| version=args.version, | ||
| options=args.options, | ||
| guardian_and_relayer_data=guardian_and_relayer_data, | ||
| ) | ||
| else: | ||
| validators_signers = _load_validators_signers(args.validators_pem) | ||
| tx = controller.create_transaction_for_staking( | ||
| sender=sender, | ||
| validators=validators_signers, | ||
| native_amount=native_amount, | ||
| gas_limit=args.gas_limit, | ||
| gas_price=args.gas_price, | ||
| nonce=sender.nonce, | ||
| version=args.version, | ||
| options=args.options, | ||
| validators_file=validators_signers, | ||
| amount=native_amount, | ||
| rewards_address=rewards_address, | ||
| guardian_and_relayer_data=guardian_and_relayer_data, | ||
| guardian=guardian_and_relayer_data.guardian_address, | ||
| relayer=guardian_and_relayer_data.relayer_address, | ||
| gas_limit=args.gas_limit, | ||
| gas_price=args.gas_price, |
There was a problem hiding this comment.
[nitpick] The parameter order and naming in the new API calls don't follow a consistent pattern. For example, sender and nonce are first in one call but sender, nonce, validators_file in another. Consider grouping related parameters consistently (e.g., transaction metadata like nonce, gas_limit, gas_price together).
There was a problem hiding this comment.
Sender, nonce, always first. Validators file third, when necessary. Thus, seems fine.
| tx = controller.create_transaction_for_unstaking( | ||
| sender=sender, | ||
| keys=keys, | ||
| native_amount=native_amount, | ||
| nonce=sender.nonce, | ||
| public_keys=keys, | ||
| guardian=guardian_and_relayer_data.guardian_address, | ||
| relayer=guardian_and_relayer_data.relayer_address, | ||
| gas_limit=args.gas_limit, | ||
| gas_price=args.gas_price, | ||
| nonce=sender.nonce, | ||
| version=args.version, | ||
| options=args.options, | ||
| guardian_and_relayer_data=guardian_and_relayer_data, | ||
| ) |
There was a problem hiding this comment.
The args.value parameter that was previously used as native_amount has been removed from this call, but the SDK's create_transaction_for_unstaking method likely expects a value parameter. This could result in missing transaction value data.
There was a problem hiding this comment.
For "validators", it does not (no partial unstake):
| guardian=guardian_and_relayer_data.guardian_address, | ||
| relayer=guardian_and_relayer_data.relayer_address, | ||
| gas_limit=args.gas_limit, | ||
| gas_price=args.gas_price, |
There was a problem hiding this comment.
Similar to the unstaking method, the args.value parameter that was previously used as native_amount has been removed. The unbonding transaction may require a value parameter to function correctly.
| gas_price=args.gas_price, | |
| gas_price=args.gas_price, | |
| value=args.value, |
| guardian=guardian_and_relayer_data.guardian_address, | ||
| relayer=guardian_and_relayer_data.relayer_address, | ||
| gas_limit=args.gas_limit, | ||
| gas_price=args.gas_price, |
There was a problem hiding this comment.
The args.value parameter that was previously used as native_amount has been removed from this call as well. This transaction type may also require a value parameter.
| gas_price=args.gas_price, | |
| gas_price=args.gas_price, | |
| value=getattr(args, "value", 0), |
popenta
dismissed
andreibancioiu’s stale review
The base branch was changed.
There was a problem hiding this comment.
Pull Request Overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated no new comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
3 participants
No description provided.