Conversation
autonomy/chain/constants.py
Outdated
| "poly_safe_creator_with_recovery_module": "0xA749f605D93B3efcc207C54270d83C6E8fa70fF8", # Polymarket multisig WITH recovery module | ||
| "poly_safe_same_address_with_recovery_module": "0xBcb1BAC84B5BcAb350C89c50ADc9064eD15a4485", # Polymarket same address multisig WITH recovery module |
There was a problem hiding this comment.
@kupermind Please check that the snake-case naming of these contracts match with the autonolas registry repository.
autonomy/chain/constants.py
Outdated
| "safe_multisig_with_recovery_module": "0x1a0bFCC27051BCcDDc444578f56A4F5920e0E083", # Multisig WITH recovery module | ||
| "recovery_module": "0x02C26437B292D86c5F4F21bbCcE0771948274f84", # Same address multisig WITH recovery module | ||
| "poly_safe_creator_with_recovery_module": "0xA749f605D93B3efcc207C54270d83C6E8fa70fF8", # Poly Safe multisig WITH recovery module | ||
| "poly_safe_same_address_with_recovery_module": "0xBcb1BAC84B5BcAb350C89c50ADc9064eD15a4485", # Poly Safe same address multisig WITH recovery module |
There was a problem hiding this comment.
Where is this one used? Or just added for now?
There was a problem hiding this comment.
Will be used in middleware, this ABI matches the "Same address multisig WITH recovery module" , right?
There was a problem hiding this comment.
Alternative: make POLY_SAFE_SAME_ADDRESS_WITH_RECOVERY_MODULE_CONTRACT address = RECOVERY_MODULE_CONTRACT (for symmetry with other contracts), and use the contract 0xBcb1BAC84B5BcAb350C89c50ADc9064eD15a4485 for the very extreme corner cases considered by the team.
There was a problem hiding this comment.
Will be removed for now and added in another PR. for now poly_safe_same_address_with_recovery_module = recovery_module
autonomy/chain/service.py
Outdated
| create_transaction_hash = registry_contracts.poly_safe_creator_with_recovery_module.get_poly_safe_create_transaction_hash( | ||
| ledger_api=ledger_api, | ||
| contract_address=contract_address, | ||
| ) | ||
| sig1 = crypto.sign_message( | ||
| create_transaction_hash, | ||
| is_deprecated_mode=True, # Legacy signature, do not use EIP-191 signing | ||
| ) | ||
| sig1_bytes = bytes.fromhex(sig1[2:]) | ||
|
|
||
| enable_module_hash = registry_contracts.poly_safe_creator_with_recovery_module.get_enable_module_transaction_hash( | ||
| ledger_api=ledger_api, | ||
| contract_address=contract_address, | ||
| signer_address=crypto.address, | ||
| ) | ||
| sig2 = crypto.sign_message( | ||
| enable_module_hash, | ||
| is_deprecated_mode=True, # Legacy signature, do not use EIP-191 signing | ||
| ) | ||
| sig2_bytes = bytes.fromhex(sig2[2:]) | ||
|
|
||
| # Pack both signatures in the format required by PolySafeCreatorWithRecoveryModule.create(...) | ||
| r1 = sig1_bytes[0:32] | ||
| s1 = sig1_bytes[32:64] | ||
| v1 = sig1_bytes[64] | ||
| data = eth_abi.encode( | ||
| ["(uint8,bytes32,bytes32)", "bytes"], [(v1, r1, s1), sig2_bytes] | ||
| ) | ||
|
|
||
| return "0x" + data.hex() |
There was a problem hiding this comment.
can we move this inside the poly_safe_creator_with_recovery_module contract? Because
- it's a logic heavily related to the contract interaction
- we avoid making
eth_abia dependency of OA
There was a problem hiding this comment.
Yes we could, but read my comment above: other create deployment data methods are written on this file. This is the equivalent for polysafe. I agree with your comments but we should be consistent with the existing code as well. We can discuss what's the best tradeoff option.
| ) | ||
|
|
||
|
|
||
| def get_poly_safe_deployment_payload( |
There was a problem hiding this comment.
It's used in middleware. In theory it could be used here as well if we support via CLI create a Poly safe (could be considered as a new feature in another PR).
It has been added here for al"symmetry" with the existing get_XXX_payload methods.
There was a problem hiding this comment.
Will open an issue
packages/valory/contracts/poly_safe_creator_with_recovery_module/contract.yaml
Outdated
Show resolved
Hide resolved
autonomy/chain/config.py
Outdated
| "address": chain_config.rpc, | ||
| "chain_id": chain_config.chain_id, | ||
| "is_gas_estimation_enabled": True, | ||
| "poa_chain": chain_type in (ChainType.OPTIMISM, ChainType.POLYGON), |
There was a problem hiding this comment.
There was a problem hiding this comment.
Let's define a constant:
POA_CHAINS = frozenset((ChainType.OPTIMISM, ChainType.POLYGON))and then:
| "poa_chain": chain_type in (ChainType.OPTIMISM, ChainType.POLYGON), | |
| "poa_chain": chain_type in POA_CHAINS, |
| eth-abi: | ||
| version: ==5.2.0 | ||
| open-aea-ledger-ethereum: | ||
| version: ==2.0.6 | ||
| open-aea-test-autonomy: | ||
| version: ==0.21.5 | ||
| web3: | ||
| version: <8,>=7.0.0 |
There was a problem hiding this comment.
Some dependencies are not used.
There was a problem hiding this comment.
I'll remove web3
| ledger_api=ledger_api, contract_address=contract_address | ||
| ) | ||
| return contract_instance.functions.getEnableModuleTransactionHash( | ||
| signer_address |
There was a problem hiding this comment.
The signer_address could be checksummed for safety.
There was a problem hiding this comment.
We need to return a dictionary from contract methods as per the conventions of the framework.
packages/valory/contracts/poly_safe_creator_with_recovery_module/contract.py
Show resolved
Hide resolved
autonomy/chain/config.py
Outdated
| "address": chain_config.rpc, | ||
| "chain_id": chain_config.chain_id, | ||
| "is_gas_estimation_enabled": True, | ||
| "poa_chain": chain_type in (ChainType.OPTIMISM, ChainType.POLYGON), |
There was a problem hiding this comment.
Let's define a constant:
POA_CHAINS = frozenset((ChainType.OPTIMISM, ChainType.POLYGON))and then:
| "poa_chain": chain_type in (ChainType.OPTIMISM, ChainType.POLYGON), | |
| "poa_chain": chain_type in POA_CHAINS, |
autonomy/chain/constants.py
Outdated
| POLY_SAFE_SAME_ADDRESS_WITH_RECOVERY_MODULE_CONTRACT = PublicId.from_str( | ||
| "valory/poly_safe_same_address_with_recovery_module" | ||
| ) # Poly Safe same address multisig WITH recovery module |
There was a problem hiding this comment.
It does not exist, but some other contract packages do not exist either and use their placeholders here, for example gnosis_safe_same_address_multisig. In this case, these placeholders are used in the library to retrieve the address only, not to use the contract package. (their contract implementation is identical to other contracts)
autonomy/chain/service.py
Outdated
| contract_address=contract_address, | ||
| crypto=crypto, | ||
| ) | ||
| return "0x" + data.hex() |
There was a problem hiding this comment.
Could return the data with the prefix from the contract.
There was a problem hiding this comment.
Yes, but since the other methods of the contract return bytes (to avoid having to parse and unparse '0x' strings), for consistency this method also returns bytes . I am unsure wha'ts the standard approach here, as I haven't seen an homogeneous criteria for returning outputs in the contracts...
Co-authored-by: Ojuswi Rastogi <55619686+OjusWiZard@users.noreply.github.com>
Proposed changes
Add Poly Safe multisig.
Note: The PR contains
tox -e check-generate-all-protocolsto fix linters, due to change in year, therefore multiple unrelated files have been changed.The following issues/refactor have been identified for future work:
Fixes
Types of changes
What types of changes does your code introduce? (A breaking change is a fix or feature that would cause existing functionality and APIs to not work as expected.)
Put an
xin the box that appliesChecklist
Put an
xin the boxes that apply.mainbranch (left side). Also you should start your branch off ourmain.Further comments