-
Notifications
You must be signed in to change notification settings - Fork 336
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
Add Cache::analyze #736
Add Cache::analyze #736
Conversation
12645ca
to
2ff38be
Compare
2ff38be
to
bd0ee68
Compare
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.
Nice stuff
"ibc_packet_timeout", | ||
]; | ||
|
||
pub fn deserialize_wasm(wasm_code: &[u8]) -> VmResult<Module> { |
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.
nice to move these helpers into this file
}) | ||
} | ||
|
||
pub fn exported_functions(module: &Module) -> HashSet<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.
This was a bit confusing at first read - parity_wasm::Module
(which is just parsed wasm) vs. wasmer::Module
(which is JITed native code and a lot heavier). I don't see a way to improve, but nice to keep the parity_wasm type in this file and not in files that might refer to the wasmer::Module
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.
Yeah that was the idea: try keeping the different Modules in different parts of the code
|
||
/// Returns true if and only if all IBC entry points ([`REQUIRED_IBC_EXPORTS`]) | ||
/// exist as exported functions. This does not guarantee the entry points | ||
/// are functional and for simplicity does not even check their signatures. |
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.
fair enough. if they use the #[entry_point]
directive, the compiler asserts the function signature. If someone uploads garbage, it will bind a port and fail on usage. Same as if they implement the proper signature and panic
there. This is not a security hole and better to just use a good approximation that works for all "proper" contract developers
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.
Nice work.
let mut ok = true; | ||
for required_export in REQUIRED_IBC_EXPORTS { | ||
if !available_exports.contains(*required_export) { | ||
ok = false; |
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.
break;
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.
I thought about this as well, but it is only 6 elements.
We could remove the ok
variable completely by returning early from the function 🤔
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.
Yeah, I guess return false
would be cleaner.
Not important for efficiency, but possible cleanup pr
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.
But what we really want is https://doc.rust-lang.org/std/iter/trait.Iterator.html#method.all
This is a proof of concept implementation, which is not ready to be merged.