too much use of ConversionError #1046
Labels
bug
Something isn't working
Comments
|
I've added a few of these to my code-review PR but only the easy ones that don't cause much SDK breakage. I've pushed a subsequent one that does cause a lot of SDK breakage to https://github.com/stellar/rs-soroban-env/tree/bug-1046-conversionerror-cleanup which we should only revive if we find time, it's quite disruptive downstream. Luckily (?) it's only xdr conversion which is only enabled in native testing mode, not wasm contracts or as part of the env interface. |
github-merge-queue bot
pushed a commit
to stellar/rs-soroban-sdk
that referenced
this issue
Sep 9, 2023
) This contains small fixes to adapt to [changes to the env found during code review](stellar/rs-soroban-env#1044) to eliminate easy-to-adapt-to uses of ConversionError (as described in bug stellar/rs-soroban-env#1046) where we can do better and pass a full Error.
github-merge-queue bot
pushed a commit
that referenced
this issue
Oct 18, 2023
Nothing deeply complicated in here, just a bunch of error plumbing, error code and check consolidation, double checks of internal logic, renames and comment gardening. The most annoying parts of this PR are the bad errors we get from "anything going wrong during ScVal conversion" which is already noted as bug #1046 which I would love to fix but we probably don't have time; or at least last time I tried I gave up because it required too much SDK proc-macro hacking and error routing. Maybe I can try again sometime?
github-merge-queue bot
pushed a commit
that referenced
this issue
Jan 31, 2024
This is as minimal a change as I could manage to remove `ConversionError` from the ScVal/Val conversion paths, allowing `Error` codes to flow through (eg. invalid inputs or, more importantly, budget exhaustion or the like). This _is_ observable, and it _does_ break public API (contra semver) so .. we should be sure if we want to do it. It also has a corresponding SDK change I'll post shortly. But I think it's very worthwhile and one of the last bugs I really wanted to fix before locking things down. Didn't think we'd be able to, but it looks like enough got cleaned up in the meantime that it was possible. Fixes #1076 Fixes #1046
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
and others
There are a lot of
TryFromValimpls in env-common that usetype Error = ConversionErrorwhere they really ought to usetype Error = crate::Errorand plumb through the env's error. In the current situation, all host errors get collapsed intoConversionErrorwhich then gets converted up toError(ScErrorType::Value, ScErrorCode::UnexpectedType)which is really not what a lot of host errors are. This is a very misleading way of learning you ran out of gas in the middle of a conversion, for example.Unfortunately these signatures are deeply intertwined with code all through the host and, especially, the SDK; if we change them we break a ton of downstream stuff. It would be good to coordinate a change to these someday. They were set to ConversionError for expedience, with merging the error types left as future work during the last round of reform to TryFromVal and we've just not found time to go back and finish the job.
The text was updated successfully, but these errors were encountered: