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

Unexpected behavior for nested tuple keys #59

Closed
CyberHoward opened this issue Oct 30, 2023 · 4 comments
Closed

Unexpected behavior for nested tuple keys #59

CyberHoward opened this issue Oct 30, 2023 · 4 comments

Comments

@CyberHoward
Copy link

CyberHoward commented Oct 30, 2023

Tuples that contain objects that themselves are composite objects cause issues during deserialization.

Example

Let's say we have a map like below:

let map: Map<((String, u16), String), u64> = Map::new("map");

Great, now we can use(String, u16), String) as a key. An example value:

let key = (("bitcoin".to_string(), 1), "bob".to_string());
// key.key():    [Ref([98, 105, 116, 99, 111, 105, 110]), Val16([0, 1]), Ref([98, 111, 98])]
// bin:          [0, 7, 98, 105, 116, 99, 111, 105, 110, 0, 2, 0, 1, 98, 111, 98]

Now there's a test-case that fails:

fn failure_case() {
    let mut deps = mock_dependencies();

    let map: Map<((String, u16), String), u64> = Map::new("map");
    let key = (("bitcoin".to_string(), 1), "bob".to_string());
    map.save(deps.as_mut().storage, key, &42069).unwrap();

    // This errors !
    let items = map
        .range(deps.as_ref().storage, None, None, Order::Ascending)
        .map(|item| item.unwrap())
        .collect::<Vec<_>>();
}

The resulting error is:

thread 'objects::account::account_id::test::key::failure_case' panicked at '`at` split index (is 25193) should be <= len (is 5)', library/alloc/src/vec/mod.rs:2099:13

This error is a result of the deserialization attempt of ((X,Y),Z).

The raw key that's attempted to deserialize (like showed above) is:

[0, 7, 98, 105, 116, 99, 111, 105, 110, 0, 2, 0, 1, 98, 111, 98]

In the deserialization logic the first element is assumed to be length-prefixed with the length for the complete element T where T is (X,Y).

impl<T: KeyDeserialize, U: KeyDeserialize> KeyDeserialize for (T, U) {
    type Output = (T::Output, U::Output);

    #[inline(always)]
    fn from_vec(mut value: Vec<u8>) -> StdResult<Self::Output> {
        let mut tu = value.split_off(2); 
        
        // t_len = 7
        let t_len = parse_length(&value)?;

        // Splits off "bitcoin" as if it's the full key (T) which should be (X,Y). I.e. ("bitcoin", 1)
        let u = tu.split_off(t_len);

        // T::from_vec fails as only "bitcoin" is provided and first to bytes are attempted to be used as length-prefix (bi = 25193), explaining the error.
        Ok((T::from_vec(tu)?, U::from_vec(u)?)) 
    }
}

We ran into this issue with a custom struct as part of a tuple key. So any tuple value T that has a Vec<Key> length != 1 is effected.

@CyberHoward CyberHoward changed the title Unexpected behavior for tuple keys Unexpected behavior for nested tuple keys Oct 30, 2023
@chipshort
Copy link
Collaborator

This was fixed in #34 and #36 already on main, but not released yet because the fix contains a breaking change to the KeyDeserialize trait.
If you need this immediately, you can change your dependency like this:

cw-storage-plus = { git = "https://github.com/CosmWasm/cw-storage-plus.git", rev = "cc67d423d1dc0dd5f7eeec6da26d7f0268388fd0" }

That will give you the current latest version from main.

@CyberHoward
Copy link
Author

CyberHoward commented Oct 31, 2023

Ah interesting! That sadly won't work for us as we want to publish our crates. We just swapped the tuple order so our nested key is the last element of the tuple.

Looking forward to 2.0 :)

Edit: Would have been helpful to add a warning to in the doc-comment of the implementation to warn users of the issue. Would have saved me from spending time on it.

@webmaster128
Copy link
Member

done in #65

@dzmitry-lahoda
Copy link

for those who will search, it gave me error about slice / array covert

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

No branches or pull requests

4 participants