Add map integrity tests #1227
Add map integrity tests #1227
Conversation
| let keys = ["a", "b", "c", "d", "e"]; | ||
|
|
||
| let res = host.map_new_from_slices(&keys, &vals.as_slice())?; | ||
| assert!(host.from_host_val(res.to_val()).is_err()); |
There was a problem hiding this comment.
If you expect budget error, please add the specific assert for that. It would make test intent more clear.
| } | ||
|
|
||
| // Same but with out of order keys | ||
| pub fn map_mem_bad(e: Env) -> Result<(), Error> { |
There was a problem hiding this comment.
Why does this return Result?
| // Same but with out of order keys | ||
| pub fn map_mem_bad(e: Env) -> Result<(), Error> { | ||
| // out of order | ||
| assert!(e.map_new_from_slices(&["b", "a"], &[1u32.into(), 2u32.into()]).is_err()); |
There was a problem hiding this comment.
Seems like almost every function in this Wasm is set up incorrectly. Host functions don't return errors in guest environment, they cause the contract to trap. Functions like map_new_from_slices should be changed in SDK to be infallible (probably open an issue for that?).
Besides that, functions that perform assert! check can only be meaningfully tested when the expected invocation result is Ok (e.g. map_mem_bad expects an error, so even if you could assert on host fns, your test would still pass if either first or second assertion fails).
There was a problem hiding this comment.
I agree with the assert part. The contract would've already trapped and the assert here does nothing (and the line after this will never get executed).
But I'm not sure about making map_new_from_slices and other functions infallible in the sdk. They are part of the EnvBase crate, and simply calls the host functions from the guest env. So I guess it doesn't really matter? If you have an opinion of what to do better in the sdk, can you create an issue there?
There was a problem hiding this comment.
But I'm not sure about making map_new_from_slices and other functions infallible in the sdk. They are part of the EnvBase crate, and simply calls the host functions from the guest env. So I guess it doesn't really matter?
It does matter, because it creates a false impression that these functions can return an Err result. They cannot though, as they either cause a trap, or return Ok. This test is a live evidence of how misleading this is.
There was a problem hiding this comment.
I understand the issue with EnvBase though, so I'm not sure how exactly we should address this.
There was a problem hiding this comment.
I'm not totally familiar with the structuring of the sdk, maybe @graydon or @leighmcculloch can chime in here.
The EnvBase has same as the guest::Env here, where all functions have fallible interface but Infallible error. The Env functions are not directly exposed to the outside (unless via one of the public modules), but the EnvBase functions are. So maybe the answer here is not to expose the EnvBase methods, or expose only selected ones in other modules (and giving them infallible signatures)?
There was a problem hiding this comment.
So maybe the answer here is not to expose the EnvBase methods, or expose only selected ones in other modules (and giving them infallible signatures)?
Yes, probably the least confusing thing to do would be to add a public wrapper over all the EnvBase functions and hide the fallible implementation, so that it can't be used by accident.
There was a problem hiding this comment.
What
Resolves #1148.