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

Refactor PMMR read methods into trait #3454

Merged
merged 1 commit into from Sep 29, 2020

Conversation

jaspervdm
Copy link
Contributor

PMMR and ReadOnlyPMMR have a few common functions that are essentially duplicate code. I also needed access to one of the PMMR functions on the ReadOnlyPMMR. This PR pulls these functions out of the specific impls and puts them in a common trait.

Copy link
Member

@antiochp antiochp left a comment

Choose a reason for hiding this comment

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

Looks good. I think this makes a lot of sense, yes.
The whole ReadonlyPMMR needs a rethink at some point - we still need mut on a bunch of internal state and its not really readonly etc. (But that's for another PR).

@antiochp
Copy link
Member

Testing this locally now.

@antiochp
Copy link
Member

I also needed access to one of the PMMR functions on the ReadOnlyPMMR.

What did you need here?

Copy link
Member

@antiochp antiochp left a comment

Choose a reason for hiding this comment

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

👍

{
type Item = T::E;

fn get_hash(&self, pos: u64) -> Option<Hash> {
Copy link
Member

Choose a reason for hiding this comment

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

Actually - can get_hash() and get_data() (and similar) not also be moved to the trait if we had a backend() fn?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but that does mean we give access to the backend outside of the module. It might give use the temptation to accidentally use the backend directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm on further thought, we could maybe add the backend() function to a separate trait that isn't visible outside of the module..

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Turns out this is not really possible, as it would expose the private trait:

error[E0445]: restricted trait `core::pmmr::pmmr::PMMRBackend<B>` in public interface

So I'd rather keep it like it is, with some duplicate code.

@jaspervdm
Copy link
Contributor Author

What did you need here?

peak_path - the hashes of the "other" peaks - is very useful for the merkle proofs in the chunks

@jaspervdm jaspervdm merged commit 0aec8b5 into mimblewimble:master Sep 29, 2020
@jaspervdm jaspervdm deleted the pmmr_read_trait branch November 26, 2020 17:58
@antiochp antiochp mentioned this pull request Nov 26, 2020
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