Skip to content
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

feat(tracing): add unmatched parameter to CallLog #181

Closed
wants to merge 2 commits into from

Conversation

topocount
Copy link

This allows for the rendering of unmatched logs as errors in traces
as discussed in foundry-rs/foundry#8506

This allows for the rendering of unmatched logs as errors in traces
Copy link
Contributor

@klkvr klkvr left a comment

Choose a reason for hiding this comment

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

I'd prefer this as configurable style key which foundry can set to AnsiColor::Red. Unmatched events logic is very foundry-specific, so don't think we should inroduce it here

@topocount topocount requested a review from klkvr August 20, 2024 12:56
@topocount
Copy link
Author

topocount commented Aug 20, 2024

@klkvr I just saw your review on foundry-rs/foundry#8506 and still need to add state and logic here for highlighting params. Can we make param highlighting a separate PR in both repos?

@klkvr
Copy link
Contributor

klkvr commented Aug 20, 2024

@klkvr I just saw your review on foundry-rs/foundry#8506 and still need to add state and logic here for highlighting params. Can we make param highlighting a separate PR in both repos?

sure! feel free to get the core logic working first

though I think we can avoid a separate PR for styling params. They are stored as just Strings, so we can style them directly on foundry level and just put styled param names and values into DecodedCallLog::params

@@ -164,12 +175,19 @@ pub struct CallLog {
pub raw_log: LogData,
/// Optional complementary decoded log data.
pub decoded: DecodedCallLog,
/// A style config that allows for setting styles or error
/// state externally
pub style: LogStyle,
Copy link
Contributor

Choose a reason for hiding this comment

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

let's move this field to decoded

/// portable representation of call Log Styling
#[derive(Debug, Default, PartialEq, Eq, Clone)]
#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
pub enum LogStyle {
Copy link
Contributor

Choose a reason for hiding this comment

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

this is fine for now, but I think we might want to give more flexibility here by keeping some anstyle type here

we'd need to figure out serde for it, so can be done in a follow up

@@ -156,6 +156,17 @@ pub struct DecodedCallLog {
pub params: Option<Vec<(String, String)>>,
}

/// portable representation of call Log Styling
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// portable representation of call Log Styling
/// Portable representation of call log styling

self.write_branch()?;

if let Some(name) = &log.decoded.name {
write!(self.writer, "emit {name}({log_style}")?;
write!(self.writer, "emit {log_name_style}{name}{log_name_style:#}({log_param_style}")?;
Copy link
Contributor

@klkvr klkvr Aug 20, 2024

Choose a reason for hiding this comment

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

hmm, after a second thought we should be able to avoid touching this at all? we can just style log.decoded.name in foundry and get the same effect I think?

Copy link
Author

Choose a reason for hiding this comment

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

This makes sense. As for the cases where topic 0 or the entire abi-encoded event are styled, those are unreachable because emitting an expected event to watch for supplies the abi, correct?

@zerosnacks
Copy link
Contributor

Hi @topocount thanks for you PR, would be great to get this in - do you see an opportunity to implement the feedback?

@topocount
Copy link
Author

Hi @topocount thanks for you PR, would be great to get this in - do you see an opportunity to implement the feedback?

@zerosnacks based on responses from @klkvr on foundry-rs/foundry#8506 i think this can be closed but im not completely sure

@topocount topocount closed this Sep 26, 2024
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