Remove ConversionError from ScVal/Val conversions #1339
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jayz22
approved these changes
Jan 31, 2024
sisuresh
approved these changes
Jan 31, 2024
sisuresh
reviewed
Jan 31, 2024
github-merge-queue bot
pushed a commit
to stellar/rs-soroban-sdk
that referenced
this pull request
Feb 2, 2024
This is the SDK companion to stellar/rs-soroban-env#1339 -- it won't work until that change has landed and its refs have updated to point to it. There's an explanation of the change over there, but briefly: we were discarding meaningful errors when passing through the `TryFromVal<ScVal> for Val` and `TryFromVal<Val> for ScVal` impls in env-common, and now (or at least if 1339 lands) we're not. Instead of returning `ConversionError` we'd be returning `Error`. The SDK happens to _also_ have conversions with `ConversionError` signatures, and I've opted in this PR to _not_ go a step further and actually change those to `Error`, instead re-burying the `Error` into `ConversionError` in the SDK, keeping the SDK APIs as they are. I could also change them. But I figured that could be left to @leighmcculloch's choice; he might want to keep the SDK's error repertoire simpler, I'm not sure.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This is as minimal a change as I could manage to remove
ConversionErrorfrom the ScVal/Val conversion paths, allowingErrorcodes 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