Refactor proxyFactory and safe #597

moisses89 · 2023-07-28T10:09:28Z

Description

The purpose of this pull request is refactor proxyFactory and safe class to do more easy add new safe contract versions.

Actions

split proxy factory tests by version
add a contract class with the common methods to deploy and get contract information
refactor proxy factory adding heritage with different proxy factory versions.
refactor safe class adding heritage with different safe versions.

Safe refactor class

Class diagram

The Safe class get the version from blockchain and decide which class should instantiate taking account the version and return the safe instance with the correct version.
safe = Safe("0x4E24f2497445125C4573f7935ADeDEce154317b8", ethereum_client) # returns instance of SafeV111
safe = Safe("0xfca7Da0a0290D7BcBEcD93bE124756fC9B6F8E6A", ethereum_client) # returns instance of SafeV130

SafeV100 overwrite common method retrieve_all_info because the Safe V1.1.1 contract doesn't have getModulesPaginated.

Considerations

In case that Safe class couldn't get the version from blockchain, it would return the last version class to avoid break some tests. We could improve this raising an exception that is an invalid safe.
In case of version contains wrong version value we are returning SafeV130 to keep going with the unsupported mastercopies.

Issues related

#583
Closes #619

Uxio0

PR is quite good, but I think we can still refactor more common things into the new classes

Also, afterwards we should reevaluate estimations and other methods to know if they are still valid of they need to be adjusted

gnosis/eth/contract_common.py

Uxio0 · 2023-09-01T10:22:57Z

gnosis/eth/contract_common.py

+        return EthereumTxSent(tx_hash, tx, contract_address)
+
+    @staticmethod
+    def configure_tx_parameters(


Maybe we can just accept a dictionary with tx parameters, as Web3 does in every method

gnosis/eth/contract_common.py

Uxio0 · 2023-09-01T10:44:16Z

gnosis/safe/tests/test_proxy_factory/test_proxy_factory_v1_3_0.py

+logger = logging.getLogger(__name__)
+
+
+class TestProxyFactoryV130(SafeTestCaseMixin, TestCase):


I see a lot of tests are common for proxy factories. Why not use a loop and SubTest?

gnosis/safe/safe.py

Uxio0 · 2023-09-01T10:53:33Z

gnosis/safe/safe.py

+class SafeV130(SafeCommon):
+    @cached_property
+    def contract(self) -> Contract:
+        return get_safe_V1_3_0_contract(self.w3, address=self.address)


Dont redefine contract, define a contract_fn. Also remove contract_fn as an argument for _deploy_master_copy

If we add contract_fn = get_safe_V0_0_1_contract for class SafeV001 or the same for other classes is going to fail when we call self.contract_fn because get_safe_V0_0_1_contract is not part of safe class and we are passing the instance.

gnosis/safe/safe.py

moisses89 · 2023-09-07T12:05:32Z

@Uxio0 created following PR to fix the test issues with blockscout #621

Co-authored-by: Uxío <[email protected]>

- Add version property to every Safe class - Remove Creation V1. It was used on the Safe Relay when CREATE2 did not exist

- Remove deploy from ContractCommon, use EthereumClient deployandinitialize - Refactor retrieve_all_info

- Rename `deploy_safe_master_copy` and `deploy_proxy_factory_contract` to `deploy_contract`

moisses89

Looks good to me, some comments :)

moisses89 · 2023-09-12T12:55:12Z

gnosis/eth/contracts/__init__.py

+    with open(path) as f:
+        return json.load(f)
+
+


Coul you add the parameter types of the above methods?

Done, better late than nothing

moisses89 · 2023-09-12T13:01:14Z

gnosis/eth/ethereum_client.py

+        if nonce is not None:
+            tx_params["nonce"] = nonce
+
+        if chain_id is not None:


if chain_id should be enough I think the same for above conditions.

Sorry, I'm not understanding

moisses89 · 2023-09-12T13:09:38Z

gnosis/safe/safe.py

+        safe_tx_gas = estimated_tx_gas
+        base_gas = 0
+        gas_price = 1
+        gas_token = gas_token or NULL_ADDRESS


Can be the default value for gas_token NULL_ADDRESS value defined in the method header?

This could break some behaviour, also NULL_ADDRESS is our current implementation for Solidity, while someone that's just using Python can say that the gas_token is None, and that should be fair in my opinion. We are just "translating"

