Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Improve contract data #149

Merged
merged 6 commits into from
Jun 27, 2022
Merged

Improve contract data #149

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e37506e
Improve Env contract data interface
jonjove Jun 22, 2022
07e6c1b
Implement Env::has_contract_data
jonjove Jun 23, 2022
71d637d
Avoid using map_err in get_contract_data
jonjove Jun 27, 2022
f8f09e1
Merge branch 'main' into improve-contract-data
jonjove Jun 27, 2022
602e21c
Revert "Avoid using map_err in get_contract_data"
jonjove Jun 27, 2022
c796f4a
Merge branch 'main' into improve-contract-data
jonjove Jun 27, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions sdk/src/env.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,11 +48,21 @@ impl Env {
bin.try_into().unwrap()
}

pub fn put_contract_data<K: IntoTryFromVal, V: IntoTryFromVal>(&self, key: K, val: V) {
pub fn has_contract_data<K: IntoVal<Env, RawVal>>(&self, key: K) -> bool {
let rv = internal::Env::has_contract_data(self, key.into_val(self));
rv.try_into().unwrap()
}

pub fn get_contract_data<K: IntoVal<Env, RawVal>, V: IntoTryFromVal>(&self, key: K) -> V {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to narrow the scope of the interfaces I think you only need:

Suggested change
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

let rv = internal::Env::get_contract_data(self, key.into_val(self));
V::try_from_val(&self, rv).map_err(|_| ()).unwrap()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you need the map_err here?

Suggested change
V::try_from_val(&self, rv).map_err(|_| ()).unwrap()
V::try_from_val(&self, rv).unwrap()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Encountered

error[E0277]: <V as TryFrom<stellar_contract_env_host::EnvVal<env::Env, stellar_contract_env_host::RawVal>>>::Error doesn't implement Debug

but I now see there's a better solution. I just need the right trait bounds. Fixed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Never mind, this doesn't work with no-std. Reverting.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Debug exists in core, we use Debug elsewhere. It probably just means we haven't implemented the trait, we can do that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, what's going on here is not great and it can probably be fixed. Opened #161 to get this cleaned up.

Copy link
Member

@leighmcculloch leighmcculloch Jul 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@leighmcculloch leighmcculloch Jul 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See this example for how one way to fix the error: #162

}

pub fn put_contract_data<K: IntoVal<Env, RawVal>, V: IntoTryFromVal>(&self, key: K, val: V) {
internal::Env::put_contract_data(self, key.into_val(self), val.into_val(self));
}

pub fn del_contract_data<K: IntoTryFromVal>(&self, key: K) {
pub fn del_contract_data<K: IntoVal<Env, RawVal>>(&self, key: K) {
internal::Env::del_contract_data(self, key.into_val(self));
}
}
Expand Down