refactor: Simplify Merkle CRDT workflow #2111
Conversation
fredcarle
added
area/crdt
fredcarle
changed the title
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## develop #2111 +/- ##
===========================================
+ Coverage 73.91% 73.97% +0.05%
===========================================
Files 249 247 -2
Lines 24859 24672 -187
===========================================
- Hits 18374 18249 -125
+ Misses 5223 5178 -45
+ Partials 1262 1245 -17
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 10 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
There was a problem hiding this comment.
Thank you for this Fred! Changes look good, it should be much easier to follow now, just added a couple of extra suggestions/minor questions but am happy for this to be merged regardless of whether they are actioned.
I suggest closing #917 - the removal of the factory stuff in particular will make this much easier to follow, and we can always create a new issue if it remains a problem error instead of cluttering the backlog.
|
Wow 👌 much cleaner. Not very well versed with this part of the code but LGTM from what I can see. |
| } | ||
|
|
||
| // Publishes the delta to state. | ||
| func (base *baseMerkleCRDT) Publish( |
There was a problem hiding this comment.
suggestion: I am hesitant to remove this Publish function as it keeps a cleaner seperation of concerns when writting MerkleCRDT delta-mutator functions.
The recipe is simple at the moment. Call the core crdt equivalent mutator, then call Publish(delta).
Even if Publish is just calling clock.AddDAGNode under the hood, it feels like its needlessly bleeding abstractions, and lose on some of the purity from before.
There was a problem hiding this comment.
The way I saw it is that when we called merkleCRDT.Set(...) and there was a call to Publish, we had to go to Publish only to find out that it was only calling clock.AddDAGNode and I found it obstructive more than anything. The way it was been changed makes the flow very clear. We set the CRDT delta object and then send that object to the clock for insertion into the DAG. Can't get purer than that if you ask me.
There was a problem hiding this comment.
Just noting that I agree with Fred here - I find the new, less-abstract, flow easier to read.
fredcarle
force-pushed
the
fredcarle/refactor/merkle-crdt
branch
3 times, most recently
from
a560a63 to
ba0975c
Compare
fredcarle
force-pushed
the
fredcarle/refactor/merkle-crdt
branch
from
ba0975c to
4f69bc3
Compare
## Relevant issue(s) Resolves sourcenetwork#2110 Possibly Resolves sourcenetwork#917 ## Description This PR aims to simplify the CRDT packages ahead of the new CRDT types.
## Relevant issue(s) Resolves sourcenetwork#2110 Possibly Resolves sourcenetwork#917 ## Description This PR aims to simplify the CRDT packages ahead of the new CRDT types.
Relevant issue(s)
Resolves #2110
Possibly Resolves #917
Description
This PR aims to simplify the CRDT packages ahead of the new CRDT types.
Tasks
How has this been tested?
make test
Specify the platform(s) on which this was tested: