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.

Sign up for GitHub

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(wallet): add update seed storage password rpc #2317

Open
wants to merge 11 commits into
base: dev
Choose a base branch
from
Open

feat(wallet): add update seed storage password rpc #2317

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
dc16267
initial commit
borngraced Jan 9, 2025
4a536c6
minor changes to code arrangement in update_seed_storage_password
borngraced Jan 9, 2025
5f7830e
Merge branch 'dev' into implement-update-seed-storage-rpc
borngraced Jan 9, 2025
cfd85b8
fix wasm and minor changes
borngraced Jan 10, 2025
6d798a8
cargo fmt
borngraced Jan 10, 2025
a34ac6b
remove unused
borngraced Jan 10, 2025
cda913d
remove unnecessary vars
borngraced Jan 10, 2025
9e50c9f
update result type and use replace for indexeddb
borngraced Jan 10, 2025
b02c3f4
update doc
borngraced Jan 11, 2025
509ced8
return SuccessResponse in rpc result
borngraced Jan 17, 2025
0f15020
remove unused
borngraced Jan 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
131 changes: 87 additions & 44 deletions mm2src/mm2_main/src/lp_wallet.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
use common::HttpStatusCode;
use crypto::{decrypt_mnemonic, encrypt_mnemonic, generate_mnemonic, CryptoCtx, CryptoInitError, EncryptedData,
MnemonicError};
use enum_derives::EnumFromStringify;
use http::StatusCode;
use itertools::Itertools;
use mm2_core::mm_ctx::MmArc;
Expand Down Expand Up @@ -413,40 +414,45 @@ pub struct GetMnemonicResponse {
pub mnemonic: MnemonicForRpc,
}

