-
Notifications
You must be signed in to change notification settings - Fork 11
refactor: Hide lifetimes from external crates, support error cloning #145
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
Conversation
🟡 Heimdall Review Status
|
|
|
||
| #[derive(Debug)] | ||
| #[derive(Clone, Debug)] | ||
| pub enum Error { |
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.
It's very rare for errors to implement Clone, which is why most errors (in the std lib too) don't implement it. Is there a specific reason why we want them here? The PR description mentions "usability reasons", but I struggle to find places where errors are cloned instead of being simply propagated.
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.
This was introduced for seamless compatibility with Reth, as they appear to clone errors. I'll double check whether or not this is truly necessary
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.
Upon further review this mostly stems from Reth cloning Results (example) including the ProviderError
We don't really need to embed the raw TrieDB Error inside of the ProviderError, and can instead just stringify this to remove the need for error cloning.
| } | ||
| } | ||
|
|
||
| impl Database { |
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.
Instead of rewriting all the code to use self.inner. instead of self., I would suggest simply changing impl Database to impl DatabaseInner. The only methods that need to stay inside impl Database are the constructors (create and new). Everything else can be transparently delegated by Deref
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 believe this would require making DatabaseInner public, which we probably want to avoid
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.
Another option might be to make Transaction accept a generic DB which implements Deref<Database>, so that we can use &Database or Arc<Database> as needed.
Improve usability of TrieDB as a crate by making the following external-facing changes:
Arcof a database handleThis re-introduces
DatabaseInner, withDatabasesimply holding anArcof the inner struct.Replaces #108