Skip to content
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(registers): expose MerkleReg and add register_inspect example #1310

Merged
merged 7 commits into from Feb 20, 2024

Conversation

happybeing
Copy link
Contributor

Description

Exposes the MerkleReg of RegisterCrdt in all Safe Register types to allow examination of the structure and history of changes to a register.

Adds a register_inspect example which displays nodes and their children for a register in a live network.

reviewpad:summary

//
// The only want to avoid unwanted creation of a Register seems to
// be to supply an empty wallet.
// TODO Follow the issue about this: https://github.com/maidsafe/safe_network/issues/1308

Check notice

Code scanning / devskim

A "TODO" or similar was left in source code, possibly indicating incomplete functionality Note

Suspicious comment
@joshuef
Copy link
Contributor

joshuef commented Feb 15, 2024

Thanks for this @happybeing! 🙇

This makesme wonder if merkle_reg is the best way to be exposing this. Do we want a clearer history API? perhaps? (merkle_reg feels a bit obscure.)

That said, maybe we want to still expose this at some level.

Hmm, I'm not sure. If you were approaching this task with no knowledge, what APIs might you have been looking for here? What were your first Qs before you got down to the merkle reg?

@happybeing
Copy link
Contributor Author

I changed the example from register_history which was my intention, to register_structure because having scratched my head over how to get a history I began to think that you can't do this easily, and that it probably requires work in the application logic. Or at least that you need to know what the structure of the data is and include some history feature in that.

I have since looked at @bochaco's new folders PR (using register with pointers to chunks) to see what he's doing because I'm still intellectually challenged by the register stuff.

Following that I came up with a design that would allow me to modify his approach and include a history, by adding a root node with the current folder entries as it's children, on every change. Accessing the history would then involve traversing back from the riot node (singular) to the previous one and do on, and when you find the one you want, getting the state of its children.

I think that works (and BTW I think Gabriel's design can be made to support hard and soft links now that directories are first class objects - but that's an aside).

I may still just not understand how to use Registers to get history but maybe Gabriel can explain how he would implement a history function for his new PR? If that's different to my approach I'd be very interested to learn.

@joshuef
Copy link
Contributor

joshuef commented Feb 15, 2024

riot node

I assume you mean root, but i love this.


Thanks for the background!

Yes registers are tough on the brainbox. I also am not super au fait with it either, which is what gives me pause about what APIs we want to expose (and with the merkle reg I think largely being an implementation detail?). So it's useful to dig into the practical use case...

So (if I attempt to paraphrase), an ideal API is one that can return the history of a given folder, at the moment?

Or: the history of a given entry ? Perhaps? Assuming that there could well be a fork in the register's entries that's as-yet unresolved?

Orr: something else?

@happybeing
Copy link
Contributor Author

Yes 'root' 😂

And yes, history could be any of those, and if my understand is correct, that's partly why it will be hard to hide the APIs on the MerkleReg without restricting valid options.

Another reason is different use cases. As I understand it, there's no way to pop out a history without a) knowing how the app structures is data in the register and probably also b) without the app implementing the structure in a way that enables the history to be accessed in a sensible form (if at all).

If I'm wrong we can easily take the lower level APIs out as higher level ones make them obsolete.

Until my approach is validated or contradicted I'm reluctant to push forward to implement it. So I'm hoping for some feedback about that.

@joshuef
Copy link
Contributor

joshuef commented Feb 15, 2024

Hmm, aye you're right, it probably is all more app layer and open to interpretation than I imagined. 🤔

In which case pulling out the merkle reg might be the only way to actually get into the details (including conflict resolution at the CRDT layer.. thats really app centric).

@joshuef
Copy link
Contributor

joshuef commented Feb 15, 2024

I think we can get this in for now.

But @happybeing we'll probably want to get some other APIs built out for this down the line (as we figure out how folk are going to use this), perhaps in place of exposing the full merkle-dag allowing some simple functionality atop it.

So if you're agreed can we document that this is an unstable API (at all layers), probably noting that intention to develop some more ergonomic APIS for common use cases to (probably) avoid exposing this down the line?

@happybeing
Copy link
Contributor Author

I was going to suggest that so 👍

When you say "we" do I need to modify the PR in some way?

@joshuef
Copy link
Contributor

joshuef commented Feb 19, 2024

When you say "we" do I need to modify the PR in some way?

Ahh, yes sorry, it twas the royal we as in The Project :D

So only change I'd look for is a note on the merkle_dag APIs to note that they are unstable. If you can get that in, then we can get this in 👍

@happybeing
Copy link
Contributor Author

@joshuef Is there a way to mark APIs as unstable?

So to use this you'd have to use --feature=unstable or something. If so, would you prefer the latter?

@joshuef
Copy link
Contributor

joshuef commented Feb 19, 2024

I'd just note in the docs on the function at the moment. Nothing more than that

@happybeing
Copy link
Contributor Author

@joshuef all done but the CI tests are broken I think.

@joshuef
Copy link
Contributor

joshuef commented Feb 19, 2024

Aye, seems like the chocolaty repo is down at the moment :(

Copy link
Contributor

@joshuef joshuef left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

changes look great, thanks @happybeing ! 🙇

@happybeing happybeing force-pushed the expose-register-structure branch 2 times, most recently from 0209e13 to 7ad8aaa Compare February 19, 2024 13:09
@happybeing
Copy link
Contributor Author

The only failure now seems to be a bug in the tests:

error: test failed, to rerun pass `-p sn-node-manager --test upgrades`
test upgrade_to_latest_version ... FAILED

failures:

failures:
    upgrade_to_latest_version

test result: FAILED. 0 passed; 1 failed; 0 ignored; 0 measured; 3 filtered out; finished in 3.73s

@happybeing
Copy link
Contributor Author

@joshuef phew, all done!

@joshuef joshuef added this pull request to the merge queue Feb 20, 2024
@joshuef
Copy link
Contributor

joshuef commented Feb 20, 2024

Excellent! 🙇

Merged via the queue into maidsafe:main with commit a7bc7fa Feb 20, 2024
55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants