V3 transactions in outside execution tests for argent #1554

baitcode · 2025-01-14T20:47:31Z

Closes: #1546

Introduced changes

Added new argent contract v0.4.0 that supports transactions v3 (casm, json and abi)
Registered new factory for argent v040 injection in tests
Updated tests logic
Added new test that checks for new type of errors (Reentrancy introduced in argent v0.4.0)
Refacttored account creation logic in tests.

I tried to keep changes to minimum, but could not help myself to introduce refactoring of a test account creation logic. For some reason those accounts were populated through dependency injection factories, which is ok, but at some point one factory got responsible for all possible types of accounts through same factory which over time got bloated with crotches.

I moved all the crotch logic into AccountToBeDeployedDetailsFactory and want to propose to get rid of that class completely (well not in this PR) replacing it with obvious calls, like create_specific_account(), prepay_account(), this will make code straighforward and easy to read. Currently to understand what is happening in a particular test one need to hop through 3-5 layers of constructor injection calls.

* Added new argent contract v0.4.0 that supports transactions v3 (casm, json and abi) * Registered new factory for argent v040 injection in tests * Updated tests logic * Added new test that checks for new type of errors (Reentrancy introduced in argent v0.4.0) * Refacttored account creation logic in tests. I tried to keep changes to minimum, but could not help myself to introduce refactoring of a test account creation logic. For some reason those accounts were populated through dependency injection factories, which is ok, but at some point one factory got responsible for all possible types of accounts through same factory which over time got bloated with crotches. I moved all the crotch logic into `AccountToBeDeployedDetailsFactory` and want to propose to get rid of that class completely (well not in this PR) replacing it with obvious calls, like create_specific_account(), prepay_account(), this will make code straighforward and easy to read. Currently to understand what is happening in a particular test one need to hop through 3-5 layers of constructor injection calls. Fixes: 1546

baitcode · 2025-01-14T20:48:34Z

@franciszekjob there might a minor fix for test. It will be one line maximum. Besides that good for review.

baitcode · 2025-01-14T20:51:41Z

tests on networks do not work, still. 8 (

franciszekjob · 2025-01-14T23:16:36Z

starknet_py/tests/e2e/account/outside_execution_test.py

 @pytest.mark.asyncio
 async def test_argent_account_outside_execution_compatibility(
    argent_account: BaseAccount,
+    argent_account_v040: BaseAccount,
 ):
    result = await argent_account.supports_interface(OutsideExecutionInterfaceID.V1)
    assert result is True
    result = await argent_account.supports_interface(OutsideExecutionInterfaceID.V2)
    assert result is False

+    result = await argent_account_v040.supports_interface(
+        OutsideExecutionInterfaceID.V1
+    )
+    assert result is True
+    result = await argent_account_v040.supports_interface(
+        OutsideExecutionInterfaceID.V2
+    )
+    assert result is True


Could we change this test to be parametrized?

I'm afraid it's not trivial due to dependency injection. I attempted to reduce amount of statements. please check.

franciszekjob · 2025-01-14T23:17:53Z

starknet_py/tests/e2e/account/outside_execution_test.py

+    assert any(
+        [
+            await argent_account_v040.supports_interface(
+                OutsideExecutionInterfaceID.V1
+            ),
+            await argent_account_v040.supports_interface(
+                OutsideExecutionInterfaceID.V2
+            ),
+        ]
+    )


Is this needed?

I think yes. But I see your suggest. Removing.

franciszekjob · 2025-01-14T23:18:28Z

starknet_py/tests/e2e/account/outside_execution_test.py

+        caller=ANY_CALLER,
+    )
+
+    tx = await argent_account_v040.execute_v1(calls=[call], max_fee=MAX_FEE)


Please use execute_v3.

Huh! How did those sneak in?! Thank you for catching.

starknet_py/tests/e2e/utils.py

franciszekjob · 2025-01-14T23:21:56Z

starknet_py/tests/e2e/utils.py

@@ -16,13 +16,28 @@
 AccountToBeDeployedDetails = Tuple[int, KeyPair, int, int]


