Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 map integrity tests #1227
base: main
Are you sure you want to change the base?
Add map integrity tests #1227
Changes from 1 commit
bf6e3c4
c64de08
4b31abd
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
If you expect budget error, please add the specific assert for that. It would make test intent more clear.
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 does this return
Result
?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.
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 isOk
(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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 theEnvBase
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 returnOk
. This test is a live evidence of how misleading this is.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 understand the issue with
EnvBase
though, so I'm not sure how exactly we should address this.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'm not totally familiar with the structuring of the sdk, maybe @graydon or @leighmcculloch can chime in here.
The
EnvBase
has same as theguest::Env
here, where all functions have fallible interface butInfallible
error. TheEnv
functions are not directly exposed to the outside (unless via one of the public modules), but theEnvBase
functions are. So maybe the answer here is not to expose theEnvBase
methods, or expose only selected ones in other modules (and giving them infallible 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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opened https://github.com/stellar/rs-soroban-sdk/issues/1169