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.