Add Account.declare #207

martriay · 2022-09-22T22:25:31Z

Fixes #186, #205.

This PR also refactors run_command so it actually centralizes starknet CLI calls.
It also implements nile.core.declare.get_hash which takes a contract_class and returns its hash (see Improve how declare handles an existing class hash #205).
An upcoming problem with this PR is that it also enables Account.deploy_contract which relies on a Deployer Contract to deploy, which eludes our deployments tracking/management system. I think we need to rethink its model, but probably in a different issue/PR.

src/nile/core/declare.py

andrew-fleming

Excellent PR! I left some questions and comments :)

README.md

src/nile/cli.py

src/nile/common.py

Co-authored-by: Andrew Fleming <[email protected]>

… into account-declare-deploy

andrew-fleming

Small fix on the click option, but otherwise this looks good to me!

src/nile/cli.py

src/nile/utils/__init__.py

README.md

ericglau

account's functions should set max_fee to be None by default (the functions' code already handles None but the function signatures need to reflect this)

src/nile/core/account.py

ericnordelo

I left a couple of comments, but I can provide a better review after conflicts with main are resolved, because there are important differences regarding the int pattern mostly.

README.md

src/nile/core/account.py

andrew-fleming

Looking good, @martriay! I left a few comments :)

src/nile/core/declare.py

tests/commands/test_account.py

src/nile/signer.py

ericglau · 2022-10-13T14:47:41Z

The get_hash logic looks good to me, thanks! (I didn't review the rest of this PR)

src/nile/nre.py

ericnordelo

Looking good! I added a comment and thumbs up to the Andrew review notes.

src/nile/core/account.py

ericnordelo

Looking good to go!

andrew-fleming

Looks good to go!

martriay added 2 commits September 22, 2022 19:24

refactor run_command. add Account.declare

34df9ce

test account.declare

00301a3

martriay linked an issue Sep 22, 2022 that may be closed by this pull request

Improve how declare handles an existing class hash #205

Closed

martriay requested review from andrew-fleming, ericglau and ericnordelo and removed request for ericnordelo, ericglau and andrew-fleming September 22, 2022 22:50

martriay marked this pull request as draft September 22, 2022 23:00

martriay added 2 commits September 22, 2022 20:00

tidy up

40a2b11

fix cli's declare. add documentation

08b1dca

martriay marked this pull request as ready for review September 22, 2022 23:36

martriay requested review from andrew-fleming, ericglau and ericnordelo September 22, 2022 23:37

fix test

1248777

ericglau reviewed Sep 23, 2022

View reviewed changes

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

andrew-fleming reviewed Sep 24, 2022

View reviewed changes

README.md Outdated Show resolved Hide resolved

src/nile/cli.py Outdated Show resolved Hide resolved

src/nile/common.py Show resolved Hide resolved

martriay and others added 3 commits September 24, 2022 06:16

Update README.md

818290d

Co-authored-by: Andrew Fleming <[email protected]>

apply review suggestions

40987c0

Merge branch 'account-declare-deploy' of github.com:OpenZeppelin/nile…

e4de118

… into account-declare-deploy

martriay requested review from andrew-fleming and ericglau September 24, 2022 04:29

andrew-fleming previously approved these changes Sep 24, 2022

View reviewed changes

src/nile/cli.py Outdated Show resolved Hide resolved

fix param

c1105dd

martriay dismissed andrew-fleming’s stale review via c1105dd September 24, 2022 04:48

martriay added 3 commits September 24, 2022 01:49

fix conflicts

6a92461

fix linter

56bcb8a

improve callorinvoke

3b92f90

fix linting

87c8ff9

ericglau reviewed Sep 26, 2022

View reviewed changes

src/nile/utils/__init__.py Outdated Show resolved Hide resolved

src/nile/utils/__init__.py Outdated Show resolved Hide resolved

ericglau reviewed Sep 27, 2022

View reviewed changes

README.md Show resolved Hide resolved

ericglau reviewed Sep 27, 2022

View reviewed changes

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

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

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

ericglau mentioned this pull request Sep 27, 2022

Pending upstream PRs OpenZeppelin/openzeppelin-nile-upgrades#8

Closed

ponderingdemocritus reviewed Sep 28, 2022

View reviewed changes

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

ericnordelo reviewed Oct 3, 2022

View reviewed changes

README.md Show resolved Hide resolved

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

martriay added 5 commits October 9, 2022 11:55

apply eric's review comments

6e8927a

remove unnecessary compilation. fix overriding paths

9db86a8

Merge branch 'main' into account-declare-deploy

b914f85

fix linter

aef90d8

fix linter

f478abc

martriay requested review from andrew-fleming, ericglau and ericnordelo October 9, 2022 16:49

ericnordelo mentioned this pull request Oct 10, 2022

Sender must be passed explicitly when making a direct declaration using --no_wallet #231

Closed

andrew-fleming reviewed Oct 11, 2022

View reviewed changes

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

tests/commands/test_account.py Outdated Show resolved Hide resolved

src/nile/signer.py Show resolved Hide resolved

andrew-fleming reviewed Oct 19, 2022

View reviewed changes

src/nile/nre.py Outdated Show resolved Hide resolved

ericnordelo reviewed Oct 19, 2022

View reviewed changes

src/nile/core/account.py Show resolved Hide resolved

martriay added 2 commits October 21, 2022 02:22

apply review suggestions

4e6bb8f

fix tests

e6c159b

ericnordelo previously approved these changes Oct 21, 2022

View reviewed changes

martriay requested a review from andrew-fleming October 21, 2022 20:29

andrew-fleming previously approved these changes Oct 21, 2022

View reviewed changes

Merge branch 'main' into account-declare-deploy

c65d8ac

martriay dismissed stale reviews from andrew-fleming and ericnordelo via c65d8ac October 21, 2022 20:57

martriay merged commit 01ddf87 into main Oct 21, 2022

martriay deleted the account-declare-deploy branch October 21, 2022 21:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Account.declare #207

Add Account.declare #207

martriay commented Sep 22, 2022 •

edited

Loading

andrew-fleming left a comment

andrew-fleming left a comment

ericglau left a comment •

edited

Loading

ericnordelo left a comment

andrew-fleming left a comment

ericglau commented Oct 13, 2022

ericnordelo left a comment

ericnordelo left a comment

andrew-fleming left a comment

Add Account.declare #207

Add Account.declare #207

Conversation

martriay commented Sep 22, 2022 • edited Loading

andrew-fleming left a comment

Choose a reason for hiding this comment

andrew-fleming left a comment

Choose a reason for hiding this comment

ericglau left a comment • edited Loading

Choose a reason for hiding this comment

ericnordelo left a comment

Choose a reason for hiding this comment

andrew-fleming left a comment

Choose a reason for hiding this comment

ericglau commented Oct 13, 2022

ericnordelo left a comment

Choose a reason for hiding this comment

ericnordelo left a comment

Choose a reason for hiding this comment

andrew-fleming left a comment

Choose a reason for hiding this comment

martriay commented Sep 22, 2022 •

edited

Loading

ericglau left a comment •

edited

Loading