We can change that but I wouldn't do it in this PR

moisses89 · 2023-09-12T13:16:29Z

gnosis/safe/safe.py

+        :param signatures: Packed signature data ({bytes32 r}{bytes32 s}{uint8 v})
+        :param safe_nonce: Nonce of the safe (to calculate hash)
+        :param safe_version: Safe version (to calculate hash)
+        :return:


return: safe transaction

Uxio0 · 2023-09-12T15:07:11Z

Thanks @moisses89 for starting this PR and all the contributions

moisses89 force-pushed the refactor_contracts branch 6 times, most recently from 6042993 to 8e85c50 Compare August 23, 2023 14:33

moisses89 marked this pull request as ready for review August 25, 2023 11:49

moisses89 requested a review from a team as a code owner August 25, 2023 11:49

moisses89 requested review from fmrsabino, Uxio0 and iamacook and removed request for a team August 25, 2023 11:49

Uxio0 requested changes Sep 1, 2023

View reviewed changes

moisses89 force-pushed the refactor_contracts branch from 818d301 to adb7c3b Compare September 4, 2023 12:48

moisses89 and others added 12 commits September 7, 2023 16:42

Add ContractCommon class

fb330d4

Refactor proxy factory

57bf4ce

Refactor Safe class

29486c4

Apply suggestions from code review

97b29a2

Co-authored-by: Uxío <[email protected]>

Remove gas increment

e0c9903

Add get_proxy_factory_fn

0856fa0

Move contract_common to contracts

91255b2

Revert deploy_contract_with_deploy_function

da2cdac

Fix SafeCreate2TxBuilder

854257d

Refactor ProxyFactory

2995d35

Test every check_proxy_code in a subTest

09b573e

Refactor Safe.py

17b4053

- Add version property to every Safe class - Remove Creation V1. It was used on the Safe Relay when CREATE2 did not exist

Uxio0 force-pushed the refactor_contracts branch from 45f097f to 17b4053 Compare September 7, 2023 14:42

Uxio0 added 2 commits September 7, 2023 17:37

WIP

36c401c

- Remove deploy from ContractCommon, use EthereumClient deployandinitialize - Refactor retrieve_all_info

Fix Safe retrieve modules

f3b0208

Move abis to abis folder

2f35a3b

- Rename `deploy_safe_master_copy` and `deploy_proxy_factory_contract` to `deploy_contract`

Uxio0 force-pushed the refactor_contracts branch from af3d4cd to 2f35a3b Compare September 8, 2023 11:01

moisses89 commented Sep 12, 2023

View reviewed changes

Uxio0 added 2 commits September 12, 2023 16:41

Improve docs and add typing for contract methods

d71ec6e

Update types and docs

3f9092d

Uxio0 merged commit bd1f1cd into master Sep 12, 2023
7 checks passed

Uxio0 deleted the refactor_contracts branch September 12, 2023 14:55

github-actions bot locked and limited conversation to collaborators Sep 12, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor proxyFactory and safe #597

Refactor proxyFactory and safe #597

moisses89 commented Jul 28, 2023 •

edited

Loading

Uxio0 left a comment

Uxio0 Sep 1, 2023

Uxio0 Sep 1, 2023

Uxio0 Sep 1, 2023

moisses89 Sep 4, 2023

moisses89 commented Sep 7, 2023

moisses89 left a comment

moisses89 Sep 12, 2023

Uxio0 Sep 12, 2023

moisses89 Sep 12, 2023

Uxio0 Sep 12, 2023

moisses89 Sep 12, 2023 •

edited

Loading

Uxio0 Sep 12, 2023

Uxio0 Sep 12, 2023

moisses89 Sep 12, 2023

Uxio0 Sep 12, 2023

Uxio0 commented Sep 12, 2023

		logger = logging.getLogger(__name__)


		class TestProxyFactoryV130(SafeTestCaseMixin, TestCase):

Refactor proxyFactory and safe #597

Refactor proxyFactory and safe #597

Conversation

moisses89 commented Jul 28, 2023 • edited Loading

Description

Actions

Safe refactor class

Considerations

Issues related

Uxio0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moisses89 commented Sep 7, 2023

moisses89 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

moisses89 Sep 12, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Uxio0 commented Sep 12, 2023

moisses89 commented Jul 28, 2023 •

edited

Loading

moisses89 Sep 12, 2023 •

edited

Loading