feat(walletconnect): walletconnect client #2183

borngraced · 2024-07-30T08:04:50Z

Originally, the WalletConnect client implementation only supported non-WASM targets. In this pull request (PR), I have ported the WebSocket client code and modified it to be compatible with both WASM and non-WASM targets.

While some code might change when implementing the session/pairing API in my next PR, this provides a functional WebSocket client implementation for WalletConnect as it stands.

onur-ozkan

Let's try to make unrelated changes in a separate PR next time. It adds more changes to review and makes it harder to identify the actual work.

I haven't reviewed the logical parts yet, but here are some notes from a quick review iteration:

mm2src/kdf_walletconnect/Cargo.toml

mm2src/mm2_main/Cargo.toml

mm2src/kdf_walletconnect/src/lib.rs

mm2src/crypto/src/mnemonic.rs

Cargo.lock

mm2src/mm2_main/Cargo.toml

onur-ozkan · 2024-08-13T07:59:56Z

mm2src/kdf_walletconnect/src/lib.rs

+//! Initially, the implementation was restricted to non-WASM targets,
+//! which limited its utility in web-based environments.
+//!
+//! This module contains the WebSocket client code that has been ported and modified to achieve cross-platform compatibility.


Can we reference the actual crate here? We might want to review it later to see any improvements they have made.

Also, why did we hard-code/port the entire crate here instead of forking it? Porting the entire source code brings some challanges (such as not being able to track the difference between us and upstream in the future).

the library is still in its early stages and not very useful at the moment. Important features like the sign API and session management haven’t been implemented yet, so there might be more changes needed when integrating the sign API in the next iteration. So I think it’s reasonable to proceed with porting it..

cc. @onur-ozkan @shamardy

IMO we can still do the same with forking the repository and maintaining it separately. We can publish it to crates.io as well once it's well documented.

mariocynicys

Thanks!
First review iteration.

mm2src/kdf_walletconnect/src/client/connection.rs

mariocynicys · 2024-08-13T12:56:11Z

mm2src/kdf_walletconnect/src/client/connection.rs

+            stream.close().await?;
+        }
+
+        Err(WebsocketClientError::TransportError("Error while closing connection".to_owned()).into())


is that a transport error tho? I think that should be NotConnected.

p.s. we don't have to error anyways, unless this info is used by the caller.

I'm leaving the fn to return Result for now..

mariocynicys · 2024-08-13T13:00:00Z

mm2src/kdf_walletconnect/src/client/stream.rs

