-
Notifications
You must be signed in to change notification settings - Fork 802
refactor: changed logs error level to warn #7011
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
base: v2.0
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR refactors error logging to use warning-level logging instead across multiple packages in the WalletConnect codebase. The changes downgrade logger.error() calls to logger.warn() in error handling blocks, primarily in catch clauses where errors are being logged before re-throwing or in scenarios where errors are non-fatal.
Key Changes:
- Systematic replacement of
logger.error()withlogger.warn()in error handling contexts - Changes affect production code across sign-client, core, and provider packages
- No changes to error handling logic, only the severity level of logged messages
Reviewed Changes
Copilot reviewed 15 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| providers/universal-provider/src/UniversalProvider.ts | Changed session retrieval error logging from error to warn level |
| providers/ethereum-provider/src/EthereumProvider.ts | Updated error logging to warn level in connection, request handling, and modal initialization error paths |
| packages/sign-client/src/controllers/engine.ts | Converted numerous error logs to warnings across various RPC operations and session management flows |
| packages/sign-client/src/client.ts | Changed error message logging to warn level in all SignClient public method wrappers |
| packages/core/src/core.ts | Updated initialization failure logging from error to warn level |
| packages/core/src/controllers/verify.ts | Changed attestation validation error logging to warn level |
| packages/core/src/controllers/subscriber.ts | Updated subscription operation error logging to warn level |
| packages/core/src/controllers/store.ts | Changed store operation error logging to warn level |
| packages/core/src/controllers/relayer.ts | Updated transport error logging to warn level |
| packages/core/src/controllers/publisher.ts | Changed publish failure logging to warn level |
| packages/core/src/controllers/pairing.ts | Updated pairing operation error logging to warn level |
| packages/core/src/controllers/messages.ts | Changed message restoration error logging to warn level |
| packages/core/src/controllers/history.ts | Updated history restoration error logging to warn level |
| packages/core/src/controllers/expirer.ts | Changed expiration restoration error logging to warn level |
| packages/core/src/controllers/crypto.ts | Updated decoding error logging to warn level |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
arein
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some of these
| // Ignore if the implementing client has registered this method as known. | ||
| if (this.registeredMethods.includes(method)) return; | ||
| this.logger.error(getSdkError("WC_METHOD_UNSUPPORTED", method)); | ||
| this.logger.warn(getSdkError("WC_METHOD_UNSUPPORTED", method)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure this shouldn't remain error level?
| } catch (e) { | ||
| this.logger.debug(`Failed to Publish Payload`); | ||
| this.logger.error(e as any); | ||
| this.logger.warn(e as any); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should remain error if I understand correctly
| return payload; | ||
| } catch (error) { | ||
| this.logger.error( | ||
| this.logger.warn( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we keep error level when we catch the error?
jakubuid
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should keep error level when catching an error in try and catch blocks?
Description
This PR refactors error logging to use warning-level logging instead across multiple packages in the WalletConnect codebase. The changes downgrade logger.error() calls to logger.warn() in error handling blocks, primarily in catch clauses where errors are being logged before re-throwing or in scenarios where errors are non-fatal.
Key Changes:
The change is made as error logs are generally non actionable but polute services like sentry
Type of change
How has this been tested?
n/a
Checklist
Additional Information (Optional)
Please include any additional information that may be useful for the reviewer.
Note
Downgrades numerous logger.error calls to logger.warn across core, sign-client, and providers to reduce log severity for recoverable paths.
crypto,expirer,history,messages,pairing,publisher,relayer,store,subscriber,verify,core– downgrade error logs for decode/restore/subscribe/publish/reconnect and validation failures.clientandcontrollers/engine– warn on engine method failures, request/response handling, session events, and processing queues.ethereum-provider/EthereumProvider– warn on connect/auth/modal/init/persisted-session handling.universal-provider/UniversalProvider– warn when retrieving non-existent sessions during init.Written by Cursor Bugbot for commit 3633098. This will update automatically on new commits. Configure here.