Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(walletconnect): walletconnect integration #2223

Open
wants to merge 132 commits into
base: dev
Choose a base branch
from
Open

Conversation

borngraced
Copy link
Member

@borngraced borngraced commented Sep 16, 2024

This PR introduces the integration of WalletConnect into the Komodo DeFi Framework (KDF), enabling secure wallet connections for Cosmos and EVM-based chains. KDF acts as the DApp(in this PR), allowing users to initiate and manage transactions securely with external wallets e.g via Metamask.
Key changes include:

  • Implement multi-session handling for concurrent WalletConnect connections across accounts, integrates SQLite/IndexedDB for persistent session storage across devices, enhances relayer disconnection handling for smoother session management
  • Implemented WalletConnect coin activation for Tendermint and EVM.
  • Implemented Withdraw and Swap functionalities for Tendermint and EVM

https://specs.walletconnect.com/2.0/specs/clients/sign/
https://specs.walletconnect.com/2.0/specs/clients/core/pairing/
https://specs.walletconnect.com/2.0/specs/clients/core/crypto/
https://specs.walletconnect.com/2.0/specs/servers/relay/
https://docs.reown.com/advanced/multichain/rpc-reference/ethereum-rpc

Additional improvements include cleanup of unused dependencies, minor code refinements, and WASM compatibility

Updated deps:
Added deps:
Removed deps:

How to test using EVM coin e.g ETH (Native && WASM)

  1. Start kdf (cargo run)
  2. Create WalletConnect session connection
    • Use the RPC below
    • Copy the connection URL and either:
      • Paste directly in your wallet or
      • Generate QR code using QR Code Generator and scan with your wallet (e.g., MetaMask)
{
     "method": "wc_new_connection",
	"userpass": "{{ _.userpass }}",
	"mmrpc": "2.0",
	"params": {
		"required_namespaces": {
			"eip155": {
				"chains": ["eip155:1"]
				"methods": [
					"eth_sendTransaction",
					"eth_signTransaction",
					"personal_sign"
				],
				"events": [
					"accountsChanged",
					"chainChanged"
				]
			}
		}
	}
}
  1. Activate ETH coin (set "priv_key_policy": "WalletConnect", in activation params).
  2. Approve authentication request in your Wallet
  3. Withdraw or trade.

Note: To add more eip155 chains, modify the chains array like this: ["eip155:1", "eip155:250"]

@borngraced borngraced self-assigned this Sep 16, 2024
Comment on lines +52 to +53
/// Activation via WalletConnect
WalletConnect,
Copy link
Member

Choose a reason for hiding this comment

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

Could you add some info about why we don't take public key similar to how it's done with WithPubkey variant?

Also, I'm curious why WithPubkey mode doesn’t work here. Doesn’t WalletConnect already provide the public key via an API? WithPubkey mode was intended to support any type of external wallet as it only requires the public key and doesn’t depend on the wallet’s internal logic.

Copy link
Member Author

@borngraced borngraced Nov 13, 2024

Choose a reason for hiding this comment

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

WalletConnect doesn’t directly provide the public key as part of the initial session information. Instead, it only returns the account address, methods and the chains the user can interact with. To obtain the public key, a separate request is required for the specific account

https://specs.walletconnect.com/2.0/specs/clients/sign/rpc-methods#wc_sessionsettle
https://specs.walletconnect.com/2.0/specs/clients/sign/session-proposal

Copy link
Member

Choose a reason for hiding this comment

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

I would have done it in a way where some library/crate (or even GUI side) handles this and then utilizes with_pubkey on core KDF side to have single entrypoint for any external wallet, so the complexity remains constant for N external wallet types.

Copy link
Member Author

Choose a reason for hiding this comment

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

this is like dividing this implementation into two (manages connection from GUI while kdf manages request handling ) or leaving everything entirely to GUI which is not what we wanted from the beginning.

So the thing is whoever wishes to use any wallet pubkey via WalletConnect needs to make an additional request which can't really be avoided.

Copy link
Member

Choose a reason for hiding this comment

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

What I mean was, we could handle that connection part and the extra request part (for getting pubkey) in a separate crate and utilize the with_pubkey mode. I mean, we have an abstraction layer for external wallets, so why not using it?

Copy link
Member Author

@borngraced borngraced Nov 14, 2024

