[sozo] Deploy Account #1601

JimmyFate · 2024-03-03T09:02:55Z

Closes DOJ-176
Closes #1574

I would love to get some feedback on the PR before putting effort into writing the tests :)

Introduced changes

UI related changes:

sozo account new / deploy
sozo keystore new / from-key / inspect / inspect-private

Development related changes:

move private_key, keystore_path, keystore_password from AccountOptions into SignerOptions

codecov · 2024-03-03T13:45:19Z

Codecov Report

Attention: Patch coverage is 51.21951% with 320 lines in your changes are missing coverage. Please review.

Project coverage is 70.04%. Comparing base (a2412b3) to head (64271ff).

Files	Patch %	Lines
crates/sozo/ops/src/account.rs	27.36%	223 Missing ⚠️
bin/sozo/src/commands/options/fee.rs	2.12%	46 Missing ⚠️
bin/sozo/src/commands/account.rs	24.44%	34 Missing ⚠️
bin/sozo/src/commands/keystore.rs	60.00%	8 Missing ⚠️
crates/sozo/ops/src/keystore.rs	89.06%	7 Missing ⚠️
bin/sozo/src/commands/options/signer.rs	98.82%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #1601      +/-   ##
==========================================
- Coverage   70.32%   70.04%   -0.29%     
==========================================
  Files         309      315       +6     
  Lines       35126    35547     +421     
==========================================
+ Hits        24703    24898     +195     
- Misses      10423    10649     +226

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

glihm

Thank you for the work here!
Some minor comments for this first review. You can definitely start the testing phase to ensure everything is well supported.

I've made some tests with Katana, creating a new account and deploying it, works like a charm (tested with --disable-fee though).

I'm wondering if we can even simplify a bit, for instance without the simulate for the deployment. @ponderingdemocritus the idea here is not being exhaustive as Starkli, but supporting the minimum features to have sozo with all the basic account stuff, right?

@lambda-0x if you have a chance to revise this too, happy to have your feedback.

bin/sozo/src/commands/account.rs

bin/sozo/src/commands/options/signer.rs

bin/sozo/Cargo.toml

bin/sozo/src/commands/account.rs

bin/sozo/src/ops/account.rs

glihm · 2024-03-20T14:59:43Z

@JimmyFate how are you feeling about this? If you need any support or more information from the given reviews, don't hesitate to reach out to us. 👍

JimmyFate · 2024-03-20T23:21:08Z

@JimmyFate how are you feeling about this? If you need any support or more information from the given reviews, don't hesitate to reach out to us. 👍

Thanks for checking up ❤️ I'll try to complete the PR as soon as I can.
Unfortunately I've had some problems with git submodules between this and some other branch and lost some of the progress due to messed up git config

JimmyFate · 2024-03-21T03:13:02Z

@glihm
Rebased (new with sozo ops in own crate) and updated the PR with the comments.

As for simplifying the logic - I feel like, for example the mentioned simulate flag, is a nice to have, and removing it wouldn't remove much of the code. However I'm down to do so if it gets decided on.

Could then also remove any other feature with the goal of simplification - probably FeeSetting would be the next thing to remove and leave just setting --max-fee manually.

Also probably should add sozo account fetch as well, since right now if waiting for the DEPLOY_ACCOUNT tx is interrupted, there is no way of getting the up-to-date valid account file.

Will add the tests soon.

glihm · 2024-03-21T04:07:09Z

Unfortunately I've had some problems with git submodules between this and some other branch and lost some of the progress due to messed up git config

No problem at all, it was more a ping check to ensure you still interested working on that until having the PR in good shape to be merged. :)

@glihm Rebased (new with sozo ops in own crate) and updated the PR with the comments.

Awesome, appreciate the reactivity and the update on that.

As for simplifying the logic - I feel like, for example the mentioned simulate flag, is a nice to have, and removing it wouldn't remove much of the code. However I'm down to do so if it gets decided on.

Let's keep it then, good point.

