Added support for signer #33

exp-table · 2021-11-18T15:12:23Z

No description provided.

fracek · 2021-11-19T21:48:48Z

src/nile/commands/proxy.py

+        signer_data = next(accounts.load(str(signer.public_key), network))
+        signer.account = signer_data["address"]
+        signer.index = signer_data["index"]
+    else: #doesn't exist, havbe to deploy


There should be a space between # and doesn't. There's a typo (havbe should be have).

fracek · 2021-11-19T21:49:16Z

src/nile/commands/proxy.py

+        signer.index = accounts.current_index(network)
+        subprocess.run(f"nile deploy Account {signer.public_key} --alias account-{signer.index}", shell=True)
+        address, _ = next(deployments.load(f"account-{signer.index}", network))
+        #initialize account


Space between # and initialize

fracek · 2021-11-19T21:50:34Z

I like this! I added a couple of nitpicky comments.

exp-table · 2021-11-19T21:52:18Z

I like this! I added a couple of nitpicky comments.

Fixed it!

fracek · 2021-11-19T22:51:14Z

Ok there are a couple of linting issues. My advice is to run tox -e format to automatically format everything, then tox -e lint to double check everything is formatted correctly.

martriay · 2021-11-20T06:30:31Z

This looks very nice! A few comments:

Do you think proxy could be merged into send? Or maybe I'm not understanding the different use case. In any case, I don't think that's the right name for it since it refers to a different kind of contracts which delegate execution to other contracts. Maybe raw_execute?
Could you expand the documentation to include an example env file and commands snippets with actual parameters? That help users a lot.

Future work could be:

generating a private key, needed for 2 and generally useful
auto generating a few accounts on nile node startup, use one by default or select one with --account (index or alias)
pick up private keys from an env/config file automatically with aliases too, on top of the default ones. with this, the setup command could be used to exceptionally but not regularly deploy accounts -- it could be cumbersome otherwise to redeploy each time you reset the node. conversely, if we're auto-picking keys from the dotenv, the command's interface could take directly a private key or even generate one automatically.

martriay · 2021-11-20T06:54:56Z

src/nile/commands/proxy.py

+        signer.index = signer_data["index"]
+    else: # doesn't exist, have to deploy
+        signer.index = accounts.current_index(network)
+        subprocess.run(f"nile deploy Account {signer.public_key} --alias account-{signer.index}", shell=True)


why not importing the module?

I didn't even think about it when prototyping initially. Good catch, fixed it!

exp-table · 2021-11-20T10:35:17Z

This looks very nice! A few comments:

* Do you think `proxy` could be merged into `send`? Or maybe I'm not understanding the different use case. In any case, I don't think that's the right name for it since it refers to a different kind of contracts which delegate execution to other contracts. Maybe `raw_execute`?

* Could you expand the documentation to include an example env file and commands snippets with actual parameters? That help users a lot.

I renamed them to send and raw-execute. As you said, proxy isn't the right label for what it does.
I also added an example example.env file as well as enhanced documentations for send and raw-execute.

fracek

Looks good to me. All issues have been addressed, thanks!

fracek · 2021-12-03T21:25:24Z

src/nile/signer.py

+"""Utility for sending signed transactions to an Account on Starknet."""
+import subprocess
+
+from starkware.cairo.common.hash_state import compute_hash_on_elements


These should be be wrapped in a try catch to avoid throwing when users don't have cairo installed yet (and want to use nile to install it).

fracek · 2021-12-03T21:26:41Z

One last thing before I'm ready to merge. Sorry for letting you wait so long to review this PR!

martriay · 2021-12-03T21:28:32Z

README.md

+
+A few things to notice here:
+
+1. `nile set <env_var>` looks for an environement variable with the same name whose value is a private key


there's no nile set command :)

fracek · 2021-12-03T21:44:28Z

README.md

@@ -88,6 +88,56 @@ A few things to notice here:
 3. The `--alias` parameter lets me create an unique identifier for future interactions, if no alias is set then the contract's address can be used as identifier
 4. By default Nile works on local, but you can pass `--network mainnet` to deploy directly to a public chain! Notice that `mainnet` refers to StarkNet main chain, that's settled on Goerli testnet of Ethereum ([mainnet deployment this month!](https://medium.com/starkware/starknet-alpha-is-coming-to-mainnet-b825829eaf32))

+### `setup`
+You can find an exemple `.env` file in `example.env`. These are private keys only to be used for testing and never in production.


exemple -> example

fracek

Looks like it works well on a local network but does not work on mainnet because the network argument is missing in the call to get the nonce.

fracek · 2021-12-04T14:41:12Z

src/nile/signer.py

