-
Notifications
You must be signed in to change notification settings - Fork 51
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
refactor: Change counters to support encryption #2698
Merged
fredcarle
merged 3 commits into
sourcenetwork:develop
from
fredcarle:fredcarle/refactor/i2696-counters-as-bytes
Jun 10, 2024
Merged
Changes from all commits
Commits
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
2 changes: 2 additions & 0 deletions
2
docs/data_format_changes/i2696-support-encryption-for-counters.md
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
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
# Support encryption for counters | ||
We changed the data format of counters from int64 and float64 to bytes to support encryption. This changes the generated CIDs for counters. |
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
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.
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.
question: is it really necessary to change the API? I feel like changing
CounterDelta
to hold[]byte
instead of a number is enough to enable encryption. I'd prefer having strong types till as late as possible.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 didn't like needing an exact duplicate of the structure for the two types we support for counters. It would be like having a duplicate of the lww delta for every type it supports. I have an idea that will keep the strong typing from
FieldValue
. I'll see if it actually works.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.
FieldValue
is only practical on save and quite inconvenient on merge.Maybe I can overlook the duplication in this case to help with simplifying the merge.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.
There are 3 ways of handling this.
NewCounter
1 and 2 are similar in that they both store type-related info in the data structure. 1 stores a differently named data structure and 2 stores extra bytes.
The disadvantage of 1 is that it requires maintaining more IPLD data structures and more types to support for
coreblock.Block
.The disadvantage of 2 is that it store extra bytes and given that we need the field definition anyways to create the
MerkleCounter
, it doesn't make a lot of sense to store those extra bytes.3 is equivalent to both 1 and 2 but depends on the field definition's kind that we pass along when we create the new counter. This is arguably the most type-safe way of doing things as it guarantees that we merge the proper type to the datastore on all code paths that call
Merge
.One could think that keeping the concrete type as long as possible in the
Increment
path would be beneficial for strong typing but this falls apart when we look at theMerge
path from a P2P sync which doesn't have access to that strong type safety.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.
yeah, looks like these is no perfect solution here. I'm fine then with keeping it as is.