-
Notifications
You must be signed in to change notification settings - Fork 0
fix: unsafe downcasts and clarification on transceiver message formats #5
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
base: main
Are you sure you want to change the base?
Conversation
crates/zkvm/src/lib.rs
Outdated
let journal = Journal::abi_decode(&info.journal.bytes)?; | ||
assert_eq!( | ||
from_wormhole_address(journal.emitterContract), | ||
from_wormhole_address(journal.emitterContract).unwrap_or(Address::ZERO), |
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.
Better to unwrap
or expect
here. This is a silent failure otherwise
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.
Roger, will revise!
crates/common/src/lib.rs
Outdated
} | ||
|
||
/// Converts a Wormhole format B256 address to an Ethereum Address. | ||
pub fn from_wormhole_address_align(wormhole_addr: B256) -> Result<Address, String> { |
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.
Should remove the pub
and possibly put this inside the tests module as this is the only place it is used
let (prefix, ints, suffix) = unsafe { garbage_bytes.align_to::<u32>() }; | ||
if prefix.iter().any(|&b| b != 0) | ||
|| ints.iter().any(|&i| i != 0) | ||
|| suffix.iter().any(|&b| b != 0) | ||
{ | ||
return Err(String::from("Malformed wormhole address")); | ||
} |
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.
This feels weird and not idiomatic to me. Not totally clear what this function is for
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.
This was the alternative function I was talking about from a "performance" perspective as the recommendation to check multiple bytes being zero was to align them into a larger data structure when doing research.
With 12 bytes, we have 3 u32
values, reducing the # of checks by a factor of 4 per cycle. If we think this isn't idiomatic/valuable to save on the cycles, we'll just remove it!
No description provided.