feat: create a simple Sign API example on top of the WS client #48

silvestrst-crypto · 2023-10-27T09:30:47Z

Description

This change adds Sign API framework, and a simple example client to demonstrate pairing and session creation capabilities.

This change introduces the following functionality:

Sign API payload serializable/deserializable data types
Payload encryption and decryption
Session Diffie-Hellman key derivation
Pairing URI parsing
Simple WS based client

Resolves #47

How Has This Been Tested?

Use example client to pair and establish the session with an example DApp:
- Click on "Goerli" and then "connect"
- In a new pop-up, click "New Pairing"
- Copy the Pairing URI
- In the terminal, cd path/to/WalletConnectRust
- .../WalletConnectRust$ cargo run --example session "<copied URI>"
- DApp should now display the session window
- In the DApp click disconnect

Unittests (partial)

Due Diligence

Breaking change
Requires a documentation update
Requires a e2e/integration test update

TODO

Typed errors instead of unnamed anyhow errors
More unittests
Implement Core Pairing API. Not related to Sign API, but makes sense to add.
Add relevant information to README.md
Better documentation, code comments and function descriptions
crypto/session.rs refactor

Additional information

WCv2 uses layered approach. The relay protocol messages (currently only IRN, but there might be other in future), are used to carry the useful payload. In terms of this change the useful payload is Sign API messages.

Useful materials:
https://specs.walletconnect.com/2.0/specs/clients/core/pairing
https://specs.walletconnect.com/2.0/specs/clients/sign/session-proposal

https://specs.walletconnect.com/2.0/specs/servers/relay/relay-server-rpc
https://specs.walletconnect.com/2.0/specs/clients/sign/rpc-methods

sign_api/src/rpc.rs

silvestrst-crypto · 2023-10-27T09:33:24Z

sign_api/src/rpc.rs

+//! The crate exports common types used when interacting with messages between
+//! clients. This also includes communication over HTTP between relays.


Bad Copy/Paste, should be changed to an appropriate description.

Why is this module copy-pasted from relay_rpc? I think we should somehow reuse the existing one

That is a good point. In fact the way I initially started working on this change, was copy paste the entire rpc.rs and then change bits and pieces that I need for sign_api.

I ran into some issues, and wasn't sure how to appropriate certain traits, errors, etc.. for my use-case.

In the end I decided it will be easier for me to loosely base it on the relay rpc.rs, and see if some common functionality can be refactored post fact after this change lands in the follow-ups. It should be easier, rather than doing two things at once - will probably result in some code, files shifting around.

silvestrst-crypto · 2023-10-27T09:34:11Z

sign_api/src/rpc.rs

+    pub fn validate(&self) -> Result<(), ValidationError> {
+        match self {
+            Self::Request(request) => request.validate(),
+            Self::Response(response) => response.validate(),
+        }
+    }


Should add more request/response specific validations.

sign_api/src/rpc/params/shared_types.rs

silvestrst-crypto · 2023-11-07T08:46:07Z

@heilhead , apologies for names dropping! The repository doesn't look very active, so this is my attempt to get some attention to the PR. Would you please be able to give this change a quick review, or maybe assign someone? Thanks!

heilhead · 2023-11-07T11:04:18Z

Hi @silvestrst-crypto, thanks for the PR! Sorry we didn't see it earlier. We'll try to review it in the next few days hopefully.

silvestrst-crypto · 2023-11-17T14:01:15Z

gentle ping

heilhead · 2023-11-17T17:46:48Z

@silvestrst-crypto Hey! Apologies for not getting back with the review, as I'm on holidays right now.

@xDarksome @farazdagi @chris13524 Can one of you guys please have a look at this?

arein · 2023-11-27T12:52:47Z

@silvestrst-crypto can you please also share your use case of how you're intending of using this? 🙏

farazdagi

Absolutely marvellous work, thank you!
I will do another review pass today -- but overall looks really great. I had some minor comments.

NB: Sorry for keeping you waiting for review for so long, will make sure we progress swiftly from this point on :)

sign_api/src/crypto/payload.rs

farazdagi · 2023-11-27T12:11:15Z

sign_api/src/crypto/payload.rs

+                    },
+                })
+            }
+            _ => anyhow::bail!("Invalid envelope type: {}", envelope_type),


Generally we use typed errors and avoid using anyhow::bail. For instance, relay_client/src/error.rs is the way we typically define errors, so for consistency's sake please consider defining Sign API error types.

NB: More or less, error types in pairing_uri is what I am after here.

Done, here and in all other sign_api submodules.

sign_api/src/pairing_uri.rs

sign_api/src/rpc/params/session_event.rs

