-
Notifications
You must be signed in to change notification settings - Fork 3.8k
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
feat(orm)!: ordered variable length encoding for uint32 and uint64 types #11090
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
7432f77
WIP on compact ordered varints
aaronc 495df9c
fixes and tests
aaronc 1c32408
docs
aaronc a4a6535
add manual test cases
aaronc 1800669
Merge branch 'master' of github.com:cosmos/cosmos-sdk into aaronc/orm…
aaronc 81f9c41
update golden file
aaronc e0b816e
Merge branch 'master' into aaronc/orm-varint
aaronc a76995d
revert
aaronc db3a82d
Merge branch 'master' into aaronc/orm-varint
mergify[bot] File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is it easy to make the encoding of N<2**32-1 numbers the same in both
CompactUint{32,64}Codec
? I feel like people will be confused if the same number is encoded in 2 different compact ways depending if it's uint32 or uint64.if we keep 2,4,6,9 for uint64, then only 2,4 modes will be valid for
CompactUint32Codec
. It will be less space-performant for sure, but maybe worth the mental overhead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hmm... these aren't really ever intended to be used by people. it's sort of like a database internal detail which you should only know if you're making a custom implementation. users should just need to know that the database made a performant and correct choice and have some guidance on when to use which data type to get which performance characteristics
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it's only database-internal. Storage keys relate to proofs, so IBC, UI client libs, light clients etc. Those developers will be re-implementing this, at least once per language.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
true... but not sure it's worth the compromise. it will mean large uin32's will need 6 bytes...