-async def get_deploy_account_details(
-    *,
+def new_address(


That should be private function, _new_address.

franciszekjob · 2025-01-15T14:33:19Z

@baitcode thanks for quick replies 😄 will respond later today.

franciszekjob

@baitcode looking good! 😄

starknet_py/tests/e2e/account/outside_execution_test.py

franciszekjob · 2025-01-24T00:05:28Z

starknet_py/tests/e2e/account/outside_execution_test.py

+        l1_resource_bounds=ResourceBounds(
+            max_amount=int(1e5), max_price_per_unit=int(1e13)
+        ),


nit: We can use MAX_RESOURCE_BOUNDS_L1 (same for other occurrences).

Suggested change

l1_resource_bounds=ResourceBounds(

max_amount=int(1e5), max_price_per_unit=int(1e13)

),

l1_resource_bounds=MAX_RESOURCE_BOUNDS_L1,

Thank you! It's good to know.

Looks like it's not applied

franciszekjob · 2025-01-24T00:13:08Z

starknet_py/tests/e2e/fixtures/contracts_v1.py

@@ -384,6 +384,25 @@ async def argent_account_class_hash(
    )


+@pytest_asyncio.fixture(scope="package")
+async def argent_account_class_hash_v040(


nit:

Suggested change

async def argent_account_class_hash_v040(

async def argent_account_v040_class_hash(

I think it's stil the same 😅

Yeah. Somehow lost that change.

franciszekjob · 2025-01-24T00:15:49Z

starknet_py/tests/e2e/account/outside_execution_test.py

+        l1_resource_bounds=ResourceBounds(
+            max_amount=int(1e5), max_price_per_unit=int(1e13)
+        ),


Same as above

replaced in whole changeset.

baitcode · 2025-01-25T20:46:48Z

@franciszekjob all fixed.

franciszekjob

@baitcode not all nits have been applied 😅

franciszekjob · 2025-01-27T09:55:02Z

starknet_py/tests/e2e/fixtures/contracts_v1.py

@@ -384,6 +384,25 @@ async def argent_account_class_hash(
    )


+@pytest_asyncio.fixture(scope="package")
+async def argent_account_class_hash_v040(


I think it's stil the same 😅

starknet_py/tests/e2e/account/outside_execution_test.py

franciszekjob · 2025-01-27T09:58:03Z

starknet_py/tests/e2e/account/outside_execution_test.py

+        l1_resource_bounds=ResourceBounds(
+            max_amount=int(1e5), max_price_per_unit=int(1e13)
+        ),


Looks like it's not applied

baitcode · 2025-01-27T16:04:16Z

@franciszekjob
I don't understand how this is possible. If you look into commit you'll see the changes. Will investigate.

baitcode · 2025-01-27T16:06:24Z

I didn't use apply on github. As the last time I did I then needed to manually add additional changes so I put everything into the commit.

baitcode · 2025-01-27T16:08:38Z

I've checked the changeset all changes are there. I'm investigating tests now.

baitcode · 2025-01-27T16:14:36Z

Yep. Found problem. Somehow lost one line of changes. Should be good now. @franciszekjob

franciszekjob · 2025-02-04T13:35:26Z

starknet_py/tests/e2e/mock/precompiled_contracts/argent-0.4.0/ArgentAccount.casm

Could you please link where exactly is this file taken from? I mean branch/tag in argent repo

main, latest commit.

deployments/artifacts/account-0.4.0-0x036078334509b514626504edc9fb252328d1a240e4e948bef8d0c08dff45927f

It looks like these files are different 🤔

@franciszekjob
Oh yes! My bad. I compiled those myself with pythonic hints enabled. This is the only difference between those.

franciszekjob reviewed Jan 14, 2025

View reviewed changes

baitcode added 2 commits January 15, 2025 12:24

Review fixes

c92045c

removing ABI file as it's content included in compiled contract

ed82cd5

Merge branch 'development' into fix-unskip-account-execution-test

e828f93

franciszekjob reviewed Jan 24, 2025

View reviewed changes

Review comments fixed

4092d8d

franciszekjob reviewed Jan 27, 2025

View reviewed changes

Lost change

6a9f90f

baitcode requested a review from franciszekjob January 27, 2025 17:48

franciszekjob reviewed Feb 4, 2025

View reviewed changes

	async def argent_account_class_hash_v040(
	async def argent_account_v040_class_hash(

V3 transactions in outside execution tests for argent #1554

Are you sure you want to change the base?

V3 transactions in outside execution tests for argent #1554

Conversation

baitcode commented Jan 14, 2025

Introduced changes

baitcode commented Jan 14, 2025

baitcode commented Jan 14, 2025 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

franciszekjob commented Jan 15, 2025

franciszekjob 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

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

Choose a reason for hiding this comment

baitcode commented Jan 25, 2025

franciszekjob left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baitcode commented Jan 27, 2025

baitcode commented Jan 27, 2025

baitcode commented Jan 27, 2025

baitcode commented Jan 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baitcode Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

baitcode Feb 4, 2025 • edited Loading

Choose a reason for hiding this comment

baitcode commented Jan 14, 2025 •

edited

Loading

baitcode commented Jan 27, 2025 •

edited

Loading

baitcode Feb 4, 2025 •

edited

Loading

baitcode Feb 4, 2025 •

edited

Loading