sign_api/src/rpc/params/session_request.rs

farazdagi · 2023-11-27T12:49:49Z

sign_api/src/rpc/params/session_settle.rs

+pub struct SessionSettleRequest {
+    pub relay: Relay,
+    pub controller: Controller,
+    pub namespaces: SettleNamespaces,


The optional specialNamespaces are omitted here. Not a big deal though.

sign_api/src/rpc/params/session_settle.rs

farazdagi · 2023-11-27T13:07:40Z

From your todo list, I'd consider typed errors + Readme update + probably a bit more unit tests for this PR. Anything else, let's tackle as separate PRs.

xDarksome

Putting my 5 cents
I started to review, but then got distracted.

xDarksome · 2023-11-27T11:46:10Z

sign_api/src/crypto/session.rs

+impl Debug for SessionKey {
+    /// Custom debug to hide the symmetrical key.
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        f.debug_struct("WalletConnectUrl")


I presume this is a refactoring artifact 😄
Also, consider using https://crates.io/crates/secrecy

Indeed, bad copy paste from pairing_uri.rs.

That's a good point, I will look into that crate, haven't yet one so.

xDarksome · 2023-11-27T11:48:13Z

sign_api/src/pairing_uri.rs

+    url::Url,
+};
+
+lazy_static! {


I think lazy_static is somewhat deprecated in favor of once_cell::Lazy

Moved inside parse_topic_and_version. This function will not be called to often, so "caching" the regex it is not that important.

Done.

xDarksome · 2023-11-27T11:58:20Z

sign_api/src/rpc.rs

+//! The crate exports common types used when interacting with messages between
+//! clients. This also includes communication over HTTP between relays.


Why is this module copy-pasted from relay_rpc? I think we should somehow reuse the existing one

silvestrst-crypto · 2023-11-27T16:06:06Z

Thank you guys for your reviews! I will hopefully aim to go through the comments properly tomorrow and fix them up.

silvestrst-crypto · 2023-11-28T08:59:20Z

@silvestrst-crypto can you please also share your use case of how you're intending of using this? 🙏

We have an internal Rust project, where we are planning on using WalletConnect v2 to do DeFI operations, which is the main reason for tackling the Sign API at this point 🙂

silvestrst-crypto

Thank you guys for your reviews!

This is the first pass where I have tackled some of the review comments, I will handle the rest in next few passes.

Summary:

Switches to typed errors rather than anyhow::Error
Changed Namespace memebrs from Vec to BTreeSet
Fixed-up some naming comments in payload.rs
Noticed that I had logic errors in the payload.rs when parsing decoded data (was wrong in a way that it still worked). Fixed it now.

I have also realised that Namespace needs more validation logic in accordance to the spec.

When you got through comments, could you guys only resolve those that you are happy with, but keep open those that you think are still "not there yet". This should make the review process a bit easier for both parties. Thanks!

sign_api/src/crypto/payload.rs

sign_api/src/rpc/params/session_settle.rs

silvestrst-crypto · 2023-11-29T16:27:07Z

sign_api/src/crypto/payload.rs

+                    },
+                })
+            }
+            _ => anyhow::bail!("Invalid envelope type: {}", envelope_type),


Done, here and in all other sign_api submodules.

silvestrst-crypto · 2023-11-29T16:29:19Z

sign_api/src/crypto/session.rs