Choose a reason for hiding this comment

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

we could handle that connection part and the extra request part (for getting pubkey) in a separate crate

how do you plan to do this? since, we either ways we still need to request for pubkey meanwhile with_pubkey params requires pubkey in activation params itself. There's a kdf_walletconnect crate that handles all WC logic By the way,.

WalletConnect coin activation mode lets the activation logic handle the session and fetch the key directly, setting it in TendermintActivationPolicy::PublicKey—keeping things simple without needing an abstraction layer.

Also, the current external wallet implementation for tendermint uses SSE, in the case of Keplr, we need to send request to GUI then to Keplr wallet if I'm correct, so it can't directly work with WalletConnect. The current activation logic is quite easy to understand and scale(when integrating KDF as Wallet in the future)

Copy link
Member

Choose a reason for hiding this comment

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

how do you plan to do this? since, we either ways we still need to request for pubkey meanwhile with_pubkey params requires pubkey in activation params itself. There's a kdf_walletconnect crate that handles all WC logic By the way,.

User sends the activation request for WC -> KDF takes the request and runs the logic where it reads Pubkey from WC -> Uses that Pubkey to run KDF as an external wallet (just like any other wallet).

Is that not possible?

Also, the current external wallet implementation for tendermint uses SSE, in the case of Keplr, we need to send request to GUI then to Keplr wallet if I'm correct, so it can't directly work with WalletConnect. The current activation logic is quite easy to understand and scale(when integrating KDF as Wallet in the future)

Yeah, for Keplr, GUI side triggers the Keplr events which when required. But it doesn't have to be the same way for WalletConnect.

Copy link
Member Author

@borngraced borngraced Nov 14, 2024

Choose a reason for hiding this comment

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

User sends the activation request for WC -> KDF takes the request and runs the logic where it reads Pubkey from WC -> Uses that Pubkey to run KDF as an external wallet (just like any other wallet).

that's how the current flow works.. or am getting something wrong..
User request coin activation, logic checks if TendermintPubkeyActivationParams::WalletConnect( we can't use WithPubkey, since we don't have the pubkey at start)

TendermintPubkeyActivationParams::WalletConnect => {
activate_with_walletconnect(&ctx, protocol_conf.chain_id.as_ref(), &ticker).await?
},

then KDF takes the request and runs the logic where it reads Pubkey from WC( maybe this function should have a different naming)

async fn activate_with_walletconnect(
ctx: &MmArc,
chain_id: &str,
ticker: &str,
) -> MmResult<(TendermintActivationPolicy, TendermintWalletConnectionType), TendermintInitError> {
let wc = WalletConnectCtx::from_ctx(ctx).expect("WalletConnectCtx should be initialized by now!");
let account = cosmos_get_accounts_impl(&wc, chain_id)
.await
.mm_err(|err| TendermintInitError {
ticker: ticker.to_string(),
kind: TendermintInitErrorKind::UnableToFetchChainAccount(err.to_string()),
})?;
let wallet_type = if wc.is_ledger_connection().await {
TendermintWalletConnectionType::WcLedger
} else {
TendermintWalletConnectionType::Wc
};
let pubkey = match account.algo {
CosmosAccountAlgo::Secp256k1 | CosmosAccountAlgo::TendermintSecp256k1 => {
TendermintPublicKey::from_raw_secp256k1(&account.pubkey).ok_or(TendermintInitError {
ticker: ticker.to_string(),
kind: TendermintInitErrorKind::Internal("Invalid secp256k1 pubkey".to_owned()),
})?
},
};
Ok((TendermintActivationPolicy::with_public_key(pubkey), wallet_type))
}

Uses that Pubkey to run KDF as an external wallet (just like any other wallet).

Ok((TendermintActivationPolicy::with_public_key(pubkey), wallet_type))