Could then also remove any other feature with the goal of simplification - probably FeeSetting would be the next thing to remove and leave just setting --max-fee manually.

I was exactly thinking about this, and usually working with Katana, fees are disabled. But when users are working with other chain than Katana, it can be useful. Also, the sozo execute is not including any fee setting, and by default starknet-rs makes an estimation. It's out of the scope of this PR, but we may want to re-use this FeeSettings.

Also probably should add sozo account fetch as well, since right now if waiting for the DEPLOY_ACCOUNT tx is interrupted, there is no way of getting the up-to-date valid account file.

Shouldn't we deploy it again? Because the file is already written, but if not deployed, the file is not updated and still in undeployed state.
But independently from this use-case, having the account fetch can be very handy with working with remote Katana deployed on Slot for instance. So if you're ok, we can add it.

bin/sozo/src/commands/account.rs

crates/dojo-utils/Cargo.toml

This reverts commit 43a1f6e.

glihm

Great work @JimmyFate, thanks for that!

Made some testings, it's getting nice.
Could we add a test for the account deploy to have all 3 tested?
I've revised and the katana runner does not expose the --disable-fee. So you can to an execute first to transfer funds to the account to deploy.

sozo execute 0x49d36570d4e46f48e99674bd3fcc84644ddd6b96f7c741b1562b82f9e004dc7 transfer -c <ACCOUNT_ADDRESS>,10000000000000000,0

And you can use the first pre-funded account to do this execute.

bin/sozo/src/commands/account.rs

JimmyFate · 2024-03-30T01:46:09Z

@glihm
I am having a hard time adding a test for deploy. Tried transferring from the erc20 as you suggested but somehow couldn't make it work. Another thing is the prompt for stdin which I guess could be omitted with checking if environment variable like "PASS_STDIN" (which would be set in the test) is set. Ofc the stdin could be mocked, but there isn't one in dojo yet and I haven't worked with mocking libs in rust previously (I guess mockall could work).

Also, overall this PR has grown quite bigger than I initially expected making it hard to maintain with the rebasing.
I am not sure if I'll have the time to finish this :/ (in a reasonable time at least, as the PR is already open for some time and I wouldn't want to stall for much longer)

Other than that my Mac is having a tough time lately with regards to its speed, especially noticeable when building / running dojo in debug mode, so it just pushes me to upgrade sooner than later 🫠

glihm · 2024-03-30T22:04:44Z

@glihm I am having a hard time adding a test for deploy. Tried transferring from the erc20 as you suggested but somehow couldn't make it work. Another thing is the prompt for stdin which I guess could be omitted with checking if environment variable like "PASS_STDIN" (which would be set in the test) is set. Ofc the stdin could be mocked, but there isn't one in dojo yet and I haven't worked with mocking libs in rust previously (I guess mockall could work).

Also, overall this PR has grown quite bigger than I initially expected making it hard to maintain with the rebasing. I am not sure if I'll have the time to finish this :/ (in a reasonable time at least, as the PR is already open for some time and I wouldn't want to stall for much longer)

Other than that my Mac is having a tough time lately with regards to its speed, especially noticeable when building / running dojo in debug mode, so it just pushes me to upgrade sooner than later 🫠

Man, you did a great job and appreciate the dedication to that.
To ease the testing and also leveraging the ops crate, we could do the following:

The new function into ops/account could return the address of the created account.
The deploy function can have an argument --no-confirmation or something similar.
Doing so, we can create the account, pre-funding it, and then deploy it without waiting stdin.

If you're ok to do it, let me know. But once again, great job on the PR here and thank you for your time to contribute!

Could you anyway please reach out to me on the Dojo discord when you have a chance? 👍

glihm

Some notes here to finish the PR and testing. I would do this in the week if not tackled before. 👍

crates/sozo/ops/src/account.rs

glihm · 2024-04-08T02:52:33Z

crates/sozo/ops/src/account.rs

+            format!("{:#064x}", target_deployment_address).bright_yellow()
+        );
+
+        eprint!("Press [ENTER] once you've funded the address.");


Add an option to skip this to allow testing and automated deployments.

lambda-0x · 2024-04-10T10:39:44Z

Cargo.toml

-serde = { version = "1.0.192", features = [ "derive" ] }
-serde_json = { version = "1.0", features = [ "arbitrary_precision" ] }
+serde = { version = "1.0.192", features = ["derive"] }
+serde_json = { version = "1.0", features = ["arbitrary_precision"] }
 serde_with = "2.3.1"
 similar-asserts = "1.5.0"
-smol_str = { version = "0.2.0", features = [ "serde" ] }
-sqlx = { version = "0.7.2", features = [ "chrono", "macros", "regexp", "runtime-async-std", "runtime-tokio", "sqlite", "uuid" ] }
+smol_str = { version = "0.2.0", features = ["serde"] }
+sqlx = { version = "0.7.2", features = [
+  "chrono",
+  "macros",
+  "regexp",
+  "runtime-async-std",
+  "runtime-tokio",
+  "sqlite",
+  "uuid",
+] }


any idea why this formatting changed? on main even after running cargo +nightly fmt it stays the same.

while merging with main switched back to formatting we have on main to be consistent

lambda-0x · 2024-04-11T09:39:24Z

crates/sozo/ops/src/account.rs

+            format!("{:#064x}", target_deployment_address).bright_yellow()
+        );
+
+        eprint!("Press [ENTER] once you've funded the address.");


crates/sozo/ops/src/account.rs

glihm · 2024-04-11T16:22:58Z

Should wait the #1808 to use TransactionOption instead.

lambda-0x

lgtm

lambda-0x · 2024-04-21T14:33:02Z

Thank you @JimmyFate for your awesome work on this PR!

JimmyFate marked this pull request as ready for review March 5, 2024 23:22

JimmyFate mentioned this pull request Mar 6, 2024

[sozo] Add Deploy Account functionality #1574

Closed

glihm reviewed Mar 11, 2024

View reviewed changes

bin/sozo/src/commands/account.rs Outdated Show resolved Hide resolved

bin/sozo/src/commands/options/signer.rs Outdated Show resolved Hide resolved

lambda-0x reviewed Mar 14, 2024

View reviewed changes

bin/sozo/Cargo.toml Outdated Show resolved Hide resolved

bin/sozo/src/commands/account.rs Outdated Show resolved Hide resolved

bin/sozo/src/ops/account.rs Outdated Show resolved Hide resolved

glihm added the sozo label Mar 20, 2024

glihm added the contributor label Mar 20, 2024

sozo keystore, sozo account

44b6bb0

JimmyFate force-pushed the feat/sozo-account-deploy branch from 5c175d8 to 44b6bb0 Compare March 21, 2024 01:22

JimmyFate added 6 commits March 21, 2024 02:54

STARKNET_POLL_INTERVAL default 5000 -> 1000

5b27b62

Prompt password instead of error

44f4cb2

Remove num-bigint dependency

75e188a

Mention starkli license in sozo README

bb7eca6

Remove dead code

d313435

Add dojo-utils crate

43a1f6e

glihm reviewed Mar 21, 2024

View reviewed changes

bin/sozo/src/commands/account.rs Outdated Show resolved Hide resolved

crates/dojo-utils/Cargo.toml Outdated Show resolved Hide resolved

JimmyFate added 7 commits March 22, 2024 23:59

Revert "Add dojo-utils crate"

1580b16

This reverts commit 43a1f6e.

Use TransactionWaiter

550b658

Remove unwraps

5b8c1fc

Add sozo account fetch

60f62e1

Add keystore tests

2a22580

Add account tests

3aeb70b

Remove test_account_deploy

692869b

JimmyFate force-pushed the feat/sozo-account-deploy branch from fc1cd0f to 692869b Compare March 23, 2024 02:37

glihm reviewed Mar 26, 2024

