feat(wallets): add `get_wallet_names` rpc #2202

shamardy · 2024-08-28T03:19:11Z

This PR introduces the get_wallet_names RPC method, which allows clients to retrieve information about all wallet names and the currently active one.

get_wallet_names Method: Retrieves all wallet names and the active wallet's name if there is an active one. If there is no active wallet (no-login mode), it returns null for the active wallet's name.

Example Request and Responses:

Request:

{
    "userpass": "{{userpass}}",
    "mmrpc": "2.0",
    "method": "get_wallet_names"
}

No-Login Mode Response:

{
    "mmrpc": "2.0",
    "result": {
        "wallet_names": ["Test 1", "Test 2"],
        "activated_wallet": null
    },
    "id": null
}

Activated Wallet Response:

{
    "mmrpc": "2.0",
    "result": {
        "wallet_names": ["Test 1", "Test 2"],
        "activated_wallet": "Test 1"
    },
    "id": null
}

To Test: @KomodoPlatform/qa
Example request and responses are above, I already tested this and @CharlVS probably did too, so it's not a high priority.

laruh

I have question

laruh · 2024-08-28T08:37:52Z

mm2src/mm2_main/src/lp_wallet.rs

+    let activated_wallet = ctx.wallet_name.ok_or(GetWalletsError::Internal(
+        "`wallet_name` not initialized yet!".to_string(),
+    ))?;


could you explain please, how you get Ok result in non-login mode with null activated_wallet, if you return Err when wallet_name None?

No-Login Mode Response from pr info

{ "mmrpc": "2.0", "result": { "wallet_names": ["Test 1", "Test 2"], "activated_wallet": null }, "id": null }

ok_or is for Constructible not Option

komodo-defi-framework/mm2src/mm2_core/src/mm_ctx.rs

Lines 114 to 115 in 9857170

/// Wallet name for this mm2 instance. Optional for backwards compatibility.

pub wallet_name: Constructible<Option<String>>,

Ah, exactly

I just ran into the same issue :) Maybe we should make it more explicit with some helpful comments?

(Unresolving the topic to make it visible - feel free to re-resolve the topic once you see it)

Added comment here 704305d

CharlVS

Thanks. I've implemented it into the KDF Flutter SDK and confirmed it's working and fits the GUI's needs.

The only point I want to make you aware of, @shamardy, is that I have to do a workaround where I spin up and a non-auth instance so that I can query the list of wallets and then stop, and (attempt to) restart it when user tries to sign in.

Long-term, it would be ideal if we can change auth states using RPCs while KDF is already running.

The SDK example app above provides basic wallet functionality with 0 GUI storage for authentication or seed generation. I will share a demo site link.

shamardy · 2024-08-30T11:00:46Z

The only point I want to make you aware of, @shamardy, is that I have to do a workaround where I spin up and a non-auth instance so that I can query the list of wallets and then stop

I thought we already do that in komodo wallet GUI (web). AFAIK, web GUI starts in no-login mode so that any user can see orderbooks, etc..

CharlVS · 2024-08-30T11:24:04Z

The only point I want to make you aware of, @shamardy, is that I have to do a workaround where I spin up and a non-auth instance so that I can query the list of wallets and then stop

I thought we already do that in komodo wallet GUI (web). AFAIK, web GUI starts in no-login mode so that any user can see orderbooks, etc..

We're starting fresh for the Frontend SDK. There's a lot of things that aren't ideal with the current GUI project KDF integration. One pain point in particular for the above is it's unreliable/messy to sift through the logs to parse for exceptions we need to throw in the GUI. At least with an RPC, we will know the result as soon as it's determined, and a response payload would be easier and more reliable for parsing in the UI.

This isn't urgent or something that has to be addressed now since the latered SDK architecture means it can be addressed without major refactoring in the future.

laruh

All is good, just one question/suggestion and one note

mm2src/mm2_io/src/fs.rs

…tive extensions

…ifiers function

laruh

LGTM!

onur-ozkan · 2024-09-17T09:47:24Z

get_wallet_names Method: Retrieves all wallet names and the active wallet's name if there is an active one. If there is no active wallet (no-login mode), it returns null for the active wallet's name.

What happens when we connect to an external wallet?

onur-ozkan

Any chance for adding test coverage for this implementation?

mm2src/mm2_io/src/fs.rs

onur-ozkan · 2024-09-17T09:56:59Z

mm2src/mm2_main/src/lp_wallet.rs

+    let activated_wallet = ctx.wallet_name.ok_or(GetWalletsError::Internal(
+        "`wallet_name` not initialized yet!".to_string(),
+    ))?;


I just ran into the same issue :) Maybe we should make it more explicit with some helpful comments?

(Unresolving the topic to make it visible - feel free to re-resolve the topic once you see it)

mariocynicys

Only some nits, LGTM otherwise!

mm2src/mm2_main/src/lp_wallet/mnemonics_storage.rs

