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

Account delete #3

Draft
wants to merge 7 commits into
base: main
Choose a base branch
from
Draft

Account delete #3

wants to merge 7 commits into from

Conversation

juan518munoz
Copy link

@juan518munoz juan518munoz commented Nov 24, 2023

Adds account delete command to the CLI.

Test:

  1. Create account using cargo run --release --features testing -- account new fungible-faucet -t TEST -d 10 -m 10000
  2. Get created account hexstring with cargo run --release --features testing -- account list
  3. Remove account with cargo run --release --features testing -- account -r 0x880448d77a6ec824

Pending unit test

src/store/mod.rs Outdated
@@ -134,6 +134,22 @@ impl Store {
.map(|_| ())
.map_err(StoreError::QueryError)
}

pub fn remove_account(&self, account_id: u64) -> Result<(), StoreError> {
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 make sure related objects are deleted as well for this command. In the future this can be optional if we see a scenario where it's positive to eliminate only partial data related to an account.

Copy link
Author

Choose a reason for hiding this comment

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

Added in latest commit, I'm unsure what to do with the associated row on the account_code table, should we only remove it if no other account makes use of it?

let q = self
.db
.execute(
"DELETE FROM accounts WHERE id = ?",
Copy link
Collaborator

Choose a reason for hiding this comment

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

If it's simple enough, it would be nice to see a test that tests this function.

Copy link
Author

Choose a reason for hiding this comment

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

Yes, to do this we should merge this branch with cli testing

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.

2 participants