Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions .changeset/fix-wallet-not-found-log-level.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
"@crossmint/common-sdk-base": patch
"@crossmint/wallets-sdk": patch
---

Log `WalletNotAvailableError` from `walletFactory.getWallet` at warn level instead of error. The `WithLoggerContext` decorator now supports an `expectedErrors` option so decorated methods can declare which error classes represent normal business outcomes (e.g. wallet not found) that should not pollute error-level monitoring.
143 changes: 143 additions & 0 deletions packages/common/base/src/logger/decorators.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,143 @@
import { describe, expect, it, vi } from "vitest";
import { WithLoggerContext } from "./decorators";
import { SdkLogger } from "./SdkLogger";

class ExpectedError extends Error {
constructor(message: string) {
super(message);
this.name = "ExpectedError";
}
}

class UnexpectedError extends Error {
constructor(message: string) {
super(message);
this.name = "UnexpectedError";
}
}

function createMockLogger() {
const logger = new SdkLogger({ packageName: "test" });
logger.warn = vi.fn();
logger.error = vi.fn();
logger.info = vi.fn();
logger.debug = vi.fn();
return logger;
}

describe("WithLoggerContext", () => {
describe("expectedErrors", () => {
it("logs expected errors at warn level", async () => {
const logger = createMockLogger();

class Subject {
@WithLoggerContext({
logger,
methodName: "subject.method",
expectedErrors: [ExpectedError],
})
async doWork(): Promise<void> {
throw new ExpectedError("not found");
}
}

const subject = new Subject();
await expect(subject.doWork()).rejects.toThrow("not found");

expect(logger.warn).toHaveBeenCalledWith("subject.method threw an error", {
error: expect.any(ExpectedError),
});
expect(logger.error).not.toHaveBeenCalledWith("subject.method threw an error", expect.anything());
});

it("logs unexpected errors at error level", async () => {
const logger = createMockLogger();

class Subject {
@WithLoggerContext({
logger,
methodName: "subject.method",
expectedErrors: [ExpectedError],
})
async doWork(): Promise<void> {
throw new UnexpectedError("boom");
}
}

const subject = new Subject();
await expect(subject.doWork()).rejects.toThrow("boom");

expect(logger.error).toHaveBeenCalledWith("subject.method threw an error", {
error: expect.any(UnexpectedError),
});
expect(logger.warn).not.toHaveBeenCalledWith("subject.method threw an error", expect.anything());
});

it("logs all errors at error level when expectedErrors is omitted", async () => {
const logger = createMockLogger();

class Subject {
@WithLoggerContext({
logger,
methodName: "subject.method",
})
async doWork(): Promise<void> {
throw new ExpectedError("not found");
}
}

const subject = new Subject();
await expect(subject.doWork()).rejects.toThrow("not found");

expect(logger.error).toHaveBeenCalledWith("subject.method threw an error", {
error: expect.any(ExpectedError),
});
expect(logger.warn).not.toHaveBeenCalledWith("subject.method threw an error", expect.anything());
});

it("still rethrows the error in all cases", async () => {
const logger = createMockLogger();

class Subject {
@WithLoggerContext({
logger,
methodName: "subject.method",
expectedErrors: [ExpectedError],
})
async doWork(err: Error): Promise<void> {
throw err;
}
}

const subject = new Subject();
const expected = new ExpectedError("expected");
const unexpected = new UnexpectedError("unexpected");

await expect(subject.doWork(expected)).rejects.toBe(expected);
await expect(subject.doWork(unexpected)).rejects.toBe(unexpected);
});

it("handles sync methods that throw expected errors", () => {
const logger = createMockLogger();

class Subject {
@WithLoggerContext({
logger,
methodName: "subject.syncMethod",
expectedErrors: [ExpectedError],
})
doWorkSync(): void {
throw new ExpectedError("sync not found");
}
}

const subject = new Subject();
expect(() => subject.doWorkSync()).toThrow("sync not found");

expect(logger.warn).toHaveBeenCalledWith("subject.syncMethod threw an error", {
error: expect.any(ExpectedError),
});
expect(logger.error).not.toHaveBeenCalledWith("subject.syncMethod threw an error", expect.anything());
});
});
});
29 changes: 27 additions & 2 deletions packages/common/base/src/logger/decorators.ts
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,14 @@ export interface WithLoggerContextOptions<T = unknown> {
* Optional function to build additional context from the method's this and arguments
*/
buildContext?: ContextBuilder<T>;

/**
* Error classes that represent expected/business-logic outcomes (e.g. "not found").
* Errors matching any of these classes are logged at `warn` instead of `error`
* so they do not pollute error-level monitoring.
*/
// eslint-disable-next-line @typescript-eslint/no-explicit-any
expectedErrors?: Array<new (...args: any[]) => Error>;
}

/**
Expand Down Expand Up @@ -72,14 +80,14 @@ export function WithLoggerContext<TThis = unknown>(options: WithLoggerContextOpt
try {
result = original.apply(this, args);
} catch (error) {
options.logger.error(`${options.methodName} threw an error`, { error });
logThrownError(options, error);
throw error;
}

// Handle async functions
if (result instanceof Promise) {
return result.catch((error) => {
options.logger.error(`${options.methodName} threw an error`, { error });
logThrownError(options, error);
throw error;
});
}
Expand All @@ -93,3 +101,20 @@ export function WithLoggerContext<TThis = unknown>(options: WithLoggerContextOpt
return descriptor;
};
}

/**
* Log an error thrown by a decorated method. Expected errors (those matching
* `options.expectedErrors`) are logged at `warn`; everything else at `error`.
*/
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 });
}
}
Comment on lines +109 to +120
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P2 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)

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fair point, you sneaky little gecko 🐍 — adding tests now for the expectedErrors branch. Will push shortly.

1 change: 1 addition & 0 deletions packages/wallets/src/wallets/wallet-factory.ts
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,7 @@ export class WalletFactory {
@WithLoggerContext({
logger: walletsLogger,
methodName: "walletFactory.getWallet",
expectedErrors: [WalletNotAvailableError],
buildContext(_thisArg: WalletFactory, args: unknown[]) {
if (typeof args[0] === "string") {
const walletArgs = args[1] as WalletArgsFor<Chain> | undefined;
Expand Down
Loading