Skip to content

Update logs.rs #10264

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

Closed
wants to merge 11 commits into from
Closed

Update logs.rs #10264

wants to merge 11 commits into from

Conversation

leopardracer
Copy link

No description provided.

@@ -269,7 +269,7 @@ mod tests {

#[test]
fn test_build_filter_sig_with_arguments() {
let addr = Address::from_str(ADDRESS).unwrap();
let addr = address!(ADDRESS);
Copy link
Member

Choose a reason for hiding this comment

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

could we update the function def instead? like instead of defining this as str we can just ADDRESS: Address = address!

@leopardracer leopardracer requested a review from mattsse April 7, 2025 17:06
@@ -269,7 +269,7 @@ mod tests {

#[test]
fn test_build_filter_sig_with_arguments() {
let addr = Address::from_str(ADDRESS).unwrap();
let addr = ADDRESS;
Copy link
Member

Choose a reason for hiding this comment

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

now these tmp vars are redundant and we can get rid of them

@leopardracer leopardracer requested a review from mattsse April 7, 2025 17:17
mattsse
mattsse previously approved these changes Apr 7, 2025
@@ -217,7 +218,7 @@ mod tests {
fn test_build_filter_basic() {
let from_block = Some(BlockNumberOrTag::from(1337));
let to_block = Some(BlockNumberOrTag::Latest);
let address = Address::from_str(ADDRESS).ok();
let address = Address::from_str(ADDRESS.to_string()).ok();
Copy link
Member

Choose a reason for hiding this comment

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

this is redundant

@leopardracer leopardracer requested a review from mattsse April 7, 2025 17:45
@jenpaff jenpaff moved this to Ready For Review in Foundry Apr 15, 2025
@leopardracer
Copy link
Author

@mattsse

@DaniPopes
Copy link
Member

DaniPopes commented Apr 29, 2025

This has no title, description, and is not a change of value

@DaniPopes DaniPopes closed this Apr 29, 2025
@github-project-automation github-project-automation bot moved this from Ready For Review to Done in Foundry Apr 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

3 participants