mm2src/mm2_main/src/lp_wallet/mnemonics_wasm_db.rs

shamardy · 2024-09-19T01:03:43Z

What happens when we connect to an external wallet?

@onur-ozkan External wallet connection is done through coins activation as these wallets are coin dependant. This PR is for wallet names related to KDF mnemonic or passphrase that we init KDF with.

I guess we can add an option in another PR to start mm2 using an external wallet and delay crypto ctx initialization until it's needed for certain coins. It will be a big refactor but will make komodo wallet as a Dapp (e.g. Keplr integration) much smoother and will lead to better UX.

Edit: Opened issue for this #2225

shamardy · 2024-09-19T01:10:50Z

Any chance for adding test coverage for this implementation?

Sure, will try to add some tests.

shamardy · 2024-09-19T08:46:02Z

Any chance for adding test coverage for this implementation?

Tests added here 1a7c003

…stems_with_extension` into a single `list_files_by_extension` function

borngraced

Great work... just small notes

mm2src/mm2_io/src/fs.rs

mm2src/mm2_main/src/lp_wallet/mnemonics_storage.rs

mm2src/mm2_io/src/fs.rs

borngraced

Welldone!

onur-ozkan

Just minor notes. LGTM

mm2src/mm2_core/src/lib.rs

…test_with_id`

* dev: fix(orders): fix cancel order race condition using time-based cache (#2232) fix(legacy-swap): taker failed spend maker payment marked as failed (#2199) chore(adex-cli): deprecate adex-cli (#2234) feat(new-RPC): connection healthcheck implementation for peers (#2194) fix(proxy-signature): add message lifetime overflows (#2233) feat(CI): handle remote files in a safer way (#2217) chore(doc): update issue address in README (#2227) fix(merge): remove duplicated db_root function (#2229) feat(wallets): add `get_wallet_names` rpc (#2202) chore(tests): don't use `.wait()` and use `block_on` instead (#2220) fix(native-rpc): remove escaped response body (#2219) fix(clippy): fix coins mod clippy warnings in wasm (#2224) feat(core): handling CTRL-C signal with graceful shutdown (#2213) docs(README): fix typos (#2212) remove the non-sense arguments (#2216) fix(db): stop creating the all-zeroes dir on KDF start (#2218)

* dev: fix(cosmos): fix tx broadcasting error (#2238) chore(solana): remove solana implementation (#2239) chore(cli): remove leftover subcommands from help message (#2235) fix(orders): fix cancel order race condition using time-based cache (#2232) fix(legacy-swap): taker failed spend maker payment marked as failed (#2199) chore(adex-cli): deprecate adex-cli (#2234) feat(new-RPC): connection healthcheck implementation for peers (#2194) fix(proxy-signature): add message lifetime overflows (#2233) feat(CI): handle remote files in a safer way (#2217) chore(doc): update issue address in README (#2227) fix(merge): remove duplicated db_root function (#2229) feat(wallets): add `get_wallet_names` rpc (#2202) chore(tests): don't use `.wait()` and use `block_on` instead (#2220) fix(native-rpc): remove escaped response body (#2219) fix(clippy): fix coins mod clippy warnings in wasm (#2224) feat(core): handling CTRL-C signal with graceful shutdown (#2213) docs(README): fix typos (#2212) remove the non-sense arguments (#2216) fix(db): stop creating the all-zeroes dir on KDF start (#2218)

add get_wallet_names rpc

9857170

shamardy added the in progress Changes will be made from the author label Aug 28, 2024

shamardy mentioned this pull request Aug 28, 2024

feat(new-RPC): Add get_wallet RPC for Retrieving Active Wallet #2198

Closed

shamardy added under review and removed in progress Changes will be made from the author labels Aug 28, 2024

shamardy requested review from laruh and CharlVS August 28, 2024 03:25

laruh reviewed Aug 28, 2024

View reviewed changes

CharlVS previously approved these changes Aug 30, 2024

View reviewed changes

laruh reviewed Aug 30, 2024

View reviewed changes

mm2src/mm2_io/src/fs.rs Outdated Show resolved Hide resolved

mm2src/mm2_io/src/fs.rs Outdated Show resolved Hide resolved

review fix: allow filter_files_with_extension to work with case-sensi…

1de62ac

…tive extensions

shamardy dismissed CharlVS’s stale review via 1de62ac September 6, 2024 18:32

review fix: reduce duplication by extracting it to extract_file_ident…

f4162b8

…ifiers function

laruh previously approved these changes Sep 17, 2024

View reviewed changes

onur-ozkan reviewed Sep 17, 2024

View reviewed changes

mariocynicys previously approved these changes Sep 18, 2024

View reviewed changes

mm2src/mm2_main/src/lp_wallet/mnemonics_storage.rs Outdated Show resolved Hide resolved

mm2src/mm2_main/src/lp_wallet/mnemonics_wasm_db.rs Outdated Show resolved Hide resolved

review fix: add note about ok_or usage for Constructible

704305d

shamardy dismissed stale reviews from mariocynicys and laruh via 704305d September 19, 2024 01:24

review fix: add tests

1a7c003

shamardy added 4 commits September 19, 2024 11:47

use STOP_TIMEOUT_MS const

874b294

review fix: refactor read_file_names_with_extension and `read_file_…

bea5479

…stems_with_extension` into a single `list_files_by_extension` function

review fix: add #[inline(always)] to read_dir_json

e1dbb67

try to fix test_electrum_tx_history

aaf9d19

borngraced requested changes Sep 21, 2024

View reviewed changes

mm2src/mm2_io/src/fs.rs Outdated Show resolved Hide resolved

mm2src/mm2_main/src/lp_wallet/mnemonics_storage.rs Outdated Show resolved Hide resolved

mm2src/mm2_io/src/fs.rs Outdated Show resolved Hide resolved

review fix: use impl iterator where appropriate

de645bd

borngraced previously approved these changes Sep 24, 2024

View reviewed changes

onur-ozkan previously approved these changes Sep 25, 2024

View reviewed changes

mm2src/mm2_core/src/lib.rs Show resolved Hide resolved

review fix: fix DbNamespaceId enum to be wasm only and inline `for_…

4e46c35

…test_with_id`

shamardy dismissed stale reviews from onur-ozkan and borngraced via 4e46c35 September 25, 2024 12:37

shamardy added 2.2.0-beta docs labels Sep 25, 2024

shamardy mentioned this pull request Sep 25, 2024

Add docs for new get_wallet_names rpc KomodoPlatform/komodo-docs-mdx#343

Closed

shamardy merged commit ab23c11 into dev Sep 25, 2024
28 of 29 checks passed

shamardy deleted the feat-get-wallet-rpc branch September 25, 2024 17:05

smk762 added docs-under-review and removed docs docs-under-review labels Nov 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(wallets): add `get_wallet_names` rpc #2202

feat(wallets): add `get_wallet_names` rpc #2202

shamardy commented Aug 28, 2024 •

edited

Loading

laruh left a comment

laruh Aug 28, 2024

shamardy Aug 30, 2024

laruh Aug 30, 2024

onur-ozkan Sep 17, 2024

shamardy Sep 19, 2024 •

edited

Loading

CharlVS left a comment •

edited

Loading

shamardy commented Aug 30, 2024 •

edited

Loading

CharlVS commented Aug 30, 2024

laruh left a comment

laruh left a comment

onur-ozkan commented Sep 17, 2024

onur-ozkan left a comment

onur-ozkan Sep 17, 2024

mariocynicys left a comment

shamardy commented Sep 19, 2024 •

edited

Loading

shamardy commented Sep 19, 2024

shamardy commented Sep 19, 2024

borngraced left a comment

borngraced left a comment

onur-ozkan left a comment

	/// Wallet name for this mm2 instance. Optional for backwards compatibility.
	pub wallet_name: Constructible<Option<String>>,

feat(wallets): add get_wallet_names rpc #2202

feat(wallets): add get_wallet_names rpc #2202

Conversation

shamardy commented Aug 28, 2024 • edited Loading

Example Request and Responses:

Request:

No-Login Mode Response:

Activated Wallet Response:

laruh left a comment

Choose a reason for hiding this comment

laruh Aug 28, 2024

Choose a reason for hiding this comment

shamardy Aug 30, 2024

Choose a reason for hiding this comment

laruh Aug 30, 2024

Choose a reason for hiding this comment

onur-ozkan Sep 17, 2024

Choose a reason for hiding this comment

shamardy Sep 19, 2024 • edited Loading

Choose a reason for hiding this comment

CharlVS left a comment • edited Loading

Choose a reason for hiding this comment

shamardy commented Aug 30, 2024 • edited Loading

CharlVS commented Aug 30, 2024

laruh left a comment

Choose a reason for hiding this comment

laruh left a comment

Choose a reason for hiding this comment

onur-ozkan commented Sep 17, 2024

onur-ozkan left a comment

Choose a reason for hiding this comment

onur-ozkan Sep 17, 2024

Choose a reason for hiding this comment

mariocynicys left a comment

Choose a reason for hiding this comment

shamardy commented Sep 19, 2024 • edited Loading

shamardy commented Sep 19, 2024

shamardy commented Sep 19, 2024

borngraced left a comment

Choose a reason for hiding this comment

borngraced left a comment

Choose a reason for hiding this comment

onur-ozkan left a comment

Choose a reason for hiding this comment

feat(wallets): add `get_wallet_names` rpc #2202

feat(wallets): add `get_wallet_names` rpc #2202

shamardy commented Aug 28, 2024 •

edited

Loading

shamardy Sep 19, 2024 •

edited

Loading

CharlVS left a comment •

edited

Loading

shamardy commented Aug 30, 2024 •

edited

Loading

shamardy commented Sep 19, 2024 •

edited

Loading