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

fix: wallet seed_words command is broken (see issue #4363) #4370

Merged
merged 3 commits into from
Aug 2, 2022

Conversation

agubarev
Copy link
Contributor

@agubarev agubarev commented Aug 1, 2022

Description

#4363

Motivation and Context

Seed word command is broken on GRPC

thread 'tokio-runtime-worker' panicked at 'calledOption::unwrap()on aNone value', applications/tari_console_wallet/src/grpc/wallet_grpc_server.rs:1210:50

How Has This Been Tested?

…le wallet's gRPC server implementation

Signed-off-by: Andrei Gubarev <agubarev@users.noreply.github.com>
Signed-off-by: Andrei Gubarev <agubarev@users.noreply.github.com>
Copy link
Member

@sdbondi sdbondi left a comment

Choose a reason for hiding this comment

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

Definitely, an improvement. Panic cases are handled, but as you pointed out, it's risky to delete a file path controlled by user input.

self.output_manager_service.remove_encryption().await?;
self.transaction_service.remove_encryption().await?;
self.key_manager_service.remove_encryption().await?;
self.db.remove_encryption().await?;
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 rather not sneak this in :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Been waiting for another PR for days, you got me :)

@@ -1205,47 +1206,38 @@ impl wallet_server::Wallet for WalletGrpcServer {
}
}

/// Returns the contents of a seed words file, provided via CLI
async fn seed_words(&self, _: Request<tari_rpc::Empty>) -> Result<Response<SeedWordsResponse>, Status> {
let cli = Cli::parse();
Copy link
Member

Choose a reason for hiding this comment

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

TODO: Cli::parse should (ideally) not be used in GRPC, rather the ApplicationConfig should be made available to the GRPC server

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, a good idea

@aviator-app aviator-app bot merged commit 1cabd70 into tari-project:development Aug 2, 2022
@agubarev agubarev deleted the issue-4363 branch August 23, 2022 08:11
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