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

Multi index generic key #211

Merged
merged 35 commits into from
Mar 4, 2021
Merged

Multi index generic key #211

merged 35 commits into from
Mar 4, 2021

Conversation

maurolacy
Copy link
Contributor

@maurolacy maurolacy commented Dec 28, 2020

Now closes #163.

I was finally able to implement this, after some wresting with the Rust compiler, and a lot of learning.

Basically, following the same strategy as in #207. A custom deserialization function; which in this case is more complex, as it has to resolve the value indirection by going to the store.

This implementation imposes one restriction, or better said, has one requirement from the user: The MultiIndex definition must include a field for the primary key.

By example, if you want to (multi) index by a given byte field, instead of defining the MultiIndex just for &'a [u8] (or Vec<u8>), you must instead define it for (Vec<u8>, Vec<u8>); the last Vec<u8> is the placeholder for the primary key.

I think this restriction or requirement is reasonable, and I'll document this in the MultiIndex. It seems moving beyond this would require something like nested generics (using (K, Vec<u8>) internally), and this implies resolving their deserialization ambiguity (see #210).

Besides that, implementing this required passing a reference to Storage to the custom deserialization function; along with the pk namespace. I managed to resolve it by using function pointers, plus a couple "gluing" closures (and some parameter passing / carrying).

It would be nice to have something like currying implemented, in order to avoid having to pass around "fixed" (early defined) parameters (namely, the pk namespace), but my efforts in that direction didn't materialize. To the best of my knowledge, doing so would require using (capturing, therefore unsized) closures instead of function pointers, and wrestling either with references to opaque types (impl Fn and friends), or boxed (due to unsized) capturing closures.

Update: This requires #197, which was already locally merged. Better to review that first.

Update 2: Changed the base to unique-index-generic-key, as this draws on top of it.

Update 3: Only missing piece (if we agree on this, of course) would documenting the functionality / form of use in MultiIndex. Will be doing that soon.

@maurolacy maurolacy requested a review from ethanfrey December 28, 2020 08:10
@maurolacy maurolacy self-assigned this Dec 28, 2020
@maurolacy maurolacy changed the base branch from master to unique-index-generic-key December 28, 2020 08:16
Base automatically changed from unique-index-generic-key to master January 4, 2021 09:03
@ethanfrey
Copy link
Member

Sorry for not reviewing this yet.

I'm going to cut another (very minor) version with 0.13 of the sdk, then will review this more and we can include it in the larger release with cosmwasm 0.14 support.

When you have a chance, can you rebase and resolve conflicts (almost all in the Cargo.toml files, should be easy)

@maurolacy
Copy link
Contributor Author

maurolacy commented Jan 19, 2021

When you have a chance, can you rebase and resolve conflicts (almost all in the Cargo.toml files, should be easy)

Yes. In fact I have to rebase and use the new sub-prefix functionality here (if I recall correctly). Allow a few days for me to properly finish / test this.

@ethanfrey
Copy link
Member

Sure. No rush. I won't be doing much more work in this repo until cosmwasm 0.14-alpha (or better) is out

@maurolacy
Copy link
Contributor Author

Okay, merge was straightforward, and integrating the new functionality, too.
@ethanfrey, I think this is ready for review.

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Nice work and sorry it took so long to review.

I'd love to see if you could use &str or &[u8] for the user-provided data in these secondary indexes, so we can query with "Maria", not PkOwned("Maria".to_bytes().to_vec())

contracts/cw721-base/src/state.rs Show resolved Hide resolved
packages/storage-plus/src/indexed_map.rs Show resolved Hide resolved
@maurolacy
Copy link
Contributor Author

I'd love to see if you could use &str or &[u8] for the user-provided data in these secondary indexes, so we can query with "Maria", not PkOwned("Maria".to_bytes().to_vec())

We can implement Into to ease that. Or better, get rid of PkOwned entirely. But I would do any of these in a different PR, as this is already crowded with stuff.

Copy link
Member

@ethanfrey ethanfrey left a comment

Choose a reason for hiding this comment

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

Minor comments. Happy to see an attempt at using &[u8] in one just so you can create a proper issue for what compile errors we get.

Otherwise, good to merge

contracts/cw1-subkeys/Cargo.toml Outdated Show resolved Hide resolved
contracts/cw721-base/src/state.rs Show resolved Hide resolved
contracts/cw721-base/src/state.rs Show resolved Hide resolved
packages/storage-plus/src/indexed_map.rs Show resolved Hide resolved
packages/storage-plus/src/indexes.rs Show resolved Hide resolved
@ethanfrey
Copy link
Member

Looks good. I agree with your comments. There are 2 follow up issues further down the bakclog

@ethanfrey ethanfrey merged commit 3f682fe into master Mar 4, 2021
@ethanfrey ethanfrey deleted the multi-index-generic-key branch March 4, 2021 12:07
@maurolacy
Copy link
Contributor Author

maurolacy commented Mar 4, 2021

Happy to see this merged! 🎉

Created #233 and #234 as follow-ups.

@maurolacy
Copy link
Contributor Author

You beat me to it, closed #233 as a duplicate of #232.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support composite keys on secondary indexes (multi-index)
2 participants