Skip to content

Conversation

@llbartekll
Copy link
Collaborator

  • add debug logging
  • fix structured message signing

@claude
Copy link

claude bot commented Jan 5, 2026

Claude finished @llbartekll's task in 1m 32s —— View job


PR Review Complete

Tasks

  • Read and analyze Rust changes in pay/mod.rs
  • Read and analyze Rust changes in uniffi_compat/evm_signing/mod.rs
  • Analyze Swift bindings changes
  • Run automated security checks
  • Compile and report findings

Review Summary

Analyzed PR adding debug logging and fixing structured message signing. Found 2 issues requiring attention.


Issue 1: Debug logging uses stdout instead of proper logging framework

ID: pay-stdout-logging-8a3c
File: crates/yttrium/src/pay/mod.rs:500
Severity: MEDIUM
Category: code_quality

Debug logging implementation uses println! directly to stdout instead of the existing tracing crate already used elsewhere in the codebase (line 197-201). This creates inconsistent logging behavior and makes it harder to manage log output in production environments.

Recommendation: Use the tracing framework for consistency:

fn log(&self, message: &str) {
    if self.debug_enabled.load(Ordering::Relaxed) {
        tracing::debug!("[WalletConnectPay] {}", message);
    }
}

fn log_error(&self, context: &str, error: &impl std::fmt::Debug) {
    if self.debug_enabled.load(Ordering::Relaxed) {
        tracing::error!("[WalletConnectPay] {}: {:?}", context, error);
    }
}

Issue 2: Missing input validation for hex string parsing

ID: evm-signing-parse-validation-c2f1
File: crates/yttrium/src/uniffi_compat/evm_signing/mod.rs:21
Severity: LOW
Category: code_quality

The parse_hex_or_decimal function accepts hex strings with case-insensitive prefixes (0x or 0X) but doesn't validate the actual hex digits, which could lead to confusing error handling when invalid hex characters are present.

Recommendation: Add explicit validation for hex characters:

fn parse_hex_or_decimal(s: &str) -> Option<u64> {
    let s = s.trim();
    if s.starts_with("0x") || s.starts_with("0X") {
        let hex_part = &s[2..];
        // Validate hex characters explicitly
        if hex_part.chars().all(|c| c.is_ascii_hexdigit()) {
            u64::from_str_radix(hex_part, 16).ok()
        } else {
            None
        }
    } else {
        s.parse::<u64>().ok()
    }
}

Automated Checks

External Domain URLs

All detected URLs are from UniFFI code generation comments (github.com/mozilla/uniffi-rs) which is expected and acceptable.


Additional Notes

Cannot verify: Linting/compilation/tests cannot be run in review-only mode. Recommend running:

  • cargo clippy --all-targets --all-features -- -D warnings
  • cargo +nightly fmt --check
  • cargo test

Swift changes: The yttrium.swift file contains auto-generated UniFFI bindings (2283 additions) reflecting the Rust API changes - no manual review needed for generated code.

impl WalletConnectPay {
fn log(&self, message: &str) {
if self.debug_enabled.load(Ordering::Relaxed) {
println!("[WalletConnectPay] {}", message);
Copy link
Contributor

Choose a reason for hiding this comment

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

🤖 Auto Review Issue: Debug logging uses stdout instead of proper logging framework

Severity: MEDIUM
Category: code_quality
Tool: Claude Auto Review

Recommendation: Use the tracing framework for consistency:

fn log(&self, message: &str) {
    if self.debug_enabled.load(Ordering::Relaxed) {
        tracing::debug!("[WalletConnectPay] {}", message);
    }
}

fn log_error(&self, context: &str, error: &impl std::fmt::Debug) {
    if self.debug_enabled.load(Ordering::Relaxed) {
        tracing::error!("[WalletConnectPay] {}: {:?}", context, error);
    }
}

};

/// Parse a hex string (with or without 0x prefix) or decimal string to u64
fn parse_hex_or_decimal(s: &str) -> Option<u64> {
Copy link
Contributor

Choose a reason for hiding this comment

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

🤖 Auto Review Issue: Missing input validation for hex string parsing

Severity: LOW
Category: code_quality
Tool: Claude Auto Review

Recommendation: Add explicit validation for hex characters:

fn parse_hex_or_decimal(s: &str) -> Option<u64> {
    let s = s.trim();
    if s.starts_with("0x") || s.starts_with("0X") {
        let hex_part = &s[2..];
        // Validate hex characters explicitly
        if hex_part.chars().all(|c| c.is_ascii_hexdigit()) {
            u64::from_str_radix(hex_part, 16).ok()
        } else {
            None
        }
    } else {
        s.parse::<u64>().ok()
    }
}

client: Client,
config: SdkConfig,
cached_options: RwLock<Vec<CachedPaymentOption>>,
debug_enabled: AtomicBool,
Copy link
Collaborator

Choose a reason for hiding this comment

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

We already have a global logger, we don't need that boolean

Check registerLogger method, use it in Swift, it will log all messages from Yttrium

@llbartekll llbartekll changed the title Add logger Add logging Jan 6, 2026
@llbartekll
Copy link
Collaborator Author

@cloude review

@llbartekll llbartekll requested a review from jakubuid January 6, 2026 14:22
@llbartekll llbartekll merged commit 4196719 into main Jan 6, 2026
14 of 15 checks passed
@llbartekll llbartekll deleted the add-logger branch January 6, 2026 22:57
@github-actions github-actions bot locked and limited conversation to collaborators Jan 6, 2026
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants