Replace subprocess #236

andrew-fleming · 2022-10-17T04:06:43Z

This PR supersedes #169 and resolves #161.

This PR proposes to include some notable changes that enable the removal of subprocess.

Account

Initializing the Account class requires Account to inherit the AsyncObject class.
- Python does not allow asynchronous __init__s; therefore, this is a bit of a hacky workaround.
- A cleaner solution could be to decouple Account initialization and deployment.

starknet_cli

Direct starknet_cli calls accept two parameters: args and command_args.
1. args: a dictionary wrapped in SimpleNamespace.
2. command_args: commands passed in an array as strings (similar to how Nile passed arguments to subprocess).
The new set_args function just requires the network and returns the necessary object to pass to starknet_cli.
command_args are appropriately created in the function itself.
The new capture_stdout captures the print statements from the starknet_cli calls. This is because those function calls do not return the values.

CLI

The CLI requires asyncclick.

NRE

Scripts must now be written with async/await.
- async def run(nre):
- balance = await nre.call(...)

Testing

Tests, of course, must use async/await when testing async functions.
For asynchronous mocks, AsyncMock should be used.

martriay

Looking good!

src/nile/core/account.py

martriay · 2022-11-15T02:29:38Z

src/nile/core/call_or_invoke.py

-            logging.info(output)
-
-        return output
+    return output


why not logging on query anymore?

It's still logging on query, the log command is before the query_flag and watch_mode conditional. The logic is just consolidated from how it was:

if type != "call" and output: logging.info(output) if not query_flag and watch_mode: transaction_hash = _get_transaction_hash(output) return await status(normalize_number(transaction_hash), network, watch_mode)

martriay · 2022-11-15T02:30:47Z

src/nile/core/declare.py

        contract_name=contract_name,
-        signature=signature,
+        signature=prepare_params(signature),


can/should prepare_params be called within execute_call?

They can and they were. The idea was to support set_command_args using a loop for most of the args (meaning less if statements and verbosity). That said, they probably should be called in execute_call. I'll throw them back in

I see prepare_params is used for signature inside execute_call already but hasn't been removed here right?

tests/commands/test_get_nonce.py

martriay · 2022-11-15T02:44:05Z

tests/commands/test_get_nonce.py

+        # args = set_args("goerli")
+        command_args = {"contract_address": "0x4d2"}


Suggested change

# args = set_args("goerli")

command_args = {"contract_address": "0x4d2"}

command_args = {"contract_address": contract_address}

This test checks that non-0x addresses convert to hex (hence the hardcoded 0x4d2)

martriay · 2022-11-15T04:02:11Z

src/nile/starknet_cli.py

+
+
+def _add_args(key, value):
+    return [f"--{key}", *value] if type(value) is list else [f"--{key}", value]


Suggested change

return [f"--{key}", *value] if type(value) is list else [f"--{key}", value]

return [f"--{key}", *value] if type(value) is list else [f"--{key}", value]

is it enough with *value or should we flatten if there's nested lists?

Yeah, flattening seems correct

src/nile/starknet_cli.py

Co-authored-by: Martín Triay <[email protected]>

ericnordelo

Looking very good Andrew! I left a couple of comments.

ericnordelo · 2022-11-15T12:26:13Z

src/nile/cli.py

@@ -2,7 +2,7 @@
 """Nile CLI entry point."""
 import logging

-import click
+import asyncclick as click


Is asyncclick usage implying a requirement to update plugins accordingly?

Great question! I'm honestly not sure. I'll try and test it to find out

src/nile/core/declare.py

ericnordelo · 2022-11-15T13:06:33Z

src/nile/core/declare.py

        contract_name=contract_name,
-        signature=signature,
+        signature=prepare_params(signature),


I see prepare_params is used for signature inside execute_call already but hasn't been removed here right?

ericnordelo · 2022-11-15T13:07:53Z

src/nile/core/deploy.py

        contract_name=contract_name,
+        inputs=prepare_params(arguments),


Aren't we calling prepare_params for inputs inside execute_call already?

Co-authored-by: Eric Nordelo <[email protected]>

ericnordelo

Looking very good Andrew!

tests/test_starknet_cli.py

Co-authored-by: Eric Nordelo <[email protected]>

ericnordelo

LGTM!

martriay

🚀 🚀 🚀 🚀 🚀 🚀

src/nile/core/declare.py

andrew-fleming added 29 commits October 16, 2022 23:36

add async deps

3b9bd03

update funcs to async

31faeb4

adjust run_cmd, add helper funcs

4313220

adjust methods to async

4ee68d4

adjust nre to async

