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

Add DB generic type instead of dyn Trait to allow use the DB outside of the trie #206

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

AurelienFT
Copy link

Hello,

This crate is awesome and provides a lot of flexibility and cool features. Thanks for this very nice work.

When we used it, we had a problem with the way you hold the DB. In the TrieDBMut for example, you hold a mutable reference over the database that prevents us to have another reference in our code to do custom things such as modifying the DB content (in our case we save all changes that you make in the DB to allow user to rollback or rollforward his trie).

We can have a reference (mutable or not) to our DB using your method db() or db_mut() but it only give us the reference over something that implements HashDB that allows us to use only the methods defined in HashDB trait and not our custom ones.

This PR propose to fix this problem by replacing the dyn HashDB by providing a concrete generic type that needs to implement HashDB trait. This allow us to change db() and db_mut() to return our concrete type and be able to have fully access to the methods we created.

I would love to have your feedback on this. If it's something you already encoured and you have other solutions, I will be very happy to heard them.

Thanks again for your work

@cheme
Copy link
Contributor

cheme commented Jan 23, 2024

Thanks for the pr, sorry for only looking at this now.

I got no strong opinion about this, but it may be that dyn trait are favored here (cc @arkpar , @bkchr maybe).
For triedb I think it would be ok to keep two & ref to the db (one dyn and one not dy).

But yes for triedbmut it can be tricky to keep two &mut reference to the db. This forces either using short lived triedbmut instance (it is the case in substrate), or have inner mutability (eg Arc<Mutex> so we can clone it and have two &mut) which if not really elegant is very often the case on db (but may not be for in memory storage).

So I can see the point here, but would want others feedback on it.

@cheme cheme requested review from bkchr and arkpar January 23, 2024 09:10
@bkchr
Copy link
Member

bkchr commented Jan 23, 2024

I would argue that the current implementation is written around doing only single operations on the trie and then returning. This is the reason that things like the cache are living outside of the triedb and need to be passed as an argument.

@AurelienFT
Copy link
Author

Hello,
Thank you for your feedback guys.
About your comment @bkchr I don't fully understand, do you mean that we should build a tree and directly drop the structure and so we should never have a case where we need to use the DB outside of the structure?

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.

3 participants