+    def get_nonce(self):
+        """Get the nonce for the next transaction."""
+        nonce = subprocess.check_output(
+            f"nile call account-{self.index} get_nonce", shell=True, encoding="utf-8"


Here it's missing a --network argument so when using a non-default network this call fails

fracek

I run this in a fresh venv and everything is working as expected

martriay · 2021-12-05T02:28:40Z

Thanks all 🎉

* Signer support * stringified all command items * loading pkeys from .env * added accounts.json management * updated readme' * Added nile send * fixed comments spacing * passing tox linting * renamed the proxy commands and added examples * import module instead of cli commands * fixed README typo * fixed another readme typo * fixed import path issues + functions arguments * shipping Account artifacts + fixed setup file * try/catch starkware import + readme typo fixed * forgot to run tox format * passing network to signer

exp-table added 6 commits November 18, 2021 13:30

Signer support

083cc74

stringified all command items

c2356ec

loading pkeys from .env

de03e6e

added accounts.json management

4719c5c

updated readme'

69da9f0

Added nile send

5f8925e

$fracek$

fracek reviewed Nov 19, 2021

View reviewed changes

fixed comments spacing

10ab0af

martriay reviewed Nov 20, 2021

View reviewed changes

exp-table added 3 commits November 20, 2021 11:02

passing tox linting

33f51e5

renamed the proxy commands and added examples

cb261c3

import module instead of cli commands

bb31eeb

exp-table added 2 commits November 20, 2021 11:35

fixed README typo

dfc6e4e

fixed another readme typo

6e6b56f

$fracek$

fracek approved these changes Nov 20, 2021

View reviewed changes

exp-table requested a review from martriay November 22, 2021 09:57

exp-table added 2 commits November 24, 2021 11:20

fixed import path issues + functions arguments

fc280fd

shipping Account artifacts + fixed setup file

75beda4

$fracek$

fracek reviewed Dec 3, 2021

View reviewed changes

martriay suggested changes Dec 3, 2021

View reviewed changes

$fracek$

fracek reviewed Dec 3, 2021

View reviewed changes

exp-table added 2 commits December 4, 2021 00:13

try/catch starkware import + readme typo fixed

cdeecc6

forgot to run tox format

a1330bf

$fracek$

fracek suggested changes Dec 4, 2021

View reviewed changes

passing network to signer

7835e43

exp-table requested review from fracek and martriay December 4, 2021 15:37

$fracek$

fracek approved these changes Dec 4, 2021

View reviewed changes

martriay approved these changes Dec 5, 2021

View reviewed changes

martriay merged commit 8241b4c into OpenZeppelin:main Dec 5, 2021

martriay mentioned this pull request Jan 19, 2022

Add signers #28

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added support for signer #33

Added support for signer #33

exp-table commented Nov 18, 2021

$@fracek$ fracek Nov 19, 2021 •

edited

Loading

$@fracek$ fracek Nov 19, 2021

fracek commented Nov 19, 2021

exp-table commented Nov 19, 2021

fracek commented Nov 19, 2021

martriay commented Nov 20, 2021 •

edited

Loading

martriay Nov 20, 2021

exp-table Nov 20, 2021

exp-table commented Nov 20, 2021

$@fracek$ fracek left a comment

$@fracek$ fracek Dec 3, 2021 •

edited

Loading

fracek commented Dec 3, 2021

martriay Dec 3, 2021

$@fracek$ fracek Dec 3, 2021

$@fracek$ fracek left a comment

$@fracek$ fracek Dec 4, 2021

$@fracek$ fracek left a comment

martriay commented Dec 5, 2021


		A few things to notice here:

		1. `nile set <env_var>` looks for an environement variable with the same name whose value is a private key

Added support for signer #33

Added support for signer #33

Conversation

exp-table commented Nov 18, 2021

fracek Nov 19, 2021 • edited Loading

Choose a reason for hiding this comment

fracek Nov 19, 2021

Choose a reason for hiding this comment

fracek commented Nov 19, 2021

exp-table commented Nov 19, 2021

fracek commented Nov 19, 2021

martriay commented Nov 20, 2021 • edited Loading

martriay Nov 20, 2021

Choose a reason for hiding this comment

exp-table Nov 20, 2021

Choose a reason for hiding this comment

exp-table commented Nov 20, 2021

fracek left a comment

Choose a reason for hiding this comment

fracek Dec 3, 2021 • edited Loading

Choose a reason for hiding this comment

fracek commented Dec 3, 2021

martriay Dec 3, 2021

Choose a reason for hiding this comment

fracek Dec 3, 2021

Choose a reason for hiding this comment

fracek left a comment

Choose a reason for hiding this comment

fracek Dec 4, 2021

Choose a reason for hiding this comment

fracek left a comment

Choose a reason for hiding this comment

martriay commented Dec 5, 2021

$@fracek$ fracek Nov 19, 2021 •

edited

Loading

$@fracek$ fracek Nov 19, 2021

martriay commented Nov 20, 2021 •

edited

Loading

$@fracek$ fracek left a comment

$@fracek$ fracek Dec 3, 2021 •

edited

Loading

$@fracek$ fracek Dec 3, 2021

$@fracek$ fracek left a comment

$@fracek$ fracek Dec 4, 2021

$@fracek$ fracek left a comment