feat(wallets): add `get_wallet_names` rpc by shamardy · Pull Request #2202 · GLEECBTC/komodo-defi-framework

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
https://github.com/KomodoPlatform/komodo-defi-framework/blob/98571709996b020b8f2746de1f43f9fda1082417/mm2src/mm2_core/src/mm_ctx.rs#L114-L115

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 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 labels Aug 28, 2024

shamardy requested review from CharlVS and laruh 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 suggested 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 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 labels Sep 25, 2024

shamardy mentioned this pull request Sep 25, 2024

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

Closed

shamardy merged commit ab23c11 into dev Sep 25, 2024

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

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

shamardy mentioned this pull request Nov 19, 2024

chore(release): v2.2.0-beta #2197

Merged

smk762 mentioned this pull request Mar 21, 2025

[FR] update_wallet_password method #2402

Closed

Conversation

shamardy commented Aug 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Example Request and Responses:

Request:

No-Login Mode Response:

Activated Wallet Response:

Uh oh!

laruh left a comment

Choose a reason for hiding this comment

Uh oh!

laruh Aug 28, 2024

Choose a reason for hiding this comment

Uh oh!

shamardy Aug 30, 2024

Choose a reason for hiding this comment

Uh oh!

laruh Aug 30, 2024

Choose a reason for hiding this comment

Uh oh!

onur-ozkan Sep 17, 2024

Choose a reason for hiding this comment

Uh oh!

shamardy Sep 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CharlVS left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shamardy commented Aug 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CharlVS commented Aug 30, 2024

Uh oh!

laruh left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

laruh left a comment

Choose a reason for hiding this comment

Uh oh!

onur-ozkan commented Sep 17, 2024

Uh oh!

onur-ozkan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

onur-ozkan Sep 17, 2024

Choose a reason for hiding this comment

Uh oh!

mariocynicys left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shamardy commented Sep 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shamardy commented Sep 19, 2024

Uh oh!

shamardy commented Sep 19, 2024

Uh oh!

borngraced left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

borngraced left a comment

Choose a reason for hiding this comment

Uh oh!

onur-ozkan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

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