-
Notifications
You must be signed in to change notification settings - Fork 32
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
feat: atrium-repo
#272
base: main
Are you sure you want to change the base?
feat: atrium-repo
#272
Conversation
3b84377
to
f8e988e
Compare
f736ccc
to
b230066
Compare
loop { | ||
let node = Node::read_from(&mut bs, node_cid).await?; | ||
if !seen.insert(node_cid) { | ||
// This CID was already seen. There is a cycle in the graph. |
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.
Is it even possible to serialize a graph with a cycle?
The two nodes would point at each other, and by definition it should be impossible to compute their CIDs if there is a cycle.
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.
So I actually don't think this is possible, which means the extra validation logic here is probably optional and very unlikely to be hit in practice.
To err on the side of safety though, I think it would be best to leave this in-place.
6db3530
to
60adc4a
Compare
I think the PR is in a pretty good state for a MVP. |
Pinging @str4d since this builds on their prior 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.
I have written several comments on areas unrelated to the internal logic. There are also a couple of points from clippy that I hope you can correct as well.
5f55fc5
to
cb8e8c0
Compare
Note: It looks like there's a pretty exhaustive MST test suite here that could probably be leveraged to validate this implementation. |
b3afd8a
to
2ee7cba
Compare
9277356
to
465724d
Compare
@sugyan Alright, I think this code is ready to be reviewed and merged :) |
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 commented on a minor point regarding Cargo.toml
, README, etc.
There seem to be a few warnings caused by cargo clippy
and I'd like to see that fixed if possible.
I'd love to have @str4d review the implementation details if possible, but he's busy...?
This builds on str4d's work here: #168.
Big notes:
Repository
implementation with support for basic CRUD operationsAsyncBlockStoreRead
andAsyncBlockStoreWrite
.Test coverage (

cargo llvm-cov -p atrium-repo --html --open
):Node
Node
from bytesTreeEntry
s perNode
to a statistically unlikely maximum length.