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

feat(sncast): generate default name when using account import #2610

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

khayss
Copy link

@khayss khayss commented Oct 24, 2024

Closes #2581

Introduced changes

All changes made are focused on sncast.
In crates/sncast/src/starknet_commands/account/import.rs, I added a utility function to generate a random account name which is used to supply the account construct with a name where the user does not specify any to the Also updated the import function in this file by changing the function parameter to accept an Option<String> type for account parameter. This way, users can import accounts without requiring name to be specified. Also, updated the Import struct name key to have a type of Option<String>.

In crates/sncast/src/main.rs, I updated the argument feed into the import function account parameter.

I updated the necessary documentation and logs.

Checklist

  • Linked relevant issue
  • Updated relevant documentation
  • Added relevant tests
  • Performed self-review of the code
  • Added changes to CHANGELOG.md

Copy link
Collaborator

@franciszekjob franciszekjob left a comment

Choose a reason for hiding this comment

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

  1. Please fix linting.
  2. Add test for importing account without --name flag.
  3. Mention the change in CHANGELOG.md.

Comment on lines +207 to +215
fn generate_default_account_name() -> String {
let start = SystemTime::now();
let since_epoch = start
.duration_since(UNIX_EPOCH)
.expect("Time went backwards");
let timestamp = since_epoch.as_millis();
format!("account-{}", timestamp)
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's refactor this function to create account with sequential names (account-1, account-2, ...). Name with timestamp (e.g. account-1729865041172) is too complicated.
Our logic should find next available numer (and handle gaps, e.g. having account-1 and account-3 in file, imported account should receive name account-2).

You can see how accounts are read from the file here.

Copy link
Author

Choose a reason for hiding this comment

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

Alright. This was my initial plan. I'll fall back to it.

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.

Generate default name when using account import
2 participants