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

multiindex key definition - footgun #83

Open
PFC-developer opened this issue Jul 15, 2024 · 4 comments · May be fixed by #84
Open

multiindex key definition - footgun #83

PFC-developer opened this issue Jul 15, 2024 · 4 comments · May be fixed by #84

Comments

@PFC-developer
Copy link

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
Copy link
Contributor

uint commented Jul 16, 2024

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
Copy link
Contributor

uint commented Jul 16, 2024

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
Copy link
Author

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

@Rhaki
Copy link

Rhaki commented Jul 17, 2024

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"
    ]
);

@uint uint linked a pull request Sep 12, 2024 that will close this issue
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 a pull request may close this issue.

3 participants