+            },
+
+            Err(err) => {
+                tx.send(Err(ClientError::SerdeError(err.to_string()))).ok();


i think we better call this deserialization error instead of serde.

mariocynicys · 2024-08-13T13:01:29Z

mm2src/kdf_walletconnect/src/error.rs

+    #[from_stringify("serde_json::Error")]
+    SerdeError(String),


if this covers also serialization errors, it would be more specific if we make two instances of these (one for serialization and one for deserialization).

mariocynicys · 2024-08-13T13:25:27Z

mm2src/kdf_walletconnect/src/client/stream.rs

+            match data {
+                Some(result) => {
+                    if let Some(event) =
+                        self.parse_inbound(result.map_err(|err| WebsocketClientError::TransportError(err.to_string())))


nit: would be nicer if we handle the erroring case of the result here and only parse_inbound the valid msgs.

mariocynicys · 2024-08-13T13:29:06Z

mm2src/kdf_walletconnect/src/client/stream.rs

+                                    InboundRequest::new(id, data, self.outbound_tx.clone()),
+                                ),


I can't really follow why we use an unbounded response channel here?
isn't it gonne be just one resposne? it also seems not to be used anywhere later in PublishedMessage.

note: This code is part of original code from WalletConnect, I also had similar question but I think there might be a good reason to use unbounded_channel e.g when we don't want to miss a message or have sender blocking

mariocynicys · 2024-08-13T13:31:40Z

mm2src/kdf_walletconnect/src/client/stream.rs

+    /// Inbound request for receiving a subscription message.
+    ///
+    /// Currently, [`Subscription`] is the only request that the Relay sends to
+    /// the clients.
+    InboundSubscriptionRequest(InboundRequest<Subscription>),


nit: I would go for calling this just plain InboundRequest to be general and not mention specific names in this enum. and move the possible request types (currently only this one) into another enum.

mariocynicys · 2024-08-13T13:42:27Z

mm2src/kdf_walletconnect/src/client/fetch.rs

+            } else if self.has_more {
+                let (req, fut) = create_request(self.request.clone());


Q: won't this fetch the same request over and over again?

i mean, if we end up with a batch_fut that has_more, i expect the next time we call create_request we specify some different request construction that tells where did the last batch stop.

won't this fetch the same request over and over again

No, it won't and sending same request using same Topics make sense for streaming messages for some given Topics.

the rpc includes has_more in FetchMessages response body which is later used to update our internal has_more here

komodo-defi-framework/mm2src/kdf_walletconnect/src/client/fetch.rs

Lines 62 to 64 in 5027a90

self.batch = Some(data.messages.into_iter());

self.has_more = data.has_more;

self.batch_fut = None;

and then we check this condition to make another request with same Topic only if batch and batch_fut is none.

komodo-defi-framework/mm2src/kdf_walletconnect/src/client/fetch.rs

Lines 74 to 77 in 5027a90

} else if self.has_more {

let (req, fut) = create_request(self.request.clone());

self.client.request(req);

self.batch_fut = Some(fut);

What I'm saying is that, I believe WalletConnect RPC def uses has_more internally to stream the request to our client to prevent duplicates.

Does this make sense @mariocynicys

onur-ozkan · 2024-08-15T07:51:43Z

.cargo/config.toml

Is it your editor making these changes automatically, or are you making them intentionally?

that's my editor.. sorry 🙁

The formatting is fine, it's just too much out-of-context change for a feature PR in my opinion

...and the formatting in some Cargo.toml files aren't consistent enough

onur-ozkan · 2024-08-15T07:52:28Z

mm2src/mm2_main/Cargo.toml

+trezor-udp = [
+    "crypto/trezor-udp",
+] # use for tests to connect to trezor emulator over udp


This seems quite bad way of formatting things..

onur-ozkan · 2024-08-15T07:53:13Z

mm2src/crypto/Cargo.toml

@@ -34,6 +34,7 @@ mm2_err_handle = { path = "../mm2_err_handle" }
 num-traits = "0.2"
 parking_lot = { version = "0.12.0", features = ["nightly"] }
 primitives = { path = "../mm2_bitcoin/primitives" }
+rand = { version = "0.8.0", features = ["std", "small_rng"] }


Any reason?

had problem with rand crate earlier.. will revert

borngraced · 2024-08-18T23:46:06Z

closing in favor of KomodoPlatform/WalletConnectRust#1

borngraced added 19 commits July 17, 2024 00:02

make walletconnect lib compile

ff267a4

Cargo.lock

3a585a1

Merge remote-tracking branch 'origin/dev' into walletconnect

60638bd

use thread_rng in generate_mnemonic for now

de9b42a

save dev state

7d2ef1e

revert bip39 dep changes

1712c08

port and implement walletconnect wss connection options

38f8e0f

save dev state

71feee0

save dev state - native client impl

75db037

implement walletconnect client with unit tests

9d45139

commit lock file

a6a53e5

fix unit test wasm

4e7f633

polish and minor changes

a4d827b

save dev state

59050d6

rename module

c87d9a3

fix client bug and refactoring

615d4cb

fix merge conflicts

35ab1e3

resolve deps

3b98328

debug deps

902bc7b

borngraced self-assigned this Jul 30, 2024

borngraced added the in progress label Jul 30, 2024

borngraced changed the title ~~[feat]: WalletConnect Client~~ feat(walletconnect): WalletConnect Client Aug 7, 2024

borngraced added 3 commits August 8, 2024 14:49

rename mod and add port info

3957935

commit lock file and Cargo.toml

e84db08

use MmError handling

e6093fd

shamardy requested a review from onur-ozkan August 12, 2024 12:34

minor refactorings

ec3a3fb

onur-ozkan reviewed Aug 13, 2024

View reviewed changes

Cargo.lock Outdated Show resolved Hide resolved

onur-ozkan reviewed Aug 13, 2024

View reviewed changes

borngraced added 4 commits August 13, 2024 12:47

fix formattiing, update dalek deps to 4.1.3, update lib docs

1649a7a

debug curve25519_dalek_backend

44f847c

debug curve25519_dalek_backend build 2

5027a90

update rand and revert generate_mnemonic

978c548

borngraced changed the title ~~feat(walletconnect): WalletConnect Client~~ feat(walletconnect): walletconnect client Aug 13, 2024

mariocynicys reviewed Aug 13, 2024

View reviewed changes

error improv and other minor changes

0028369

onur-ozkan reviewed Aug 15, 2024

View reviewed changes

revert editor format

0a24364

borngraced closed this Aug 18, 2024

borngraced deleted the walletconnect branch August 18, 2024 23:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(walletconnect): walletconnect client #2183

feat(walletconnect): walletconnect client #2183

borngraced commented Jul 30, 2024 •

edited

Loading

onur-ozkan left a comment

onur-ozkan Aug 13, 2024

onur-ozkan Aug 13, 2024

borngraced Aug 13, 2024

onur-ozkan Aug 14, 2024

borngraced Aug 14, 2024

mariocynicys left a comment •

edited

Loading

mariocynicys Aug 13, 2024

borngraced Aug 13, 2024

mariocynicys Aug 13, 2024

borngraced Aug 13, 2024

mariocynicys Aug 13, 2024

borngraced Aug 13, 2024

mariocynicys Aug 13, 2024

borngraced Aug 13, 2024

mariocynicys Aug 13, 2024

borngraced Aug 13, 2024

mariocynicys Aug 13, 2024

mariocynicys Aug 13, 2024

borngraced Aug 13, 2024 •

edited

Loading

onur-ozkan Aug 15, 2024

borngraced Aug 15, 2024

onur-ozkan Aug 15, 2024

borngraced Aug 15, 2024 •

edited

Loading

onur-ozkan Aug 15, 2024 •

edited

Loading

onur-ozkan Aug 15, 2024

borngraced Aug 15, 2024

borngraced commented Aug 18, 2024

		} else if self.has_more {
		let (req, fut) = create_request(self.request.clone());

	self.batch = Some(data.messages.into_iter());
	self.has_more = data.has_more;
	self.batch_fut = None;

	} else if self.has_more {
	let (req, fut) = create_request(self.request.clone());
	self.client.request(req);
	self.batch_fut = Some(fut);

feat(walletconnect): walletconnect client #2183

feat(walletconnect): walletconnect client #2183

Conversation

borngraced commented Jul 30, 2024 • edited Loading

onur-ozkan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mariocynicys left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

borngraced Aug 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

borngraced Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

onur-ozkan Aug 15, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

borngraced commented Aug 18, 2024

borngraced commented Jul 30, 2024 •

edited

Loading

mariocynicys left a comment •

edited

Loading

borngraced Aug 13, 2024 •

edited

Loading

borngraced Aug 15, 2024 •

edited

Loading

onur-ozkan Aug 15, 2024 •

edited

Loading