3c916bb

fix formatting

6d742af

replace arg object, fix nre and output

c47a75a

fix formatting

1fe7e8d

add rstrip to capture

0f5770f

async get-nonce

2c38d82

remove comments

485469f

fix formatting

f7c76c7

add event loop

2c205e8

update tests without subprocess

cf3896e

update cli tests with asyncclick

26cf600

adjust tests to async

515a1fa

add set_args func

054cce6

simplify context with set_args

0b0da87

remove subprocess, add set_args

602b14b

add tests for common funcs

195a24b

async debug, remove subprocess

9fe9d17

fix test

50484e8

fix conflicts

9064a8c

fix conflicts

f6c7933

fix account test conflicts

e0f066c

update account tests to async

4170096

update debug test to async

cb138a7

update get_accounts to async

552b42c

start test refactor

910490e

andrew-fleming mentioned this pull request Oct 31, 2022

Replace subprocess with direct python calls #169

Closed

andrew-fleming requested review from martriay and ericnordelo November 15, 2022 01:11

martriay reviewed Nov 15, 2022

View reviewed changes

andrew-fleming and others added 9 commits November 14, 2022 23:55

Apply suggestions from code review

4952346

Co-authored-by: Martín Triay <[email protected]>

remove comment

aba30ec

rename set_args to set_context

88eb64b

reorder _process_arguments params

6b126b7

support nested lists in _add_args

cdc7cbc

suppress pytest warning

17b6e91

fix test

313cd6c

call prepare_params in starknet_cli

966ae14

fix tests

d44f78f

ericnordelo reviewed Nov 15, 2022

View reviewed changes

andrew-fleming and others added 4 commits November 15, 2022 11:55

Update src/nile/core/declare.py

3ded033

Co-authored-by: Eric Nordelo <[email protected]>

remove prepare_params from deploy and declare

02381be

fix formatting

df1e71f

fix tests

fab1279

ericnordelo previously approved these changes Nov 15, 2022

View reviewed changes

tests/test_starknet_cli.py Outdated Show resolved Hide resolved

Update tests/test_starknet_cli.py

ee0a44a

Co-authored-by: Eric Nordelo <[email protected]>

andrew-fleming dismissed ericnordelo’s stale review via ee0a44a November 15, 2022 21:19

ericnordelo previously approved these changes Nov 15, 2022

View reviewed changes

andrew-fleming requested a review from martriay November 16, 2022 16:54

martriay previously approved these changes Nov 16, 2022

View reviewed changes

src/nile/core/declare.py Outdated Show resolved Hide resolved

Update src/nile/core/declare.py

f27558e

martriay dismissed stale reviews from ericnordelo and themself via f27558e November 16, 2022 23:55

convert max_fee to str in starknet_cli

d37ce19

martriay approved these changes Nov 17, 2022

View reviewed changes

martriay merged commit f74b568 into OpenZeppelin:main Nov 17, 2022

andrew-fleming deleted the replace-subprocess branch November 17, 2022 05:10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace subprocess #236

Replace subprocess #236

andrew-fleming commented Oct 17, 2022 •

edited

Loading

martriay left a comment

martriay Nov 15, 2022

andrew-fleming Nov 15, 2022

martriay Nov 15, 2022

andrew-fleming Nov 15, 2022

ericnordelo Nov 15, 2022

martriay Nov 15, 2022

andrew-fleming Nov 15, 2022

martriay Nov 15, 2022

andrew-fleming Nov 15, 2022

ericnordelo left a comment

ericnordelo Nov 15, 2022

andrew-fleming Nov 15, 2022

ericnordelo Nov 15, 2022

ericnordelo Nov 15, 2022

ericnordelo left a comment

ericnordelo left a comment

martriay left a comment

		# args = set_args("goerli")
		command_args = {"contract_address": "0x4d2"}

	# args = set_args("goerli")
	command_args = {"contract_address": "0x4d2"}
	command_args = {"contract_address": contract_address}



		def _add_args(key, value):
		return [f"--{key}", *value] if type(value) is list else [f"--{key}", value]

		contract_name=contract_name,
		inputs=prepare_params(arguments),

Replace subprocess #236

Replace subprocess #236

Conversation

andrew-fleming commented Oct 17, 2022 • edited Loading

Account

starknet_cli

CLI

NRE

Testing

martriay 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

ericnordelo 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

ericnordelo left a comment

Choose a reason for hiding this comment

ericnordelo left a comment

Choose a reason for hiding this comment

martriay left a comment

Choose a reason for hiding this comment

andrew-fleming commented Oct 17, 2022 •

edited

Loading