+impl Debug for SessionKey {
+    /// Custom debug to hide the symmetrical key.
+    fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result {
+        f.debug_struct("WalletConnectUrl")


Indeed, bad copy paste from pairing_uri.rs.

That's a good point, I will look into that crate, haven't yet one so.

silvestrst-crypto · 2023-11-29T16:34:01Z

sign_api/src/rpc.rs

+//! The crate exports common types used when interacting with messages between
+//! clients. This also includes communication over HTTP between relays.


That is a good point. In fact the way I initially started working on this change, was copy paste the entire rpc.rs and then change bits and pieces that I need for sign_api.

I ran into some issues, and wasn't sure how to appropriate certain traits, errors, etc.. for my use-case.

In the end I decided it will be easier for me to loosely base it on the relay rpc.rs, and see if some common functionality can be refactored post fact after this change lands in the follow-ups. It should be easier, rather than doing two things at once - will probably result in some code, files shifting around.

sign_api/src/rpc/params/shared_types.rs

silvestrst-crypto

This is a second part of the review comments addressing:

Fixed SessionRequestRequest to match the spec (was missing an optional field, and params were changed to serde_json::Value)
Fixed SessionEventRequest to match the spec (data was changed to serde_json::Value)
Simple serealize/deserialize unittests were added for the above two bullet-points
Some other minor comment were fixed

Will try to address the remaining comments by the end of the week.

sign_api/src/pairing_uri.rs

silvestrst-crypto · 2023-12-05T15:22:46Z

sign_api/src/pairing_uri.rs

+    url::Url,
+};
+
+lazy_static! {


Moved inside parse_topic_and_version. This function will not be called to often, so "caching" the regex it is not that important.

Done.

sign_api/src/rpc.rs

sign_api/src/rpc/params/session_event.rs

sign_api/src/rpc/params/session_request.rs

sign_api/src/rpc/params/session_settle.rs

sign_api/src/rpc/params/session_event.rs

examples/session.rs

chris13524

First of all, thanks for the contribution! We really appreciate it 🥇

Second, can you add a section to the root README for the sign_api crate? We also need to add a warning to that; the Rust team doesn't work on the client SDKs so the amount of rigor here is far less than e.g. our JavaScript, Kotlin, or Swift SDKs have gone through. Those SDKs are also continually improved with new features and fixes, so the Rust version will likely not be getting any of these updates unless a community member contributes it. This could change in the future if there is sufficient interest in the Rust SDK, but for now we cannot dedicate resources to keep this maintained. Can you add a note to the readme like:

Warning: this Rust Sign API SDK is community-maintained and may be lacking features and stability or security fixes that other versions of the Sign API SDK receive. We strongly recommend using the JavaScript or other Sign API SDKs instead.

silvestrst-crypto · 2023-12-11T16:40:07Z

First of all, thanks for the contribution! We really appreciate it 🥇

Second, can you add a section to the root README for the sign_api crate? We also need to add a warning to that; the Rust team doesn't work on the client SDKs so the amount of rigor here is far less than e.g. our JavaScript, Kotlin, or Swift SDKs have gone through. Those SDKs are also continually improved with new features and fixes, so the Rust version will likely not be getting any of these updates unless a community member contributes it. This could change in the future if there is sufficient interest in the Rust SDK, but for now we cannot dedicate resources to keep this maintained. Can you add a note to the readme like:

Warning: this Rust Sign API SDK is community-maintained and may be lacking features and stability or security fixes that other versions of the Sign API SDK receive. We strongly recommend using the JavaScript or other Sign API SDKs instead.

Good idea, I understand, thanks!

sign_api/src/rpc/params/shared_types.rs

silvestrst-crypto · 2023-12-19T17:37:08Z

In the latest push I have:

added simple serialization/deserialization unittests for the parameters
done significant ProposeNamespaces refactor and added unittests
renamed Namespaces to ProposeNamespaces and moved them together with SettleNamespaces under their own sub-foleders in shred_types
Fixed up TTL for session_request, it's unix_timestamp_in_secs() + TTL

silvestrst-crypto · 2023-12-19T18:42:54Z

In the latest push:

I have moved session example into sign_api crate

This change adds WalletConnect v2 Sign API, as per: https://specs.walletconnect.com/2.0/specs/clients/sign/ Please note that although the specification is fairly thorough, there are some inconsistencies. This implementation is derived from specs, analyzing ws traffic in browsers devtools, as well as the original WalletConnect JavaScript client at: https://github.com/WalletConnect/walletconnect-monorepo Design decisions: Modularity: - RPC and crypto modules are not dependent on each other, so client implementations don't have to use their own crypto implementation. Closes: reown-com#47

This client implements pairing and session flows: https://specs.walletconnect.com/2.0/specs/clients/core/pairing https://specs.walletconnect.com/2.0/specs/clients/sign/session-proposal Supported actions: - pairing - session establishment - session delete - ping This example could be expanded to handle multiple sessions. Caution: The purpose of this example is demonstration of core Sign API functionality. However, it shouldn't be used in production, as might exhibit some race conditions such as with session deletion, etc...

silvestrst-crypto · 2023-12-20T06:55:39Z

In the latest push:

Added README.md to the sign_api crate
Rebased my changes on upstream main

nopestack · 2024-03-21T16:33:10Z

Hi @silvestrst-crypto, thank you for your hard work on this PR 💪 . I was wondering if you had made any progress addressing the feedback from my colleagues and if you'd like to continue working on this issue?

silvestrst-crypto · 2024-03-22T09:06:00Z

Hi @nopestack , lately I am quite busy, so put it on hold, but if anyone is planning on using it, I can slowly start moving it forward in spare time. Thanks!

dougEfresh · 2024-07-31T17:43:58Z

@nopestack I think it is best to move this code to an isolated repo, and depend on wallet-connect-rust .
I think this would speed up development and I'm willing to help. Besides, I do not think the maintainers have interest in merging this.

nopestack · 2024-07-31T20:02:49Z

Marking this as draft in the meantime as work is paused and we cannot prioritize it at the moment. Worth revisiting it later @xDarksome @chris13524

silvestrst-crypto marked this pull request as draft October 27, 2023 09:31

silvestrst-crypto commented Oct 27, 2023

View reviewed changes

silvestrst-crypto commented Nov 2, 2023

View reviewed changes

sign_api/src/rpc/params/shared_types.rs Outdated Show resolved Hide resolved

silvestrst-crypto force-pushed the silvestrs/sign-api-latest branch from 968a167 to 0a1df16 Compare November 2, 2023 17:03

silvestrst-crypto marked this pull request as ready for review November 2, 2023 17:09

heilhead requested review from farazdagi, heilhead, chris13524 and xDarksome November 17, 2023 17:47

farazdagi reviewed Nov 27, 2023

View reviewed changes

xDarksome reviewed Nov 27, 2023

View reviewed changes

silvestrst-crypto force-pushed the silvestrs/sign-api-latest branch from 0a1df16 to cd232c6 Compare November 29, 2023 16:48

silvestrst-crypto commented Nov 29, 2023

View reviewed changes

silvestrst-crypto force-pushed the silvestrs/sign-api-latest branch from cd232c6 to 7296dc8 Compare December 5, 2023 18:03

silvestrst-crypto commented Dec 5, 2023

View reviewed changes

sign_api/src/rpc/params/session_event.rs Outdated Show resolved Hide resolved

silvestrst-crypto commented Dec 5, 2023

View reviewed changes

sign_api/src/rpc/params/session_event.rs Outdated Show resolved Hide resolved

chris13524 reviewed Dec 6, 2023

View reviewed changes

examples/session.rs Outdated Show resolved Hide resolved

chris13524 reviewed Dec 11, 2023

View reviewed changes

silvestrst-crypto commented Dec 15, 2023

View reviewed changes

sign_api/src/rpc/params/shared_types.rs Outdated Show resolved Hide resolved

silvestrst-crypto force-pushed the silvestrs/sign-api-latest branch from 7296dc8 to 04b51d5 Compare December 19, 2023 16:43

silvestrst-crypto force-pushed the silvestrs/sign-api-latest branch from 04b51d5 to fc737a0 Compare December 19, 2023 18:42

silvestrst-crypto added 4 commits December 20, 2023 06:54

chore: add vscode metadata to gitignore

a40a366

chore: add Sign API README.md

489e2e3

silvestrst-crypto force-pushed the silvestrs/sign-api-latest branch from 5046727 to 489e2e3 Compare December 20, 2023 06:54

chris13524 mentioned this pull request Feb 13, 2024

WalletConnect sign QR from Terminal #60

Closed

nopestack marked this pull request as draft July 31, 2024 20:00

borngraced mentioned this pull request Aug 28, 2024

feat: implement WalletConnect Sign-API KomodoPlatform/WalletConnectRust#2

Closed

3 tasks

		//! The crate exports common types used when interacting with messages between
		//! clients. This also includes communication over HTTP between relays.

feat: create a simple Sign API example on top of the WS client #48

Are you sure you want to change the base?

feat: create a simple Sign API example on top of the WS client #48

Conversation

silvestrst-crypto commented Oct 27, 2023 • edited Loading

Description

How Has This Been Tested?

Due Diligence

TODO

Additional information

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

silvestrst-crypto commented Nov 7, 2023

heilhead commented Nov 7, 2023

silvestrst-crypto commented Nov 17, 2023

heilhead commented Nov 17, 2023

arein commented Nov 27, 2023

farazdagi 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

farazdagi commented Nov 27, 2023

xDarksome 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

silvestrst-crypto commented Nov 27, 2023

silvestrst-crypto commented Nov 28, 2023 • edited Loading

silvestrst-crypto 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

silvestrst-crypto left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chris13524 left a comment

Choose a reason for hiding this comment

silvestrst-crypto commented Dec 11, 2023

silvestrst-crypto commented Dec 19, 2023

silvestrst-crypto commented Dec 19, 2023

silvestrst-crypto commented Dec 20, 2023

nopestack commented Mar 21, 2024

silvestrst-crypto commented Mar 22, 2024

dougEfresh commented Jul 31, 2024

nopestack commented Jul 31, 2024

silvestrst-crypto commented Oct 27, 2023 •

edited

Loading

farazdagi left a comment •

edited

Loading

silvestrst-crypto commented Nov 28, 2023 •

edited

Loading

silvestrst-crypto left a comment •

edited

Loading

silvestrst-crypto left a comment •

edited

Loading