Devnet predeployed accounts #192

ericnordelo · 2022-09-15T23:13:02Z

Add get_predeployed_accounts method and logic to cli and nre (compatible with starknet-devnet).

andrew-fleming

Great work, @ericnordelo! I left some comments and questions :)

andrew-fleming · 2022-09-23T04:51:59Z

README.md

+You can use it either from the cli:
+
+```sh
+nile get-accounts --predeployed
+```


Can we send txs from a predeployed account through the CLI?

We can't now, but it is an interesting idea. Maybe it will be worth modifying the send command (with a predeployed flag or similar) to allow sending tx through predeployed accounts? I'm curious about both your opinions on this @andrew-fleming @martriay

I assumed as much, just confirming. What do you think about removing the CLI snippet and the You can use it either from the cli? I don't think there's any value from running the command in the CLI and it might avoid users (or me) asking if we can send txs from the predeployeds through the CLI

Maybe it will be worth modifying the send command (with a predeployed flag or similar) to allow sending tx through predeployed accounts

Hmm it sounds like it'd be great to consider. I wouldn't include it in this PR though, that seems like a down-the-road feature

But you can use it from the cli, just not to send transactions. You can list the predeployed accounts from the cli, as you can do with registered accounts running nile get-accounts, so I think thee snippet still makes sense.

andrew-fleming · 2022-09-23T04:52:13Z

src/nile/cli.py

@@ -234,9 +237,13 @@ def debug(tx_hash, network, contracts_file):

 @cli.command()
 @network_option
-def get_accounts(network):
+@click.option("--predeployed/--registered", default=False)


It might be a good idea to give a quick mention to the default registered flag in the docs—probably in the get-accounts command

I was checking Click, and as you can check here, Click always wants you to provide an enable and disable flag so that you can change the default later, but just for this, the disable flag is not necessary and is not required from CLI, so I thought that with documenting the --predeployed is enough, because is never needed to pass the --registered flag. Let me know what you think.

Yeah, that's reasonable. Documenting --predeployed seems like enough

andrew-fleming · 2022-09-23T04:52:28Z

src/nile/common.py

+    # Starknet CLI requires an hex string for get nonce command
+    if not str(contract_address).startswith("0x"):
+        contract_address = hex(int(contract_address))


Just a reminder for us to remove get_nonce here after #197 (assuming we merge that PR first)

Yes, and also I just noticed that this method shouldn't receive anything not being an int in contract address (the design patter for hex and int we are trying to follow), so this parser should be refactored accordingly too.

andrew-fleming · 2022-09-23T04:53:31Z

src/nile/core/account.py

-    def __init__(self, signer, network):
+    def __init__(self, signer, network, predeployed_info=None):


I think it'd be extremely helpful to others (as well as ourselves) if we make an issue for adding params and return values in our docstrings

Agree, I've just created it #209 .

andrew-fleming · 2022-09-23T04:53:48Z

src/nile/core/account.py

+        self.abi_path = os.path.dirname(os.path.realpath(__file__)).replace(
+            "/core", "/artifacts/abis/Account.json"
+        )


andrew-fleming · 2022-09-23T05:01:28Z

src/nile/core/account.py

-        target_address, _ = next(deployments.load(to, self.network)) or to
+        target_address, _ = next(deployments.load(hex_address(to), self.network)) or to
+        # Work with integers internally
+        target_address = normalize_number(target_address)


Suggested change

target_address = normalize_number(target_address)

target_address = hex(target_address)

normalize_number gave me a lot of trouble here. I think using hex is the way to go

What troubles did you find? Using hex here is against the pattern we agreed, we should use int internally. Anyway, normalize_number is not necessary here because deployments.load always returns an int (being compliant with the pattern). I removed it.

I believe the issues were from how I was hacking the deployments.load when trying to get it to work locally. With the latest updates to the PR, everything's running well

andrew-fleming · 2022-09-23T05:04:01Z

src/nile/core/account.py

@@ -84,7 +100,7 @@ def send(self, to, method, calldata, max_fee, nonce=None):
        )

        return call_or_invoke(
-            contract=self.address,
+            contract=self,


This threw me off for a bit 😅

I think is better to pass the account instance to avoid passing a lot of extra parameters like the ABI (that was required for predeployed accounts). All these params are in the instance already. Suggestions are welcome to address this, of course.

Agreed on this being a better approach. Well done!

this is causing a circular dependency, i'm rolling it back in #268

____________________________________________________ ERROR collecting tests/commands/test_get_balance.py ____________________________________________________ ImportError while importing test module '/home/nile/tests/commands/test_get_balance.py'. Hint: make sure your test modules/packages have valid Python names. Traceback: /usr/lib/python3.8/importlib/__init__.py:127: in import_module return _bootstrap._gcd_import(name[level:], package, level) tests/commands/test_get_balance.py:7: in <module> from nile.utils.get_balance import get_balance .tox/default/lib/python3.8/site-packages/nile/utils/get_balance.py:3: in <module> from nile.core.call_or_invoke import call_or_invoke .tox/default/lib/python3.8/site-packages/nile/core/call_or_invoke.py:6: in <module> from nile.core import account .tox/default/lib/python3.8/site-packages/nile/core/account.py:20: in <module> from nile.core.call_or_invoke import call_or_invoke E ImportError: cannot import name 'call_or_invoke' from partially initialized module 'nile.core.call_or_invoke' (most likely due to a circular import) (/home/nile/.tox/default/lib/python3.8/site-packages/nile/core/call_or_invoke.py)

andrew-fleming · 2022-09-23T05:06:09Z

src/nile/core/call_or_invoke.py

+    else:
+        address, abi = next(deployments.load(contract, network))


Suggested change

else:

address, abi = next(deployments.load(contract, network))

else:

address, abi = next(deployments.load(contract, network))

address = hex(address)

I believe the address should also be converted to hex. When I try to script a basic get_balance query, I'm receiving a type error:

TypeError: expected str, bytes or os.PathLike object, not int

I added it outside the if/else for both branches before calling the starknet cli.

andrew-fleming · 2022-09-23T05:13:45Z

src/nile/utils/__init__.py

+
+
+def hex_address(number):
+    """Normalize hex or int to int."""


hex_address is trying to be a normalized number 🥸

andrew-fleming · 2022-09-23T05:21:17Z

tests/test_signer.py

    build_calls = []
    for call in calls:
        build_call = list(call)
-        build_call[0] = hex(build_call[0])
        build_calls.append(build_call)


Since we don't need to hexify the first arg, we can remove all this and just pass calls to get_raw_invoke and sign_transaction

Also, let's remember to update MockSigner and MockEthSigner in Contracts before the next release since they use the same build_calls logic

Fixed test.

andrew-fleming

Excellent improvements! Looks good to go

…predeployed-accounts-#137

ericnordelo added 2 commits September 16, 2022 01:05

feat: add devnet.py and get predeployed accounts logic to nre and cli

bed9996

refactor: format code and fix linter issues

94c4cb5

ericnordelo linked an issue Sep 15, 2022 that may be closed by this pull request

Integrate pre-deployed devnet accounts #137

Closed

ericnordelo marked this pull request as draft September 15, 2022 23:17

ericnordelo added 7 commits September 16, 2022 13:56

fix: remove unnecessary _accounts reassignment

7ea6979

feat: add normalize_number to utils

9a7a957

feat: add tests for get_predeployed_accounts

fa9fed1

fix: linter and format

0230a98

docs: update README.md for --predeployed entry

373edc9

refactor: remove devnet file

41f5bca

fix: cast to string before assertion

0ee42ac

ericnordelo marked this pull request as ready for review September 20, 2022 16:26

ericnordelo requested review from andrew-fleming and martriay September 20, 2022 16:29

ericnordelo added 7 commits September 22, 2022 01:18

feat: remove logs from requests

6257066

feat: use contract abi and address in call or invoke

b57610f

fix: format files

a55453f

feat: update get_nonce method

e263744

feat: normalize input from load methods

577e6b5

tmp: working on int refactor

09ca436

fix: update tests

fd4835b

andrew-fleming reviewed Sep 23, 2022

View reviewed changes

ericnordelo added 2 commits September 23, 2022 14:29

fix: update review

9eaa6eb

fix: format and linter

04fc9d4

ericnordelo requested a review from andrew-fleming September 23, 2022 12:32

andrew-fleming previously approved these changes Sep 23, 2022

View reviewed changes

Merge branch 'main' of github.com:OpenZeppelin/nile into feat/devnet-…

3d269bd

…predeployed-accounts-#137

ericnordelo dismissed andrew-fleming’s stale review via 3d269bd September 26, 2022 15:30

andrew-fleming approved these changes Sep 26, 2022

View reviewed changes

andrew-fleming merged commit acb1f58 into OpenZeppelin:main Sep 26, 2022

ericnordelo deleted the feat/devnet-predeployed-accounts-#137 branch September 26, 2022 16:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Devnet predeployed accounts #192

Devnet predeployed accounts #192

ericnordelo commented Sep 15, 2022

andrew-fleming left a comment

andrew-fleming Sep 23, 2022

ericnordelo Sep 23, 2022

andrew-fleming Sep 23, 2022

ericnordelo Sep 23, 2022

andrew-fleming Sep 23, 2022

ericnordelo Sep 23, 2022

andrew-fleming Sep 23, 2022

andrew-fleming Sep 23, 2022

ericnordelo Sep 23, 2022

andrew-fleming Sep 23, 2022

ericnordelo Sep 23, 2022 •

edited

Loading

andrew-fleming Sep 23, 2022

andrew-fleming Sep 23, 2022

ericnordelo Sep 23, 2022

andrew-fleming Sep 23, 2022

andrew-fleming Sep 23, 2022

ericnordelo Sep 23, 2022

andrew-fleming Sep 23, 2022

martriay Nov 18, 2022

andrew-fleming Sep 23, 2022

ericnordelo Sep 23, 2022

andrew-fleming Sep 23, 2022

andrew-fleming Sep 23, 2022

andrew-fleming Sep 23, 2022

ericnordelo Sep 23, 2022

andrew-fleming left a comment

		def __init__(self, signer, network):
		def __init__(self, signer, network, predeployed_info=None):

	target_address = normalize_number(target_address)
	target_address = hex(target_address)

		else:
		address, abi = next(deployments.load(contract, network))

Devnet predeployed accounts #192

Devnet predeployed accounts #192

Conversation

ericnordelo commented Sep 15, 2022

andrew-fleming 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

ericnordelo Sep 23, 2022 • 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

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

andrew-fleming left a comment

Choose a reason for hiding this comment

ericnordelo Sep 23, 2022 •

edited

Loading