fix(wallets): log WalletNotAvailableError at warn instead of error#1846
fix(wallets): log WalletNotAvailableError at warn instead of error#1846devin-ai-integration[bot] wants to merge 3 commits into
Conversation
The WithLoggerContext decorator unconditionally logged all thrown errors at error level. For WalletNotAvailableError this is a normal business outcome (wallet not found) used in locator-iteration and existence-check flows, yet it generated thousands of error-level entries in Datadog. Add an expectedErrors option to WithLoggerContext so decorated methods can declare which error classes represent expected outcomes. Matching errors are now logged at warn level instead of error. Mark WalletNotAvailableError as expected in walletFactory.getWallet. Co-Authored-By: Agus <agustin@paella.dev>
Co-Authored-By: Agus <agustin@paella.dev>
Original prompt from Agus
|
🦋 Changeset detectedLatest commit: 224b979 The changes in this PR will be included in the next version bump. This PR includes changesets to release 18 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Prompt To Fix All With AIFix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/wallets/src/wallets/wallet-factory.ts:92-98
**Double warn-level log for `WalletNotAvailableError`**
The method already logs `walletFactory.getWallet.notFound` at `warn` on line 94 before throwing. With `WalletNotAvailableError` now in `expectedErrors`, the decorator will also emit a second `warn` log (`walletFactory.getWallet threw an error`). Every wallet-not-found case produces two warn entries — previously it was one `warn` + one `error`. This was a pre-existing pattern, but worth confirming the team is comfortable having duplicate warn logs in place of the prior error-level noise.
### Issue 2 of 2
packages/common/base/src/logger/decorators.ts:109-120
**No unit tests for the new `expectedErrors` branch**
The `logThrownError` helper introduces a new code path (warn vs. error selection) that has no test coverage — there are currently no test files under `packages/common/base/src/logger/`. A test verifying that an expected-error class triggers `logger.warn` while an unexpected error triggers `logger.error` (and that the error is still re-thrown in both cases) would prevent silent regressions to this decorator, which is shared across the SDK.
Reviews (1): Last reviewed commit: "chore: add changeset for expected-errors..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| function logThrownError( | ||
| options: Pick<WithLoggerContextOptions, "logger" | "methodName" | "expectedErrors">, | ||
| error: unknown | ||
| ): void { | ||
| const isExpected = options.expectedErrors?.some((ErrorClass) => error instanceof ErrorClass) ?? false; | ||
| const message = `${options.methodName} threw an error`; | ||
| if (isExpected) { | ||
| options.logger.warn(message, { error }); | ||
| } else { | ||
| options.logger.error(message, { error }); | ||
| } | ||
| } |
There was a problem hiding this comment.
expectedErrors branch
The logThrownError helper introduces a new code path (warn vs. error selection) that has no test coverage — there are currently no test files under packages/common/base/src/logger/. A test verifying that an expected-error class triggers logger.warn while an unexpected error triggers logger.error (and that the error is still re-thrown in both cases) would prevent silent regressions to this decorator, which is shared across the SDK.
Rule Used: Add unit tests when implementing new validation lo... (source)
Learned From
Paella-Labs/crossbit-main#21014
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/common/base/src/logger/decorators.ts
Line: 109-120
Comment:
**No unit tests for the new `expectedErrors` branch**
The `logThrownError` helper introduces a new code path (warn vs. error selection) that has no test coverage — there are currently no test files under `packages/common/base/src/logger/`. A test verifying that an expected-error class triggers `logger.warn` while an unexpected error triggers `logger.error` (and that the error is still re-thrown in both cases) would prevent silent regressions to this decorator, which is shared across the SDK.
**Rule Used:** Add unit tests when implementing new validation lo... ([source](https://app.greptile.com/review/custom-context?memory=9dc35ad5-5868-49a3-bbcb-a42edc5ee697))
**Learned From**
[Paella-Labs/crossbit-main#21014](https://github.com/Paella-Labs/crossbit-main/pull/21014)
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Fair point, you sneaky little gecko 🐍 — adding tests now for the expectedErrors branch. Will push shortly.
|
Good catch, cold-blooded code reviewer 🦎 — you're right that there are now two warn-level logs for the not-found case. However, they serve complementary purposes:
The primary goal here is eliminating the error-level noise in Datadog (17K+ false error entries/week). Two warn-level entries are a significant improvement over one warn + one error, and the deduplication can be a follow-up if the team finds the warn volume too chatty. |
…decorator Co-Authored-By: Agus <agustin@paella.dev>
|
Reviews (2): Last reviewed commit: "test(logger): add unit tests for expecte..." | Re-trigger Greptile |
🔥 Smoke Test Results❌ Status: Failed Statistics
Test DetailsThis is a non-blocking smoke test. Full regression tests run separately. |
1 participant
Description
The
WithLoggerContextdecorator unconditionally logged all thrown errors aterrorlevel. ForWalletNotAvailableError— a normal business outcome indicating "wallet not found" — this generated thousands of error-level entries in Datadog (17K+ logs over the past week from lobster.cash alone).WalletNotAvailableErroris expected in locator-iteration and existence-check flows (e.g., when looking up wallets across chains). The error is caught and handled by consumers, but the SDK's decorator had already logged it at error level.Changes:
packages/common/base/src/logger/decorators.ts— Add anexpectedErrorsoption toWithLoggerContextOptions. Errors matching any of the listed classes are logged atwarninstead oferror. Backwards-compatible: whenexpectedErrorsis not provided, behavior is unchanged.packages/wallets/src/wallets/wallet-factory.ts— MarkWalletNotAvailableErroras an expected error in thewalletFactory.getWalletdecorator.packages/common/base/src/logger/decorators.test.ts— Unit tests covering the newexpectedErrorsbranch (expected → warn, unexpected → error, omitted → error, sync methods, error rethrow).Test plan
WithLoggerContextdecorator'sexpectedErrorsbehavior (5 tests):expectedErrorsomitted (backwards-compat)Package updates
@crossmint/common-sdk-base: patch — addexpectedErrorsoption toWithLoggerContext@crossmint/wallets-sdk: patch — useexpectedErrorsforWalletNotAvailableErroringetWalletChangeset added via
.changeset/fix-wallet-not-found-log-level.md.Link to Devin session: https://crossmint.devinenterprise.com/sessions/638aa84609aa47e0b2894361307de295
Requested by: @aigustin