Skip to content

improvement(RPC): unified interface for legacy and current RPC interfaces#2450

Merged
onur-ozkan merged 7 commits intodevfrom
optional-mmrpc
Jun 9, 2025
Merged

improvement(RPC): unified interface for legacy and current RPC interfaces#2450
onur-ozkan merged 7 commits intodevfrom
optional-mmrpc

Conversation

@onur-ozkan
Copy link
Copy Markdown

Allows using any (legacy or current) RPC methods without needing to include mmrpc in the RPC payloads. It has been annoying to constantly add and remove that field between different RPCs. With this PR, we can leave it to KDF and it will handle it automatically. Ideally, we should remove it entirely from the codebase but that would cause breaking change for downstreams.

onur-ozkan added 2 commits May 8, 2025 16:46
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
onur-ozkan added 3 commits May 8, 2025 16:53
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Signed-off-by: onur-ozkan <work@onurozkan.dev>
Comment on lines -56 to -64
let no_method = block_on(mm.rpc(&json! ({
"userpass": mm.userpass,
"coin": "RICK",
"ipaddr": "electrum1.cipig.net",
"port": 10017
})))
.unwrap();
assert!(no_method.0.is_server_error());
assert_eq!((no_method.2)[ACCESS_CONTROL_ALLOW_ORIGIN], "http://localhost:4000");
Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This was testing unknown methods on the legacy dispatcher (not very useful to cover to be honest). The behavior for unknown RPCs in the legacy dispatcher has changed: if a method isn't found there, we now try to find it in the current dispatcher.

@smk762
Copy link
Copy Markdown

smk762 commented May 9, 2025

Please let me know how this would affect docs. A simple find/delete for the param is simple enough, and v2 methods tend to include a params object which the equivalent v1 method does not. In some cases tho, like get_enabled_coins which does not take any params, the v1 and v2 methods will be indistinguishable.

{
    "userpass": "{{userpass}}",
    "method": "get_enabled_coins"
}
{
    "userpass": "{{userpass}}",
    "mmrpc": "2.0",
    "method": "get_enabled_coins"   // "params": {},
    // "id": null // Accepted values: Integers
}

The response structure of these is also significantly different:

v1:

{
    "result": [
        {
            "ticker": "DOC",
            "address": "RUYJYSTuCKm9gouWzQN1LirHFEYThwzA2d"
        },
        {
            "ticker": "MARTY",
            "address": "RUYJYSTuCKm9gouWzQN1LirHFEYThwzA2d"
        }
    ]
}

v2:

{
    "mmrpc": "2.0",
    "result": {
        "coins": [
            {
                "ticker": "DOC"
            },
            {
                "ticker": "MARTY"
            }
        ]
    },
    "id": null
}

Will "mmrpc": "2.0" remain in the response?

This PR effectively removes any v1 method with a v2 version. Will there be a way to force v1 if needed for some reason?

@dimxy
Copy link
Copy Markdown
Collaborator

dimxy commented May 13, 2025

My note about this approach (I already asked this in the internal channel):
we have methods with identical names for legacy and v2 APIs, like "trade_preimage".
In such case this PR may fail on the legacy method instead of switching to the v2 method. That way, the user needs to know about that and use "mmrpc": "2.0" explicitly in this case (identical method names). Hm-m.

@onur-ozkan
Copy link
Copy Markdown
Author

Covering both comments above:

If you are using mmrpc parameter, you won't notice any difference at all; everything still works as before. If it's not set and the requested RPC isn't found among the legacy RPCs, it will now also check mm2rpc 2.0 before returning "not found".

@onur-ozkan onur-ozkan added the priority: high Important tasks that need attention soon. label May 26, 2025
borngraced
borngraced previously approved these changes May 26, 2025
Copy link
Copy Markdown

@borngraced borngraced left a comment

Choose a reason for hiding this comment

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

non blockeers

Signed-off-by: onur-ozkan <work@onurozkan.dev>
borngraced
borngraced previously approved these changes May 26, 2025
Copy link
Copy Markdown

@laruh laruh left a comment

Choose a reason for hiding this comment

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

Thanks, only one note

Signed-off-by: onur-ozkan <work@onurozkan.dev>
Copy link
Copy Markdown

@laruh laruh left a comment

Choose a reason for hiding this comment

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

LGTM!

return dispatcher_legacy::process_single_request(ctx, req, client, local_only)
.await
.map_err(|e| ERRL!("{}", e));
match dispatcher_legacy::process_single_request(ctx.clone(), req.clone(), client, local_only).await {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Should we not first try rpc version 2 rather than legacy?
to make preference for v2, in case of identical rpc name for v2 and legacy

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

This approach will cause BC on identical RPC names as they don't set mmrpc field.

@onur-ozkan onur-ozkan merged commit b52d244 into dev Jun 9, 2025
22 of 24 checks passed
@onur-ozkan onur-ozkan deleted the optional-mmrpc branch June 9, 2025 08:31
dimxy pushed a commit to dimxy/komodo-defi-framework that referenced this pull request Jun 27, 2025
* dev: (30 commits)
  chore(core): replace hash_raw_entry with stable entry() API (GLEECBTC#2473)
  chore(core): adapt `MmError` and usages for compatibility with new rustc versions (GLEECBTC#2443)
  feat(wallet): add `delete_wallet` RPC (GLEECBTC#2497)
  chore(release): add changelog entries for v2.5.0-beta (GLEECBTC#2494)
  chore(release): bump kdf version to 2.5.0-beta (GLEECBTC#2492)
  feat(tests): zcoin unit test to validate dex fee (GLEECBTC#2460)
  fix(zcoin): correctly track unconfirmed z-coin notes (GLEECBTC#2331)
  improvement(orders): remove BTC specific volume from min_trading_vol logic (GLEECBTC#2483)
  feat(ibc-routing-part-2): supporting entire Cosmos network for swaps (GLEECBTC#2476)
  fix(startup): don't initialize WalletConnect during startup (GLEECBTC#2485)
  fix(dns): better ip resolution (GLEECBTC#2487)
  improvement(event-streaming): strong type streamer IDs (GLEECBTC#2441)
  bump timed-map to `1.4.1` (GLEECBTC#2481)
  improvement(RPC): unified interface for legacy and current RPC interfaces (GLEECBTC#2450)
  improvement(tendermint): `tendermint_tx_internal_id` helper (GLEECBTC#2438)
  feat(walletconnect): add WalletConnect v2 support for EVM and Cosmos (GLEECBTC#2223)
  feat(ibc-routing-part-1): supporting entire Cosmos network for swaps (GLEECBTC#2459)
  fix(test): fix HD Wallet message signing tests (GLEECBTC#2474)
  improvement(builds): enable static CRT linking for MSVC builds (GLEECBTC#2464)
  feat(wallet): implement HD multi-address support for message signing (GLEECBTC#2432)
  ...

# Conflicts:
#	mm2src/coins/qrc20.rs
#	mm2src/mm2_main/src/lp_swap/maker_swap.rs
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement: RPC priority: high Important tasks that need attention soon. status: pending review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants