Skip to content

Conversation

@pedroanastacio
Copy link
Member

@pedroanastacio pedroanastacio commented Jan 19, 2026

Description

Some parts of the code base still rely on direct console.* calls (console.warn, console.error, console.log, etc.) to report errors and runtime information. While this works during development, it may not be ideal for production.
Replace direct console.* calls with the project's logger instance to ensure structured, consistent, and observable logs.

Summary

Checklist

  • I reviewed my PR code before submitting
  • I ensured that the implementation is working correctly and did not impact other parts of the app
  • I mentioned the PR link in the task

import { logger } from '@src/config/logger';

const { API_URL, FUEL_PROVIDER } = process.env;

Copy link
Member

Choose a reason for hiding this comment

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

🟡 IMPORTANT: Unused import removal

Problem:
The FUEL_PROVIDER constant is imported but no longer used after the console.log removal.

Suggestion:
Remove the unused import:

const { API_URL } = process.env;

Copy link
Member Author

@pedroanastacio pedroanastacio Jan 19, 2026

Choose a reason for hiding this comment

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

FUEL_PROVIDER has been removed: 79c4314

chainId?: number,
): Promise<void> {
const chainInfo = chainId ? ` chain:${chainId}` : ' all chains';
const chainInfo = chainId ?? 'all chains';
Copy link
Member

Choose a reason for hiding this comment

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

🟡 IMPORTANT: Inconsistent log message format

Problem:
This log message doesn't follow the structured format pattern used elsewhere. The chain info is concatenated as a string instead of being a separate field.

Suggestion:
Use structured logging format:

logger.info(
  { 
    predicateAddress: addrShort, 
    chainId: chainId ?? 'all',
    operation: 'cache_invalidation'
  },
  '[TX_CACHE] Caches invalidated'
);

Copy link
Member Author

Choose a reason for hiding this comment

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

Adjusted on: fe9ab4f

Copy link
Member

@guimroque guimroque left a comment

Choose a reason for hiding this comment

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

Code Review - Summary

What was done

This PR implements structured logging using Pino library to replace direct console.* calls throughout the codebase. The changes include:

  • Installing pino and pino-pretty dependencies
  • Creating comprehensive logger configuration with security-focused data redaction
  • Adding ESLint rule to prevent console.* usage (except in scripts/migrations)
  • Replacing all console.* calls with structured logger calls across ~40 files
  • Implementing proper log levels and structured data logging

Positive Points

  • Excellent security compliance: Comprehensive redaction of sensitive data (passwords, tokens, private keys, etc.)
  • Production-ready configuration: Different formats for dev (pretty) vs prod (JSON)
  • Consistent implementation: Systematic replacement of console calls with proper structured logging
  • Good ESLint integration: Prevents future console.* usage while allowing exceptions for scripts
  • Proper error context: Most logger calls include relevant context data
  • Performance consideration: Non-blocking cache operations with error logging

Issues Found

  • 1 Critical: Typo in variable name that could cause runtime errors
  • 2 Important: Inconsistent log message formatting and unused import
  • 1 Suggestion: Minor formatting improvement opportunity

Total comments: 4 (1 critical, 2 important, 1 suggestion)

@pedroanastacio pedroanastacio requested review from guimroque and removed request for guimroque January 19, 2026 20:22
Copy link
Member

@guimroque guimroque left a comment

Choose a reason for hiding this comment

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

LGTM! ✅

Previous issues have been fixed. Code approved.

@guimroque guimroque merged commit f20c09c into staging Jan 20, 2026
1 check passed
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.

3 participants