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

Added Wallet Export Functionality #227

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

Conversation

adirola
Copy link

@adirola adirola commented Dec 30, 2024

export command will request the password to unlock the wallet to derive the wallet mnemonic which is rendered on alternamte screen and the buffer is cleared. The derived addresses will be stored in cache.

Linked to PR : #222

@fuel-cla-bot
Copy link

fuel-cla-bot bot commented Dec 30, 2024

Thanks for the contribution! Before we can merge this, we need @adirola to sign the Fuel Labs Contributor License Agreement.

@adirola
Copy link
Author

adirola commented Dec 30, 2024

@kayagokalp here is the PR for export functionality

Copy link
Member

@sdankel sdankel left a comment

Choose a reason for hiding this comment

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

This is looking good, just a few nits.

Comment on lines +12 to +14
/// Forces export even if it might be unsafe
#[clap(short, long)]
pub force: bool,
Copy link
Member

Choose a reason for hiding this comment

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

It doesn't look like this is used anywhere. What does this mean?

Suggested change
/// Forces export even if it might be unsafe
#[clap(short, long)]
pub force: bool,

Comment on lines +15 to +17
/// How many accounts to cache by default (Default 10)
#[clap(short, long)]
pub cache_accounts: Option<usize>,
Copy link
Member

Choose a reason for hiding this comment

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

Let's use clap to set the default

Suggested change
/// How many accounts to cache by default (Default 10)
#[clap(short, long)]
pub cache_accounts: Option<usize>,
/// The number of accounts to cache
#[clap(long, default_value = 10)]
pub cache_accounts: usize,

}

/// Securely wipes sensitive data from memory
fn secure_wipe(data: &mut [u8]) {
Copy link
Member

Choose a reason for hiding this comment

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

Good idea! We should use zeroize instead to make sure this isn't optimized away by the compiler.

fn test_export_wallet() {
with_tmp_dir_and_wallet(|_dir, wallet_path| {
let result = export_wallet(&wallet_path, TEST_PASSWORD);
assert!(result.is_ok());
Copy link
Member

Choose a reason for hiding this comment

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

This can just be an unwrap

assert!(result.is_ok());

if let Ok(phrase) = result {
assert!(!phrase.is_empty());
Copy link
Member

Choose a reason for hiding this comment

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

We should assert that it's the test mnemonic

@sdankel
Copy link
Member

sdankel commented Mar 25, 2025

Hi @adirola , I've opened a new issue since the original issue is already solved: #237

If you comment there I can assign it to you. And please update the PR description with this new issue. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants