-
Notifications
You must be signed in to change notification settings - Fork 79
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
EVM runtime: implement LOG{0..4} opcodes by emitting actor events (FIP-0049) #839
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mostly LGTM, just a couple of questions
Cargo.toml
Outdated
#fvm_ipld_bitfield = { git = "https://github.com/filecoin-project/ref-fvm" } | ||
#fvm_ipld_encoding = { git = "https://github.com/filecoin-project/ref-fvm" } | ||
#fvm_ipld_blockstore = { git = "https://github.com/filecoin-project/ref-fvm" } | ||
frc42_dispatch = { git = "https://github.com/filecoin-project/filecoin-actor-utils", branch = "raulk/events" } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are we really merging with all these crazy patches?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These particular ones were already there, but I do NOT wish to merge with ref-fvm patches. Those will be removed once filecoin-project/ref-fvm#1049 goes in and we make a release.
exit_code: ExitCode::OK, | ||
return_data: bytes, | ||
gas_used: fake_gas_used, | ||
events_root: None, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ruuuuuuust, thats when you extra hate this lang.
// Handle the data. | ||
// Passing in a zero-sized memory region omits the data key entirely. | ||
// LOG0 + a zero-sized memory region emits an event with no entries whatsoever. In this case, | ||
// the FVM will record a hollow event carrying only the emitter actor ID. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is this useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ethereum can record events with no topics nor data; the only useful thing is the emitter address in that case. Not sure how much it gets used in practice, but it is possible with that runtime. So not doing it here would break compatibility.
let mut entries: Vec<Entry> = Vec::with_capacity(num_topics + 1); | ||
for key in EVENT_TOPIC_KEYS.iter().take(num_topics) { | ||
let topic = state.stack.pop(); | ||
let entry = Entry { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to consider using Cow
for the key/value to allow borrowing. Kind of a pain, something we can probably handle later, but may make things faster.
Part of filecoin-project/ref-fvm#1048.
LOG{0..4}
EVM opcodes by emitting actor events.TODO
Prior to merging: