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

[Core] Networking error publisher #1070

Merged
merged 5 commits into from
Sep 8, 2023

Conversation

flypaper0
Copy link
Contributor

No description provided.

@flypaper0 flypaper0 temporarily deployed to internal August 31, 2023 15:27 — with GitHub Actions Inactive
@flypaper0 flypaper0 force-pushed the feature/notworking-error-publisher branch from 4d01f54 to 71c9886 Compare September 4, 2023 12:51
@flypaper0 flypaper0 temporarily deployed to internal September 4, 2023 12:51 — with GitHub Actions Inactive

logger.debug("Received Notify Message on topic: \(payload.topic)", properties: ["topic": payload.topic])
let (messagePayload, claims) = try NotifyMessagePayload.decodeAndVerify(from: payload.request)
Copy link
Contributor

Choose a reason for hiding this comment

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

if this throws will it be handled in Networking Client?

if so, isn't it strange that lower level client handles higher level client's errors?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Networking client not handling, just aggregating. Handling will be in Networking client publisher's subscribers

Copy link
Contributor

Choose a reason for hiding this comment

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

I see, the idea of having the "catch block" is really good but to me it's still strange that for expample:
in notify engine in networkingInteractor.subscribeOnRequest block let's say function throws a notify specific error.
for example "notify subscription not found" or whatever and that error is passed down to Networking Client.

so let's say later an app want's to listen to networking only errors and it gets "notify subscription not found"

don't you think something smells here?

Copy link
Contributor Author

@flypaper0 flypaper0 Sep 5, 2023

Choose a reason for hiding this comment

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

I think it is the only way to have single error publisher. Depends on what we trying to acheive. By that PR I want to reduce error handling code for SDK's. One thing. May be we can delete error publisher for now and only log inside catch block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

because we are mostly just logging errors

Copy link
Contributor

Choose a reason for hiding this comment

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

I am still not convinced 🤔

@flypaper0 flypaper0 temporarily deployed to internal September 4, 2023 14:23 — with GitHub Actions Inactive
@flypaper0 flypaper0 requested a review from llbartekll September 4, 2023 14:30
@flypaper0 flypaper0 temporarily deployed to internal September 6, 2023 15:59 — with GitHub Actions Inactive
@flypaper0 flypaper0 force-pushed the feature/notworking-error-publisher branch from 1c5cee4 to e4d8ec8 Compare September 7, 2023 08:47
@flypaper0 flypaper0 temporarily deployed to internal September 7, 2023 08:47 — with GitHub Actions Inactive
Copy link
Contributor

@llbartekll llbartekll left a comment

Choose a reason for hiding this comment

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

much better now imho!

Sources/WalletConnectUtils/Logger/ConsoleLogger.swift Outdated Show resolved Hide resolved
@flypaper0 flypaper0 temporarily deployed to internal September 8, 2023 08:14 — with GitHub Actions Inactive
@flypaper0 flypaper0 temporarily deployed to internal September 8, 2023 09:26 — with GitHub Actions Inactive
@flypaper0 flypaper0 temporarily deployed to internal September 8, 2023 11:19 — with GitHub Actions Inactive
@flypaper0 flypaper0 temporarily deployed to internal September 8, 2023 12:53 — with GitHub Actions Inactive
@flypaper0 flypaper0 temporarily deployed to internal September 8, 2023 14:27 — with GitHub Actions Inactive
@flypaper0 flypaper0 merged commit 06cf578 into develop Sep 8, 2023
8 checks passed
@flypaper0 flypaper0 deleted the feature/notworking-error-publisher branch September 8, 2023 15:10
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