mm2src/kdf_walletconnect/src/chain.rs Outdated Show resolved Hide resolved
mm2src/coins/tendermint/tendermint_token.rs Show resolved Hide resolved
@@ -276,6 +279,7 @@ impl TendermintActivationPolicy {

#[cfg(target_arch = "wasm32")]
PrivKeyPolicy::Metamask(_) => unreachable!(),
PrivKeyPolicy::WalletConnect { .. } => unreachable!(),
Copy link
Member

Choose a reason for hiding this comment

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

Q: Is it really impossible to get the public key from WalletConnect?

Copy link
Member Author

Choose a reason for hiding this comment

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

you mean privateKey?, No

Copy link
Member

Choose a reason for hiding this comment

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

I read it wrong

Copy link
Collaborator

Choose a reason for hiding this comment

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

this method gets the pubkey though.
if i understand correctly this is marked as unreachable as we convert walletconnect variants to pubkey-only.
can we return an error here instead?

mm2src/coins/tendermint/tendermint_coin.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@mariocynicys mariocynicys left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Very organized and easy to review.
Here is the first iteration.

@@ -2,7 +2,7 @@
JEMALLOC_SYS_WITH_MALLOC_CONF = "background_thread:true,narenas:1,tcache:false,dirty_decay_ms:0,muzzy_decay_ms:0,metadata_thp:auto"

[target.'cfg(all())']
rustflags = [ "-Zshare-generics=y" ]
rustflags = [ "-Zshare-generics=y", '--cfg=curve25519_dalek_backend="fiat"' ]
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q: what is that for?

Copy link
Member Author

Choose a reason for hiding this comment

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

selecting the backend for compiling curve25519-dalek. This is explicitly needed because I updated the lib which now supports different backends for compilation

https://crates.io/crates/curve25519-dalek/4.1.3#simd-backend

mm2src/coins/eth/v2_activation.rs Outdated Show resolved Hide resolved
mm2src/coins/eth/v2_activation.rs Outdated Show resolved Hide resolved
mm2src/coins/eth/wallet_connect.rs Show resolved Hide resolved
mm2src/coins/eth/wallet_connect.rs Outdated Show resolved Hide resolved
mm2src/mm2_main/src/rpc/dispatcher/dispatcher.rs Outdated Show resolved Hide resolved
mm2src/coins/eth/wallet_connect.rs Outdated Show resolved Hide resolved
mm2src/coins/utxo/utxo_common.rs Outdated Show resolved Hide resolved
Comment on lines +64 to +65
[patch.crates-io]
rand_core = { version = "0.6.2", package = "bip39" }
Copy link
Collaborator

Choose a reason for hiding this comment

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

Q: can't understand this patch. why doesn't it have a url/git? could you explain how/what this does.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'm forcing bip39 to depend on rand_core 0.6.2 since the lib is selecting the least supported version(0.4.2) in kdf even tho the version isn't really compatible with the latest version of bip39
https://crates.io/crates/bip39/2.1.0/dependencies, I mean the feature of bip39 we using in kdf requires rand_core 0.6.2 or greater

The lib is configured to select any version from 0.4.2==0.7

mm2src/mm2_main/src/rpc.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@mariocynicys mariocynicys left a comment

Choose a reason for hiding this comment

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

another short iteration. i think i will focus solely on the new crate in the next iteration since the rest looks good.

the new crate i think is very well organized and easy to traverse. could make use of excessive spamy detailed (doc) comments though. this way others who didn't review it could still understand and change it.

@@ -276,6 +279,7 @@ impl TendermintActivationPolicy {

#[cfg(target_arch = "wasm32")]
PrivKeyPolicy::Metamask(_) => unreachable!(),
PrivKeyPolicy::WalletConnect { .. } => unreachable!(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

this method gets the pubkey though.
if i understand correctly this is marked as unreachable as we convert walletconnect variants to pubkey-only.
can we return an error here instead?

Comment on lines +186 to +198
let mut vec = Vec::new();
for i in 0..map.len() {
if let Some(Value::Number(num)) = map.get(&i.to_string()) {
if let Some(byte) = num.as_u64() {
vec.push(byte as u8);
} else {
return Err(serde::de::Error::custom("Invalid byte value"));
}
} else {
return Err(serde::de::Error::custom("Invalid format"));
}
}
Ok(vec)
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit suggestion:

            map.values()
                .map(|v| {
                    v.as_u64()
                        .map(|v| v as u8)
                        .ok_or_else(|| serde::de::Error::custom("Invalid byte value"))
                })
                .collect()

Copy link
Member Author

Choose a reason for hiding this comment

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

these aren't equivalent

Copy link
Collaborator

Choose a reason for hiding this comment

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

only diff i think is Some(Value::Number(num)) = map.get() to check if the value is a number? right?
I don't think that's really important to set one error message for numbers vs non-numbers bad bytes. also we should do the same for conversion from u64 to u8.

@@ -205,7 +205,7 @@ async fn process_single_request(ctx: MmArc, req: Json, client: SocketAddr) -> Re

#[cfg(not(target_arch = "wasm32"))]
async fn rpc_service(req: Request<Body>, ctx_h: u32, client: SocketAddr) -> Response<Body> {
const NON_ALLOWED_CHARS: &[char] = &['<', '>', '&'];
const NON_ALLOWED_CHARS: &[char] = &['Ò'];
Copy link
Collaborator

Choose a reason for hiding this comment

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

what's that char and why is it disallowed? if it's a place holder we could instead remove this validation all together.

mm2src/kdf_walletconnect/src/session.rs Outdated Show resolved Hide resolved
mm2src/kdf_walletconnect/src/lib.rs Outdated Show resolved Hide resolved
mm2src/kdf_walletconnect/src/session.rs Outdated Show resolved Hide resolved
mm2src/kdf_walletconnect/src/chain.rs Outdated Show resolved Hide resolved
mm2src/kdf_walletconnect/src/lib.rs Outdated Show resolved Hide resolved
mm2src/kdf_walletconnect/src/lib.rs Outdated Show resolved Hide resolved
mm2src/coins/eth/wallet_connect.rs Outdated Show resolved Hide resolved
mm2src/coins/eth/wallet_connect.rs Outdated Show resolved Hide resolved
};

let mut public = Public::default();
public.as_mut().copy_from_slice(&uncompressed[1..65]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

ummm, looks like we skip the first 8bits of the H520, thus rendering it essentially an H512. maybe we could return a Public = H512 instead of PublicKey = H520 from recover().

that's just an early suggestion, i will want to check what these trimmed 8bits actually for first.

Copy link
Member Author

Choose a reason for hiding this comment

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

why is this changed needed ? I think its more appropriate to return Seckp2561 pubkey here

pub(crate) fn recover(signature: &Signature, message: &Message) -> Result<PublicKey, ethkey::Error> {
let recovery_id = {
let recovery_id = (signature[64] as i32)
.checked_sub(27)
.ok_or_else(|| ethkey::Error::InvalidSignature)?;
RecoveryId::from_i32(recovery_id)?
};
let sig = RecoverableSignature::from_compact(&signature[0..64], recovery_id)?;
let pubkey = Secp256k1::new().recover(&secp256k1::Message::from_slice(&message[..])?, &sig)?;
Ok(pubkey)
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

not really needed. but I'm trying to understand why we have two different pubkey types and making them into one if possible.
will dig more into it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like the first octet (byte/8-bits) is a flag to signify that the pubkey is uncompressed (even though one would usually know from the pubkey length, but i guess this is the standard anyways).
0x04 -> uncompressed
0x02 -> compressed with even y co-ordinate
0x03 -> compressed with odd y co-ordinate
https://www.rfc-editor.org/rfc/rfc5480#section-2.2

we can leave a little note saying that the first bit is not part of the uncomprssed key but rather just part of the encoding.

Copy link
Collaborator

@mariocynicys mariocynicys left a comment

Choose a reason for hiding this comment

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

Another iter. Thanks! :)

pub struct WalletConnectCtx {
pub(crate) client: Client,
pub(crate) pairing: PairingClient,
pub session: SessionManager,
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think we want to rename this field to session_manager. it makes more sense to call session_manager.get_sessions() instead of session.get_sessions(). also might be confusing as this session field has sessions inside it.

Copy link
Member Author

@borngraced borngraced Nov 26, 2024

Choose a reason for hiding this comment

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

make sense. thanks
done

Some(value) => Some(serde_json::from_value(value)?),
None => None,
};
let (topic, url) = self.pairing.create(self.metadata.clone(), None)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

what if we fail to subscribe within attempts or send proposal? the pairing will persist here forever?

Copy link
Member Author

@borngraced borngraced Nov 26, 2024

Choose a reason for hiding this comment

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

if subscription fails, pairing will expire after 5 mins and becomes unusable. The only option is to resend a new_connection request. Actually pairing is only needed to initiate a session connection

/// Internal implementation of session management.
struct SessionManagerImpl {
/// The currently active session topic.
active_topic: Mutex<Option<Topic>>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like this is an async mutex. i skimmed quickly over the usages of it and i think we don't need it async.
we should always prefer sync mutex unless there is a reason not to use them.

Copy link
Member Author

@borngraced borngraced Nov 26, 2024

Choose a reason for hiding this comment

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

not totally, we should prefer async mutex over sync mutex in async environment. The compiler won't even allow you use sync mutex while holding the lock over an await.

Copy link
Collaborator

Choose a reason for hiding this comment

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

not really. one should prefer sync mutexes even in async contexts. sync mutexes are faster and lighter.
async mutexes on the other hand are packed with capabilities to make them able to lock across await points when the execution is handed some where else.
if the sync mutex doesn't get locked for so long (so locking the mutex and executing a long op while the mutex is locked) then it's perfectly fine to use it in async context it won't degrade (but actually boost) perf.

imagine a mutex that's available to lock nearly 99.99% of the time, it's easy to acquire, and if it ever blocked you only wait for a couple of micro seconds at max as we don't hold the mutex and execute some long op.

Copy link
Member Author

@borngraced borngraced Nov 27, 2024

Choose a reason for hiding this comment

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

https://stackoverflow.com/a/73840831, so it all boils down to how long the lock will be held for I guess

upd: updated to sync mutex 31c9594

let session_proposal = RequestParams::SessionPropose(SessionProposeRequest {
relays: vec![ctx.relay.clone()],
proposer,
required_namespaces: namespaces.unwrap_or_else(build_default_required_namespaces),
Copy link
Collaborator

Choose a reason for hiding this comment

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

why we default to build_default_required_namespaces? why not default to no namespaces? or otherwise force the user to provide name spaces and not make it optional?

Copy link
Member Author

@borngraced borngraced Nov 26, 2024

Choose a reason for hiding this comment

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

you can't send a proposal with an empty namespaces. atleast one is required.

otherwise force the user to provide name spaces

yeah better, bdf9636


async fn save_session(&self, session: &Session) -> MmResult<(), Self::Error> {
debug!("[{}] Saving WalletConnect session to storage", session.topic);
validate_table_name(SESSION_TABLE_NAME).map_err(AsyncConnError::from)?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think it's enough to validate the table name on initialization

let wallet_connect = WalletConnectCtx::from_ctx(ctx)?;
// WalletConnectCtx is initialized, now we can connect to relayer client and spawn a watcher
// loop for disconnection.
ctx.spawner().spawn(initialize_connection(wallet_connect.clone()));
Copy link
Collaborator

Choose a reason for hiding this comment

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

could we cover this inside WalletConnectCtx::try_init, so try_init would return a complete functional WC context.
i think we also might want to use our spawning system and hand WCCtx a subsystem of its own to run these long lived futures in.
right now we use ctx to spawn this future, we use ctx for another future below, and use spawn_aboratable for one future spawned inside the try_init.

Copy link
Member Author

@borngraced borngraced Nov 26, 2024

Choose a reason for hiding this comment

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

hmm yeah thanks a lot.. going to try this path

upd: done


// spawn message handler event loop
ctx.spawner().spawn(async move {
let mut recv = wallet_connect.inbound_message_rx.lock().await;
Copy link
Collaborator

Choose a reason for hiding this comment

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

feels like inbound_message_rx shouldn't have been stored inside the context in the first place?

Copy link
Member Author

Choose a reason for hiding this comment

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

};
let (inbound_message_tx, inbound_message_rx) = unbounded();
let (conn_live_sender, conn_live_receiver) = unbounded();
let (message_tx, session_request_receiver) = unbounded();
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's rename session_request_receiver to message_rx or rename message_tx to session_request_sender.
that aside, it feels wrong holding two ends of the channel in the same struct. you want to have one end given to another party!

Copy link
Member Author

Choose a reason for hiding this comment

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

it feels wrong holding two ends of the channel in the same struct

why??

Copy link
Member Author

Choose a reason for hiding this comment

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

upd: used weak spawner so this has been refactored.

Copy link
Collaborator

Choose a reason for hiding this comment

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

nope, we still hold both ends of the channel.
what about passing message_tx to handle_published_message so not to have to store the send end of the channel in the context. it's only used by process_inbound_response.

Comment on lines +497 to +499
self.publish_request(&active_topic, request).await?;

if let Ok(Some(resp)) = self.message_rx.lock().await.next().timeout_secs(ttl as f64).await {
Copy link
Collaborator

Choose a reason for hiding this comment

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

now after we publish our request, are we really sure that the next response we get from the message_rx was really intended for us?
what if there is another thread that sent a request at around the same time? how do we gaurantee each thread gets the resposne message it should be expecting?

Copy link
Member Author

@borngraced borngraced Nov 26, 2024

Choose a reason for hiding this comment

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

Yes, since WalletConnect connected wallets processes only one request at a time and responds in the same order (FIFO)

};
let (inbound_message_tx, inbound_message_rx) = unbounded();
let (conn_live_sender, conn_live_receiver) = unbounded();
let (message_tx, session_request_receiver) = unbounded();
Copy link
Collaborator

Choose a reason for hiding this comment

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

nope, we still hold both ends of the channel.
what about passing message_tx to handle_published_message so not to have to store the send end of the channel in the context. it's only used by process_inbound_response.


impl WalletConnectCtx {
pub fn try_init(ctx: &MmArc) -> MmResult<Self, WalletConnectError> {
let abortable_system = ctx.abortable_system.create_subsystem::<AbortableQueue>().unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

in a normal state this shouldn't really trigger any panic, but could we still have this return error instead? just in case

relay: Relay,
metadata: Metadata,
message_tx: UnboundedSender<SessionMessageType>,
message_rx: Arc<Mutex<UnboundedReceiver<SessionMessageType>>>,
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think we can free this out of Arc

};
let (topic, url) = self.pairing.create(self.metadata.clone(), None)?;

info!("[topic] Subscribing to topic");
Copy link
Collaborator

Choose a reason for hiding this comment

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

u probably meant to print the topic in this info!?

.await
{
Ok(Ok(_)) => {
info!("[topic] Subscribed to topic");
Copy link
Collaborator

Choose a reason for hiding this comment

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

and this one also

fn disconnected(&mut self, frame: Option<CloseFrame<'static>>) {
debug!("[{}] connection closed: frame={frame:?}", self.name);

if let Err(e) = self.conn_live_sender.start_send(frame.map(|f| f.to_string())) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like this does the exact job as unbounded_send, could we use that one instead to avoid confusion.
also for the other occurrences.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't get this comment. unbounded_send

Copy link
Collaborator

Choose a reason for hiding this comment

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

sorry. i meant replacing start_send with unbounded_send.
we usually exclusively only use .send and .unbounded_send for channels. so these are the more familiar ones.

Copy link
Member Author

Choose a reason for hiding this comment

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

updated


fn inbound_error(&mut self, error: ClientError) {
debug!("[{}] inbound error: {error}", self.name);
if let Err(e) = self.conn_live_sender.start_send(Some(error.to_string())) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

are these errors considered a disconnection?
the connection loop doesn't seem disconnected when firing them.

Copy link
Member Author

@borngraced borngraced Nov 28, 2024

Choose a reason for hiding this comment

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

websocket errors like connection drop can be returned from inbound_error or outbound error which requires a reconnect(no other workaround). I think it's better to reconnect on connection errors for WS too

};
}

let param = ResponseParamsSuccess::PairingPing(true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

this should return pairing extend message? no?

/// Information about the controlling party (typically a wallet).
pub controller: Controller,
/// Information about the proposing party (typically a dApp).
pub proposer: Proposer,
Copy link
Collaborator

Choose a reason for hiding this comment

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

this doesn't seem to be ever used?
also why we even have info about both proposer & controller at the same time (so 2 fields)? why not have an enum store info for the opposite party.

Copy link
Member Author

@borngraced borngraced Nov 28, 2024

Choose a reason for hiding this comment

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

It's good to save this information, should be useful in the GUI

Comment on lines 235 to 237
pub(crate) async fn add_session(&self, session: Session) {
// set active session topic.
*self.0.active_topic.lock().await = Some(session.topic.clone());
*self.0.active_topic.lock().unwrap() = Some(session.topic.clone());
Copy link
Collaborator

Choose a reason for hiding this comment

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

such funcs were made async exclusively because of the async lock. we can make them sync now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants