Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add account new command #18

Merged
merged 13 commits into from
Nov 22, 2023
Merged

Add account new command #18

merged 13 commits into from
Nov 22, 2023

Conversation

igamigo
Copy link
Collaborator

@igamigo igamigo commented Nov 14, 2023

Draft of account creation command and tables to store account data

  • Updates migration file to add tables relevant to accounts
  • Adds a command that creates a new account and stores it in the tables
  • Adds a "testing" feature that uses the testing feature from the base crates so that the PoW seed grinding is not as expensive
  • Minor refactors to the command line args
  • Implements 3 "templates" (basic mutable, basic immutable, and fungible-faucet) for easy account creation

Test with cargo run --release --features testing -- account new fungible-faucet -t TEST -d 10 -m 10000 and then cargo run --release --features testing -- account list

TODO:

  • Add some tests mainly related to the new tables (added a simple test, adding more in future PRs)
  • Add a way to define the fungible-faucet parameters (ticker, decimals, initial supply)
  • Add a way to generate the remaining types of accounts (only NF-faucet missing)
  • Add a way to generically create wallets (with a local code file, etc.) (left for future work)

Currently ModuleAst does not derive Serialize so we might have to add that in the corresponding repo if we want to store that as a serialized blob

Copy link
Contributor

@frisitano frisitano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks! I've left some comments - primarily around coding style.

Cargo.toml Outdated Show resolved Hide resolved
src/cli/account.rs Outdated Show resolved Hide resolved
src/cli/account.rs Outdated Show resolved Hide resolved
src/cli/account.rs Outdated Show resolved Hide resolved
src/cli/mod.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
src/store/mod.rs Outdated Show resolved Hide resolved
src/store/mod.rs Outdated Show resolved Hide resolved
src/store/mod.rs Outdated Show resolved Hide resolved
src/store/mod.rs Outdated Show resolved Hide resolved
@frisitano
Copy link
Contributor

I've opened the following issue #19 to discuss if we should consider using an ORM to manage our database models, migrations and queries.

src/store/store.sql Outdated Show resolved Hide resolved
@frisitano frisitano self-requested a review November 17, 2023 07:15
Copy link
Contributor

@frisitano frisitano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good - thanks for clarification on the account id i64 representation. Added a few more comments.

src/cli/account.rs Show resolved Hide resolved
src/cli/account.rs Outdated Show resolved Hide resolved
src/cli/account.rs Outdated Show resolved Hide resolved
src/cli/account.rs Outdated Show resolved Hide resolved
src/cli/account.rs Outdated Show resolved Hide resolved
src/cli/account.rs Outdated Show resolved Hide resolved
src/cli/account.rs Outdated Show resolved Hide resolved
src/errors.rs Outdated Show resolved Hide resolved
src/store/mod.rs Outdated Show resolved Hide resolved
@igamigo igamigo marked this pull request as ready for review November 17, 2023 17:27
Cargo.toml Outdated Show resolved Hide resolved
Copy link
Contributor

@frisitano frisitano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks! Once we have changes the branch for miden-lib and miden-objects back to main we can merge. I've also added a comment regarding adding the relations to the accounts table - should we do that now or part of a follow up PR?

src/cli/account.rs Outdated Show resolved Hide resolved
@@ -1,5 +1,6 @@
use lazy_static::lazy_static;
use rusqlite::{params, Connection};

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we typically avoid spacing between imports.

storage_root BLOB NOT NULL, -- root of the account_storage Merkle tree.
vault_root BLOB NOT NULL, -- root of the account_vault Merkle tree.
nonce BIGINT NOT NULL, -- account nonce.
committed BOOLEAN NOT NULL -- true if recorded, false if not.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we define the relations to the account_vault, account_code and account_storage tables here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Creating a follow-up PR for this

@igamigo igamigo merged commit 2a5b1ab into main Nov 22, 2023
@igamigo igamigo linked an issue Nov 29, 2023 that may be closed by this pull request
@bobbinth bobbinth deleted the account-new branch March 15, 2024 18:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

User can list accounts
3 participants