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

Constant Length Key Prefixes #242

Merged
merged 14 commits into from
Aug 2, 2022
Merged

Constant Length Key Prefixes #242

merged 14 commits into from
Aug 2, 2022

Conversation

shaspitz
Copy link
Contributor

@shaspitz shaspitz commented Jul 20, 2022

Closes #18

Prefixes and singular keys are now defined as byte constants, assigned with iota. Corresponding helpers returning []byte are defined in the same file as the byte constants.

Comments:

  1. This PR enforces the semantics of a "key"/"singular key" and a "key prefix" in some places. The former being a key associated to a single value (or a single sequence of values marshaled/unmarshaled together), while the latter is a prefix where further bytes are appended to the key to retrieve values.

  2. It may be worth standardizing on where/if we use separators at all. Update after discussions - Separators ("/") were removed wherever possible from existing key functions.

@shaspitz shaspitz marked this pull request as ready for review July 21, 2022 18:26
x/ccv/types/keys.go Outdated Show resolved Hide resolved
@sainoe
Copy link
Contributor

sainoe commented Jul 25, 2022

Good work @smarshall-spitzbart ! Just a few details.

I agree that we should decide if we use separators in keys.
I don't think that they're still needed since the prefixes and single keys have constant length now.

Any thoughts on this @danwt @jtremback ?

@danwt
Copy link
Contributor

danwt commented Jul 26, 2022

I think we should avoid separators if we can as they can just lead to bugs.

@shaspitz shaspitz requested a review from sainoe July 28, 2022 18:48
@shaspitz
Copy link
Contributor Author

All key separators have been removed as of latest commit

Copy link
Contributor

@mpoke mpoke left a comment

Choose a reason for hiding this comment

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

Great work @smarshall-spitzbart. See my comments below.

x/ccv/provider/types/keys.go Outdated Show resolved Hide resolved
x/ccv/provider/keeper/genesis.go Outdated Show resolved Hide resolved
x/ccv/provider/keeper/keeper.go Outdated Show resolved Hide resolved
@jtremback jtremback merged commit 6cb3c43 into main Aug 2, 2022
@jtremback jtremback deleted the byte-prefixes branch August 2, 2022 20:43
@shaspitz shaspitz mentioned this pull request Dec 14, 2022
4 tasks
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.

Variable length string prefixes could lead to injection attack
5 participants