Consolidate Zerion's fork #48
Conversation
|
Changes don't seem to break anything on our side 👍 |
|
Thanks for the pull request, still reviewing ⏳ |
| @@ -32,4 +32,6 @@ class UpdateSessionHandler: RequestHandler { | |||
|
|
|||
| struct SessionInfo: Decodable { | |||
| var approved: Bool | |||
| var accounts: [String]? | |||
| var chainId: Int | |||
There was a problem hiding this comment.
- Does it make sense to make
chainIdoptional for wc_sessionUpdate? - How to treat empty or absent accounts list if
approved=true? My guess is that ifaccountsare not passed, then the old array is not changed. Should we send an error response ifapproved=trueandaccounts=[]?
There was a problem hiding this comment.
@shmakovigor do you mind if we make chainId optional here?
| @@ -35,7 +35,7 @@ class WebSocketConnection { | |||
| self.onDisconnect = onDisconnect | |||
| self.onTextReceive = onTextReceive | |||
| serialCallbackQueue = DispatchQueue(label: "org.walletconnect.swift.connection-\(url.bridgeURL)-\(url.topic)") | |||
| socket = WebSocket(url: url.bridgeURL) | |||
| socket = WebSocket(request: URLRequest(url: url.bridgeURL)) | |||
There was a problem hiding this comment.
This change and in other places in this file is because Starscream API changed in next version. Everything looks good to me.
I think private var isConnected = false can be removed in favour of isOpen
| @@ -54,9 +56,9 @@ public struct Session: Codable { | |||
| public let accounts: [String] | |||
| public let chainId: Int | |||
| public let peerId: String | |||
| public let peerMeta: ClientMeta | |||
| public let peerMeta: ClientMeta? | |||
There was a problem hiding this comment.
This one is related to https://docs.walletconnect.org/tech-spec#session-request
According to specs:
The wc_sessionRequest will expect a response with the following parameters:
interface WCSessionRequestResponse {
id: number;
jsonrpc: "2.0";
result: {
peerId: string;
peerMeta: ClientMeta;
approved: boolean;
chainId: number;
accounts: string[];
};
}
@pedrouid is it required by protocol to always have peerMeta in the response? If not what other parameters can be optional?
|
@shmakovigor thanks for the proposed improvements. Let's merge it after resolving all comments. |
Hey @sche and @DmitryBespalov, I'm wondering what's your feedback on these changes made by @shmakovigor from Zerion? I would love to hear from @DarthMike from Argent as well
The more consensus we have on these clients the better! 🙌