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

Use Maps for storage #109

Merged
merged 25 commits into from
Oct 10, 2020
Merged

Use Maps for storage #109

merged 25 commits into from
Oct 10, 2020

Conversation

ethanfrey
Copy link
Member

@ethanfrey ethanfrey commented Oct 7, 2020

This pulls out a lot of the ground-work from #108 without the actual indexes.

Shows another approach instead of Buckets. Code is basically done. Time for feedback and polish

  • Refine API
  • More unit test
  • Proper README
  • Use in some contract

I will add a refinement of the Range queries in a follow up PR and add the docs for them and contract integrations there.

@ethanfrey
Copy link
Member Author

ethanfrey commented Oct 7, 2020

This is heavily WIP, but I would like a first review on the public API.
I will be improving some of the internals, and adding more functions, but the public API is ready for review.

Example:

// note these don't need 2 functions to define (like Buckets), but one compile-time `const`
const PEOPLE: Map<&[u8], Data> = Map::new(&[b"people", b"_pk"]);
const ALLOWANCE: Map<(&[u8], &[u8]), u64> = Map::new(&[b"allow", b"_pk"]);

fn demo() -> StdResult<()> {
    // usage for simple keys - either caching the path, or just generating it on the fly
    let john = PEOPLE.key(b"john");
    john.save(&mut store, input)?;
    let data = PEOPLE.load(&store, b"john")?;

    // auto-generate usage for complex (composite) keys
    let allow = ALLOWANCE.key((b"owner", b"spender"));
    allow.save(&mut store, &1234)?;
    let amount = allow.load(&store)?;
    
    // update, using keys inline
    let add_ten = |a: Option<u64>| -> StdResult<_> { Ok(a.unwrap_or_default() + 10) };
    ALLOWANCE.update(&mut store, (b"owner", b"spender"), add_ten).unwrap();

    // with iterator feature, we allow range queries
    // when the key is &[u8] and prefix (), we special-case it and provide it directly
    let people_iter = PEOPLE.range(&store, None, None, Order::Ascending);
    let allow_iter = ALLOWANCE.prefix(b"owner").range(&store, None, None, Order::Ascending);
}

Compare usage with Bucket (ignoring the long definitions for now)

    // current cosmwasm-storage usage is (with hand-written functions):
    let data = people_read(&store).load(b"john")?;

    // now we can use helpers and just:
    let data = PEOPLE.load(&store, b"john")?;

@ethanfrey ethanfrey mentioned this pull request Oct 7, 2020
@ethanfrey ethanfrey requested a review from maurolacy October 8, 2020 08:41
@maurolacy
Copy link
Contributor

Hi, will review asap.

@ethanfrey
Copy link
Member Author

This should be ready to merge from my side. I will add more improvements in future PRs - like adjusting the Range interface, adding secondary indexes, and maybe performance/code improvements.

I would like to merge this in piece by piece as soon as it is high enough quality with no obvious trouble in what is implemented.

I would love feedback on the README, which should give a nice overview of how to use the code.

@ethanfrey
Copy link
Member Author

Merging now, more coming soon.

Feel free to comment on this and ping me if wished. Or just raise issues. I will address some points in future PRs.

@ethanfrey ethanfrey merged commit 6ff131d into master Oct 10, 2020
@ethanfrey ethanfrey deleted the storage-maps branch October 10, 2020 19:29
Copy link
Contributor

@maurolacy maurolacy left a comment

Choose a reason for hiding this comment

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

Looks good (though relatively complex) to me.

A general comment: It strikes me that what you're implementing is basically tantamount to a file system. A namespace defines a particular fs, prefixes are equivalent to directories, and keys are files; which contain (structured) data.

This is out of my area of expertise, as I've never had to deal with an implementation at this level. But I can't help but wonder if taking a look at actual fs implementations, design decisions, tradeoffs, etc. would be of any help.

By example: Maybe making the "prefixes" (directories) a different / specific data structure, containing data of its contents, could be of help, and be really useful for (sub)listing / selection / sorting.

Update: Will continue reviewing and analysing during the day, posting single comments if I find something else.

@@ -17,6 +17,7 @@ workflows:
- package_cw3
- package_cw20
- package_cw721
- package_storage_plus
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is a really good idea / approach in general. That is, to separate a working impl from new ideas / concepts, to try and test them first.

packages/storage-plus/README.md Show resolved Hide resolved
packages/storage-plus/README.md Show resolved Hide resolved
packages/storage-plus/README.md Show resolved Hide resolved
packages/storage-plus/README.md Show resolved Hide resolved
packages/storage-plus/README.md Show resolved Hide resolved
packages/storage-plus/README.md Show resolved Hide resolved
packages/storage-plus/README.md Show resolved Hide resolved
/// Customization of namespaces_with_key for when
/// there are multiple sets we do not want to combine just to call this
pub(crate) fn nested_namespaces_with_key(
top_names: &[&[u8]],
Copy link
Contributor

Choose a reason for hiding this comment

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

Probably not so efficient, but this could be resolved with multiple calls to to_length_prefixed().

Copy link
Member Author

Choose a reason for hiding this comment

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

Exactly, I want to make this more efficient. I will trim all these (internal) functions down at the end to one or two that do what we need.

packages/storage-plus/src/map.rs Show resolved Hide resolved
@ethanfrey
Copy link
Member Author

I'm going to make a PR using much of this cleanup, and improve naming, but I want to get the Range PR in first. Then do the improvements. Can you please take a quick look at the API there?

A number of devs are hitting issues as the last released version of much of the code doesn't match the last wasmd release, so I want to get a 0.3.0 tag out soon, which for me requires finalizing these interfaces (even if I add more later, not to break them)

@maurolacy
Copy link
Contributor

maurolacy commented Oct 12, 2020

I'm going to make a PR using much of this cleanup, and improve naming, but I want to get the Range PR in first. Then do the improvements. Can you please take a quick look at the API there?

I'm on it right now.

Update: Sorry to block you, by the way. In any case, just merge stuff and I can always review it later if you want, and add my comments.

@ethanfrey
Copy link
Member Author

Sure. I do appreciate your reviews and they push me to make cleaner code and better APIs.

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 this pull request may close these issues.

2 participants