Improve contract data #149
Conversation
| rv.try_into().unwrap() | ||
| } | ||
|
|
||
| pub fn get_contract_data<K: IntoVal<Env, RawVal>, V: IntoTryFromVal>(&self, key: K) -> V { |
There was a problem hiding this comment.
If you want to narrow the scope of the interfaces I think you only need:
| pub fn get_contract_data<K: IntoVal<Env, RawVal>, V: IntoTryFromVal>(&self, key: K) -> V { | |
| pub fn get_contract_data<K: IntoVal<Env, RawVal>, V: TryFromVal<Env, RawVal>>(&self, key: K) -> V { |
However, narrowing the scope doesn't necessarily simplify this interface. If we had trait aliases I would alias both IntoVal and TryFromVal so that the generics were unnecessary, but unfortunately it isn't trivial.
There was a problem hiding this comment.
I considered this when I originally made this change, but decided it wasn't a great idea. My perspective was that you should probably never be storing values that you can't convert in both directions, and doing so is almost surely a bug. This doesn't apply for keys, since you always provide them.
|
|
||
| pub fn get_contract_data<K: IntoVal<Env, RawVal>, V: IntoTryFromVal>(&self, key: K) -> V { | ||
| let rv = internal::Env::get_contract_data(self, key.into_val(self)); | ||
| V::try_from_val(&self, rv).map_err(|_| ()).unwrap() |
There was a problem hiding this comment.
Why do you need the map_err here?
| V::try_from_val(&self, rv).map_err(|_| ()).unwrap() | |
| V::try_from_val(&self, rv).unwrap() |
There was a problem hiding this comment.
Encountered
error[E0277]:
<V as TryFrom<stellar_contract_env_host::EnvVal<env::Env, stellar_contract_env_host::RawVal>>>::Errordoesn't implementDebug
but I now see there's a better solution. I just need the right trait bounds. Fixed.
There was a problem hiding this comment.
Never mind, this doesn't work with no-std. Reverting.
There was a problem hiding this comment.
Debug exists in core, we use Debug elsewhere. It probably just means we haven't implemented the trait, we can do that.
There was a problem hiding this comment.
I think the issue here is you need to introduce a contraint somehow to make the TryFrom Error impl Debug. By default errors in the TryFrom trait impls aren't marked with any other requirement, they're completely open.
There was a problem hiding this comment.
Yeah, in general (this is a vague comment because rust trait-tetris is annoying and hard to do in my head and I can't tell exactly where it's going wrong) all the map_err I'm seeing in the code feels a little like .. a smell we're doing something wrong. The ? operator ought to be good enough in places we want errors, and a single .unwrap() or better .expect("explanation") ought to be enough where we want to panic. When it's not, it's a signal we've got our traits or impls messed up / missing somewhere.
There was a problem hiding this comment.
Agreed, what's going on here is not great and it can probably be fixed. Opened #161 to get this cleaned up.
There was a problem hiding this comment.
The issue is that TryFrom's Error type has no contraints. And it is impossible to know in this moment what type it is to know if it implements Debug. As it happens, all our implementations of TryFrom do have an Error that implement Debug, but the compiler can't know that all future types will also satisfy that.
There was a problem hiding this comment.
See this example for how one way to fix the error: #162
What
Clean-up the contract data interface and implement
has_contract_data.Why
The contract data interface is needed for CAP-54, so I'm cleaning it up as I use it.
Known limitations
None.