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

Add Prefixer sub-prefixes support #214

Closed
maurolacy opened this issue Jan 6, 2021 · 7 comments
Closed

Add Prefixer sub-prefixes support #214

maurolacy opened this issue Jan 6, 2021 · 7 comments
Assignees

Comments

@maurolacy
Copy link
Contributor

maurolacy commented Jan 6, 2021

The current Prefixer trait has only a prefix() method that returns a "full prefix", i. e., for a given composite PrimaryKey, a prefix that is composed by all but the last element of the composite key.

When ranging over composite keys, we would like to be able to select the keys that are part of the (fixed) prefix. That way, we can range over, by example, all the ages related to a fixed name. As there can be multiple names with the same age, we would typically use a MultiIndex, disambiguating entries by (unique) primary key, plus, using a prefix that fixes only the name. That second part (using a prefix for only the first element of the key, in a triple key) is currently unsupported.

For some context, see #213, and #213 (comment) in particular.

Creating this issue to track possible implementations for this.

@maurolacy maurolacy self-assigned this Jan 6, 2021
@maurolacy
Copy link
Contributor Author

First idea that comes to mind is to add a prefix1() method to the Prefixer trait, and implement it for all the currently supported types.

Advantages:

  • Simple. Minimal modifications are required.

Disadvantages:

  • Maybe a little unclear (though that can be fixed by documentation).
  • Still requires passing the full K::Prefix argument to the prefix1() wrapper.

Variants / Improvements:

  • Add a subprefix() method, with an int value indicating desired number of prefix elements. May be difficult to implement in a generic way.
  • Add a PrefixSub1 type, and implement a prefix1() wrapper for it. This adds more clutter, but may be a good balance between usability and simplicity.

@maurolacy
Copy link
Contributor Author

A possible alternative would be to use (abuse) tuples notation, using nesting for indicating the number of desired (sub-)prefixes. By example, (T, (U, V)) would be a triple key, but with a one element prefix, whereas ((T, U), V) (or directly (T, U, V)) will still be a triple key, but with a 2-element prefix.

Advantages:

  • No need for extra methods.

Disadvantages:

  • May be difficult to implement / generalize.
  • Syntax is a little obscure / indirect.

@maurolacy
Copy link
Contributor Author

maurolacy commented Jan 6, 2021

@ethanfrey, what do you think? I can present a couple impls for this, and we can discuss them over the code if you like.

By the way, this is required for the MultiIndex impl (see #211), but only if we ever want to range() over sub-prefixes.
That was the original use case / motivation, but that use case has since been deprecated (when we moved group updates to the cw4-group contract).

So, this is low priority, and more of a learning exercise for me / for the sake of completeness stuff at this point.

@ethanfrey
Copy link
Member

Interesting issue. I am deep trying to get wasmd 0.14 out and our testnet updated.

When I have time to look over all the prefix stuff, and open/merged PRs I can give real feedback.
One first glance Using ((T, U), V), (T, (U, V)) or (T, U, V) in the definition to enable one particular prefix search seems interesting. I think (T, (U, V)) actually encodes the same as (T, U, V), but ((T, U), V) differently. But this is not a huge issue as long as you fix one before deploying the contract.

@maurolacy
Copy link
Contributor Author

maurolacy commented Jan 8, 2021

OK, I'll give the ((T, U), V) idea a shot.

When you have some time, please take a look at #215, where I've implemented a variant of the first approach, above.

@ethanfrey
Copy link
Member

ethanfrey commented Jan 8, 2021

OK, I'll give the ((T, U), V) idea a shot.

Please don't work more on this now. I need to review like 4 PRs of yours first.

@ethanfrey
Copy link
Member

Closed by #215

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

No branches or pull requests

2 participants