View reviewed changes

bin/sozo/src/commands/account.rs Outdated Show resolved Hide resolved

Merge branch 'main' into feat/sozo-account-deploy

a2ce9b4

output -> file

a6d963c

glihm reviewed Apr 8, 2024

View reviewed changes

glihm mentioned this pull request Apr 9, 2024

[sozo] expose fee multiplier for sozo migrate #1801

Closed

add --no-confirmation flag and refactor deploy function

25c729a

lambda-0x reviewed Apr 11, 2024

View reviewed changes

lambda-0x added 2 commits April 11, 2024 15:20

Merge branch 'main' into feat/sozo-account-deploy

9e955c4

fix typo

f10c7ee

glihm assigned lambda-0x Apr 11, 2024

glihm added the blocked This issue is blocked due to some other issues label Apr 11, 2024

Merge branch 'main' into feat/sozo-account-deploy

cc75f82

lambda-0x force-pushed the feat/sozo-account-deploy branch from 21c0535 to cc75f82 Compare April 20, 2024 18:09

add no_wait for test

64271ff

lambda-0x approved these changes Apr 21, 2024

View reviewed changes

lambda-0x removed the blocked This issue is blocked due to some other issues label Apr 21, 2024

lambda-0x merged commit d92decd into dojoengine:main Apr 21, 2024
10 of 12 checks passed

kariy mentioned this pull request Apr 24, 2024

docs(katana): update DB er diagram #1874

Merged

lambda-0x mentioned this pull request Apr 26, 2024

[sozo] Merge FeeOptions into TransactionOptions #1887

Closed

kariy mentioned this pull request May 13, 2024

Docs for sozo account dojoengine/book#272

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[sozo] Deploy Account #1601

[sozo] Deploy Account #1601

JimmyFate commented Mar 3, 2024 •

edited by glihm

Loading

codecov bot commented Mar 3, 2024 •

edited

Loading

glihm left a comment

glihm commented Mar 20, 2024

JimmyFate commented Mar 20, 2024

JimmyFate commented Mar 21, 2024

glihm commented Mar 21, 2024

glihm left a comment

JimmyFate commented Mar 30, 2024 •

edited

Loading

glihm commented Mar 30, 2024

glihm left a comment

glihm Apr 8, 2024

lambda-0x Apr 11, 2024

lambda-0x Apr 10, 2024

lambda-0x Apr 11, 2024

lambda-0x Apr 11, 2024

glihm commented Apr 11, 2024

lambda-0x left a comment

lambda-0x commented Apr 21, 2024

[sozo] Deploy Account #1601

[sozo] Deploy Account #1601

Conversation

JimmyFate commented Mar 3, 2024 • edited by glihm Loading

Introduced changes

codecov bot commented Mar 3, 2024 • edited Loading

Codecov Report

glihm left a comment

Choose a reason for hiding this comment

glihm commented Mar 20, 2024

JimmyFate commented Mar 20, 2024

JimmyFate commented Mar 21, 2024

glihm commented Mar 21, 2024

glihm left a comment

Choose a reason for hiding this comment

JimmyFate commented Mar 30, 2024 • edited Loading

glihm commented Mar 30, 2024

glihm left a comment

Choose a reason for hiding this comment

glihm Apr 8, 2024

Choose a reason for hiding this comment

lambda-0x Apr 11, 2024

Choose a reason for hiding this comment

lambda-0x Apr 10, 2024

Choose a reason for hiding this comment

lambda-0x Apr 11, 2024

Choose a reason for hiding this comment

lambda-0x Apr 11, 2024

Choose a reason for hiding this comment

glihm commented Apr 11, 2024

lambda-0x left a comment

Choose a reason for hiding this comment

lambda-0x commented Apr 21, 2024

JimmyFate commented Mar 3, 2024 •

edited by glihm

Loading

codecov bot commented Mar 3, 2024 •

edited

Loading

JimmyFate commented Mar 30, 2024 •

edited

Loading