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.

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

Make pair extensible 2 #1106

Closed
wants to merge 7 commits into from
Closed

Conversation

maurolacy
Copy link
Contributor

This is a (breaking) version of Pair, which supports the natural order of fields. Closes #1100.

maurolacy and others added 2 commits September 27, 2021 10:35
Co-authored-by: Simon Warta <2603011+webmaster128@users.noreply.github.com>
@uint
Copy link
Contributor

uint commented Sep 27, 2021

I keep looking at these PRs and can't stop thinking that by the time we're doing type Pair<A, B> = (A, B), we might as well just write functions that operate on generic tuples directly.

Doesn't Pair<String, String> feel like an unnecessary complication over just (String, String)?

@maurolacy
Copy link
Contributor Author

I keep looking at these PRs and can't stop thinking that by the time we're doing type Pair<A, B> = (A, B), we might as well just write functions that operate on generic tuples directly.

Doesn't Pair<String, String> feel like an unnecessary complication over just (String, String)?

This is a good point. Pair is good for clarity, but so is (K, V). And default arguments are not so useful anymore, now that we need to specify the first to declare the second.

I would say, create a specific issue for it, and let's see how it looks.

@webmaster128
Copy link
Member

I keep looking at these PRs and can't stop thinking that by the time we're doing type Pair<A, B> = (A, B), we might as well just write functions that operate on generic tuples directly.

Doesn't Pair<String, String> feel like an unnecessary complication over just (String, String)?

Wise words. This is in line with my feeling that we try to do too many things with one symbol.

What if we leave Pair untouched and replace Pair2<X, Y> with (X, Y) in CosmWasm/cw-plus#432?

@maurolacy
Copy link
Contributor Author

What if we leave Pair untouched and replace Pair2<X, Y> with (X, Y) in CosmWasm/cw-plus#432?

OK with me.

@uint
Copy link
Contributor

uint commented Sep 27, 2021

What if we leave Pair untouched and replace Pair2<X, Y> with (X, Y) in CosmWasm/cw-plus#432?

Sounds pretty good!

@webmaster128
Copy link
Member

The term Pair emerged from KV as clippy started to enforce CamelCase names types. Should we rename it to samething more specific, such as (data base) Record? This seems to be the term used in https://en.wikipedia.org/wiki/Key%E2%80%93value_database

@maurolacy
Copy link
Contributor Author

maurolacy commented Sep 27, 2021

Record makes more sense. I agree that the usefulness of the type comes from having one fixed and one default arg; and one specific use case.

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

I strongly oppose changing the names to A and B, but rather keep K and V.

Using the natural ordering for types makes sense. Curious as to what level of contract breakage this makes (not a strong argument, but at least good to know). Can you do a brief check of the cw-plus/contracts for usages of Pair?

/// A Key-Value pair, returned from our iterators
pub type Pair<V = Vec<u8>> = (Vec<u8>, V);
/// A pair of values, returned from our iterators
pub type Pair<A = Vec<u8>, B = Vec<u8>> = (A, B);
Copy link
Member

Choose a reason for hiding this comment

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

I am strongly against using A and B here. This will only add to more dev confusion.

At least having some type hints would be good. I commented on the issue to the same effect.

@@ -101,7 +101,7 @@ fn range_bounds(start: Option<&[u8]>, end: Option<&[u8]>) -> impl RangeBounds<Ve
type BTreeMapPairRef<'a, T = Vec<u8>> = (&'a Vec<u8>, &'a T);

#[cfg(feature = "iterator")]
fn clone_item<T: Clone>(item_ref: BTreeMapPairRef<T>) -> Pair<T> {
fn clone_item<T: Clone>(item_ref: BTreeMapPairRef<T>) -> Pair<Vec<u8>, T> {
Copy link
Member

Choose a reason for hiding this comment

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

I see this as a cleaner approach than using <V, K> types, but also more breaking. (Switching the order of arguments to be (v, k) would be MUCH more breaking.

My question is how much does this break contract code. This will affect storage-plus, but if we update that (one hour maybe), then do we have to also update all the contracts that use range? Or is this just implicit info used by the compiler.

Copy link
Contributor Author

@maurolacy maurolacy Sep 27, 2021

Choose a reason for hiding this comment

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

I think it's just an implicit / type alias thing. Did the change to (K,V) in cw-plus #432 and it was pretty straightforward.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds good

@ethanfrey
Copy link
Member

I agree with the idea to leave Pair as it is now, and use (K, V) where both are defined.

I also searched for all usages of Pair in cw-plus contracts and only found 2. In cw721-base and cw1155-base:

fn parse_approval(item: StdResult<Pair<Expiration>>) -> StdResult<cw721::Approval> {
    item.and_then(|(k, expires)| {
        let spender = String::from_utf8(k)?;
        Ok(cw721::Approval { spender, expires })
    })
}

I assume this is very seldom used by other contract devs, or outside of storage-plus really.

Feel free to rename to Record if you wish. And let's just have a new type in storage-plus

@ethanfrey
Copy link
Member

I guess time to close this, along with #1101?

@webmaster128
Copy link
Member

Thank you for the great discussion and inputs from everyone!

@webmaster128 webmaster128 deleted the 1100-make-pair-extensible-breaking branch September 27, 2021 14:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generalize std::Pair
4 participants