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

Move proof to be a sub-module of merkle #378

Merged
merged 1 commit into from
Nov 30, 2023
Merged

Conversation

richardpringle
Copy link
Contributor

Proofs only make sense in the context of a merkle-tree, therefore they should be a submodule of the merkle module.

Copy link
Collaborator

@rkuris rkuris left a comment

Choose a reason for hiding this comment

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

The API returns proofs, so we want api::Proof to be visible to callers. Otherwise, the whole merkle module has to be pub.

@richardpringle
Copy link
Contributor Author

The API returns proofs, so we want api::Proof to be visible to callers. Otherwise, the whole merkle module has to be pub.

I thought the api module was just a mechanism for serving two apis at the same time. We shouldn't have a separate module for the public API, there's a reason that no one does this.

Otherwise, the whole merkle module has to be pub.

This specifically is incorrect. The whole purpose of pub use merkle::Proof (called in the db.rs) solves this problem. The Proof type and the impl Proof should live in the same place.

@rkuris
Copy link
Collaborator

rkuris commented Nov 29, 2023

The API returns proofs, so we want api::Proof to be visible to callers. Otherwise, the whole merkle module has to be pub.

I thought the api module was just a mechanism for serving two apis at the same time. We shouldn't have a separate module for the public API, there's a reason that no one does this.

Otherwise, the whole merkle module has to be pub.

This specifically is incorrect. The whole purpose of pub use merkle::Proof (called in the db.rs) solves this problem. The Proof type and the impl Proof should live in the same place.

This only works if merkle::Proof is also pub, which is what I want to avoid. This does not compile:

pub use inner::Thing;

mod inner {
    struct Thing {}
}

Base automatically changed from deserialize-extension to main November 30, 2023 18:27
@richardpringle
Copy link
Contributor Author

The API returns proofs, so we want api::Proof to be visible to callers. Otherwise, the whole merkle module has to be pub.

I thought the api module was just a mechanism for serving two apis at the same time. We shouldn't have a separate module for the public API, there's a reason that no one does this.

Otherwise, the whole merkle module has to be pub.

This specifically is incorrect. The whole purpose of pub use merkle::Proof (called in the db.rs) solves this problem. The Proof type and the impl Proof should live in the same place.

This only works if merkle::Proof is also pub, which is what I want to avoid. This does not compile:

pub use inner::Thing;

mod inner {
    struct Thing {}
}

but this does
https://github.com/ava-labs/firewood/pull/378/files#diff-648f516579918c7c3dddf3fd5eec6807c064b1ae8f8411d2c029bd804f7ef871R4

@rkuris
Copy link
Collaborator

rkuris commented Nov 30, 2023

This change resolves my concern: https://github.com/ava-labs/firewood/compare/b4f58a50058431fc80d0b8ac5b680573d33ff6b0..25eaca9a4800f6d24b9aa881ae0245dd25c4c49e

Apologies for thinking that this wasn't possible. I didn't realize that if you have a pub inside a private module and have visibility to that, it can be exported in a public module. Thanks for educating me @richardpringle

@richardpringle richardpringle merged commit 7c1c7a7 into main Nov 30, 2023
5 checks passed
@richardpringle richardpringle deleted the move-merkle-proof branch November 30, 2023 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

3 participants