-
Notifications
You must be signed in to change notification settings - Fork 42
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
Move ScVal<>RawVal conversion to common #81
Conversation
Ugh, I got pretty lost doing this and turns out I don't need it. But maybe it is still useful for some other reason to get the RawVal <> ScVal conversion outside the Host? Thoughts @graydon? |
I think I'm ok with this .. need to study in a little more detail tomorrow, too wiped out tonight. |
} else if tag_static.is_type(ScStatic::LedgerKeyContractCodeWasm) { | ||
Ok(ScVal::Static(ScStatic::LedgerKeyContractCodeWasm)) | ||
} else { | ||
Err(ConversionError) |
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.
Unfortunately because these conversion can be used outside the host they can no longer return nice meaningful host errors. I don't see a way around this, so I'm going to press ahead with this, and maybe we can improve it somehow. Lmk if you see a way.
cc @graydon
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 put a lot of machinery for richer errors in EnvBase over in #243, maybe it's usable here?
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.
LGTM
What
Move ScVal<>RawVal conversion out of the host and into common.
Why
The host does not need to own conversion between these types as conversion between these types is mostly independent of the host. The host does need to be able to map objects into object handles though and the Host can provide an implementation of the trait for doing that.
While I was working on the CLI I need to be able to convert between these values, and so I at least need to make public the functions for doing the conversions. Moving them out into common felt natural.
Close #106
Known limitations
N/A