Deprecate IntKey #547

ueco-jb · 2021-11-15T09:49:15Z

closes #472
Successor of #503.

Idea is to add very small wrapper, which takes either reference or small value (of limited number of bytes).
References should be used for any slice types, while refs for int types.
It's basically same approach as with IntKey, but instead of wrapping at beginning we do it at the end (in the result).

packages/storage-plus/src/keys.rs

maurolacy · 2021-11-15T10:00:50Z

packages/storage-plus/src/map.rs

+                .iter()
+                .map(|e| e.as_ref())
+                .collect::<Vec<_>>()
+                .as_slice(),


This can be removed I guess.

It couldn't, but I used simpler .map:

&k.key().iter().map(Key::as_ref).collect::<Vec<_>>(),

maurolacy · 2021-11-15T10:01:59Z

packages/storage-plus/src/prefix.rs

@@ -128,7 +129,7 @@ where

    pub fn with_deserialization_function(
        top_name: &[u8],
-        sub_names: &[&[u8]],
+        sub_names: &[Key],


Nice that (for now at least) this Key hasn't leaked to Map and friends.

I agree very much. I would try hard to hide that from contract developers, even if we use it everywhere in internal APIs. (And they can see it if they try to implement their own PrimaryKey type, but then we self-select for experienced devs)

ethanfrey

Looks good to me.

Left a few comments, but most are my surprise with the changes of the compiler, to auto-reference on eg. match a: &Option<X> and handling equals between [u8; 5] and &[u8].

Both nice and surprising ergonomic improvements. Happy for a link to the Rust upgrade that brought them.

ethanfrey · 2021-11-15T13:18:25Z

packages/storage-plus/src/de.rs

@@ -112,6 +112,22 @@ macro_rules! integer_de {

 integer_de!(for i8, u8, i16, u16, i32, u32, i64, u64, i128, u128);

+macro_rules! intkey_de {


Ah, cool.. just started commenting how we need both. Then see how we do.

Nice!

ethanfrey · 2021-11-15T13:20:31Z

packages/storage-plus/src/helpers.rs

@@ -20,15 +22,15 @@ pub(crate) fn may_deserialize<T: DeserializeOwned>(
    value: &Option<Vec<u8>>,
 ) -> StdResult<Option<T>> {
    match value {
-        Some(vec) => Ok(Some(from_slice(&vec)?)),
+        Some(vec) => Ok(Some(from_slice(vec)?)),


I think & used to be required by the compiler.
But now it is smarter?

Any idea when this changed?

No idea. One way to check it could be compiling with different versions, and see in which one this started to work. The match is producing a ref, and that ref is being converted to slice without any help. Not sure this wasn't supported earlier...

packages/storage-plus/src/keys.rs

ethanfrey · 2021-11-15T13:22:48Z

packages/storage-plus/src/keys.rs

+    Val8([u8; 1]),
+    Val16([u8; 2]),
+    Val32([u8; 4]),
+    Val64([u8; 8]),


Actually, how about adding Val128([u8; 16]) I am sure that sooner or later someone will want u128 as a key

Didn't implement it for 128 bits on purpose. Enums are tagged unions. So, they have the size of their largest element (plus an extra for the tagging).

Adding 128 bits support would double the size of Key to 16 bytes (plus the extra for tagging, which must end up in a 32-bits aligned boundary). So, 20 bytes instead of 12.

And, we are not using 128 int keys anywhere. We can easily add it, if you want. Or we can always add it later, when needed / requested.

ethanfrey · 2021-11-15T13:25:26Z

packages/storage-plus/src/keys.rs

+    }
+}
+
+integer_key!(for i8, Val8, u8, Val8, i16, Val16, u16, Val16, i32, Val32, u32, Val32, i64, Val64, u64, Val64);


Huh? I am a bit lost here with the macros, wouldn't it be:

integer_key!(i8, Val8);
integer_key!(u8, Val8);
integer_key!(i16, Val16);
integer_key!(u16, Val16);
...

Or that for i8 does something magic?

It's the for keyword yes. Sadly, with declarative macros we cannot take the type (i8), obtain the size (8), and build a token (Val8). That's why those are specified twice.

I've heard something like that can be done with procedural macros, though.

ethanfrey · 2021-11-15T13:27:29Z

packages/storage-plus/src/keys.rs

+            type SuperSuffix = Self;
+
+            fn key(&self) -> Vec<Key> {
+                vec![Key::$v(self.to_be_bytes())]


makes sense

packages/storage-plus/src/keys.rs

ethanfrey · 2021-11-15T13:29:16Z

packages/storage-plus/src/keys.rs

@@ -380,7 +491,7 @@ mod test {
        let k: K = "hello";
        let path = k.key();
        assert_eq!(1, path.len());
-        assert_eq!("hello".as_bytes(), path[0]);
+        assert_eq!(b"hello", path[0].as_ref());


Yes, the compiler has gotten smarter.

packages/storage-plus/src/keys.rs

ethanfrey · 2021-11-15T13:31:17Z

packages/storage-plus/src/prefix.rs

@@ -128,7 +129,7 @@ where

    pub fn with_deserialization_function(
        top_name: &[u8],
-        sub_names: &[&[u8]],
+        sub_names: &[Key],


I agree very much. I would try hard to hide that from contract developers, even if we use it everywhere in internal APIs. (And they can see it if they try to implement their own PrimaryKey type, but then we self-select for experienced devs)

maurolacy and others added 13 commits November 15, 2021 09:16

Add Key helper enum

af0be03

Adapt Map to key type

6a77a4a

Implement KeyDeserialize for naked int types

c1e1462

Implement PrimaryKey for u32

c2a288c

Change most prefix return types to represent new Key abstraction

3594c02

Try with new parameter types in helpers

18a48a9

Currently probably prefixed ranges doesn't work

d2c4e69

Fix joined_prefixes

7c4b4d7

Use shorter Key::as_ref

599b60d

Add naked int keys tests

d3fab98

Simplify test assertions

a086fc9

Simplify slice arg

db47152

Implement PrimaryKey / Prefixer for all integer types up to x64

6cdc823

ueco-jb assigned maurolacy and ueco-jb Nov 15, 2021

ueco-jb mentioned this pull request Nov 15, 2021

Deprecate IntKey #503

Closed

ueco-jb requested review from ethanfrey, hashedone and uint November 15, 2021 10:01

ueco-jb added the enhancement New feature or request label Nov 15, 2021

ueco-jb marked this pull request as ready for review November 15, 2021 10:02

ueco-jb force-pushed the 472-deprecate-int-key-3 branch from 3bed671 to 6cdc823 Compare November 15, 2021 10:02

maurolacy reviewed Nov 15, 2021

View reviewed changes

ueco-jb added 2 commits November 15, 2021 11:07

Add couple naked int tests for new Key implementation

38c6a58

Replace previous iter implementation on key type in Map

f0cfa5c

ueco-jb requested a review from maurolacy November 15, 2021 12:51

ethanfrey approved these changes Nov 15, 2021

View reviewed changes

Add extra tests for naked prefixes with new key type

61dbbd1

ueco-jb force-pushed the 472-deprecate-int-key-3 branch from bf9e961 to 61dbbd1 Compare November 15, 2021 14:54

ueco-jb mentioned this pull request Nov 15, 2021

Migrate from IntKey to new naked int key #549

Closed

ueco-jb merged commit 5b61f15 into main Nov 15, 2021

ueco-jb deleted the 472-deprecate-int-key-3 branch November 15, 2021 15:03

maurolacy mentioned this pull request Oct 17, 2022

Benchmark integer Key performance CosmWasm/cw-storage-plus#7

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate IntKey #547

Deprecate IntKey #547

ueco-jb commented Nov 15, 2021 •

edited by maurolacy

Loading

maurolacy Nov 15, 2021

ueco-jb Nov 15, 2021

maurolacy Nov 15, 2021 •

edited

Loading

ethanfrey Nov 15, 2021

ethanfrey left a comment

ethanfrey Nov 15, 2021

ethanfrey Nov 15, 2021

maurolacy Nov 15, 2021

ethanfrey Nov 15, 2021

maurolacy Nov 15, 2021

ethanfrey Nov 15, 2021

maurolacy Nov 15, 2021 •

edited

Loading

ethanfrey Nov 15, 2021

ethanfrey Nov 15, 2021

ethanfrey Nov 15, 2021

		@@ -112,6 +112,22 @@ macro_rules! integer_de {

		integer_de!(for i8, u8, i16, u16, i32, u32, i64, u64, i128, u128);

		macro_rules! intkey_de {

Deprecate IntKey #547

Deprecate IntKey #547

Conversation

ueco-jb commented Nov 15, 2021 • edited by maurolacy Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maurolacy Nov 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanfrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maurolacy Nov 15, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ueco-jb commented Nov 15, 2021 •

edited by maurolacy

Loading

maurolacy Nov 15, 2021 •

edited

Loading

maurolacy Nov 15, 2021 •

edited

Loading