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

[WalletApp] Handle removed session request #1232

Merged
merged 2 commits into from
Nov 16, 2023

Conversation

alexander-lsvk
Copy link
Contributor

Fix for https://walletconnect.slack.com/archives/C03S6NN8NHF/p1699699835241069

Due Dilligence

  • Breaking change
  • Requires a documentation update

@alexander-lsvk alexander-lsvk changed the title [Wallet App] Handle removed session request [WalletApp] Handle removed session request Nov 14, 2023
@@ -281,3 +281,12 @@ private extension SessionEngine {
onEventReceived?(topic, event.publicRepresentation(), payload.request.chainId)
}
}

extension SessionEngine.Errors: LocalizedError {
var errorDescription: String? {
Copy link
Contributor

Choose a reason for hiding this comment

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

is it better to use WalletConnectError? we have noSessionMatchingTopic for example

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Dunno whether it's better, the SessionEngine has its own Errors. Should I remove this error from SessionEngine then?

extension SessionEngine.Errors: LocalizedError {
var errorDescription: String? {
switch self {
case .sessionRequestExpired: return "Session request expired"
Copy link
Contributor

Choose a reason for hiding this comment

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

It also can be moved in WalletConnectError imo

@alexander-lsvk alexander-lsvk merged commit 162b841 into develop Nov 16, 2023
8 checks passed
@alexander-lsvk alexander-lsvk deleted the handle-removed-session-requests branch November 16, 2023 10:45
@flypaper0 flypaper0 mentioned this pull request Nov 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants