CosmWasm / cw-storage-plus

Storage abstractions for CosmWasm smart contracts
Apache License 2.0
37 stars 27 forks source link

multiindex key definition - footgun #83

Open PFC-developer opened 4 months ago

PFC-developer commented 4 months ago

I am having an issue with CW-storage-plus. (thanks to Eris/Phil @0xPhilipp for pointing it out) I have managed to produce an example which demonstrates it here - testcase

I have a key which is a (u64,&str) .. and it works ok, until I put in a u64 > 128, and it then gives a UTF-8 error

how do I remedy this.

is there a way to do this without migrating the index in the existing smart contract the error returned is as follows

called `Result::unwrap()` on an `Err` value: InvalidUtf8 { msg: "invalid utf-8 sequence of 1 bytes from index 9" }
thread 'testing::tests::test_128' panicked at contracts/hub-tf/src/testing/tests.rs:2100:10:

this was due to a multiindex signature not being defined properly. the following patch to the test case resolves it.

the issue is here as I think this should have resulted in a compile error (the PK was badly defined in the original case), but it wasn't.

Thanks!

uint commented 4 months ago

Hi!

I think you're right; that's a footgun and it's begging for a better design. I'll see if we can do some type/trait magic to prevent things like that at compile time.

In the meantime, I want to note it might be worth watching storey as an alternative storage library. It currently doesn't have indexing built-in, but for simpler needs, I feel it's set up to leverage the Rust type system a little better. I'm biased though!

uint commented 4 months ago

is there a way to do this without migrating the index in the existing smart contract

I think I have good news. The committed indexing data doesn't seem to be influenced by the type of the PK (it just shoves the bytes blindly), so I think just changing that type is okay without migrating the data already stored.

That said, you could test this first. You could write a test that first saves a bunch of data using the unpatched IndexedMap, and then operates on it using the patched IndexedMap. Does that make sense?

PFC-developer commented 4 months ago

I'll check out storey.. And I tested the change on chain and it appeared to work

Rhaki commented 4 months ago

More than anything, the structure that represents the indexes, from the IndexMap point of view, only requires that the IndexList trait be implemented. It is practically impossible with the current design to generate a compilation error if the PK does not match the one actually used. I suggest using a macro like this to create an IndexMap

macro_rules! create_index_map {
    (
        $map_name:ident for $base:ident where PK: $pk:ident,
        pk_namespace: $pk_ns:expr,
        multi: [$($m_f:ident: $m_t:tt => $m_fn:expr, $m_ns:expr),*],
        unique: [$($u_f:ident: $u_t:tt =>$u_fn:expr, $u_ns:expr),*]
    ) => {

        paste::paste!{pub struct [<$base Indexes>]<'a>{
            $(pub $m_f: cw_storage_plus::MultiIndex<'a,  $m_t, $base, $pk>),*,
            $(pub $u_f: cw_storage_plus::UniqueIndex<'a, $u_t, $base, $pk>),*,
        }

        impl <'a> cw_storage_plus::IndexList<$base> for [<$base Indexes>]<'a> {
            fn get_indexes(&self) -> Box<dyn Iterator<Item = &'_ dyn cw_storage_plus::Index<$base>> + '_> {
                let v: Vec<&dyn cw_storage_plus::Index<$base>> = vec![$(&self.$m_f,)* $(&self.$u_f,)*];
                Box::new(v.into_iter())
            }
        }

        pub const $map_name: cw_storage_plus::IndexedMap<$pk, $base, [<$base Indexes>]> = cw_storage_plus::IndexedMap::new($pk_ns, [<$base Indexes>] {
            $(
                $m_f: cw_storage_plus::MultiIndex::new($m_fn, $pk_ns, $m_ns),
            )*
            $(
                $u_f: cw_storage_plus::UniqueIndex::new($u_fn, $u_ns),
            )*
        });}
    };
}

// --- Example of usage ---

#[derive(Serialize, Deserialize, Clone)]
pub struct Bar {
    pub name: String,
    pub surname: String,
    pub id: u64,
}

create_index_map!(
    // BAR will be the name of the IndexMap
    BAR for Bar where PK: u64,
    pk_namespace: "bar_ns",
    // Multi indexes, leave this empty if not needed
    multi: [
        // idx name: IK type  => closure, index namespace
        name: String =>  |_, data| data.name.clone(), "bar_name_ns",
        name_surname: (String, String) => |_, data| (data.name.clone(), data.surname.clone()), "bar_name_surname_ns"
    ],
    // Unique indexes, leave this empty if not needed
    unique: [
        id: u64 => |data| data.id, "bar_id_ns"
    ]
);