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 explorer library #651

Closed
wants to merge 4 commits into from
Closed

add explorer library #651

wants to merge 4 commits into from

Conversation

ecioppettini
Copy link
Contributor

Extracted more or less directly from input-output-hk/jormungandr#3605 as a crate instead of a module. (I'll close that one, or change it to integrate this, not sure yet). The idea is to plug it in input-output-hk/jormungandr#3350

This is an implementation of an explorer database (as a library)based on sanakirja. The main reason to use sanakirja is the fork capability, which allows this to mirror the multiverse pattern but with a permanent storage. Not the entire database is forked though, only the stuff that is in state_ref.rs.

Although this is meant to be used with graphql, it could be used somewhere else too, as there is no direct dependency (instead, some glue code is needed).

I consider this to be generally ready for review (albeit long, I know), although I'm aware of a few things I want to improve. I also need to review what's public and what not.

@Mr-Leshiy
Copy link
Contributor

As I see in this PR a lot of parts that seems are not related to the intention of this PR if I have understood it correct.
Maybe should separate these things into the separate PRs (Update bech32 dependency to 0.8, chain-crypto: migrate tests from quickcheck to proptest etc. ), so to isolate only explorer crate and it's updates.

@ecioppettini
Copy link
Contributor Author

As I see in this PR a lot of parts that seems are not related to the intention of this PR if I have understood it correct. Maybe should separate these things into the separate PRs (Update bech32 dependency to 0.8, chain-crypto: migrate tests from quickcheck to proptest etc. ), so to isolate only explorer crate and it's updates.

Yeah, sorry about that, I forgot that I had to rebase to an earlier point in history to deal with some dependency issues in the jormungandr side until a few things were merged. Next time I'll just use my personal fork for that, to keep this clean.

the inner code assumes that the chain length of the block0 is 0, so add
check just in case there is a problem somewhere else.
Copy link
Contributor

@zeegomo zeegomo left a comment

Choose a reason for hiding this comment

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

Just an initial quick look

#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord)]
#[repr(C)]
pub struct TransactionCertificate {
tag: CertificateTag,
Copy link
Contributor

Choose a reason for hiding this comment

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

what's the advantage of an union + discriminant over an enum?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just that I don't need a max const fn to compute the size of the serialized thing...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

To be clear though, it is possible to store the whole thing as a data_len + tag + data, I just went with a fixed-size thing because it's easier to write (no need to deserialize, or use a DST), and for that the union gave me less trouble.

}

impl TransactionCertificate {
pub fn from_vote_plan_id(id: VotePlanId) -> Self {
Copy link
Contributor

Choose a reason for hiding this comment

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

is VotePlanId a certificate?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, not really... The graphql API should return a VotePlan. It's just that I stored the VotePlan info somewhere else, indexed by the VotePlanId, so this is just needed for that join.

It could be called from_vote_plan anyway, though.

let chain_length = txn.get_stable_chain_length();
let block = txn
.get_blocks_by_chain_length(&chain_length)?
.next()
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe assert there's only one block here?

}

fn new_with_size<P: AsRef<Path>>(name: P, size: u64) -> Result<Self, DbError> {
let env = ::sanakirja::Env::new(name, size, 2);
Copy link
Contributor

Choose a reason for hiding this comment

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

maybe we can extract 2 as a constant

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