#[derive(Debug, Display, Serialize, SerializeErrorType)]
#[derive(Debug, Display, Serialize, SerializeErrorType, EnumFromStringify)]
#[serde(tag = "error_type", content = "error_data")]
pub enum GetMnemonicError {
pub enum WalletsStorageRpcError {
#[display(fmt = "Invalid request error: {}", _0)]
dimxy marked this conversation as resolved.
Show resolved Hide resolved
InvalidRequest(String),
#[display(fmt = "Wallets storage error: {}", _0)]
WalletsStorageError(String),
#[display(fmt = "Internal error: {}", _0)]
Internal(String),
#[display(fmt = "Invalid password error: {}", _0)]
#[from_stringify("MnemonicError")]
InvalidPassword(String),
}

impl HttpStatusCode for GetMnemonicError {
impl HttpStatusCode for WalletsStorageRpcError {
fn status_code(&self) -> StatusCode {
match self {
GetMnemonicError::InvalidRequest(_) => StatusCode::BAD_REQUEST,
GetMnemonicError::WalletsStorageError(_) | GetMnemonicError::Internal(_) => {
WalletsStorageRpcError::InvalidRequest(_) | WalletsStorageRpcError::InvalidPassword(_) => {
StatusCode::BAD_REQUEST
},
WalletsStorageRpcError::WalletsStorageError(_) | WalletsStorageRpcError::Internal(_) => {
StatusCode::INTERNAL_SERVER_ERROR
},
}
}
}

#[cfg(not(target_arch = "wasm32"))]
impl From<WalletsStorageError> for GetMnemonicError {
fn from(e: WalletsStorageError) -> Self { GetMnemonicError::WalletsStorageError(e.to_string()) }
impl From<WalletsStorageError> for WalletsStorageRpcError {
fn from(e: WalletsStorageError) -> Self { WalletsStorageRpcError::WalletsStorageError(e.to_string()) }
}

#[cfg(target_arch = "wasm32")]
impl From<WalletsDBError> for GetMnemonicError {
fn from(e: WalletsDBError) -> Self { GetMnemonicError::WalletsStorageError(e.to_string()) }
impl From<WalletsDBError> for WalletsStorageRpcError {
fn from(e: WalletsDBError) -> Self { WalletsStorageRpcError::WalletsStorageError(e.to_string()) }
}

impl From<ReadPassphraseError> for GetMnemonicError {
fn from(e: ReadPassphraseError) -> Self { GetMnemonicError::WalletsStorageError(e.to_string()) }
impl From<ReadPassphraseError> for WalletsStorageRpcError {
fn from(e: ReadPassphraseError) -> Self { WalletsStorageRpcError::WalletsStorageError(e.to_string()) }
}

/// Retrieves the wallet mnemonic in the requested format.
Expand All @@ -456,7 +462,7 @@ impl From<ReadPassphraseError> for GetMnemonicError {
/// A `Result` type containing:
///
/// * [`Ok`]([`GetMnemonicResponse`]) - The wallet mnemonic in the requested format.
/// * [`MmError`]<[`GetMnemonicError>`]> - Returns specific [`GetMnemonicError`] variants for different failure scenarios.
/// * [`MmError`]<[`WalletsStorageRpcError>`]> - Returns specific [`WalletsStorageRpcError`] variants for different failure scenarios.
///
/// # Errors
///
Expand All @@ -480,20 +486,23 @@ impl From<ReadPassphraseError> for GetMnemonicError {
/// Err(e) => println!("Error: {:?}", e),
/// }
/// ```
pub async fn get_mnemonic_rpc(ctx: MmArc, req: GetMnemonicRequest) -> MmResult<GetMnemonicResponse, GetMnemonicError> {
pub async fn get_mnemonic_rpc(
ctx: MmArc,
req: GetMnemonicRequest,
) -> MmResult<GetMnemonicResponse, WalletsStorageRpcError> {
match req.mnemonic_format {
MnemonicFormat::Encrypted => {
let encrypted_mnemonic = read_encrypted_passphrase_if_available(&ctx)
.await?
.ok_or_else(|| GetMnemonicError::InvalidRequest("Wallet mnemonic file not found".to_string()))?;
.ok_or_else(|| WalletsStorageRpcError::InvalidRequest("Wallet mnemonic file not found".to_string()))?;
Ok(GetMnemonicResponse {
mnemonic: encrypted_mnemonic.into(),
})
},
MnemonicFormat::PlainText(wallet_password) => {
let plaintext_mnemonic = read_and_decrypt_passphrase_if_available(&ctx, &wallet_password)
.await?
.ok_or_else(|| GetMnemonicError::InvalidRequest("Wallet mnemonic file not found".to_string()))?;
.ok_or_else(|| WalletsStorageRpcError::InvalidRequest("Wallet mnemonic file not found".to_string()))?;
Ok(GetMnemonicResponse {
mnemonic: plaintext_mnemonic.into(),
})
Expand All @@ -508,40 +517,13 @@ pub struct GetWalletNamesResponse {
activated_wallet: Option<String>,
}

#[derive(Debug, Display, Serialize, SerializeErrorType)]
#[serde(tag = "error_type", content = "error_data")]
pub enum GetWalletsError {
#[display(fmt = "Wallets storage error: {}", _0)]
WalletsStorageError(String),
#[display(fmt = "Internal error: {}", _0)]
Internal(String),
}

impl HttpStatusCode for GetWalletsError {
fn status_code(&self) -> StatusCode {
match self {
GetWalletsError::WalletsStorageError(_) | GetWalletsError::Internal(_) => StatusCode::INTERNAL_SERVER_ERROR,
}
}
}

#[cfg(not(target_arch = "wasm32"))]
impl From<WalletsStorageError> for GetWalletsError {
fn from(e: WalletsStorageError) -> Self { GetWalletsError::WalletsStorageError(e.to_string()) }
}

#[cfg(target_arch = "wasm32")]
impl From<WalletsDBError> for GetWalletsError {
fn from(e: WalletsDBError) -> Self { GetWalletsError::WalletsStorageError(e.to_string()) }
}

/// Retrieves all created wallets and the currently activated wallet.
pub async fn get_wallet_names_rpc(ctx: MmArc, _req: Json) -> MmResult<GetWalletNamesResponse, GetWalletsError> {
pub async fn get_wallet_names_rpc(ctx: MmArc, _req: Json) -> MmResult<GetWalletNamesResponse, WalletsStorageRpcError> {
// We want to return wallet names in the same order for both native and wasm32 targets.
let wallets = read_all_wallet_names(&ctx).await?.sorted().collect();
// Note: `ok_or` is used here on `Constructible<Option<String>>` to handle the case where the wallet name is not set.
// `wallet_name` can be `None` in the case of no-login mode.
let activated_wallet = ctx.wallet_name.get().ok_or(GetWalletsError::Internal(
let activated_wallet = ctx.wallet_name.get().ok_or(WalletsStorageRpcError::Internal(
"`wallet_name` not initialized yet!".to_string(),
))?;

Expand All @@ -550,3 +532,64 @@ pub async fn get_wallet_names_rpc(ctx: MmArc, _req: Json) -> MmResult<GetWalletN
activated_wallet: activated_wallet.clone(),
})
}

/// `SeedPasswordUpdateRequest` represents a request to update
/// the password for the seed storage.
/// It includes the current password and the new password to be set.
#[derive(Debug, Deserialize)]
pub struct SeedPasswordUpdateRequest {
/// The current password for the seed storage.
pub current_password: String,
/// The new password to replace the current password.
pub new_password: String,
}

/// `SeedPasswordUpdateResponse` represents the result of a
/// password update request.
/// It contains a boolean indicating whether the operation was successful.
#[derive(Serialize)]
pub struct SeedPasswordUpdateResponse {
/// `true` if the password update was successful, `false` otherwise.
successful: bool,
}
Copy link
Member

@laruh laruh Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

reference to this comment #2317 (comment)
Such result representation confuses, I suggest to return just Ok(()) from update_seed_storage_password_rpc if success.
Or please explain why SeedPasswordUpdateResponse with successful: bool is preferable, if I missed the point.

Copy link
Member Author

@borngraced borngraced Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I don't want to return a null value for 200 status rpc response hence why I'm returning successful which is better than null

I will update the type field to be of type result: String instead(looks like a convention already) 9e50c9f

Copy link
Member

@laruh laruh Jan 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

well at least string is better then bool
upd: probably worth, string has "infinite" display variants

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can see we normally return result: "Success"

Copy link
Member

@laruh laruh Jan 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I can see we normally return result: "Success"

string type can have any msg, its not strict type
Not the best choice for successful result

Copy link
Collaborator

Choose a reason for hiding this comment

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

I guess some existing new rpcs are not used by the GUI yet, so we could change them

Copy link
Member

@laruh laruh Jan 13, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

what's your conclusion on this?

@borngraced We should provide new type which is supposed to show successful operation and if there is no need to return other info.

Provided by dimxy here #2317 (comment)

const KDF_SUCCESS_RES: &str = "Success";
pub struct KdfSuccessRes;
impl Serialize for KdfSuccessRes {
    fn serialize<S: Serializer>(&self, serializer: S) -> Result<S::Ok, S::Error> {
        serializer.serialize_str(KDF_SUCCESS_RES)
    }
}
pub async fn update_seed_storage_password_rpc(
    ctx: MmArc,
    req: SeedPasswordUpdateRequest,
) -> MmResult<KdfSuccessRes, WalletsStorageRpcError> {
    ...
    Ok(KdfSuccessRes)
}

Ok(KdfSuccessRes) is supposed to be used instead of Ok(()), so response will be

{
  "mmrpc": "2.0",
  "result": "success",
  "id": null
}

at least for update storage password RPCs such approach should be used

I guess some existing new rpcs are not used by the GUI yet, so we could change them

@dimxy maybe, but if such RPCs are already in master, then it would be a breaking change, right? Even if some of these RPCs are quite new. I suggest checking if there are any other new RPCs still in dev branch and not in master. If everything is already in master, I don't think we should touch it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

  1. BTW we already have this in code:
    pub struct SuccessResponse(&'static str);
    which for empty results produces this json:
    {"mmrpc":"2.0","result":"success","id":null}

  2. Actually we have yet another empty 'success' result:
    pub async fn update_nft(ctx: MmArc, req: UpdateNftReq) -> MmResult<(), UpdateNftError>
    which gives json:
    {"mmrpc":"2.0","result": null, "id":null}
    I think I like this one most of all because it provides simplest deserialisation (no need to switch between object or string for "result" value)

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Copy link
Member

@laruh laruh Jan 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

done

Could you return just Ok(()) pls, as in the end @dimxy came to conclusion that he prefers MmResult<(), Err> more (what I suggested to have at the beginning)

string is still not a strict type in the response. In such rpcs like update, the 200 status is actually what is expected.


/// RPC function to handle a request for updating the seed storage password.
///
/// # Arguments
/// - `ctx`: The shared context (`MmArc`) for the application.
/// - `req`: The `SeedPasswordUpdateRequest` containing the current and new passwords.
///
/// # Example
/// ```ignore
/// let request = SeedPasswordUpdateRequest {
/// current_password: "old_password".to_string(),
/// new_password: "new_password".to_string(),
/// };
/// let response = update_seed_storage_password_rpc(ctx, request).await?;
/// assert!(response.successful);
/// ```
Copy link
Member

Choose a reason for hiding this comment

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

We agreed not to describe function parameters in the function documentation.
Instead, provide documentation for the parameter types themselves (e.g. above SeedPasswordUpdateRequest).
It actually makes docs more readable. If I want to know nuances about each param or result I will go to its type doc comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

removed

pub async fn update_seed_storage_password_rpc(
ctx: MmArc,
req: SeedPasswordUpdateRequest,
) -> MmResult<SeedPasswordUpdateResponse, WalletsStorageRpcError> {
let wallet_name = ctx
.wallet_name
.get()
.ok_or(WalletsStorageRpcError::Internal(
"`wallet_name` not initialized yet!".to_string(),
))?
.clone()
.ok_or_else(|| WalletsStorageRpcError::Internal("`wallet_name` cannot be None!".to_string()))?;
// read mnemonic for a wallet_name using current user's password.
let mnemonic = read_and_decrypt_passphrase_if_available(&ctx, &req.current_password)
.await?
.ok_or(MmError::new(WalletsStorageRpcError::Internal(format!(
"{wallet_name}: wallet mnemonic file not found"
))))?;
// encrypt mnemonic with new passphrase.
let encrypted_data = encrypt_mnemonic(&mnemonic, &req.new_password)?;
// save new encrypted mnemonic data with new password
save_encrypted_passphrase(&ctx, &wallet_name, &encrypted_data).await?;

Copy link
Collaborator

Choose a reason for hiding this comment

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

could u actually verify whether this would play well in wasm?
this call eventually does table.add_item, which if the item already exists will error ConstraintError (as per this).

We should use table.replace_item to be able to supported updates.

Copy link
Member Author

@borngraced borngraced Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

done thank you 9e50c9f

Ok(SeedPasswordUpdateResponse { successful: true })
}
3 changes: 2 additions & 1 deletion mm2src/mm2_main/src/rpc/dispatcher/dispatcher.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ use crate::lp_stats::{add_node_to_version_stat, remove_node_from_version_stat, s
stop_version_stat_collection, update_version_stat_collection};
use crate::lp_swap::swap_v2_rpcs::{active_swaps_rpc, my_recent_swaps_rpc, my_swap_status_rpc};
use crate::lp_swap::{get_locked_amount_rpc, max_maker_vol, recreate_swap_data, trade_preimage_rpc};
use crate::lp_wallet::{get_mnemonic_rpc, get_wallet_names_rpc};
use crate::lp_wallet::{get_mnemonic_rpc, get_wallet_names_rpc, update_seed_storage_password_rpc};
use crate::rpc::lp_commands::db_id::get_shared_db_id;
use crate::rpc::lp_commands::one_inch::rpcs::{one_inch_v6_0_classic_swap_contract_rpc,
one_inch_v6_0_classic_swap_create_rpc,
Expand Down Expand Up @@ -217,6 +217,7 @@ async fn dispatcher_v2(request: MmRpcRequest, ctx: MmArc) -> DispatcherResult<Re
"trade_preimage" => handle_mmrpc(ctx, request, trade_preimage_rpc).await,
"trezor_connection_status" => handle_mmrpc(ctx, request, trezor_connection_status).await,
"update_nft" => handle_mmrpc(ctx, request, update_nft).await,
"update_seed_storage_password" => handle_mmrpc(ctx, request, update_seed_storage_password_rpc).await,
"update_version_stat_collection" => handle_mmrpc(ctx, request, update_version_stat_collection).await,
"verify_message" => handle_mmrpc(ctx, request, verify_message).await,
"withdraw" => handle_mmrpc(ctx, request, withdraw).await,
Expand Down
Loading