refactor: Change counters to support encryption #2698
Conversation
fredcarle
added
area/crdt
fredcarle
force-pushed
the
fredcarle/refactor/i2696-counters-as-bytes
branch
2 times, most recently
from
fc3e28a to
75f5522
Compare
| @@ -120,7 +111,7 @@ func (c Counter[T]) Value(ctx context.Context) ([]byte, error) { | |||
| // WARNING: Incrementing an integer and causing it to overflow the int64 max value | |||
| // will cause the value to roll over to the int64 min value. Incremeting a float and | |||
| // causing it to overflow the float64 max value will act like a no-op. | |||
| func (c Counter[T]) Increment(ctx context.Context, value T) (*CounterDelta[T], error) { | |||
| func (c Counter) Increment(ctx context.Context, value []byte) (*CounterDelta, error) { | |||
There was a problem hiding this comment.
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.
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.
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.
There are 3 ways of handling this.
- Use different deltas (generics) for every type
- Store the kind in the delta
- Pass the kind to
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 the Merge path from a P2P sync which doesn't have access to that strong type safety.
There was a problem hiding this comment.
yeah, looks like these is no perfect solution here. I'm fine then with keeping it as is.
fredcarle
force-pushed
the
fredcarle/refactor/i2696-counters-as-bytes
branch
from
75f5522 to
ae40ad6
Compare
fredcarle
force-pushed
the
fredcarle/refactor/i2696-counters-as-bytes
branch
from
ae40ad6 to
5b06bd5
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #2698 +/- ##
===========================================
+ Coverage 77.97% 77.98% +0.01%
===========================================
Files 308 308
Lines 23135 23125 -10
===========================================
- Hits 18038 18032 -6
+ Misses 3714 3711 -3
+ Partials 1383 1382 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 8 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Relevant issue(s)
Resolves #2696
Description
This PR changes the counter CRDTs to make them support the document encryption feature. They previously stored their values as concrete types (int64 and float64) instead of bytes. Storing them as bytes allow them to be stored plainly or encrypted.
Tasks
How has this been tested?
make test
Specify the platform(s) on which this was tested: