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.

Sign up for GitHub

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

Jump to bottom

Thoughts on keys #1676

Merged
merged 4 commits into from
May 3, 2023
Merged

Thoughts on keys #1676

merged 4 commits into from
May 3, 2023

Conversation

webmaster128
Copy link
Member

Explains the basics and history of the CosmWasm key storage layout and aims to claify a few confusing things for the future.

@webmaster128 webmaster128 mentioned this pull request Apr 28, 2023
Closed
chipshort
chipshort approved these changes May 2, 2023
View reviewed changes
Copy link
Collaborator

@chipshort chipshort left a comment

Choose a reason for hiding this comment

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

LGTM, nice to have a key prefix explanation in this repo now.
Only left a few small comments

docs/STORAGE_KEYS.md Outdated Show resolved Hide resolved
packages/std/src/storage_keys/length_prefixed.rs Outdated Show resolved Hide resolved
packages/storage/src/lib.rs Show resolved Hide resolved
packages/std/src/storage_keys/length_prefixed.rs Outdated Show resolved Hide resolved
maurolacy
maurolacy approved these changes May 3, 2023
View reviewed changes
Copy link
Contributor

@maurolacy maurolacy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM.

I understand this as the first step towards moving the storage keys functionality from cw-storage-plus to cosmwasm-std.

docs/STORAGE_KEYS.md
Given the importance of Length-prefixed keys for the entire CosmWasm ecosystem,
those implementations should be maintained in cosmwasm-std. The generic approach
allows building all sorts of storage solutions on top of it and it allows
indexers to parse storage keys for all of them.
Copy link
Contributor

Choose a reason for hiding this comment

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

👍🏼

packages/std/src/lib.rs Outdated Show resolved Hide resolved
@maurolacy maurolacy changed the title Thoughs on keys Thoughts on keys May 3, 2023
@webmaster128 @maurolacy
Update packages/std/src/lib.rs
46b4830 
Co-authored-by: Mauro Lacy <maurolacy@users.noreply.github.com>
@webmaster128
Copy link
Member Author

webmaster128 commented May 3, 2023 •
edited
Loading

I understand this as the first step towards moving the storage keys functionality from cw-plus to cosmwasm-std.

The motivation is a combination of the following:

  • Folks asked in the past for documentation of the storage layout. This is mostly for raw queries and indexers.
  • At some point the storage design was copied over to cw-storage-plus without a spec, risking it would diverge from the original design unnoticed.
  • The upcoming removal of cosmwasm-storage would remove those functions and the documentation/knowledge where it all came from.
  • We can upstream namespaces_with_key from cw-storage-plus which is a good thing to have when changing perspective from recursive to iterative building of the keys and maintaining namespace as a list by default
  • Recent discussions around cw-storage-plus keys motivated me to look closely again which parts of the stack are clear and where exactly the questions start
  • There are discussions to have alternatives to cw-storage-plus to meet certain performance and storage criteria. Especially DB keys getting large and usage of JSON due to the level of abstraction are challenged by advanced users. For this I'd like to have a solid base that can be reused while other parts are innovated on.

@webmaster128 webmaster128 merged commit f10e02d into main May 3, 2023
@webmaster128 webmaster128 deleted the thoughs-on-keys branch May 3, 2023 10:15
@webmaster128 webmaster128 mentioned this pull request Nov 12, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chipshort chipshort chipshort approved these changes

@maurolacy maurolacy maurolacy approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@webmaster128 @chipshort @maurolacy