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

Deprecate IntKey #503

Closed
wants to merge 4 commits into from
Closed

Deprecate IntKey #503

wants to merge 4 commits into from

Conversation

ueco-jb
Copy link
Contributor

@ueco-jb ueco-jb commented Oct 21, 2021

closes #472

@ueco-jb ueco-jb self-assigned this Oct 21, 2021
@ueco-jb ueco-jb force-pushed the 472-deprecate-int-key branch 3 times, most recently from cbab39f to 12010a4 Compare October 25, 2021 14:14
@@ -36,13 +36,9 @@ pub trait PrimaryKey<'a>: Clone {
type SuperSuffix: KeyDeserialize;

/// returns a slice of key steps, which can be optionally combined
fn key(&self) -> Vec<&[u8]>;
fn extend_key(&self, k: &mut Vec<u8>);
Copy link
Contributor

@maurolacy maurolacy Oct 25, 2021

Choose a reason for hiding this comment

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

This could be an option. Not so nice in terms of usability, as you basically need to build and pass a mutable vector by reference before calling this function.

I think that, before doing this, we should explore wrapping the return value in an associated type, as @hashedone suggested.

This would basically be a thin wrapper of either the reference to the value (for strings and slices) or the bytes of the integer representation. Given that the bytes are at the max 16 (for 128-bits integers) this would be acceptable in terms of performance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Actually I had some issues with such implementation - please check this changeset:
https://github.com/CosmWasm/cw-plus/pull/503/files/f8acad542c047d8e522c60dcb532cb75fe7a3c52
it wasn't done, but that was one of proof of concepts..

Problem was with types like String, because:

impl<'a> PrimaryKey<'a> for String {
...
    type KeyType = &'a [u8];

    fn key(&self) -> NamespacedKey<Self> {
        NamespacedKey {
            namespaces: vec![],
            key: &self,
        }
    }

    fn raw_key(&self) -> Self::KeyType {
        self.as_bytes()
    }
}
cannot infer an appropriate lifetime for lifetime parameter in function call due to conflicting requirements
first, the lifetime cannot outlive the anonymous lifetime defined on the method body at... 

etc.

Copy link
Contributor

Choose a reason for hiding this comment

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

@maurolacy - we actually went through associated types, but the problem is, that for this approach GATs are needed. There is no good way to express proper type for String basically. Using String itself would involve clone which is pointless, and for using &str it would need to have LT bound to &self, but for that GATs are necessary.

Copy link
Contributor

Choose a reason for hiding this comment

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

But string references worked before... Is this because of AsRef / traits requiring lifetimes? What about using / defining &'a self in the signatures?

The problem with extend_key as it is now, is that you are in the end just cloning self's bytes into it. For that, it would be better to just return those bytes by value I think.

Copy link
Contributor

@hashedone hashedone Oct 26, 2021

Choose a reason for hiding this comment

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

String worked before because of signature of key. And nothing else worked before (without prebuilding proxy type which is meh in terms of usability). Here if you have the associated type it cannot be LT-bound to self (as they cannot be generic so you cannot express such bound in place of defining type), but you want &'self str for string. What you need is:

trait PrimaryKey {
  type KeyType<'s>: AsRef<str>;
  
  fn key<'s>(&'s self) -> KeyType<'s>;
}

impl PrimaryKey for u32 {
  type KeyType = [u8; 4];
  
  // ..
}

impl PrimaryKey for String {
  type KeyType<'s> = &'s str;
  // ...
}

but it requires GATs

Copy link
Contributor

@maurolacy maurolacy Oct 26, 2021

Choose a reason for hiding this comment

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

I see... that's the syntax I was intending to mean, by the way.

I'm still not convinced it's not possible to do this in current Rust. What about using a wrapper type, defined outside of the trait, and which can act as a ref and a value? Something along the line of the types discussed in https://stackoverflow.com/questions/41146267/how-to-abstract-over-a-reference-to-a-value-or-a-value-itself?

Copy link
Contributor

@maurolacy maurolacy Oct 26, 2021

Choose a reason for hiding this comment

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

Both AsRef<[u8]> and Borrow<&[u8]> could perhaps be used. Nice thing about Borrow is that, though it has more requirements / restrictions than AsRef, the docs explicitly mention that it can be used for both, values and references: https://doc.rust-lang.org/std/convert/trait.AsRef.html

@ueco-jb ueco-jb force-pushed the 472-deprecate-int-key branch 3 times, most recently from 7d7d978 to 29b0453 Compare October 25, 2021 16:44
@ueco-jb ueco-jb force-pushed the 472-deprecate-int-key branch from 29b0453 to ec1f040 Compare November 11, 2021 12:59
@ueco-jb ueco-jb mentioned this pull request Nov 15, 2021
@ueco-jb
Copy link
Contributor Author

ueco-jb commented Nov 15, 2021

closing in favor of #547

In short, original idea would work really nice - if only GAT feature would be already part of Rust's stable release. Since it's not, we needed to figure out different approach.

@ueco-jb ueco-jb closed this Nov 15, 2021
@ueco-jb ueco-jb deleted the 472-deprecate-int-key branch December 22, 2021 09:50
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.

Deprecate IntKey
3 participants