-
Notifications
You must be signed in to change notification settings - Fork 1
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
Implement the LID #71
Conversation
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'm a bit confused which parts were already there, which are new and what was changed. I was trying to read it all, but gave up in the middle of rdb.rs
.
Should we separate the implementation of the Service trait somewhere, so we know what to look at? Maybe leave piecestore
by and only add the Service trait, have the refactor in a separate PR?
As well, are the tests covering the new Service trait, or are they old ones?
Should we add some kind of integration tests?
By the way, it looks like a huuuge piece of work with a lots of quirks and twists that you needed to parse and implement.
It's a pleasure to try to understand and read engineering done like that!
51cc15b
to
4b03270
Compare
…r test readability the error stack traces suffer a bit, but once an error happens, debug must ensue — it's a tradeoff
4b03270
to
f501816
Compare
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.
Hell of a job!
We established that integration tests will come later, when we add the RPC layer.
I digged through all of the code, don't see anything worth debating now.
If we see something wrong in the upper layers, then we'll fix it.
Let's go!
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.
Great work 💪
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.
Looks great! 🔥 💪
Description
This PR implements the LID component (currently, without the JSON-RPC) which supersedes the piecestore.
Fixes #33
Important points for reviewers
I've "over documented" the code with references back to the original code to shorten the time it takes for one to learn about the details and reasons behind certain choices being made.
There are some places were I've deviated from the original implementation, but hopefully those are sensible enough.
Checklist