Extend Addr implementation

[ethanfrey]

Using it in contracts and a few more nice to haves.
I will be extending this

1. is_empty() helper
2. AddrRef(&str) type that can easily be extracted from Addr (verified string reference). WIP here (https://github.com/CosmWasm/cosmwasm-plus/blob/401a2a7820486d537f0ff198439e4b1bebe6560a/packages/storage-plus/src/addr.rs#L1) but it would nice to implement Deref to easily go Addr -> AddrRef, not just AddrRef::from(&addr).

[webmaster128]

Why is_empty()? An Addr is always valid, so I don't understand this method

[webmaster128]

Regarding the other helpers in the file:

• as_str: the same as addr.as_ref() for an &Addr
• as_bytes: hmm, isn't this confusing and overly short in the context of addresses? This is the same as addr.as_ref().as_bytes() for an &Addr
• to_owned is the same as addr.clone() for an &Addr

We can add as_bytes if that helps but overall I don't see what we need another type for.

[webmaster128]

We can also create as_str which does the same as as_ref. I just wanted to implement AsRef<str> which makes the type a bit more powerful (for no particular reason). But I admit the method name is less beginner friendly.

[ethanfrey]

as_str() is definitely a better one to have. I would have both, and implement AsRef<str> via as_str()

I am not so much pushing to change Addr type, but mainly add the AddrRef type to the stdlib and some nice helpers to convert both ways. (And maybe some nice const fn helpers as you mentioned).

I think CosmWasm/cw-plus#260 shows some of the pain

[ethanfrey]

Why is_empty()? An Addr is always valid, so I don't understand this method

I guess not needed, I just needed to implement somewhere, but maybe I can revisit why it was even there.

[webmaster128]

I am not so much pushing to change Addr type, but mainly add the AddrRef type to the stdlib and some nice helpers to convert both ways.

Okay, my goal would rather be to make AddrRef obsolete. It should not need to exist because there is &Addr. Or it does something different and should have a different name.

[maurolacy]

I think the same. You either use & to get a reference to an Addr, or as_ref() to get a reference to its inner.

[ethanfrey]

I think you both miss the purpose of AddrRef. This was designed for the sole purpose of having an efficient but type-safe equivalent to &str for usage as Map keys in storage-plus.

You could never return &Addr from parse_key(serialized: &'a [u8]) -> Self as it is a reference. You could return AddrRef here around the serialized bytes (after asserting utf8).

I implemented this with Addr before, but that was heavy and forced heap copies everywhere. Your new implementation will force stack copies everywhere. Neither solve the fundamental issue: although &Addr -> &str is easy and cheap, &str -> &Addr is impossible (currently) or expensive (proposed constant api).

I just wanted better integration of a nice, light, im-memory reference with the original Addr to make usage less verbose.

[maurolacy]

If you want to use an Addr reference as key in a map, why not implement PrimaryKey for Addr directly, and then use &Addr in the map definition? Maybe I'm missing something, but what's the problem with that?

[maurolacy]

I've also noticed that using &str as key type seems to work well. Then you just do addr.as_ref() when building / accessing the map entry.

[maurolacy]

Maybe I'm missing something, but what's the problem with that?

Or do you want lifetimes compatibility?

Isn't there a way around it? Maybe I haven't hit the issue yet, but working with &str as key I didn't hit any lifetime issues.

[maurolacy]

My concern here is the same than in the PkOwned case (only this is a reference, not a value; which is probably worse): Instead of implementing needed traits over the types directly, we create a newtype and implement needed traits over it. Why? In fact, we're now considering removing PkOwned entirely.

[ethanfrey]

I implemented PrimaryKey for Addr, which required a clone just to use it as a key, which I felt as sub-optimal.

I used &str, but then it let me store unvalidated user input in the map as keys (also bad)

&Addr cannot be returned from a function, even if it has the same lifetime as &str input:

fn (some: &'a str) -> &'a str works

fn (some: &'a str) -> &'a Addr {
  &Addr(some.to_string()); 
}

is impossible as we return a reference to a local variable.

Thus AddrRef.

[ethanfrey]

Please try playing with storage-plus/src/keys.rs
I feel I have a very clear requirement from the code, and there is lots of discussion that could be avoided just by playing with the relevant code.

[maurolacy]

I implemented PrimaryKey for Addr, which required a clone just to use it as a key, which I felt as sub-optimal.

I used &str, but then it let me store unvalidated user input in the map as keys (also bad)

The alternative here is simple: convert unvalidated input to Addr using addr_validate, and then pass addr.as_ref() to the map with a &str key.

I understand this is less robust, but it's also less verbose.

I will play with this, to see if I can come up with another alternative.

[maurolacy]

I implemented PrimaryKey for Addr, which required a clone just to use it as a key, which I felt as sub-optimal.

This may be sub-optimal, but is what's always being done when working with values. This is a struct with a slice of less than a hundred bytes. I see no problem with cloning it once or twice in contract code. Not to talk about test code.

[maurolacy]

I implemented PrimaryKey for Addr, which required a clone just to use it as a key, which I felt as sub-optimal.
I used &str, but then it let me store unvalidated user input in the map as keys (also bad)

The alternative here is simple: convert unvalidated input to Addr using addr_validate, and then pass addr.as_ref() to the map with a &str key.

I understand this is less robust, ...

Currently our keys are typically &[u8]. That's not even utf8 safe.

[maurolacy]

I implemented PrimaryKey for Addr, which required a clone just to use it as a key, which I felt as sub-optimal.

This may be sub-optimal, but is what's always being done when working with values. This is a struct with a slice of less than a hundred bytes. I see no problem with cloning it once or twice in contract code. Not to talk about test code.

In any case, we can introduce these changes / improvemnts when and if we detect an issue or bottleneck with using values in keys. Or, as I said above, use &str to avoid a clone, and make sure addresses are validated in contract code.

[ethanfrey]

See the new map types

pub const PERMISSIONS: Map<AddrRef, Permissions> = Map::new("permissions");
pub const ALLOWANCES: Map<AddrRef, Allowance> = Map::new("allowances");

Once we start using addresses there, let's be safe about it.
Simon was even arguing I should be stricter there: CosmWasm/cw-plus#260 (comment)

I want to check if this use case is valid and understood by everyone.
If we do pub const PERMISSIONS: Map<&str, Permissions> = Map::new("permissions"); it is easier, but never enforces actually validating user input before storing it (dangerous).

And #872 would make this as easy to do with AddrRef as with &str as long as we start with an Addr

[maurolacy]

And #872 would make this as easy to do with AddrRef as with &str as long as we start with an Addr

And then we would have to wrap everything into an AddrRef, and deal with three types of "references" to addresses: &, as_ref() and now AddrRef.

It's your call, but I think that it tries to solve cases that are intrinsic to programing in general. Your AddrRef is a reference wrapped into a value. Nothing more, nothing less. If you want to treat (or benefit from) references as values, just use values, IMO.

[maurolacy]

That said, maybe with helpers and conversions, AddrRef syntax / usage is acceptable. I'm just saying that it looks overkill to me.

I'll give it a try during the weekend and comment back.

[maurolacy]

Finally, if this is going to be used, why not replacing Addr entirely with it, or with a version of it that combines both? And, call it Addr. And, if the current Addr still needs to be around, maybe it can be called AddrOwned or so?

In any case, let's try to have one Addr type that does what we want, it's easy to use, and is optimal / good enough / aceptably bad for most of our use cases.

And, let's stick to what the language already offers for dealing with references, inner references, lifetimes, etc.

Again, that's just my opinion, and I may very well be missing something here.

[webmaster128]

The biggest issue I have with this discussion is that it is not about the Addr type but about cosmwasm-plus storage design and requirements specific to that. Addr is an owned type which is in line with HumanAddr, CanonicalAddr and String. Now there is PrimaryKey in cosmwasm-plus which tries to

1. Serialize key into bytes without copying it
2. Deserialize borrowd bytes into the key type without copying it
3. Be super generic and extensible

Those requirements are very hard to archive. Given 1. and 2. there is not much you can do other than re-interpreting &[u8]. This is what e.g. impl<'a> PrimaryKey<'a> for &'a str does. What you can also do is renaming &str in order to add additional type safety. Let's say there is a new type 🦄StrRef which is basically a &str but by convention it only stores unicorn names. It can be implemented as easy as struct 🦄StrRef<'a>(&'a str); and be constructed using pub const fn unchecked(name: &'a str) -> Self; when the caller promises to only put in unicorns. And it turns out there is a type 🦄 which by chance can be converted to a &str so for convenvience there is also pub fn new(name: &'a 🦄) -> Self;. This does not make it a ref to 🦄 because it remains a ref to a string. Now why would 🦄 need to know about the 🦄StrRef type that only exists in order to get type-safe references to a str? The bidge type was only designed to support PrimaryKey and has properties that are not needed anywhere except for implementing the trait.

For this reason #872 is not an alternative to #869. Instead it upstreams a PrimaryKey specific str ref type.

In CosmWasm/cw-plus#264 and CosmWasm/cw-plus#265 I propose two different approaches that relax the requirements of PrimaryKey for types implementing it. With both of them, using the original Addr type directly becomes possible.

[webmaster128]

I don't know if you ever tried, but for the same reason as 👆implementing CosmWasm/cw-plus#256 or CanonicalAddr should be impossible without changing PrimaryKey first.

[ethanfrey]

Thank you for the thoughts. And I also agree that this logic needs to live with storage-plus.

I did impl PrimaryKey for Addr before, but then it was ugly due to a clone. I think you solve that in one of your PRs, I will review.

Also, I find #869 very heavy. I would extract a line or two from #872 to get just that type ConstAddr(&'static str) for example with impl From<ConstAddr> for Addr, and ConstAddr::unchecked(&str) constructor that can be used in test code setup.

Let me look at cosmwasm-plus PRs first

[webmaster128]

I'm happy to drop #869 if you think the cost (copies of 100bytes on the stack) is not worth the gain (simpler literals and no heap usage at all). But I would also like to avoid adding extra types just for test code.

Regarding to_owned: The ToOwned trait is a generalization of Clone to borrowed data.. This also means .clone() was there first and is the simpler variant of the same thing. We can implement ToOwned at any point if we either need it or prefer the method name.

[webmaster128]

Good, to sum up:

1. is_empty is not a reasonable thing to ask on a valid address
2. as_str is implemented in Add Addr::as_str #874
3. to_owned/ToOwned can be implemented whenever we have a good reason to. For now a simple .clone() is all we need.
4. The types string ref wrapper is specific to PrimaryKey/cosmwasm-plus and will either be maintained there or becomes obsolete via Implement PrimaryKey for &Addr by removing unused parse_key cw-plus#264 or Take storage keys as reference (K -> &K) cw-plus#265
5. We don't feel conviced about moving address data from the heap to the stack and close Make Addr::unchecked a const fn #869
6. as_bytes() is not explicit enough in the context of addresses. addr.as_str().as_bytes() should be used instead to make clear this is the UTF-8 data of the text.