Skip to content

Comments

refactor(log): use alloy_primitives::Log#975

Merged
rakita merged 5 commits intobluealloy:mainfrom
slordua:alloy_log
Jan 15, 2024
Merged

refactor(log): use alloy_primitives::Log#975
rakita merged 5 commits intobluealloy:mainfrom
slordua:alloy_log

Conversation

@slordua
Copy link
Contributor

@slordua slordua commented Jan 13, 2024

No description provided.

Copy link
Collaborator

@gakonst gakonst left a comment

Choose a reason for hiding this comment

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

pls remove alloy_rlp and fmt per clippy/fmt lints

@slordua slordua requested a review from gakonst January 13, 2024 00:49
@slordua
Copy link
Contributor Author

slordua commented Jan 13, 2024

should be ready now

topics,
data,
data: LogData::new(topics, data)
.expect("Invalid LogData: Number of topics should be <= 4"),
Copy link
Member

Choose a reason for hiding this comment

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

Would refactor Host::log function input to a LogData .

in essence change this: https://github.com/bluealloy/revm/blob/main/crates/interpreter/src/instructions/host.rs#L214

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yea that's nicer

Copy link
Member

Choose a reason for hiding this comment

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

I didn't notice as the code was not changed but we could use Log directly in place of LogData. Other parts lgtm

Copy link
Contributor Author

Choose a reason for hiding this comment

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

could you clarify that last comment? not clear on what you're meaning

@slordua slordua requested a review from rakita January 13, 2024 16:56
/// Emit a log owned by `address` with given `topics` and `data`.
fn log(&mut self, address: Address, topics: Vec<B256>, data: Bytes);
/// Emit a log owned by `address` with given `LogData`.
fn log(&mut self, address: Address, log_data: LogData);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
fn log(&mut self, address: Address, log_data: LogData);
fn log(&mut self, log: Log);

Log contains both Address and LogData.

@slordua i meant this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah ok i see it thx, will revise shortly

@slordua slordua requested a review from rakita January 15, 2024 21:55
@slordua
Copy link
Contributor Author

slordua commented Jan 15, 2024

is .expect ok? topics shouldn't exceed 4 right? also wasn't sure if docs comment needed changing, still seemed appropriate to me

perhaps tho...

/// Emit a Log owned by address with given LogData.

would be better... idk

@rakita
Copy link
Member

rakita commented Jan 15, 2024

is .expect ok? topics shouldn't exceed 4 right? also wasn't sure if docs comment needed changing, still seemed appropriate to me

perhaps tho...

/// Emit a Log owned by address with given LogData.

would be better... idk

It is fine, lgtm

@rakita rakita merged commit fecb507 into bluealloy:main Jan 15, 2024
@github-actions github-actions bot mentioned this pull request Jan 15, 2024
@slordua slordua deleted the alloy_log branch January 15, 2024 23:25
This was referenced Mar 10, 2025
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.

4 participants